fix(cli): avoid destroying .nuxt while dev runs#1337
Draft
harlan-zw wants to merge 10 commits into
Draft
Conversation
Running `nuxt typecheck` against the same project as a live `nuxt dev`
made typecheck's `writeTypes`+`buildNuxt` wipe `.nuxt/dist`, which the dev
dist-watcher hard-restarts on. With dev and typecheck sharing a buildDir
this produced a restart loop.
`nuxt dev` now writes a `nuxt.lock` *presence* marker (detection only, it no
longer refuses a second dev — multiple dev servers may run concurrently).
`nuxt typecheck` reads it and, when a live `dev` lock owns `<cwd>/.nuxt` and
types already exist, skips its own prepare and type-checks against the
dev-maintained `.nuxt`. No `buildNuxt` -> no `dist` removal -> no restart.
- lockfile: split write (always, unless `NUXT_IGNORE_LOCK`) from enforcement
(`acquireLock(.., { enforce })`); per-acquisition token so a same-process
re-acquire (dev reload) isn't clobbered by the prior release; add
`readActiveLock`. `build` keeps enforcement.
- typecheck: `--prepare` / `--no-prepare` (default auto). `--extends` forces
prepare in auto mode. Falls back to preparing for custom buildDirs.
commit: |
… live build
Addresses two adversarial-review findings:
- Typecheck could skip prepare against stale/mid-rebuild `.nuxt`. The dev lock
is written before `writeTypes`/`buildNuxt`, so a typecheck during dev startup
or reload could check old generated types. Dev now sets `typesReady` on the
lock only after the build completes (and clears it while (re)building);
typecheck reuses `.nuxt` only when `typesReady` is set.
- Dev's detection-only `acquireLock({ enforce: false })` unconditionally
overwrote any lock, including an active `build` (which mutates the buildDir).
The non-enforce path now still refuses a live `build` lock; only peer `dev`
locks are taken over. `updateLock` now merges so partial updates keep `url`.
harlan-zw
changed the title
harlan-zw
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds per-process lock markers and token-safe lock APIs, updates the dev server to set typesReady:false/true around rebuilds, preserves locks/ when clearing the build dir, and extends Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/nuxi/src/commands/typecheck.ts (1)
56-58: 💤 Low valueCustom
buildDirconfigurations will always trigger prepare.The comment acknowledges this limitation, but users with custom
buildDirinnuxt.configwon't benefit from the optimization. Consider documenting this behavior in the--prepareflag description so users understand why auto-skip may not work for their setup.🤖 Prompt for 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. In `@packages/nuxi/src/commands/typecheck.ts` around lines 56 - 58, Update the help/description for the --prepare flag to document that custom buildDir settings in nuxt.config will force the command to run prepare because resolving a custom buildDir requires loading the config (see the buildDir = join(cwd, '.nuxt') assumption in typecheck command). Specifically, edit the --prepare flag description in the typecheck command help text to state that auto-skip only works when the default buildDir is used and that custom buildDir values will cause prepare to run. Ensure the wording references the prepare behavior and the buildDir assumption so users understand why auto-skip may not apply.packages/nuxi/src/utils/lockfile.ts (2)
193-201: 💤 Low valueSpread order preserves caller-supplied
token, undermining release safety.In
updateLock, the merge spreads...infoafter...current, so a caller passingtokenininfowould overwrite the original acquisition token. The finaltoken: current?.tokenline fixes this, but the intermediate spread creates a confusing code path. Consider filteringtokenfrominfoor documenting that callers must not pass it.🤖 Prompt for 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. In `@packages/nuxi/src/utils/lockfile.ts` around lines 193 - 201, The merge in updateLock creates next by spreading ...current then ...info which allows a caller-supplied token in info to temporarily override the original token; although you later set token: current?.token, this is confusing and unsafe—change the merge to explicitly strip token from the caller-supplied info (e.g. const { token: _unusedToken, ...safeInfo } = info) and then build next using ...current, ...safeInfo, pid, startedAt, token: current?.token so callers cannot override the acquisition token; reference updateLock and the next: LockInfo creation to locate the change.
217-222: ⚖️ Poor tradeoffTOCTOU window between reading and unlinking the lock file.
Between
readLockFileandtryUnlink, another process could overwrite the lock with a different token. While unlikely in typical usage (same-process re-acquire is the main concern), a concurrentacquireLockfrom a peer dev server could race here. The current design accepts this since the token check provides best-effort protection. Consider documenting this limitation or using atomic compare-and-delete if stronger guarantees are needed.🤖 Prompt for 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. In `@packages/nuxi/src/utils/lockfile.ts` around lines 217 - 222, There's a TOCTOU race between readLockFile(lockPath) and tryUnlink(lockPath) where another process may replace the lock; either document this limitation near that block or replace with an atomic compare-and-delete helper (e.g., compareAndUnlink) that reads the file via fs.promises.open, verifies the token, re-checks file identity (stat/inode or mtime+size) and only then unlinks, optionally retrying if identity changed; update usages in the release path (the block using readLockFile, token, tryUnlink and related acquireLock) to call the new helper or add a clear comment explaining the best-effort semantics.
🤖 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 `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 111-117: fullInfo currently builds token before spreading incoming
info so a caller-supplied token in info can overwrite the generated value;
change the construction of fullInfo (used where token, pid, startedAt and info
are combined) so that ...info is spread first and then token (the generated
`${process.pid}:${++acquireCounter}`) is assigned after the spread, ensuring the
generated token in the variable token (and the increment of acquireCounter)
cannot be overridden by info.
In `@packages/nuxi/test/unit/typecheck-prepare.spec.ts`:
- Around line 96-101: The test "forces a prepare when --extends is passed"
currently writes a lock without typesReady true so it doesn't validate the
override; update the test to write a lock that includes typesReady: true (use
the existing writeLock helper with typesReady: true for buildDir) before calling
resolvePrepareDecision(buildDir, { extends: '../base' }) so the expectation
verifies that resolvePrepareDecision returns { prepare: true } even when types
are already ready; reference the test block containing resolvePrepareDecision,
writeLock, and buildDir to locate and change the lock payload.
---
Nitpick comments:
In `@packages/nuxi/src/commands/typecheck.ts`:
- Around line 56-58: Update the help/description for the --prepare flag to
document that custom buildDir settings in nuxt.config will force the command to
run prepare because resolving a custom buildDir requires loading the config (see
the buildDir = join(cwd, '.nuxt') assumption in typecheck command).
Specifically, edit the --prepare flag description in the typecheck command help
text to state that auto-skip only works when the default buildDir is used and
that custom buildDir values will cause prepare to run. Ensure the wording
references the prepare behavior and the buildDir assumption so users understand
why auto-skip may not apply.
In `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 193-201: The merge in updateLock creates next by spreading
...current then ...info which allows a caller-supplied token in info to
temporarily override the original token; although you later set token:
current?.token, this is confusing and unsafe—change the merge to explicitly
strip token from the caller-supplied info (e.g. const { token: _unusedToken,
...safeInfo } = info) and then build next using ...current, ...safeInfo, pid,
startedAt, token: current?.token so callers cannot override the acquisition
token; reference updateLock and the next: LockInfo creation to locate the
change.
- Around line 217-222: There's a TOCTOU race between readLockFile(lockPath) and
tryUnlink(lockPath) where another process may replace the lock; either document
this limitation near that block or replace with an atomic compare-and-delete
helper (e.g., compareAndUnlink) that reads the file via fs.promises.open,
verifies the token, re-checks file identity (stat/inode or mtime+size) and only
then unlinks, optionally retrying if identity changed; update usages in the
release path (the block using readLockFile, token, tryUnlink and related
acquireLock) to call the new helper or add a clear comment explaining the
best-effort semantics.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06d36231-913c-426e-8dfd-d0962c0e9cde
📒 Files selected for processing (6)
packages/nuxi/src/commands/typecheck.tspackages/nuxi/src/dev/utils.tspackages/nuxi/src/utils/lockfile.tspackages/nuxi/test/unit/lockfile-presence.spec.tspackages/nuxi/test/unit/lockfile.spec.tspackages/nuxi/test/unit/typecheck-prepare.spec.ts
clearBuildDir() removes every generated artifact before buildNuxt rewrites them. When a dev server is live its watcher reloads into that gap and resolves aliases (e.g. drizzle #schema/<n>) against a momentarily-absent file, throwing ENOENT; wiping under a running build is equally destructive. buildNuxt regenerates all templates in place (content-diffed) regardless, so reuse is safe and still hands callers like db:generate fresh artifacts. Stop the owning process when a clean wipe is genuinely needed.
The single shared nuxt.lock cannot represent more than one owner. When peer dev servers share a buildDir (e.g. parallel agents): acquire clobbered the prior record, updateLock no-op'd for every non-owner (current.pid !== process.pid), and whichever server exited first unlinked the file out from under the others — making a still-live dev server invisible so typecheck/prepare would rebuild/wipe under it. Move presence markers to one-file-per-process under <buildDir>/locks/ <pid>.json. Each process only ever writes or removes its own path, so reads enumerate the directory and the three hazards disappear. readActiveLocks() returns every live owner (pruning dead/stale files on read); readActiveLock() keeps the single-owner shape (build wins, else newest dev). typecheck now reuses types from any ready dev peer, and clearBuildDir preserves locks/ so a wipe can't race-delete a fresh marker.
harlan-zw
changed the title
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 `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 190-198: acquireLock currently does a non-atomic read-then-write
(readActiveLocks -> writeFileSync) which allows a race; change to an
atomic-create first approach: attempt to atomically create the lock file (use
fs.openSync(lockPath, 'wx') or equivalent) and write JSON there; if the create
fails with EEXIST, then call readActiveLocks() and apply the existing blocker
logic (enforce vs command==='build'); if the existing lock belongs to this
process/token then allow overwrite (use writeFileSync) and return
makeRelease(lockPath, token); keep makeRelease and fullInfo usage but ensure the
initial write is atomic so two processes cannot both succeed. Ensure EEXIST
handling maps to the same return shapes ({ existing } or { release }).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84e657b1-26ff-402c-be96-d5495ff6eec3
📒 Files selected for processing (7)
packages/nuxi/src/commands/prepare.tspackages/nuxi/src/commands/typecheck.tspackages/nuxi/src/utils/fs.tspackages/nuxi/src/utils/lockfile.tspackages/nuxi/test/unit/lockfile-presence.spec.tspackages/nuxi/test/unit/lockfile.spec.tspackages/nuxi/test/unit/typecheck-prepare.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nuxi/src/commands/typecheck.ts
- packages/nuxi/test/unit/typecheck-prepare.spec.ts
Two dev servers sharing a buildDir coexist (the lock is detection-only for dev), but they write and watch the same .nuxt — concurrent rebuilds can trigger conflicting reloads. acquireLock now reports the live peers it coexists with, and dev warns when one is already running so the user knows the build dir is shared (and can isolate with a separate buildDir).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxi/src/utils/lockfile.ts (1)
27-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't expire a live owner just because the marker is old.
isLockActive()returns false oncestartedAtis older than 24 hours even if the PID is still alive, andreadActiveLocks()then prunes that marker. Any long-lived dev/build process stops protecting its buildDir after a day, which reopens the.nuxtwipe/race this lock is meant to prevent. This needs a heartbeat or a real process-start check rather than a hard TTL on live PIDs.Also applies to: 71-76
🤖 Prompt for 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. In `@packages/nuxi/src/utils/lockfile.ts` around lines 27 - 29, isLockActive() and readActiveLocks currently expire locks solely by MAX_LOCK_AGE_MS which allows live long-running processes to be pruned; change the logic so that before treating a lock as stale you verify the owner process is not actually running (e.g., a processExists check using the stored PID) and only prune markers when the PID is dead AND startedAt is older than MAX_LOCK_AGE_MS; update isLockActive, readActiveLocks and any helper that uses startedAt/PID to first try a live-process check (or fallback to TTL if liveness cannot be determined) instead of relying on the hard TTL alone.
♻️ Duplicate comments (1)
packages/nuxi/src/utils/lockfile.ts (1)
183-193:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMake exclusive acquisition atomic.
acquireLock()still does a read-then-write on the marker set. Two enforced callers can both observeothersas empty and both return{ release }, so they proceed as simultaneous "exclusive" owners and can mutate the same buildDir concurrently. The per-PID JSON files are fine for discovery, but exclusivity needs one atomic ownership gate that both build acquisition and conflict detection consult.🤖 Prompt for 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. In `@packages/nuxi/src/utils/lockfile.ts` around lines 183 - 193, acquireLock currently does a read-then-write race (readActiveLocks -> writeFileSync) so two enforce callers can both see no blockers and both become "exclusive"; fix by introducing an atomic ownership gate before writing fullInfo: attempt to create a dedicated atomic owner marker (e.g. open/create lockPath.owner or lockPath + '.claim' with exclusive create semantics like fs.open with O_EXCL/'wx'); if the exclusive create fails, readActiveLocks again and return the existing blocker, otherwise writeFileSync(fullInfo) and return { release: makeRelease(lockPath, token), peers: others }; update acquireLock to consult this new atomic create step (and clean it up in makeRelease) so exclusivity is enforced even under concurrent processes.
🤖 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 `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 192-193: The current in-place JSON writes (e.g.,
writeFileSync(lockPath, JSON.stringify(fullInfo, null, 2)) used when acquiring
locks and in updateLock()) can race with concurrent
readActiveLocks()/readLockFile() and produce transient missing-owner reads;
change those writes to atomic replace: write the JSON to a temp file in the same
directory and then atomically rename/move it to lockPath (ensuring fsync of the
temp file and directory if available), and apply the same change for the other
occurrence around lines 223-224 so all lock file updates use the
temp-file-then-rename pattern to avoid truncate/write windows.
---
Outside diff comments:
In `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 27-29: isLockActive() and readActiveLocks currently expire locks
solely by MAX_LOCK_AGE_MS which allows live long-running processes to be pruned;
change the logic so that before treating a lock as stale you verify the owner
process is not actually running (e.g., a processExists check using the stored
PID) and only prune markers when the PID is dead AND startedAt is older than
MAX_LOCK_AGE_MS; update isLockActive, readActiveLocks and any helper that uses
startedAt/PID to first try a live-process check (or fallback to TTL if liveness
cannot be determined) instead of relying on the hard TTL alone.
---
Duplicate comments:
In `@packages/nuxi/src/utils/lockfile.ts`:
- Around line 183-193: acquireLock currently does a read-then-write race
(readActiveLocks -> writeFileSync) so two enforce callers can both see no
blockers and both become "exclusive"; fix by introducing an atomic ownership
gate before writing fullInfo: attempt to create a dedicated atomic owner marker
(e.g. open/create lockPath.owner or lockPath + '.claim' with exclusive create
semantics like fs.open with O_EXCL/'wx'); if the exclusive create fails,
readActiveLocks again and return the existing blocker, otherwise
writeFileSync(fullInfo) and return { release: makeRelease(lockPath, token),
peers: others }; update acquireLock to consult this new atomic create step (and
clean it up in makeRelease) so exclusivity is enforced even under concurrent
processes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 625dece2-06ee-4f44-8976-53bd1cb03705
📒 Files selected for processing (3)
packages/nuxi/src/dev/utils.tspackages/nuxi/src/utils/lockfile.tspackages/nuxi/test/unit/lockfile.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxi/src/dev/utils.ts
harlan-zw
changed the title
Addresses CodeRabbit review: - Major: in-place writeFileSync truncates then writes, so a concurrent readActiveLocks could land in the window and see the owner vanish. Per-process marker writes + updateLock now write a sibling temp and rename over the target (atomic replace). - Critical: the per-PID redesign lost the build-vs-build mutex the old single-file 'wx' gave — two 'nuxt build' could both pass the advisory read then both write their own marker. A build now claims a shared 'build.lock' sentinel via an atomic linkSync-from-temp (create-if- absent against an already-populated inode); the loser gets the live winner back, a stale sentinel is taken over. readActiveLocks reads all non-.tmp markers so the sentinel is still discoverable.
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.
🔗 Linked issue
❓ Type of change
📚 Description
When running multiple agents on the same workspace, they will happily work on top of each other, spinning up multiple dev servers, typechecking, preparing, etc. As these agents run these commands, many of them will delete the
.nuxtfolder, causing the nitro instance to do a full reload without any clear idea why (per logs).This in itself isn't a breaking issue, but it causes agents to start diagnosing why the dev server isn't responding as well as being quite annoying for end-users who are trying to test things themselves (unresponsive requests while the rebuild goes through).
To solve this, we port the lock mechanism to dev and change destructive behaviors based on dev servers are running (lock existence). This new lock for dev should also help agents debug the dev server instance themselves if they find it unresponsive (as it surfaces the state and PID).
🧭 Behaviour summary
Keyed off the per-process markers in
<buildDir>/locks/<pid>.json. Dead/stale markers are taken over silently;clearBuildDirnow preserveslocks/.nuxt devnuxt devnuxt buildnuxt typecheck.nuxt— "Reusing types from the running dev server…"nuxt prepare