fix(cli): keep the shared ESM cache when another dev server holds the port - #3582
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 571c930f6b
ℹ️ 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".
… port Found during a DX dogfood walk of the developer journey. Running `veryfront dev` a second time in the same project, while the first server is still up, destroyed the shared `.cache` — the original walk saw 432 compiled `.mjs` files go to 0, forcing the running server to recompile everything on its next request. `cli/commands/dev/handler.ts` cleared the caches unconditionally, before it even called `devCommand`. The cache directory (`getCacheBaseDir()`, i.e. `<project>/.cache`) is shared by every dev server rooted at the same project, so the destructive step ran for a process that had no claim on the dev port. Move the clear into `devCommand`, behind a probe on the resolved port. A taken port is the signal that another dev server is already serving this project and still needs the modules it compiled, so the clear is skipped: the second `veryfront dev` falls forward to a free port (#3562) and starts on a cache it did not just destroy. A free port still clears, so the stale-module behaviour the clear exists for is unchanged. `clearLocalCaches` is opt-in, so `veryfront demo`'s programmatic `devCommand` call keeps its current behaviour of never clearing. `clearLocalCachesIfPortFree` takes its clear and probe as parameters, the same seam `startDevServerOnFreePort` uses, so the decision is testable without booting a dev server. Regression tests in cli/commands/dev/dev-cache-guard.integration.test.ts, all three confirmed red first against an unconditional clear: - a real held socket makes the probe report taken, and the seeded MDX-ESM and HTTP bundle entries survive; - a released port still clears them; - `handleDevCommand` driven against an unbindable port aborts inside devCommand with the seeded entries intact, which locks the handler ordering that caused the bug.
1e3741b to
6426cc9
Compare
CI note: known flake, not from this changeThe first run on the rebased commit went red on three checks, all from one test:
That test is a known flake on
|
Found during a DX dogfood walk of the developer journey.
Symptom
Running
veryfront deva second time in the same project, while the firstserver is still up, destroys the shared
.cache.Measured on the parent commit with a seeded cache directory:
The second command took the still-running server's compiled modules with it.
The surviving server then has to recompile everything on the next request; the
original walk saw 432
.mjsfiles go to 0.Root cause
cli/commands/dev/handler.tscleared the caches unconditionally, before it evencalled
devCommand:The dev port is not resolved until
devCommandmergesconfig.dev.port, andnot bound until
startDevServerruns — several steps later. The cache directory(
getCacheBaseDir(), i.e.<project>/.cache) is shared by every dev serverrooted at the same project, so the destructive step ran for a process that had
no claim on the port.
Fix
Move the clear into
devCommand, behind a probe on the resolved port:A taken dev port is the signal that another dev server is already serving this
project and still needs the modules it compiled, so the clear is skipped: the
second
veryfront devfalls forward to a free port (#3562) and starts on acache it did not just destroy. A free port still clears, so the stale-module
behaviour the clear exists for is unchanged.
clearLocalCachesIfPortFreetakes its clear and its probe as parameters — thesame seam
startDevServerOnFreePortuses — so the decision is testable withoutbooting a dev server. The probe itself is
isPortAvailablefromcli/commands/dev/port-fallback.ts, already used by the port fall-forward andalready cross-runtime.
One other consequence, intended:
clearLocalCachesis opt-in, soveryfront demo's programmaticdevCommandcall keeps its current behaviour of neverclearing.
Regression tests
cli/commands/dev/dev-cache-guard.integration.test.ts, all three confirmed redfirst against an unconditional clear:
HTTP bundle entries survive;
clear;
handleDevCommanddriven against an unbindable port aborts insidedevCommandwith the seeded entries intact. That one locks the handlerordering that caused the bug — anything clearing ahead of the port probe has
already destroyed the running server's modules by the time the command gives
up.
Verification
deno test cli/commands/dev cli/commands/demo— 14 passed, 0 failed.deno check cli/main.ts,deno lint,deno fmtclean.Rebase note
Rebased onto current
main, which now carries #3562 (veryfront devfallsforward to a free port instead of hard-failing). The first cut of this PR added
its own
cli/commands/dev/port-guard.tsprobe and made a busy port a hardfailure. Both are gone: the hard failure would have reverted #3562, and
port-fallback.tsalready ships the same bind-and-release probe. What survivesis the part #3562 does not address — the shared cache must not be wiped by a
process that does not own the port. Without this change the fall-forward makes
the bug quieter, not smaller: the second
veryfront devnow succeeds on thenext port and still clears the first server's cache.