test(cli): serialize the tests that change the process directory - #3494
Merged
Conversation
`cli/commands/skills/validate.test.ts` failed once on main, on a commit that touched only CSP, and passed on the next run. It resolves a relative path through `Deno.chdir`, and five CLI test files do the same. The working directory is process-global, so when a shard groups two of them the second `chdir` lands while the first is still awaiting, and the first resolves against the wrong directory. Which files a shard groups decides whether it happens, which is why it fails rarely and somewhere unrelated to the change under test. Adds `withCwd`, which queues callers so at most one holds the directory at a time and each is restored before the next begins, and moves the test that failed onto it. The other four files still call `Deno.chdir` directly, so the race is narrowed rather than closed. `webhook/handler.test.ts` has grown its own queue for this, which says the hazard was already felt -- and also why a per-file queue is not the answer, since it orders only its own callers while every other file races it. I tried moving that file onto the shared helper and it broke a test that passes in isolation and fails in the full suite, so it is left alone rather than half-understood. `router.test.ts`, `app/operations/project-creation.test.ts` and `commands/schedule/handler.test.ts` set the directory in setup and restore it in a distant `finally`, which means restructuring each test rather than swapping a helper. Noted here so the next person does not read one migration as the whole job.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe change adds a shared ChangesCurrent-directory test handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/testing/cwd.ts`:
- Around line 31-45: The withCwd queue deadlocks when its callback awaits
another withCwd call. Define and implement nested-call behavior in withCwd,
preferably failing fast before enqueueing or otherwise supporting reentrancy,
and add a focused regression test that exercises an awaited nested call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bd5018e-e30d-4792-81a5-19bb30d3afd8
📒 Files selected for processing (2)
cli/commands/skills/validate.test.tssrc/testing/cwd.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0082b2ddb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A nested call waited for the queue, which waited for the outer call, which waited for the inner one, so the suite hung rather than failed. Reentrancy is not the fix either: the inner chdir would move the directory out from under the outer caller, which is the hazard this helper exists to prevent. It now rejects with a named error. Adds the tests the helper should have had when it was introduced: restore, serialization of overlapping callers, the nested rejection, and the queue still advancing after a caller throws.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/testing/cwd.ts`:
- Around line 40-43: Replace the generic error in src/testing/cwd.ts lines 40-43
with the registered VeryfrontError for nested withCwd calls, using its defined
slug. Update src/testing/cwd.test.ts lines 41-45 to assert the VeryfrontError
type and expected slug rather than matching only the message text.
- Around line 39-44: Update withCwd’s ownership tracking so nested-call
rejection is scoped to the current execution context rather than the
process-global held flag; independent callers must wait through the existing
queue while another callback is awaiting. Preserve rejection for true
same-context nesting, and add coverage where one callback signals start, waits
on a gate, and a second caller starts and runs only after the gate opens.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 74955929-a53e-4f7f-9d0e-d0200856bea3
📒 Files selected for processing (2)
src/testing/cwd.test.tssrc/testing/cwd.ts
The nested-call guard I added used a process-global flag, which rejects any caller that arrives while another callback is awaiting -- and that caller is usually an independent test, for which queueing is exactly the right answer. Worse than the deadlock it replaced, and my own serialization test could not see it: both calls were made synchronously before either body ran, so the flag was still clear. Ownership is tracked by async context now, so only code running inside a callback is treated as nested. The regression test starts a callback, waits for it to signal, then calls from outside while it is still awaiting, and asserts the second caller runs after the first rather than being rejected. It fails against the global-flag version.
`withCwd` rejected nested calls with a generic `Error`, which left the contract resting on message text -- callers wanting to distinguish it from any other failure had to substring-match the wording. Registers `nested-cwd-scope` under GENERAL so the condition has a name that survives rewording. Both count assertions move with it: the total and the per-category one, which is easy to miss because the failure names only the category. The RSC client bundle is regenerated because the error registry is reachable from it, so any registry entry changes its bytes.
The queue only ever ordered its own file. Under `deno test --parallel` each test file runs in its own isolate, sharing neither module state nor `globalThis` with its peers, while the working directory it mutates belongs to the process they all share. So every file held a private queue and raced every other one -- the helper could not serialize the failing test against the peers that actually race it, which is the whole reason it exists. The turn is now taken from the operating system, the only mutex those isolates can both see: a directory created with `Deno.mkdir`, which fails atomically when it already exists. It is keyed on the pid because the directory it guards is per-process; two concurrent `deno test` runs have no reason to wait for each other, and no run can inherit a lock from an earlier one. The in-isolate queue stays in front of it so callers within a file still take their turn in arrival order. Restoring no longer reads `Deno.cwd()`. That read was unsafe under exactly the concurrency this guards: the directory it reports may belong to a sibling, and if the sibling has since removed it the call throws `NotFound` outright. Callers return to a root derived from the module URL instead. That was the bug behind the migration reverted earlier -- it broke a webhook test that passed alone and failed in the suite. `restores the previous directory` was written on that same unsafe assumption, capturing `Deno.cwd()` before the call, so it now asserts what holds under a peer instead. The cross-file property needs two test files to be observable at all; inside one isolate the module queue is already sufficient and the bug is invisible. Hence the exclusion pair, which fails against the previous implementation.
No test calls `Deno.chdir` directly any more, which is what the helper needs to be worth having: a queue that half the callers ignore serializes nothing. `webhook/handler.test.ts` had grown its own queue for this, which is why the hazard was already felt there. Its queue ordered only its own callers, so it is replaced rather than kept. The others held the directory from setup to a distant `finally`, spanning work that never needed it. Each now scopes it to the call that actually resolves a relative path, so the turn is held for a command rather than a test. The `afterEach` restores are gone too. They were the same hazard in a quieter form: reaching for the directory outside a turn, which takes it from whichever file holds it now. `src/platform/compat/process.test.ts` is included -- it moved the process to /tmp mid-suite. It tests `chdir` itself, so it takes the turn first and exercises the call inside it.
`src/errors/index.ts` re-exports every registered error, so adding one makes `docs/api-reference/veryfront/errors.md` stale and fails `ci (lint)`. The line pins shift with it; regenerated rather than hand-edited.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cli/commands/skills/validate.test.tsfailed once on main, on a commit that touched only CSP, and passed on the next run. It resolves a relative path throughDeno.chdir, and five test files do the same. The working directory is process-global, so when a shard groups two of them the secondchdirlands while the first is still awaiting, and the first resolves against the wrong directory.Adds
withCwd, and moves every process-directory mutation in the suite onto it. No test callsDeno.chdirdirectly any more.The queue could not have worked
The first version of this PR queued callers in module state. That cannot serialize anything across files. Four probes against
deno test --parallel:Deno.pidglobalThissharedcwdsharedEach test file is its own isolate sharing one OS process.
cwdis process state;let queueis not. Every file held a private queue and raced every other one — including the one file that had been migrated to it.The turn is now taken from the operating system, the only mutex those isolates can both see: a directory created with
Deno.mkdir, which fails atomically when it already exists. It is keyed on the pid, because the directory it guards is per-process — two concurrentdeno testruns have no reason to wait for each other, and no run can inherit a lock from an earlier one. The in-isolate queue stays in front of it so callers within a file keep arrival order.Restoring no longer reads
Deno.cwd()That read was unsafe under exactly the concurrency it guards. The directory it reports may belong to a sibling, and if the sibling has since removed it, the call throws rather than returning a stale path:
This was the bug behind a migration I reverted earlier in this PR:
webhook/handler.test.tspassed alone and failed in the suite. That file had grown its own queue and deliberately never readDeno.cwd(), so the code I reverted to was more correct than the helper replacing it. Callers now return to a root derived from the module URL. TheafterEachrestores are gone for the same reason — reaching for the directory outside a turn takes it from whichever file holds it now.restores the previous directorywas written on that same assumption, capturingDeno.cwd()before the call, and now asserts what holds while a peer is running.Tests
src/testing/cwd-exclusion-{a,b}.test.tspin the cross-file property. They come as a pair because one file cannot observe it: inside a single isolate the module queue is already sufficient, so the bug is invisible by construction. Against the previous implementation the pair fails withOVERLAP.Nested calls now reject with a registered
nested-cwd-scoperather than a genericError, so the contract does not rest on message text.Notes
src/platform/compat/process.test.tswas a fifth chdir site not previously listed; it moved the process to/tmpmid-suite.generate:manifests:checkfails until regenerated.Full suite green: 3773 passed, 0 failed.
lint,lint:test-typecheck,typecheck,fmt --checkall clean.Summary by CodeRabbit
Bug Fixes
Documentation
Tests