Repository navigation
Router idea 2 revisit - #1
Merged
Merged
Conversation
Add complete vitest testing infrastructure with: - vitest, @vitest/ui, @vitest/coverage-v8 for testing - happy-dom for fast DOM environment - Test configuration with 80% coverage thresholds - Test scripts: test, test:watch, test:ui, test:coverage - Initial sanity tests (3 passing) - Global test setup file - Implementation changeset tracking system All tests passing ✅ TypeScript compilation successful ✅ See: packages/rsc-router/.implementation-changesets/phase-0.1-testing-infrastructure.md
Implement parseClientSegments() to parse the _has query parameter for differential rendering. During SPA navigation, clients report current segments via ?_has=L0,L1,R2 to enable server-side differential updates. Features: - Parse comma-separated segment IDs from _has parameter - Return Set for O(1) lookup performance - Handle null/empty (initial navigation) - Trim whitespace and filter empty strings - Automatic deduplication via Set Tests: 24 new tests (all passing) Total: 326 tests (100% passing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Enhancements to Phase 1.1 revalidation implementation: - Add CurrentURL client component showing real-time browser state - Add query param display in blog and shop handlers (server snapshot vs client) - Fix empty diff handling in entry.browser.tsx (prevent white screen) - Update examples to show route params vs query string behavior - Update NEXT.md to mark Phase 1.1 complete Visual demo now clearly shows: - Timer keeps running when revalidation returns false - Server snapshot stays frozen (query params don't update) - Current URL always updates (client component) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ivogt
added a commit
that referenced
this pull request
Apr 26, 2026
…ediscovery Stacked on top of the cold-start fix (#485). Pre-fix, after a successful cloudflare dev cold start, any source-file change collapsed `router.named-routes.gen.ts` from the full runtime manifest back to the ~19-route static-parser view, then propagated that partial map into workerd via the manifest virtual module — `worker.rsc.tsx`'s module-level `router.reverse("shop.product.item42")` then threw `Unknown route` and killed the dev server. The mechanism had three independent contributors and the fix addresses each: 1. **Static-parser write was authoritative on HMR.** `scheduleRouteRegeneration` ran `writeCombinedRouteTypesWithTracking(s)` first; for cf-stress the static parser only sees 19 literal-named routes. The supplement helper (`supplementGenFilesWithRuntimeRoutes`) only restores `factoryOnlyPrefixes` — dot-prefixes entirely absent from static output — and cf-stress has `shop.home`, `api.benchFirst`, etc. as static names, so the `shop.`/`api.` prefixes are not factory-only and the factory siblings under those prefixes (`shop.product.item42` etc.) were dropped. Fix: when `!rscEnv?.runner && s.perRouterManifests.length > 0` (cloudflare dev with cold-start runtime state), skip the static write entirely. The runtime rediscovery below owns the gen file in this environment. 2. **Runtime rediscovery never ran in cloudflare HMR.** `refreshRuntimeDiscovery` bailed early when `rscEnv.runner` was unavailable, which is the entire cloudflare-dev case (workerd is a separate runtime). Even with #1 skipping the static write, no fresh runtime discovery would happen, so newly-added factory routes wouldn't appear until restart. Fix: add a no-main-runner branch that mirrors cold start — close+null both `prerenderTempServer` AND `prerenderNodeRegistry` (`getOrCreateTempServer`'s cached-registry short-circuit at line ~387 would otherwise keep the stale module graph), recreate via `getOrCreateTempServer()` under the `__rscRouterDiscoveryActive` flag, re-import the entry, refresh `s.perRouterManifests`, write via `writeRouteTypesFiles(s)`. ~700ms per save — acceptable, optimizable later via finer-grained module-graph invalidation. 3. **Manifest readiness gate didn't reset on HMR.** `s.discoveryDone` is the promise the manifest virtual module's `load()` awaits; once resolved at cold-start completion it stays resolved. So during HMR, any workerd reload triggered by the source change cascaded straight through the gate against whatever the current gen file said — even if rediscovery was about to overwrite it 700ms later. Fix: extract `beginDiscoveryGate()` / `resolveDiscoveryGate()` helpers that own the promise lifecycle and replace ad-hoc construction at both cold-start and HMR sites. Reset the gate IMMEDIATELY in `handleRouteFileChange` (before the 100ms debounce — refinement from review), so a workerd reload firing within the debounce window blocks on the new pending promise. Resolve the gate only after rediscovery's `writeRouteTypesFiles` completes (or in the `catch` so reloads don't hang on errors). ## Order of operations on HMR (cloudflare) source file change (chokidar) ↓ handleRouteFileChange() beginDiscoveryGate() ← new pending promise, before any async / fanout ↓ scheduleRouteRegeneration() (100ms debounce) ↓ (skip static write — owned by rediscovery) refreshRuntimeDiscovery() — cloudflare branch: close+null prerenderTempServer + prerenderNodeRegistry ↓ getOrCreateTempServer() (sets __rscRouterDiscoveryActive) ↓ discoverRouters(s, tempRscEnv) — refresh perRouterManifests ↓ writeRouteTypesFiles(s) — gen file rewritten (or unchanged bytes) ↓ resolveDiscoveryGate() ← workerd's pending manifest load() unblocks ↓ workerd evaluates router.tsx with the fresh gen file — reverse() works ## Tests Extends `tests/cloudflare-stress-demo/e2e/dev-vs-build-named-routes.test.ts` with a second case that covers the cold → touch → post-HMR scenario. Counts `[rsc-router] Router "X" -> N routes` log lines (fired unconditionally per discoverRouters cycle) rather than the gen-file-write log (only fires when content changes, but HMR rediscovery often writes identical bytes). Pins: - cold runtime discovery completes - settle 2.5s (vite stdout buffering on non-TTY swallows "VITE ready" so we use a fixed pause) - touch a route source file (writeFile with content+"\n", because chokidar/vite ignore mtime-only bumps in some setups) - second runtime discovery completes (the regression signal — pre-fix this never fired in cf dev, the test would time out) - process is still alive (no `Unknown route` exit) - gen file matches build byte-for-byte Process-tree cleanup and chunk-safe log scanning carried over from the cold-start parity test (detached process group, `process.kill(-pid)`, buffer-then-line-scan). ## What stayed the same Node dev path is unchanged — its `rscEnv.runner` is real, so it continues to use the existing in-process discoverRouters branch. The static-parser- then-supplement dance is preserved for any environment where `s.perRouterManifests.length === 0` (degraded mode where cold-start runtime discovery never populated state, e.g. an unrelated startup error). ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 8/8 pass Manual repro: rm gen file, pnpm dev → cold-start writes 14228 routes, touch shop-patterns.tsx → second discovery cycle logs `Router "c26945ad" -> 14219 routes (1215 static, 13004 dynamic)`, gen file stays at 14228 lines, workerd reverses `/shop/product/42`, no `Unknown route`.
ivogt
added a commit
that referenced
this pull request
Apr 26, 2026
…ediscoveries
Two race conditions on top of the previous HMR fix, both of which could
leave workerd hanging on the manifest virtual module's `s.discoveryDone`
await with no path to resolution. Surfaced by code review of the previous
commit on this branch.
## Bug 1 — beginDiscoveryGate() orphans the in-flight promise
The previous version overwrote `s.discoveryDone` and `gateResolver` on
every call. File watchers can fire multiple change/add events for a
single save (chokidar's polling mode on CI Linux, atomic-save flows that
unlink+rename instead of writing in-place). When the second event fires
beginDiscoveryGate() while workerd is already awaiting the first
promise, the resolver pointer moves to the new promise — the original is
no longer reachable, never resolves, and workerd's load() hangs forever.
Fix: track `gatePending` and make `beginDiscoveryGate()` a no-op while
pending. The same single promise covers all events that arrive within a
single rediscovery cycle. resolveDiscoveryGate() is similarly idempotent.
## Bug 2 — runtimeRediscoveryInProgress early-return orphans a fresh gate
`refreshRuntimeDiscovery()` returned immediately when a previous run was
in flight. handleRouteFileChange (which fires before the debounce) had
already reset the gate to a new pending promise — but the early-return
path didn't resolve it, and the in-flight run's `finally` only resolved
the gate it was originally tied to. Result: a second gate gets created
and never resolved.
Fix: queue a follow-up cycle instead of dropping the event. If a
rediscovery is in flight, set `runtimeRediscoveryQueued = true` and
return. The active run's `finally` consumes the queue (recurses) and
ONLY resolves the gate at the tail of the chain — when no further work
is queued. Belt-and-suspenders: the queue's outer .catch() also resolves
the gate so a thrown queued cycle can't hang.
## Order of operations (cf dev, two events overlapping)
t=0 Touch 1 fires
handleRouteFileChange → beginDiscoveryGate() (gate1 pending)
scheduleRouteRegeneration debounces 100ms
t=100 Refresh #1 begins (~700ms work)
runtimeRediscoveryInProgress = true
t=200 Touch 2 fires
handleRouteFileChange → beginDiscoveryGate() (no-op: gate1 still pending)
scheduleRouteRegeneration debounces 100ms (timer reset)
t=300 Refresh #2 called → in flight → runtimeRediscoveryQueued = true, return
(gate1 still pending — does NOT get replaced)
t=800 Refresh #1 finally
runtimeRediscoveryInProgress = false
queued = true → consume: recursive refresh()
(do NOT resolve gate1 yet)
t=800 Recursive refresh begins (~700ms)
t=1500 Recursive refresh finally
queued = false → resolveDiscoveryGate() → gate1 resolves
workerd's manifest load() unblocks against the freshly written gen
## Tests
Adds a third e2e case to cf-stress: cold start, then two source-file
edits with a 200ms gap (tuned to land the second edit during the first
refresh's ~700ms work window). Asserts:
- dev process stays alive
- gen file equals build byte-for-byte
- the queue path actually fired by matching the new rango:discovery
debug lines: "rediscovery in flight — queued for a follow-up cycle"
and "consuming queued rediscovery". Without this assertion, a
timing-collapse where both events fall in the same debounce window
would silently skip the bug surface and pass.
DEBUG=rango:discovery is set explicitly on this test's spawn so the
queue debug lines surface.
## Verification
pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo
pnpm --filter @rangojs/router exec tsc --noEmit — clean
pnpm -w run lint — 0/0
pnpm -w run format — clean
pnpm --filter cloudflare-stress-demo exec playwright test
— 9/9 pass (incl. 3 dev/build/HMR + 1 rapid-events)
Manual repro under DEBUG=rango:* with a 200ms gap between touches:
Router "c26945ad" -> 14219 routes # cold-start
hmr: rediscovery in flight — queued for a follow-up cycle
Router "c26945ad" -> 14219 routes # HMR cycle #1
hmr: consuming queued rediscovery
Router "c26945ad" -> 14219 routes # queued cycle
reverse shop.product.item42: /shop/product/42 # workerd unblocks
ivogt
added a commit
that referenced
this pull request
Apr 27, 2026
…ediscovery (#486) * fix(router): cloudflare HMR resettable discovery gate + temp-runner rediscovery Stacked on top of the cold-start fix (#485). Pre-fix, after a successful cloudflare dev cold start, any source-file change collapsed `router.named-routes.gen.ts` from the full runtime manifest back to the ~19-route static-parser view, then propagated that partial map into workerd via the manifest virtual module — `worker.rsc.tsx`'s module-level `router.reverse("shop.product.item42")` then threw `Unknown route` and killed the dev server. The mechanism had three independent contributors and the fix addresses each: 1. **Static-parser write was authoritative on HMR.** `scheduleRouteRegeneration` ran `writeCombinedRouteTypesWithTracking(s)` first; for cf-stress the static parser only sees 19 literal-named routes. The supplement helper (`supplementGenFilesWithRuntimeRoutes`) only restores `factoryOnlyPrefixes` — dot-prefixes entirely absent from static output — and cf-stress has `shop.home`, `api.benchFirst`, etc. as static names, so the `shop.`/`api.` prefixes are not factory-only and the factory siblings under those prefixes (`shop.product.item42` etc.) were dropped. Fix: when `!rscEnv?.runner && s.perRouterManifests.length > 0` (cloudflare dev with cold-start runtime state), skip the static write entirely. The runtime rediscovery below owns the gen file in this environment. 2. **Runtime rediscovery never ran in cloudflare HMR.** `refreshRuntimeDiscovery` bailed early when `rscEnv.runner` was unavailable, which is the entire cloudflare-dev case (workerd is a separate runtime). Even with #1 skipping the static write, no fresh runtime discovery would happen, so newly-added factory routes wouldn't appear until restart. Fix: add a no-main-runner branch that mirrors cold start — close+null both `prerenderTempServer` AND `prerenderNodeRegistry` (`getOrCreateTempServer`'s cached-registry short-circuit at line ~387 would otherwise keep the stale module graph), recreate via `getOrCreateTempServer()` under the `__rscRouterDiscoveryActive` flag, re-import the entry, refresh `s.perRouterManifests`, write via `writeRouteTypesFiles(s)`. ~700ms per save — acceptable, optimizable later via finer-grained module-graph invalidation. 3. **Manifest readiness gate didn't reset on HMR.** `s.discoveryDone` is the promise the manifest virtual module's `load()` awaits; once resolved at cold-start completion it stays resolved. So during HMR, any workerd reload triggered by the source change cascaded straight through the gate against whatever the current gen file said — even if rediscovery was about to overwrite it 700ms later. Fix: extract `beginDiscoveryGate()` / `resolveDiscoveryGate()` helpers that own the promise lifecycle and replace ad-hoc construction at both cold-start and HMR sites. Reset the gate IMMEDIATELY in `handleRouteFileChange` (before the 100ms debounce — refinement from review), so a workerd reload firing within the debounce window blocks on the new pending promise. Resolve the gate only after rediscovery's `writeRouteTypesFiles` completes (or in the `catch` so reloads don't hang on errors). ## Order of operations on HMR (cloudflare) source file change (chokidar) ↓ handleRouteFileChange() beginDiscoveryGate() ← new pending promise, before any async / fanout ↓ scheduleRouteRegeneration() (100ms debounce) ↓ (skip static write — owned by rediscovery) refreshRuntimeDiscovery() — cloudflare branch: close+null prerenderTempServer + prerenderNodeRegistry ↓ getOrCreateTempServer() (sets __rscRouterDiscoveryActive) ↓ discoverRouters(s, tempRscEnv) — refresh perRouterManifests ↓ writeRouteTypesFiles(s) — gen file rewritten (or unchanged bytes) ↓ resolveDiscoveryGate() ← workerd's pending manifest load() unblocks ↓ workerd evaluates router.tsx with the fresh gen file — reverse() works ## Tests Extends `tests/cloudflare-stress-demo/e2e/dev-vs-build-named-routes.test.ts` with a second case that covers the cold → touch → post-HMR scenario. Counts `[rsc-router] Router "X" -> N routes` log lines (fired unconditionally per discoverRouters cycle) rather than the gen-file-write log (only fires when content changes, but HMR rediscovery often writes identical bytes). Pins: - cold runtime discovery completes - settle 2.5s (vite stdout buffering on non-TTY swallows "VITE ready" so we use a fixed pause) - touch a route source file (writeFile with content+"\n", because chokidar/vite ignore mtime-only bumps in some setups) - second runtime discovery completes (the regression signal — pre-fix this never fired in cf dev, the test would time out) - process is still alive (no `Unknown route` exit) - gen file matches build byte-for-byte Process-tree cleanup and chunk-safe log scanning carried over from the cold-start parity test (detached process group, `process.kill(-pid)`, buffer-then-line-scan). ## What stayed the same Node dev path is unchanged — its `rscEnv.runner` is real, so it continues to use the existing in-process discoverRouters branch. The static-parser- then-supplement dance is preserved for any environment where `s.perRouterManifests.length === 0` (degraded mode where cold-start runtime discovery never populated state, e.g. an unrelated startup error). ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 8/8 pass Manual repro: rm gen file, pnpm dev → cold-start writes 14228 routes, touch shop-patterns.tsx → second discovery cycle logs `Router "c26945ad" -> 14219 routes (1215 static, 13004 dynamic)`, gen file stays at 14228 lines, workerd reverses `/shop/product/42`, no `Unknown route`. * fix(router): make HMR discovery gate idempotent + queue overlapping rediscoveries Two race conditions on top of the previous HMR fix, both of which could leave workerd hanging on the manifest virtual module's `s.discoveryDone` await with no path to resolution. Surfaced by code review of the previous commit on this branch. ## Bug 1 — beginDiscoveryGate() orphans the in-flight promise The previous version overwrote `s.discoveryDone` and `gateResolver` on every call. File watchers can fire multiple change/add events for a single save (chokidar's polling mode on CI Linux, atomic-save flows that unlink+rename instead of writing in-place). When the second event fires beginDiscoveryGate() while workerd is already awaiting the first promise, the resolver pointer moves to the new promise — the original is no longer reachable, never resolves, and workerd's load() hangs forever. Fix: track `gatePending` and make `beginDiscoveryGate()` a no-op while pending. The same single promise covers all events that arrive within a single rediscovery cycle. resolveDiscoveryGate() is similarly idempotent. ## Bug 2 — runtimeRediscoveryInProgress early-return orphans a fresh gate `refreshRuntimeDiscovery()` returned immediately when a previous run was in flight. handleRouteFileChange (which fires before the debounce) had already reset the gate to a new pending promise — but the early-return path didn't resolve it, and the in-flight run's `finally` only resolved the gate it was originally tied to. Result: a second gate gets created and never resolved. Fix: queue a follow-up cycle instead of dropping the event. If a rediscovery is in flight, set `runtimeRediscoveryQueued = true` and return. The active run's `finally` consumes the queue (recurses) and ONLY resolves the gate at the tail of the chain — when no further work is queued. Belt-and-suspenders: the queue's outer .catch() also resolves the gate so a thrown queued cycle can't hang. ## Order of operations (cf dev, two events overlapping) t=0 Touch 1 fires handleRouteFileChange → beginDiscoveryGate() (gate1 pending) scheduleRouteRegeneration debounces 100ms t=100 Refresh #1 begins (~700ms work) runtimeRediscoveryInProgress = true t=200 Touch 2 fires handleRouteFileChange → beginDiscoveryGate() (no-op: gate1 still pending) scheduleRouteRegeneration debounces 100ms (timer reset) t=300 Refresh #2 called → in flight → runtimeRediscoveryQueued = true, return (gate1 still pending — does NOT get replaced) t=800 Refresh #1 finally runtimeRediscoveryInProgress = false queued = true → consume: recursive refresh() (do NOT resolve gate1 yet) t=800 Recursive refresh begins (~700ms) t=1500 Recursive refresh finally queued = false → resolveDiscoveryGate() → gate1 resolves workerd's manifest load() unblocks against the freshly written gen ## Tests Adds a third e2e case to cf-stress: cold start, then two source-file edits with a 200ms gap (tuned to land the second edit during the first refresh's ~700ms work window). Asserts: - dev process stays alive - gen file equals build byte-for-byte - the queue path actually fired by matching the new rango:discovery debug lines: "rediscovery in flight — queued for a follow-up cycle" and "consuming queued rediscovery". Without this assertion, a timing-collapse where both events fall in the same debounce window would silently skip the bug surface and pass. DEBUG=rango:discovery is set explicitly on this test's spawn so the queue debug lines surface. ## Verification pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 9/9 pass (incl. 3 dev/build/HMR + 1 rapid-events) Manual repro under DEBUG=rango:* with a 200ms gap between touches: Router "c26945ad" -> 14219 routes # cold-start hmr: rediscovery in flight — queued for a follow-up cycle Router "c26945ad" -> 14219 routes # HMR cycle #1 hmr: consuming queued rediscovery Router "c26945ad" -> 14219 routes # queued cycle reverse shop.product.item42: /shop/product/42 # workerd unblocks * fix(router): close the HMR tail-race window — pendingRouteEvents flag Stacks on the previous commit. Code review surfaced a third state-tracking bug: the queue mechanism only handles overlap during refresh A's WORK phase, not the brief window between A's writeRouteTypesFiles and A's finally — events arriving in that window mark `gatePending=true` (no-op on the still-pending gate1) and arm a fresh debounce, but their `refreshRuntimeDiscovery()` call hasn't happened yet, so `runtimeRediscoveryQueued` is still false. A's finally then resolves gate1, and the workerd window between resolve and refresh B's start can read stale gen. ## Repro window t=0 Touch 1 → handleRouteFileChange → beginDiscoveryGate (gate1) → debounce 100ms t=100 Refresh A begins (~700ms work) t=750 Touch 2 (during A's TAIL) → handleRouteFileChange: - beginDiscoveryGate (no-op: gatePending=true) - schedule debounce, fires at t=850 queued is still false (debounce hasn't called refresh yet) t=800 Refresh A's finally: queued=false → pre-fix: RESOLVES gate1 ┌─── workerd reload window (~50ms) ───┐ │ manifest load() awaits resolved │ │ gate → returns IMMEDIATELY against │ │ the post-A gen file, which doesn't │ │ yet reflect Touch 2's change │ └─────────────────────────────────────┘ t=850 Refresh B starts — but no fresh gate to bind to ## Fix Add `pendingRouteEvents` flag, set in handleRouteFileChange at event time (alongside the gate reset). Cleared at the start of refresh's main work path (the events that triggered THIS run are now being consumed). Refresh's finally checks the flag and KEEPS the gate pending if it's true, with no need to recurse — the debounced timer that armed for the late event is still scheduled and will fire on its own. The next refresh cycle's finally then clears the flag and resolves. Three branches in the finally now: 1. queued=true → recurse (queue overlap; gate stays pending) 2. pending=true → don't resolve, don't recurse — debounce will fire and trigger the next refresh; gate stays pending 3. neither → resolve gate ## Diagnostic log lines Added two for traceability: - `hmr: discoveryDone resolved` — fires only when gatePending was true and we actually transitioned it to false. Useful for ordering assertions in tests and debug sessions. - `hmr: holding gate for pending events (debounce not yet fired)` — fires when the new pendingRouteEvents branch keeps the gate alive. ## Tests - The existing rapid-events test now uses `waitForLog` for deterministic timing: waits for `hmr: closing cached temp server` (refresh A in-flight) before writing touch 2. That guarantees we exercise the queue path. Asserts the queue debug pair AND the gate-resolution log line appears at the trailing end (proving idempotent gate behavior). - New `rapid edits near refresh tail keep gen file consistent` test: waits for `hmr writeRouteTypesFiles` (refresh A's last sync work step) before writing touch 2. Asserts dev stays alive and gen ends up matching build. Doesn't assert on the specific code path (pendingRouteEvents vs queued) because chokidar latency on macOS/ Linux/CI varies ~50–200ms and can push the event into either window; the user-visible contract (dev alive + gen correct + 3 discoveries) is what we pin. ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 10/10 pass Manual repro under DEBUG=rango:* with touches around the refresh tail: Router "c26945ad" -> 14219 routes # cold-start hmr: closing cached temp server # refresh A starts hmr: rediscovery in flight — queued for a follow-up cycle Router "c26945ad" -> 14219 routes # refresh A hmr: consuming queued rediscovery Router "c26945ad" -> 14219 routes # queued cycle hmr: discoveryDone resolved # gate held until tail reverse shop.product.item42: /shop/product/42 # workerd reads fresh gen * test(cf-stress): tighten HMR tests with route-meaningful edits + gen-file polling Two refinements from code review on the HMR/rapid-events tests: ## 1. Whitespace-only edits couldn't distinguish stale-A from fresh-B Previously both touches in the rapid-events and tail-race tests appended whitespace. Refresh A and refresh B produced byte-identical gen files, so the assertion `postGen === buildGen` would pass even if the gate had been resolved against stale A and refresh B never observed touch 2 — the test was mostly proving "dev survives two HMR cycles," not "the gate-state fix protects a route-changing edit." Touch 2 in both tests now changes `Array.from({ length: 100 }` to `{ length: 101 }` in shop-patterns.tsx, adding a `shop.product.item101` route that ONLY exists if refresh B successfully re-imports the edited source. The assertion is now `postGen.toContain('shop.product.item101')` which directly proves refresh B's output reached disk. ## 2. `discoveryDone resolved` waits matched cold-start `resolveDiscoveryGate()` logs the same line on cold-start completion and on every subsequent HMR resolve. The previous `waitForLog(...)` calls searched the whole buffer, which short-circuited on the cold-start match and didn't pin the post-HMR resolution. Switching from `waitForLog(/discoveryDone resolved/)` to `waitForGenFileContains(genFilePath, 'shop.product.item101')` is more robust on two axes: it ignores cold-start log entries entirely (the content check is a direct end-state probe), and it doesn't depend on log-line ordering (refresh A's gate resolution can race with chokidar latency — touch 2's handler sometimes fires before A's finally and sometimes after; either path leads to the same correct end state). For the in-flight `hmr: rediscovery in flight — queued for a follow-up cycle` and `hmr: closing cached temp server` markers, the existing `waitForLogFromIdx(..., preHmrIdx)` is preserved because those lines genuinely don't fire pre-HMR. ## What the tests now prove Test 1 ("rapid duplicate route-file events"): - cold-start writes 14219 routes - touch 1 (whitespace) starts refresh A; we wait for "closing cached temp server" to confirm A is in-flight - touch 2 (length 100→101) lands in queue path - assert: queue debug pair fires - assert: gen file ends up containing item101 → proves refresh B observed the edited source Test 2 ("rapid edits near refresh tail"): - same flow, but touch 2 fires after `hmr writeRouteTypesFiles` (refresh A's last sync step) → debounce can fire before/after A's finally depending on chokidar latency - assert: dev process stays alive - assert: gen file ends up containing item101 → proves refresh B observed the edited source regardless of which code path (queue vs pendingRouteEvents vs new gate) handled it Both tests now correctly distinguish stale-A from fresh-B. ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3050 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 10/10 pass Helper added: `waitForGenFileContains` polls the gen file on disk for a target substring, with a 30s timeout. Cleaner than log-line scraping because it tests the user-visible contract directly. Removed: the inline `waitForLog` helpers in both tests — replaced by `waitForLogFromIdx` (offset-anchored, defined once at module scope) for log markers and `waitForGenFileContains` for end-state assertions. * refactor(router): extract discovery gate state machine + add deterministic unit tests The gate/queue/pending state machine that keeps `s.discoveryDone` consistent across HMR fan-out is now in its own module (`./discovery/gate-state.ts`) with a focused factory function and a documented four-flag invariant. router-discovery.ts shrinks by ~80 lines; the env-specific work (close+recreate temp server, discoverRouters, writeRouteTypesFiles, propagateDiscoveryState) is the only thing it still owns — the state transitions are delegated. ## Why now Code review flagged that the cf-stress e2e tail-race test was probabilistic: > It waits for the child process to print `hmr writeRouteTypesFiles`, > then the parent writes touch 2. Since stdout delivery is async, the > dev process may already have run refresh A's `finally` before the > parent observes that log. In that common path, touch 2 creates a > normal fresh gate after A, so pre-fix code could still pass. Three options were on the table: a test-only delay between the write and the finally, asserting the `holding gate for pending events` log under that delay, or extracting the state machine into a unit test. Extraction is the longest-lived option: the state machine becomes unit-testable in isolation, the e2e tests stop carrying the burden of proving timing-sensitive invariants, and router-discovery.ts gets notably more readable. ## What's in gate-state.ts createDiscoveryGate(s, debug?) → DiscoveryGate beginGate() idempotent: reset s.discoveryDone, no-op if pending resolveGate() idempotent: settle the pending gate noteRouteEvent() set pendingEvents=true + beginGate() runRefreshCycle() manage queued/pending around the work callback state() test-only state snapshot The module's JSDoc spells out the four flags and the t=0..t=1500 timeline diagram for the cf-stress repro. Three terminal cases in the cycle's finally: - queued → recurse with the work fn (gate stays pending) - pendingEvents → hold gate (next debounced cycle resolves) - neither → resolveGate Belt-and-suspenders: the queued-cycle's recursion is fire-and-forget with a .catch that calls resolveGate, so a thrown work fn can't hang workerd indefinitely. ## Unit tests (9 cases, all deterministic) - beginGate creates a pending promise; resolveGate settles it - beginGate is a no-op when already pending — preserves the existing resolver (regression: chokidar-fired duplicate events) - resolveGate is a no-op when no gate is pending - clean run with no events resolves the gate at the tail - **noteRouteEvent during work does NOT prematurely resolve the gate** (the exact tail-race window the cf-stress e2e couldn't deterministically cover; this test drives state transitions synchronously) - overlap (queued path) — second call during in-flight is queued and consumed at tail - combined queued + pendingEvents flows through correctly - thrown work in a top-level cycle still releases the gate - thrown work in a queued cycle still releases the gate (fire-and-forget recursion's .catch belt-and-suspenders) 3050 → 3059 unit tests after this commit. ## E2E test refinements The cf-stress `waitForGenFileContains` helper now accepts an optional `getBuffer` so timeouts append the captured dev stdout/stderr — without that, gen-file-polling failures lost the most useful diagnostic (reviewer flagged this on the previous round). Both call sites updated to pass `() => buffer`. The two tail-race / rapid-events e2e tests are kept (they prove the integrated path under real chokidar/Vite/workerd timing) but the gate-state invariants are now pinned by the unit tests above. The e2es' role is "the wired-up plumbing works"; the unit tests' role is "the state machine cannot regress." ## What didn't change - Public surface of router-discovery.ts (the gate is still exposed via `s.discoveryDone` for the manifest virtual module's load() hook). - HMR semantics, debug log lines, or the env-specific work paths. - Cold-start: still uses beginGate + setTimeout(discover, 0) + resolveGate. ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3059 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 10/10 pass * docs(internal): list gate-state.ts in the Vite plugin ecosystem feature map Per the repo convention (each new production source file should appear in the internal feature/file ownership map), `vite/discovery/gate-state.ts` gets added to the "Vite plugin ecosystem" row. The note column also calls out the four-flag state machine and that it's owned by `gate-state.ts` and consumed by `router-discovery.ts`, so future maintainers can find the contract docs (and the unit tests) by following the file reference. Pure docs change — no code or tests affected. * refactor(router): cf HMR uses module-graph invalidation instead of close+recreate Closes the first-edit transport-disconnect window the user observed and hardens the temp-server refresh against staleness via direct e2e proof. ## Why Pre-fix HMR rediscovery did `prerenderTempServer.close() + createTempRscServer(...)` — closing one Vite instance and creating another while the main dev server's transport to workerd was still mid-reconnect (workerd's first post-cold-start module-fetch window). The user-visible symptom was, on the FIRST edit of a top-of-graph file like `urls.tsx`: [vite] An error happened during full reload transport was disconnected, cannot call "fetchModule" at /packages/rangojs-router/src/router/manifest.ts:9:31 at /packages/rangojs-router/src/router/match-api.ts:10:31 The system recovered (workerd reconnected, retried, eventually rendered the right routes), but the error in the log was alarming and pointed at real transport disruption. Subsequent edits were fine because workerd's module cache was warm by then — its eval finished before our close() ran. ## What changed `vite/router-discovery.ts`: 1. Extracted `importEntryAndRegistry(tempRscEnv)` — owns the `__rscRouterDiscoveryActive` flag handshake + entry import + RouterRegistry read. Used by both cold-start (`getOrCreateTempServer`) and HMR (`refreshTempRscEnv`). 2. Added `refreshTempRscEnv()` — keep the same temp Vite instance alive across HMR cycles. Steps: a. Ensure server exists (create on first call, reuse otherwise). b. Pick the module graph: env-level (Vite 6+ environments API, where the module-runner cache lives) preferred; server-level fallback. If neither exposes `invalidateAll()`, fall back to close+recreate so we never lose the freshness contract. c. Invalidate the chosen graph. d. Drop cached `prerenderNodeRegistry`. e. `importEntryAndRegistry` re-fetches source through the now-invalidated runner, refreshes registry. 3. `refreshRuntimeDiscovery`'s cloudflare branch now calls `refreshTempRscEnv()` instead of close+recreate. The same Vite instance staying alive means workerd's transport is never torn down at HMR time. Verified manually: edited urls.tsx multiple times from cold start, 0 `transport was disconnected` errors. Add / remove / rename of factory routes all flow through correctly (env-level `invalidateAll()` actually re-reads source on disk — proven by the remove case below). ## Tests Three new e2e cases on `dev-vs-build-named-routes.test.ts`: - **HMR remove factory route — invalidation drops it from gen**: bumps `length: 100 → 101` (item101 appears) then back to `100` (item101 must disappear). If the runner served cached source on the second edit, the gen file would still contain item101 and the `waitForGenFileMissing` would time out. Pre-fix close+recreate handled this correctly because the new Vite instance had no cache; with module-graph invalidation we have to prove it actively re-reads disk on each cycle. - **HMR rename factory route — old names disappear, new appear**: rename `name: \`item${i + 1}\`` → `\`product${i + 1}\``. Asserts `product1` appears AND `item42` disappears. Catches "additive" staleness (where both versions persist) and "no-update" staleness (where only old persists). - **first urls.tsx edit after cold start does NOT log `transport was disconnected`**: directly pins the absence of the error string in the post-edit buffer slice. Also warns (not fails) if the close+recreate fallback path was taken instead of invalidation, so an API drift in Vite's `moduleGraph.invalidateAll` would surface in test logs without hard-failing the suite. The existing `rapid duplicate route-file events` test was updated to wait for the new `refreshTempRscEnv: invalidating module graph` log line instead of the now-removed `hmr: closing cached temp server`. Tests also added a `runDevAndWaitForRediscovery` shared helper to deduplicate the spawn+watcher+cold-discovery scaffolding across the three new cases (eliminates ~120 lines of repetition). ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router run test:unit — 3059 pass / 3 todo pnpm -w run lint — 0/0 pnpm -w run format — clean pnpm --filter cloudflare-stress-demo exec playwright test — 13/13 pass Manual repro under DEBUG=rango:* — multiple urls.tsx edits from cold start, all clean: refreshTempRscEnv: invalidating module graph (env) hmr re-discovery done (756ms) Router "c26945ad" -> 14219 routes (1215 static, 13004 dynamic) reverse shop.product.item42: /shop/product/42 # ... no `transport was disconnected` ... Plus add (100→101 → 14220 routes, item101 in gen) and remove (101→100 → 14219 routes, item101 gone). * fix(router): close two cf HMR lifecycle edges (orphan temp server + ghost router registry) Reviewer flagged two lifecycle edges in the new module-graph invalidation path: ## 1. Persistent temp server retains removed routers in RouterRegistry `createRouter()` does `RouterRegistry.set(routerId, router)` on a package-level singleton Map. `moduleGraph.invalidateAll()` clears Vite's module cache but NOT this Map. For an edit that removes an entire `createRouter()` call or changes a router id, the OLD entry persists in the registry alongside whatever the freshly re-imported source declares — discoverRouters then emits ghost routes from the removed/renamed router. Fix: new `clearTempRegistries(tempRscEnv)` helper that imports `@rangojs/router/server` through the runner (same module instance the re-imported entry will populate) and calls `.clear()` on both `RouterRegistry` and `HostRouterRegistry`. Defensive: optional-chained `.clear?.()` so the helper survives a non-Map registry shape. Wired into `refreshTempRscEnv` between `target.invalidateAll()` and `importEntryAndRegistry()`. The cf-stress fixture only has one router so the multi-router scenario isn't directly exercised — but the existing "HMR remove factory route" test now asserts the new `clearTempRegistries: cleared RouterRegistry + HostRouterRegistry` debug log fires post-edit, pinning the lifecycle code path. ## 2. Failed refresh leaves an orphaned temp server `getOrCreateTempServer()` used `prerenderNodeRegistry` as the cache sentinel. `refreshTempRscEnv()` flow was: 1. getOrCreateTempServer (returns existing env) 2. invalidateAll 3. prerenderNodeRegistry = null 4. importEntryAndRegistry — IF THIS THROWS …the previous `prerenderTempServer` was still alive but with no registry. The next `getOrCreateTempServer()` call saw no registry, fell through to the create path, and overwrote the handle — the previous server became orphaned (still alive, no references, never closed). Fix: `getOrCreateTempServer()` now checks for an existing temp server FIRST. Three branches: - server alive + registry cached → return immediately (steady-state) - server alive + registry missing → re-import on the existing runner; on success, return; on failure, close+null the orphan and fall through to create-new - server alive but rsc.runner missing → close+null, create new - no server → create new This guarantees we never silently leak a temp Vite instance after a failed refresh. ## What didn't change - `refreshTempRscEnv`'s happy path still goes invalidateAll → clearTempRegistries → importEntryAndRegistry. Both lifecycle fixes layer cleanly on top. - close+recreate fallback is still present for Vite versions where neither env nor server graph exposes `invalidateAll()`. - Node dev path is unchanged. ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter cloudflare-stress-demo exec playwright test — 13/13 pass pnpm --filter @rangojs/router run test:unit — 3059 pass / 3 todo pnpm -w run lint / format — clean Manual repro under `DEBUG=rango:* pnpm dev` confirms the lifecycle log fires post-edit: refreshTempRscEnv: invalidating module graph (env) clearTempRegistries: cleared RouterRegistry + HostRouterRegistry hmr re-discovery done (619ms) ## Follow-up not in this PR The reviewer also suggested an e2e test for the multi-router / removed-router scenario specifically. The cf-stress fixture only declares one router, so adding that case requires a separate fixture app (or a temporary second `createRouter()` in cf-stress that the test harness adds and removes). Tracked as future work; for this PR the lifecycle code path is pinned by the debug-log assertion. * fix(router): cold-start resolveGate must defer when HMR cycle is in flight ultrareview caught a real race I missed in the gate state machine: beginDiscoveryGate(); setTimeout(() => discover().then(resolveDiscoveryGate, resolveDiscoveryGate), 0); `resolveGate` only checked `gatePending`, ignoring `inProgress` / `pendingEvents`. If a user edits a route file between `discoverRouters` committing `s.perRouterManifests` and cold-start's `discover()` returning: 1. handleRouteFileChange → noteRouteEvent (pendingEvents=true) 2. 100ms debounce fires → runRefreshCycle → inProgress=true, work starts 3. cold-start's discover() resolves 4. its `.then(resolveGate)` fires → naively settles s.discoveryDone 5. workerd's manifest virtual module load() unblocks against the stale cold-start gen file (HMR cycle's writeRouteTypesFiles hasn't run yet) The new e2e suite's 2.5s settle pause masked it — every existing test waited until cold-start was fully past the window before editing. ## Fix `resolveGate` now defers when ANY in-flight signal is set (`inProgress | queued | pendingEvents`). The runRefreshCycle's `finally` is the canonical resolver — it calls resolveGate at the tail when all work is drained. Cold-start's resolveGate becomes a no-op if it fires while a cycle is in progress; the cycle's finally handles resolution. ```ts const resolveGate = (): void => { if (!gatePending) return; if (inProgress || queued || pendingEvents) { debug?.("hmr: resolveGate deferred — work in flight ..."); return; } gatePending = false; debug?.("hmr: discoveryDone resolved"); gateResolver(); }; ``` ## Why this over routing cold-start through runRefreshCycle The reviewer's suggested fix was to make cold-start use `runRefreshCycle(discover)`, so cold-start participates in the same queue contract. That's architecturally cleaner, but has a subtle correctness issue: when the queue recurses, it re-runs the SAME work fn — for cold-start that's `discover()`, which uses `getOrCreateTempServer()` (no module-graph invalidation). The HMR work fn that triggered the queue uses `refreshTempRscEnv()` (with invalidation). Reusing cold-start's work would skip invalidation and re-introduce stale-source risk. The state-aware resolveGate is more surgical: - cold-start retains its raw beginGate/discover/resolveGate lifecycle - resolveGate becomes a no-op when work is queued, so cold-start can't prematurely settle the gate - HMR's runRefreshCycle is the canonical resolver, with the correct (invalidating) work fn Same correctness, no work-fn ownership question. ## Tests Two new gate-state unit tests cover both shapes: - **resolveGate is deferred when runRefreshCycle is in flight (cold-start race)**: beginGate → noteRouteEvent → runRefreshCycle (work pending) → resolveGate fires → assert gate STILL pending → finish work → cycle's finally resolves. - **resolveGate is deferred when pendingEvents is true**: beginGate → noteRouteEvent → resolveGate fires BEFORE debounce expires → assert deferred → debounce → cycle resolves. Verified by temporarily reverting the guard: both new tests fail. With guard restored: 11/11 pass. ## Verification pnpm --filter @rangojs/router exec tsc --noEmit — clean pnpm --filter @rangojs/router exec vitest run gate-state.test.ts — 11/11 (+2 new) pnpm --filter @rangojs/router run test:unit — 3061 pass / 3 todo pnpm --filter cloudflare-stress-demo exec playwright test — 13/13 pass pnpm -w run lint / format — clean ## Note on the 2.5s settle The reviewer also suggested dropping the 2.5s settle pause in dev-vs-build-named-routes.test.ts now that the gate handles the race. Keeping it for now — the settle also covers chokidar watcher-readiness and vite's full-reload fan-out, which are independent concerns from the gate. Reducing it can land in a follow-up after observing CI stability across a few runs. * chore: re-trigger CI * fix(router): suppress duplicate cf HMR cycle from gen-file self-write Every cf HMR cycle was producing two `[vite] program reload` events instead of one — the second came from vite's chokidar detecting the gen-file write that `refreshTempRscEnv → writeRouteTypesFiles` performs at the tail of the cycle. The chokidar watcher was already short-circuited via `consumeSelfGenWrite` in our own `handleRouteFileChange`, but vite's independent HMR pipeline still processed the change and re-evaluated importers (visible client-side as a duplicate "HMR: version changed" event). Add a `handleHotUpdate` plugin hook that returns `[]` when the changed file is in `selfWrittenGenFiles`, dropping the cascade. Use a new non-consuming `peekSelfGenWrite` so the chokidar handler that fires later can still consume the same entry. The lookup is keyed by absolute path, not filename, so it works for any configured gen-file naming scheme — no hardcoded suffix list.
This was referenced May 30, 2026
Merged
ivogt
added a commit
that referenced
this pull request
Jun 5, 2026
…rvability)
Review-driven hardening of the cache-tag error path. Three substantive fixes
plus consistency/typing nits, all backed by regression tests.
Correctness / availability:
- kvGetOrEvict no longer deletes a still-good cross-colo KV entry on a transient
outage. It read via kv.get(key, {type:"json"}), which fused the network read
and the JSON parse, so a 5xx/429/network blip was treated as corruption and
evicted the entry (self-inflicted cross-colo miss storm). Now reads as text:
a transient error propagates to the caller's outer catch (reported cache-read,
entry intact); only a JSON.parse/validate failure is true corruption -> evict.
- kvGetResponse: the full Response reconstruction (base64 + headers + status) is
now inside one corrupt-guard, so malformed `hd`/out-of-range `st` self-heals
too; eviction goes through evictKvKey (reports cache-delete instead of silently
swallowing the delete failure).
- Reserved-namespace guard: a misconfigured store keyGenerator returning a key
under a reserved tag-marker prefix - `__tag__/` (KV marker key) or
`__tagmarker__/` (L1 Cache API marker request) - can no longer clobber (on set)
or evict (on get/delete) a live tag marker. The segment op is a reported
no-op/miss, naming which prefix collided.
Observability:
- revalidateTag now routes a failed background invalidation through reportingAsync
(loud log + onError, category cache-invalidate) instead of swallowing it; the
JSDoc claim is now true. Passes the captured ctx since the run is detached.
- The stale-while-revalidate ("use cache") failure goes through reportCacheError
(loud) with the captured ctx, and `stale-revalidation` is now a typed category.
- reportCacheError/reportingAsync accept an explicit ctx for detached waitUntil
tasks where the ALS context is gone, so onError still fires. The CF store's own
background writes deliberately omit it (request-agnostic singleton -> console
loud only); a comment documents the asymmetry.
- CacheErrorCategory is exported from `@rangojs/router/cache`, the
_reportBackgroundError signature is typed to it, and onError's metadata.category
is documented.
- cache-scope segment-write failure now uses reportCacheError, not console.error.
#2 (L1 document body served verbatim): documented as a deliberate asymmetry -
hashing the body would require buffering the whole document on every hit,
defeating streaming; integrity relies on the Cache API, and the KV tier (the real
partial-read vector) is structurally validated.
Tests: KV transient-read -> miss WITHOUT eviction (#1 guard) + L1 transient
non-eviction assertion; per-tag L1 marker write-through fan-out; reserved-prefix
get/set/delete for both __tag__/ and __tagmarker__/; revalidateTag failure ->
onError; non-CF (CacheScope) corrupt segment self-heal.
This was referenced Jun 8, 2026
ivogt
added a commit
that referenced
this pull request
Jun 11, 2026
…534) * feat(cache): tag invalidation — cacheTag / updateTag / revalidateTag Adds runtime cache-tag invalidation across the in-memory and Cloudflare cache stores. Public API (server-only, from @rangojs/router): - cacheTag(...tags): tag the current "use cache" entry at runtime. - updateTag(...tags): awaitable, read-your-own-writes invalidation for Server Actions (resolves after in-process invalidation completes; on CFCacheStore, rejects if a durable KV marker write fails instead of reporting a false success). - revalidateTag(...tags): fire-and-forget background invalidation for route handlers / webhooks. Like updateTag it hard-purges; the only difference is awaitability (it is NOT stale-while-revalidate). Tags attach via three axes: runtime cacheTag() inside "use cache", cache({ tags }) on the route DSL, and loader cache tags. A per-handler explicit-store registry (LRU-bounded) lets updateTag/revalidateTag reach every configured store, including per-scope cache({ store }) boundaries. Stores: - MemorySegmentCacheStore: forward + reverse tag index; eager delete on invalidate. - CFCacheStore: per-tag KV markers compared against an entry's taggedAt; optional per-colo L1 marker cache (tagCacheTtl) and onRevalidateTag CDN purge hook; tags encodeURIComponent'd so commas / non-Latin1 stay header-safe; over-512-byte marker keys and failed KV writes surface as a rejected updateTag rather than silent stale-serve. Document caching: createDocumentCacheMiddleware tags full-page entries with the request's resolved tags, snapshotted after the render stream drains so Suspense-streamed cacheTag()/"use cache" tags are captured, so updateTag/revalidateTag invalidate cached pages. Docs and skills updated. e2e covers cacheTag / cache(tags) / updateTag / revalidateTag and document invalidation in dev and production on both the memory and the CF + KV stores. Known limitation: combining whole-page document caching with segment cache({ tags }) DSL tags can miss the document-level tag (resolved in the deferred cacheRoute waitUntil); the segment entry itself still invalidates. * perf+docs(cache): parallelize L1 marker write-through; document revalidateTag/keyGenerator contracts From a 4-dimension (correctness/performance/consistency/security) review of the branch: - perf: invalidateTags wrote the per-colo L1 tag markers sequentially (await per tag inside the loop, only when tagCacheTtl>0). The memo write stays synchronous (read-your-own-writes); the independent L1 Cache API writes now fan out via Promise.all instead of stalling on each other. - docs (security): revalidateTag JSDoc now states that, being fire-and-forget, a failed durable marker write is NOT surfaced to the caller (logged only) - use await updateTag() when the invalidation must be confirmed/retried. - docs (security): CFCacheStore.keyGenerator now documents the reserved __tag__/ and __tagmarker__/ prefixes a returned key must not collide with (tag markers share the data KV namespace). The review's other confirmed items were verified as deliberate, documented trade-offs and intentionally not changed: read-side isGloballyInvalidated fail-open (availability over a KV-outage stampede), unbounded tag cardinality and the marker/data key-collision (documented; key-prefix hard-namespacing would invalidate existing caches), and the segment-DSL document-tag race (documented caveat). Consistency + most security probes verified clean (naming, normalization symmetry, header encode/decode, no injection/poisoning). * fix(cache): report cache errors via onError + self-heal corrupt entries Audited every cache read/write/parse path (67 sites). The resilience contract is now: report loudly via the router's onError hook, degrade (never throw), and evict corrupt/partial entries. - onError routing: a new reportCacheError(error, category) helper logs (loud, works in background tasks) AND routes through the request context's _reportBackgroundError -> router onError (deduped). Both stores used only console.error before, so a consumer's onError never saw cache read/write failures. All store + document-cache + cache-runtime/scope sites now route through it with a category (cache-read/-write/-delete/-corrupt/-invalidate). The non-blocking L1 write (waitUntil) previously swallowed its rejection entirely; it now reports too. - Self-heal on corrupt/partial: CF reads distinguish a PARSE failure (truncated Cache API body, malformed KV envelope, bad base64) from a transient infra error. On corruption the faulty entry is EVICTED (cache.delete / kv.delete) and reported as cache-corrupt, so subsequent reads miss + re-render instead of re-failing until TTL; a transient infra error degrades to a miss WITHOUT deleting a still-good entry. New parseOrEvict / kvGetOrEvict helpers + envelope shape validation. cache-runtime + cache-scope report corrupt deserializes (and cache-scope evicts; use-cache items self-heal via the miss-path overwrite). - Degrade hardening: memory putResponse's un-guarded arrayBuffer() now degrades to a no-op instead of throwing. The deliberate exception is unchanged: updateTag -> invalidateTags still REJECTS on a failed durable marker write (read-your-own-writes honesty), not swallowed. Tests: infra read error -> reports cache-read + null; corrupt L1 entry -> evicted + cache-corrupt; corrupt KV envelope -> kv.delete + cache-corrupt; write error -> cache-write. * fix(cache): harden cache-error resilience (transient vs corrupt, observability) Review-driven hardening of the cache-tag error path. Three substantive fixes plus consistency/typing nits, all backed by regression tests. Correctness / availability: - kvGetOrEvict no longer deletes a still-good cross-colo KV entry on a transient outage. It read via kv.get(key, {type:"json"}), which fused the network read and the JSON parse, so a 5xx/429/network blip was treated as corruption and evicted the entry (self-inflicted cross-colo miss storm). Now reads as text: a transient error propagates to the caller's outer catch (reported cache-read, entry intact); only a JSON.parse/validate failure is true corruption -> evict. - kvGetResponse: the full Response reconstruction (base64 + headers + status) is now inside one corrupt-guard, so malformed `hd`/out-of-range `st` self-heals too; eviction goes through evictKvKey (reports cache-delete instead of silently swallowing the delete failure). - Reserved-namespace guard: a misconfigured store keyGenerator returning a key under a reserved tag-marker prefix - `__tag__/` (KV marker key) or `__tagmarker__/` (L1 Cache API marker request) - can no longer clobber (on set) or evict (on get/delete) a live tag marker. The segment op is a reported no-op/miss, naming which prefix collided. Observability: - revalidateTag now routes a failed background invalidation through reportingAsync (loud log + onError, category cache-invalidate) instead of swallowing it; the JSDoc claim is now true. Passes the captured ctx since the run is detached. - The stale-while-revalidate ("use cache") failure goes through reportCacheError (loud) with the captured ctx, and `stale-revalidation` is now a typed category. - reportCacheError/reportingAsync accept an explicit ctx for detached waitUntil tasks where the ALS context is gone, so onError still fires. The CF store's own background writes deliberately omit it (request-agnostic singleton -> console loud only); a comment documents the asymmetry. - CacheErrorCategory is exported from `@rangojs/router/cache`, the _reportBackgroundError signature is typed to it, and onError's metadata.category is documented. - cache-scope segment-write failure now uses reportCacheError, not console.error. #2 (L1 document body served verbatim): documented as a deliberate asymmetry - hashing the body would require buffering the whole document on every hit, defeating streaming; integrity relies on the Cache API, and the KV tier (the real partial-read vector) is structurally validated. Tests: KV transient-read -> miss WITHOUT eviction (#1 guard) + L1 transient non-eviction assertion; per-tag L1 marker write-through fan-out; reserved-prefix get/set/delete for both __tag__/ and __tagmarker__/; revalidateTag failure -> onError; non-CF (CacheScope) corrupt segment self-heal. * fix(cache): address tag-invalidation review findings - record cached entry tags into the request tag union on segment-cache hits, so a document re-cached over a segment hit stays tag-invalidatable; narrow the document-cache caveat to the first-write waitUntil race - gate the invalidateTags memo/L1 marker write-through on KV (markers are never read without it) and update the stale constructor comment - warn with the real cause when updateTag/revalidateTag run outside a request context (queue consumer / cron) instead of the store-config message - reword revalidateTag as background hard-purge (not SWR) in consumer JSDoc, the export comment, and the unit test title - annotate URL-param tag e2e fixtures as test-only (untrusted-input warning) - unit tests pin hit-path tag recording, the no-KV no-op, and the no-context warnings; unique namespace for the no-KV warn fixture * fix(cache): adapt cache-tag e2e to bare-T JSON responses (#539) The cache-tag e2e helpers read (await res.json()).data.ts from path.json routes, but #539 (bare-T JSON responses) removed the { data } envelope: path.json now serializes the handler's return value verbatim. The tagged routes return a bare { ts, id }, so .data was undefined and reading .ts threw "Cannot read properties of undefined (reading 'ts')". This broke the cache-tag invalidation suites in four places (router dev + production, cloudflare-basic dev + production). On the rangojs-router shards the failing test's 8s expect.poll retries also destabilized the shard, cascading into is-action and cache-loader-null timeouts; fixing the root read clears those too. - e2e/cache.test.ts: pollTs reads .ts (dev + production helpers) - cloudflare-basic/e2e/cache-tag.test.ts: ts() reads .ts - urls/cache-tag.tsx: correct the stale { data } envelope comment - docs/internal/feature-map.md: oxfmt table padding Verified locally: router dev cache-tag (6 passed), router production cache-tag + is-action + cache-loader-null (9 passed), unit (3501), typecheck, lint, format. * harden(cache): fold post-reconcile review findings into CFCacheStore Adversarial review of the #534<->#558 reconcile surfaced ten findings; this folds the worthwhile ones in before the single CI run (cheaper than a second follow-up round). Read-your-own-writes / correctness: - fetchTagMarker no longer clobbers a concurrent invalidateTags() memo write that lands mid-read, so a parallel-slot updateTag() stays read-your-own-writes. - Per-store isolation of the tag-marker memo/inflight maps: two stores in one request (app store + a cache({ store }) override on a different KV binding) can no longer cross-pollute marker values. - kvGetOrEvict guards a KV body that parses to null/a primitive BEFORE validate(), so 'null'/'42' are evicted as corrupt instead of throwing past the validator into a transient cache-read that never self-heals. - Non-array tags fail safe (Array.isArray at the write guards and the central isGloballyInvalidated check) instead of throwing .map on every read. Fail-safe reads/writes (no perf degradation): - Corrupt-entry KV eviction is dispatched via waitUntil, never awaited on the read path (an unbounded kv.delete would re-add exactly the stall the read budgets exist to prevent). - The transient L1 tag-marker match error is reported as cache-read like the data read paths, instead of being silently discarded at the destructuring. - The L1 marker write-through failure on invalidate is surfaced (cache-invalidate) and the stale marker evicted, instead of swallowed while updateTag reports success. DX / hygiene: - tagInvalidationTtl below KV's 60s expirationTtl floor is raised (with a one-time warning) so a too-small value no longer makes EVERY invalidation throw; a non-finite tagCacheTtl is coerced to 0 (disabled) rather than silently enabling an invalid max-age=Infinity marker. - Remove the now-dead parseOrEvict (replaced during the reconcile by readJsonWithTimeout/healCorruptL1 + kvGetOrEvict). Observability + API completeness (review below-the-cut items): - New `markerMs` debug field: the tagged-read marker-resolution tail (memo -> L1 marker cache -> KV) is no longer invisible in CFCacheReadDebugEvent (measured only under debug). New `match-error` debug outcome so a match() rejection is distinct from a genuine l1-miss and agrees with the cache-read on onError. - Export CacheItemOptions / CacheItemResult from ./cache so a custom SegmentCacheStore author can name every type its getItem/setItem methods use. Docs/comments corrected: caching skill corrupt-entry self-heal table (serves from L2, evicts only when no L2 copy), healCorruptL1 cross-request timeout note, the tag-invalidated tier-divergence comment, and a tagCacheTtl same-colo race caveat. Tests: +15 (11 proven red-before-green); strengthened the corrupt-L1 heal-by-overwrite and fail-open assertions to be non-vacuous; the fail-open mock keys on the exported TAG_MARKER_PREFIX so a prefix rename can't make it pass vacuously; added real-timer teardown to both CF suites. Unit 3562 green; cache-tag e2e green dev+production against the real CFCacheStore. * chore(cache): raise CFCacheStore default kvReadTimeoutMs 50 -> 170 [skip ci] The L2/KV read budget defaulted to 50ms, right at the documented healthy read, which false-misses legitimate latency tails (larger payloads, far-from-colo regions) into a full render. Raise the default to 170ms: a few multiples above the ~50ms healthy read for headroom, still well under a degraded namespace's multi-second tail. Per-store override (kvReadTimeoutMs) and the <=0-disables contract are unchanged; the tag-marker KV reads inherit the same budget. Unit tests reference the KV_READ_TIMEOUT_MS constant (not a literal 50) so they adapt; the explicit-override tests (200, 0) are independent. SKILL default table and the option JSDoc updated to 170. * docs(cache): document markerMs/match-error debug surface + tag-marker read-latency rationale [skip ci] - skills/caching/SKILL.md: bring the debug outcome list current with the full CFCacheReadDebugEvent union; document the markerMs marker-resolution diagnostic and match-error (distinct from l1-miss). - docs/internal/feature-map.md: add match-error + markerMs to the ./cache debug-sink description. - docs/design/cache-tags-flow.md: add a Tag-marker read latency section - measured baseline (1-6ms warm / ~73ms cold), why an in-isolate marker cache is rejected, and the tagCacheTtl -> marker-parallel ladder if latency must drop. Docs-only; the markerMs field and match-error outcome shipped in bcb8477. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
This was referenced Jun 13, 2026
ivogt
added a commit
that referenced
this pull request
Jun 13, 2026
… decisions, #570 cleanup (#573) * test(router): dual-mode e2e for prerender+loading()+passthrough (M16) and redirect under loading() (M9) Adds the dev + production e2e fixtures the M16/M9 unit-tested fixes still lacked (CLAUDE.md mandates dual-mode coverage). Deferred from the stabilization PR; addresses the two main gaps in #567. - prerender-loading: Prerender + loading() + ctx.passthrough(). The loading() boundary defers the build handler so the passthrough sentinel arrives Promise-wrapped; detectPrerenderPassthrough awaits it. getParams lists [baked, skip]: baked is prerendered, skip passes through to the live handler. Production asserts the manifest holds baked but NOT skip (M16 regression guard: count === 1; without the fix skip would bake a corrupt artifact). - loading-redirect: a synchronous redirect() return under loading() short-circuits to a real HTTP redirect. Verified for hard + soft navigation in both modes. Regenerated router.named-routes.gen.ts for the two new includes (urls.gen.ts is legacy/unreferenced and no longer auto-generated, so left untouched). Lower-priority #567 gaps (build-time intercept _currentSegmentId handle drop, server-action-bridge pre-dispatch over-invalidation, document-cache private+s-maxage) need fixes first and are deferred; #567 stays open for them. * chore(router): safe cleanup batch from review (#570) Internal-only, behavior-preserving cleanups; grep-verified safe and gate-green (typecheck, full unit suite, lint, format). - Remove write-only MiddlewareEntry.mountPrefix (read nowhere; scoping folds into fullPattern before the entry is built). Dropped 3 production assignments plus the test fixtures. - Dedupe cookie helpers: delete the test-only middleware-cookies.ts (byte-for-byte dup of request-context's parseCookiesFromHeader/serializeCookieValue), export the canonical copies from request-context for the unit tests, drop the middleware.ts re-export. - Delete the zero-importer src/handles/index.ts barrel (Meta/MetaTags/Breadcrumbs are exported directly from the client/index entries); repoint collect-handle.test. - Drop the MiddlewareCollectableEntry re-export from middleware.ts; its one importer now imports from middleware-types. - Extract hasContextVars() into context-var.ts and use it in the two prerender entry points that duplicated the keys-or-symbols predicate. - renderStaticSegment: throw a TypeError when a Static handler returns a Response (was silently serialized as a corrupt build artifact); the fresh/revalidation paths already throw via handleHandlerResult. Adds a unit test. - ctx.reverse() middleware fallback: widen the signature to (name, params?, search?) to match ScopedReverseFunction and make the error message accurate. - docs: tree-structure.md inTransitionScope also covers error/notFound segments. Deferred, still open in #570: the optionalParams all-or-nothing removal and the loaderPromises param removal both sit on matrix-critical matching/segment-resolution paths and warrant a semantic-matrix run rather than a cleanup sweep. The document-cache "timing-gap caveat" cited at document-cache.ts:45-53 does not exist there (those lines are Cache-Control parsing), so that doc item is a no-op. * feat(router): pre-1.0 public-API surface decisions from review (#569 part 1) Type/naming/export-surface decisions (no behavior change). See #569. - #1 router.use(pattern, mw): add a pattern-generic overload so `.use("/users/:id", (ctx) => ...)` types `ctx.params.id` via ExtractParams. Bare `.use(mw)` stays generic. Added on both Rango and RangoInternal. Pins it with a type-level test (router-use-types.test.ts). - #2 useAction: export a stable public `ActionState` (and `ActionLifecycleState`) from ./client so consumers can name the hook's return type; TrackedActionState stays the @internal impl name (ActionState aliases it). - #4 buildEnv: move off the shared base onto per-preset interfaces. Cloudflare keeps `"auto"` (wrangler getPlatformProxy); Node now rejects `"auto"` at the type level (Exclude<BuildEnvOption,"auto">) instead of only at build time. - #5 poke plugin: rename name "vite-plugin-poke" -> "@rangojs/router:poke" so it matches the @rangojs/router:* family for consumers ordering/filtering by name. - #6 ./build: move realm-crossing pipeline internals (buildRouteTrie, buildPerRouterTrie, TrieNode, TrieLeaf, collectFallbackClientRefs) to a new ./build/internal subpath; ./build keeps the documented public API (generateManifest*, route-type generation, createScanFilter, hashParams). - #8 mergeHandlerUse: document the intentional validation asymmetry (explicit use() items bypass the mount-site allow-list); chosen over enforcing it. Deferred to part 2 (behavior changes): #3 prefetch options, #7 scanFilter wiring, #9 profile-registry trim, #10 redirect-in-action idiom + examples. * feat(router): prefetch source-scope option + redirect-in-action ergonomics (#569 part 2a) - #3 router.prefetch(url, { key: ":source" }): add an options arg so imperative prefetch can source-scope its cache entry, at parity with <Link prefetchKey=":source">. Wired through to prefetchDirect. - #10 redirect() in actions: drop the `as any` on non-form actions by typing them Promise<Response> (saveAndRedirect, actionSimpleRedirect). The form-bound peReturnRedirect keeps a documented intrinsic cast (React's form-action type is () => void | Promise<void>, which can't express a Response return); consumers should prefer `throw redirect(...)`. Strengthened the server-actions skill. Deferred to part 2b: #7 (complete scanFilter — needs a new public plugin option) and #9 (trim profile-registry — runtime cache-resolution refactor). * feat(router): complete scanFilter discovery option + trim profile registry (#569 part 2b) - #7 complete the scanFilter feature. Adds a public rango() option `discovery: { include, exclude }` (glob, root-relative; mirrors the CLI's --include/--exclude). The consume side was already wired (findRouterFiles(state.projectRoot, state.scanFilter) at 3 sites); this threads the option through PluginOptions and compiles it via createScanFilter into state.scanFilter in configResolved, once projectRoot is known. Filters which files route discovery / static type-gen scan. Tests in generate-route-types.test.ts (include restricts, exclude removes). - #9 trim the profile registry. Removed the global mutable `_profiles`, `setCacheProfiles`, and `getCacheProfile` — all dead: `getCacheProfile` had no production reader (its own JSDoc said so), and `setCacheProfiles` only populated the unread global. Kept the load-bearing `resolveCacheProfiles`, which createRouter() already uses to thread a per-request profile map (the "use cache: <profile>" path reads it request-scoped via ctx._cacheProfiles). Dropped the router.ts setCacheProfiles() call; repointed the tests to resolveCacheProfiles (and reframed the "global replace" test as per-router map independence). #569 is now fully implemented (all 10 decisions). * fix(router): keep build internals on ./build (revert #569.6) + throw in return-form redirect actions (#569.10) CI on #573 caught a regression from #569 part 1: the Vite discovery pipeline imports buildRouteTrie / buildPerRouterTrie / collectFallbackClientRefs at build time via runner.import("@rangojs/router/build") (discover-routers.ts), and the RSC runner does not resolve a ./build/internal subpath in that realm. Moving them left collectFallbackClientRefs undefined, so the router-level default error / notFound boundary was never collected into the app-fallback chunk (mini.test.ts clientChunks: "mini-default-error must be in app-fallback"). Revert #569.6: restore buildRouteTrie/buildPerRouterTrie/collectFallbackClientRefs + TrieNode/TrieLeaf on ./build; delete ./build/internal + its package.json export. #569.6 is re-deferred — internalizing these needs runner subpath resolution or a different seam (noted in build/index.ts). #569.10 revision: per owner, the return-form redirect test actions (saveAndRedirect, actionSimpleRedirect) now `throw redirect()` instead of being typed Promise<Response>. The form-bound peReturnRedirect keeps its documented intrinsic cast. Verified: mini clientChunks 6/6, location-state e2e 47/47 (dev+prod), typecheck, 3866 unit, lint, format.
This was referenced Jun 13, 2026
This was referenced Jul 3, 2026
Merged
9 tasks done
ivogt
added a commit
that referenced
this pull request
Jul 10, 2026
…#751) The lazy virtual:rsc-router/routes-manifest/<routerId> module re-exported the name->path map (__flat(NamedRoutes), or a second inlined JSON.parse copy for routers without a gen file) even though the eager manifest module already sets the identical map via setRouterManifest(). ensureRouterManifest then overwrote the eagerly-set map with the lazy copy, so a divergence between the two paths would have been silently masked by last-writer-wins. The lazy module now carries only the derived match data (trie + precomputedEntries); a bare side-effect import of the gen file is kept as the module-graph edge that invalidates the module on route changes in dev. ensureRouterManifest ignores any manifest field on the loaded module and gates purely on the trie. Also adds a bundle guard to build-test-app.setup.ts pinning Bundle Hygiene rule #1: serialized route data lands in exactly one RSC chunk, never in client/ssr, and is referenced only via dynamic import().
This was referenced Jul 10, 2026
This was referenced Jul 23, 2026
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
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.
No description provided.