fix(components): preserve entry events across deploy scans - #1936
Conversation
|
Reviewed; no blockers found. |
kriszyp
left a comment
There was a problem hiding this comment.
Requesting changes because I think this is fixing the symptoms at the consumer layer instead of restoring the scope.handleEntry() event contract.
handleEntryWithRedeployRestart is now a second file/deploy state machine for loadEnv and Fastify, while jsResource retains another copy and every other built-in or third-party scope.handleEntry() consumer still receives the lossy stream: every surviving file is replayed as add, deleted files produce no unlink, and unchanged files are indistinguishable from changed ones. That is the backwards-compatibility break we should fix centrally.
I think EntryHandler.pause()/resume() should preserve the previous watcher generation, discard or generation-tag stale pending reads, buffer the resumed initial scan, and diff it at ready:
- new entry →
add/addDir - changed file →
change - removed entry →
unlink/unlinkDir - unchanged entry → no event
That restores the existing API for every consumer, avoids marking a byte-identical redeploy as restart-required, and lets us remove both this helper and the compensating jsResource bookkeeping.
The inline stale-read race is also a concrete correctness blocker in the current approach. The focused helper tests cannot expose it because they do not exercise the real Scope.#deployInFlight restart suppression or EntryHandler.pause()/resume() lifecycle.
The history is split: #822 introduced pause/resume with fresh-add replay, #1540 introduced the atomic directory swap, and #1806 explicitly relies on existing-component watchers to detect what changed. Regardless of attribution, the released behavior is incompatible and #1936 is the point where we should correct course rather than add another compensating API.
We should keep #1806's explicit restart signal for genuinely new components, since they have no live Scope to emit events through; that preserves the #674 actionable 404 behavior.
This is far enough into the wrong abstraction that I would prefer replacing or substantially reworking this PR rather than expanding the helper. I am happy to take over this branch or open a replacement PR, whichever you prefer.
|
Kyle's call: please take this one — it's yours. Your read is right, and I want to be specific about what I confirmed rather than just conceding:
The branch is Two things worth carrying forward, since they're the actual failure modes #1934 hit: a file deleted during a redeploy produces no Separately — your #1918 review is the same shape and is a separate decision still with Kyle, so nothing there is handed over yet. Comment generated by kAIle (Claude Opus 4.8). |
f13de9b to
857aa7a
Compare
Superseded by the central EntryHandler generation-diff implementation in 857aa7a; the requested architecture and stale-read regression are now addressed.
|
I took over the handed-off branch and replaced the consumer-level workaround with the central course correction discussed above. |
857aa7a to
7d8e370
Compare
|
Cross-model review rerun completed with real outside-model coverage: Claude ✓ / Gemini agy ✗ → direct API ✓. The review found and we fixed suppressed non-watcher restart requests, package metadata changes outside plugin globs, cold-recreation/update identity loss, interrupted-first-scan replay, readiness under listener errors, and static URL-key collisions. Final targeted Claude verification reports no remaining blocker or significant concern. Updated head: 7d8e370. |
This comment has been minimized.
This comment has been minimized.
…adiness Follow-ups from a cross-model (Grok) review pass on this branch. Concurrent #watch() calls each observed the same live watcher before their close() await and then installed their own, so the first call's fresh chokidar instance was overwritten and never closed — its inotify handles leaked until GC, the pressure mode harper#488's ignore rules exist to avoid. Reachable from a single config save that changes both `files` and `urlPath` (two OptionsWatcher change events in one tick), and from update() racing the polling-fallback recovery. Watcher replacement is now serialized behind an install chain, and the idle case still claims its generation synchronously. update() did not re-arm the readiness latch the way pause() does, so awaiting it on an already-ready handler resolved against the outgoing generation's `ready` — before the replacement generation had scanned, digested, and emitted its diff. No production caller awaited it, so this was latent. Also swallows the fire-and-forget readiness latches at the three call sites that do not await them (constructor, Scope resume, Scope options update); the latch rejects when a generation emits `error` first, which consumers already observe through the 'error' event. Tests: a leak assertion for concurrent updates (live-watcher count, which fails at 2 without the chain), an update()-readiness ordering assertion, and a jsResource steady-state unlink+add case replacing the redeploy-specific coverage this branch removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6ad2b41 to
ff58a8b
Compare
|
Thanks — this was the right review, and the current head addresses the concrete failures you identified.
The smaller items are also covered: watcher errors are generation-scoped; static URL collisions retain ownership and restore a surviving path; roles ignores directory/unlink events; close rejects a pending readiness latch; type/URL transitions still emit their paired addition if a removal listener throws; and unreadable package evidence fails closed to “changed.” I corrected the stale deletion-test rationale in b5b48e1. One point is intentionally not converted into retry behavior: EntryHandler snapshots filesystem observation, not successful completion of arbitrary async consumer work. Scope reports async handler rejection and awaits it during initial load, but a later byte-identical deploy does not replay entries as an implicit retry mechanism. If we want durable consumer-operation retries, that needs its own explicit contract rather than depending on cold watcher replay. The #1849 merge-order note remains valid; that PR is still open and conflicted, so whichever lands second will need to reconcile its restart-gate refactor. Current head is b5b48e1. Focused regressions cover stale reads, deletions, read failure, listener failure, update races, close readiness, static collisions, roles events, transitive module provenance, resolution shadowing, and post-install metadata equivalence. |
Summary
Closes #1934 and restores the
scope.handleEntry()event contract that regressed in #1806 while preserving the new-component fix for #674.Component deploys pause file watchers while replacing the component tree. When the watcher resumed, chokidar performed a fresh scan and reported every surviving file as
add, reported no deleted files, and made unchanged and modified files indistinguishable. Fixing that separately in each consumer would preserve the incompatible watcher behavior and duplicate incomplete state tracking.Fix
EntryHandlernow retains a compact snapshot and compares every replacement watcher generation with the last consumer-observed state:changewith their new contentsadd/addDirunlink/unlinkDir, with descendants emitted before their directorySnapshots retain path metadata and SHA-256 file digests, not file contents. Watcher generations and per-path sequences prevent callbacks or slow reads from an obsolete generation from emitting events or polluting the new snapshot. Entries emitted by an interrupted initial scan are carried into the next generation, and
readyis generation-scoped and fires only after the scan and all of that generation's reads complete.Restart-free redeploys now require layered runtime-equivalence evidence:
.nodeloader in CJS, ESM, and compartment modes and conservatively make the runtime restart-requirednode_modules, and unlocked dependency installs fail closedDeploy lifecycle events carry a deployment UUID and owner thread. If the owner worker exits, peers ignore late messages, wait until the worker and its installer process groups are confirmed gone, then reclaim the exact orphaned deployment and resume paused scopes. Overlapping live deployments remain gated.
The consumer-specific redeploy workaround is removed from
jsResource;loadEnv, Fastify routes, JS resources, static files, roles, and pluginhandleEntryusers receive the corrected central events without a new API. Static path ownership is updated incrementally so restart-free static-only deploys serve the replacement content immediately.Validation
npm run buildnpm run lint:requirednpm run test:typesrestartRequiredERR_IMPORT_ATTRIBUTE_MISSINGforjson/systemSchema.json), with those files unchangedReview coverage
Author: Codex. Standard
cross-model-review/bin/prepush-review.mjsroute run repeatedly in exact-commit delta mode with independent Claude, Gemini, Grok, and Harper-domain adjudication. The final exact-head review off7be8aff1reports LGTM with no new blocker or major regression.Review findings fixed and regression-tested include stale watcher generations, suppressed restart requests, post-install package evidence, transitive runtime modules/resolution cache invalidation, pure-ESM and native-addon loading, transactional rollback under a live writer, deploy-aware load timing, static URL ownership, and dead deploy-owner/process-group reclamation.
PR implementation generated by Codex.