Repository navigation
Fix Windows telemetry flush worker cwd race - #341
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Review request for current head @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
More reviews will be available in 37 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughFixes a Windows EBUSY error in the telemetry non-blocking integration test by changing the ChangesWindows EBUSY Telemetry Test Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a Windows-specific EBUSY cleanup race in the telemetry OTLP non-blocking integration test by changing the working directory of the spawned CLI process from the disposable sandbox directory to the repository root. This prevents the detached telemetry worker from locking the temporary directory. The review feedback correctly identifies that adding native retry options (maxRetries and retryDelay) to the rm call introduces redundant retry logic, as it is already wrapped in an outer retry loop, and suggests simplifying the cleanup logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for (let attempt = 0; attempt < cleanupAttempts; attempt += 1) { | ||
| try { | ||
| await rm(context.tempRoot, { recursive: true, force: true }); | ||
| await rm(context.tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 150 }); |
There was a problem hiding this comment.
The addition of maxRetries: 10 and retryDelay: 150 to the rm options introduces redundant retry logic. The outer for loop already retries the cleanup up to 8 times with a linear backoff delay (delay(100 * (attempt + 1))). Combining both means that each of the 8 outer attempts will internally retry up to 10 times, potentially leading to up to 80 total retries and significantly delaying test failure reporting if the directory is genuinely locked. Recommendation: Either rely entirely on Node's native retry options by removing the outer loop, or keep the outer loop and revert rm to its simpler form.
| await rm(context.tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 150 }); | |
| await rm(context.tempRoot, { recursive: true, force: true }); |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Details ==="
gh pr view 341 --repo unbraind/pm-cli --json title,body,files,commits,additions,deletions,changedFiles
echo ""
echo "=== PR Diff ==="
gh pr diff 341 --repo unbraind/pm-cli
echo ""
echo "=== Changed Files ==="
gh pr view 341 --repo unbraind/pm-cli --json files --jq '.files[].path'Length of output: 19269 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Full test file ==="
cat tests/integration/telemetry-otel-nonblocking.spec.ts
echo ""
echo "=== repoRoot usage in codebase ==="
rg "repoRoot" tests/integration/telemetry-otel-nonblocking.spec.ts -nLength of output: 6813
Review of
|
There was a problem hiding this comment.
Code Review
This pull request addresses a Windows-specific cleanup race (GH-340) in the telemetry integration tests by running the spawned CLI from the repository root instead of the temporary directory, and adding retry options to the directory cleanup process. The reviewer points out that changing the test's working directory to the repository root could compromise test isolation and pollute the repository. They suggest a more robust solution: modifying the production code where the detached telemetry worker is spawned to explicitly use a safe directory like os.tmpdir(), thereby preventing it from locking the test's temporary directory.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const startedAt = Date.now(); | ||
| const completed = spawnSync(process.execPath, [distCli, ...args], { | ||
| cwd: context.tempRoot, | ||
| cwd: repoRoot, |
There was a problem hiding this comment.
Running the spawned CLI with cwd: repoRoot executes the CLI inside the actual repository root. If any command or dependency in the CLI writes to the current working directory (or resolves paths relative to it), it could pollute the repository with untracked files or cause interference between concurrent test runs, violating test isolation.
A much cleaner and more robust fix is to address the root cause directly in the production code where the detached telemetry flush worker is spawned (src/core/telemetry/runtime.ts). By explicitly setting cwd: os.tmpdir() (or another safe directory) when spawning the detached worker, we prevent it from inheriting and locking the temporary directory (context.tempRoot). This allows the test to keep using cwd: context.tempRoot for perfect isolation without any Windows EBUSY cleanup races.
Suggested Production Fix in src/core/telemetry/runtime.ts:
const child = spawn(process.execPath, [telemetryFlushRunnerPath()], {
detached: true,
stdio: "ignore",
cwd: os.tmpdir(), // Prevent inheriting and locking the parent's temp cwd on Windows
env: {
...process.env,
PM_GLOBAL_PATH: globalPmRoot,
[PM_TELEMETRY_FLUSH_CHILD_ENV]: "1",
},
});If you apply the above fix in src/core/telemetry/runtime.ts, you can revert the cwd change in this test back to context.tempRoot to maintain complete test isolation.
| cwd: repoRoot, | |
| cwd: context.tempRoot, |
3b4bf16 to
ce315f3
Compare
unbraind
changed the title
ce315f3 to
322ba3a
Compare
|
Review request after addressing Gemini feedback on current head
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Code Review
This pull request addresses a Windows-specific cleanup issue (GH-340) where a detached telemetry flush worker locks the temporary project directory, causing EBUSY errors during test cleanup. The fix updates scheduleTelemetryFlush in src/core/telemetry/runtime.ts to spawn the detached worker with its current working directory (cwd) set to the OS temporary directory (os.tmpdir()). There are no review comments to address, and the changes successfully resolve the directory locking issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR diff ==="
gh pr diff 341 --repo unbraind/pm-cli
echo ""
echo "=== Current head info ==="
gh pr view 341 --repo unbraind/pm-cli --json headRefOid,headRefName,commitsLength of output: 20147
Review of
|
Record pm-zpe7 for GH-340 and set the detached telemetry flush worker cwd to os.tmpdir() so Windows does not keep a caller's disposable project directory locked after the foreground CLI exits. PM_GLOBAL_PATH remains explicit, so the worker still drains the intended telemetry queue without inheriting the command cwd. Verification: node scripts/run-tests.mjs test -- tests/integration/telemetry-otel-nonblocking.spec.ts; pnpm build; pnpm quality:static; node scripts/run-tests.mjs coverage (100/100/100/100, 242 files, 4840 tests); pnpm security:scan; pnpm audit --audit-level moderate; pnpm changelog:pm:check; pm health --check-only --summary --json; pm validate --check-resolution --check-history-drift; manual isolated PM_PATH/PM_GLOBAL_PATH temp smoke init/create/update/get/health.
322ba3a to
eae0268
Compare
|
Final review request for current head
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Code Review
This pull request resolves a Windows-specific cleanup issue (GH-340) where a detached telemetry flush worker inherits the caller's current working directory, locking disposable project directories and causing EBUSY errors during cleanup. The fix sets the cwd of the spawned child process to os.tmpdir() in src/core/telemetry/runtime.ts. Additionally, issue tracking files and a changelog entry have been added. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Current PR diff ==="
gh pr diff 341 --repo unbraind/pm-cli
echo ""
echo "=== Current head info ==="
gh pr view 341 --repo unbraind/pm-cli --json headRefOid,headRefName,commits --jq '{head: .headRefOid, branch: .headRefName}'
echo ""
echo "=== Changed files ==="
gh pr view 341 --repo unbraind/pm-cli --json files --jq '.files[].path'Length of output: 22357
|
Summary
cwdtoos.tmpdir()insrc/core/telemetry/runtime.ts.tests/integration/telemetry-otel-nonblocking.spec.ts: the foreground CLI still runs from the disposable temp root whilePM_PATHandPM_GLOBAL_PATHremain sandboxed.pm-zpe7with duplicate-check, GitHub issue/run findings, linked files, tests, reviewer follow-up, and verification evidence.Root Cause
The scheduled Nightly Validation run
27944143257failed only onwindows-latest / Node 20. The GH-209 foreground nonblocking assertions passed, thenafterAllfailed to remove the temp root withEBUSY. The telemetry flush worker is detached and inherited cwd from the foreground CLI, so a worker spawned from a disposable project directory could keep that directory busy on Windows after the foreground command exited.Fix
scheduleTelemetryFlush()now starts the detached worker withcwd: os.tmpdir(). This prevents the worker from locking the caller/project cwd while keeping telemetry queue selection explicit throughPM_GLOBAL_PATH.Verification
node scripts/run-tests.mjs test -- tests/integration/telemetry-otel-nonblocking.spec.tspnpm buildnode scripts/run-tests.mjs coverage-> 100/100/100/100, 242 files, 4840 testspnpm quality:staticpnpm security:scanpnpm audit --audit-level moderatepnpm changelog:pm:checkpm health --check-only --summary --jsonpm validate --check-resolution --check-history-drift(history drift OK; pre-existing closed-item resolution warnings remain)PM_PATH/PM_GLOBAL_PATH: init, create, update, get, healthCloses #340