fix: Drop replayed watch events before they reach the dirty buffer - #107
Merged
Conversation
deliver() records into dirtyPaths before invoking onChange (fc87599), so a consumer that filters inside onChange gates its own rebuild loop but cannot keep the event out of the buffer. Replayed paths therefore still became modifiedFiles, bypassing the early-return in rebuildAffectedExternals and clearing shared-bundle cache entries for files that never changed; any consumer gating on get().size === 0 silently stopped working after the first replay. Move the filter into the watcher, ahead of dirtyPaths.add. macOS FSEvents re-delivers 'changed' for recently edited files roughly every 30s with mtime untouched; once the watch list covers every compiled source that replay alone keeps a rebuild loop awake indefinitely (angular-adapter#94/#96). addPaths seeds each newly watched file's mtime so the first replay is already recognised as one -- directories are skipped, since seeding a recursive watch means walking the tree, and their entries seed themselves on the event that first reports them. mtimeOf follows one symlink level, as maxMtime does, because io.stat is lstat-based and a symlinked source would otherwise always read as a replay. replayGraceMs (default 2000) lets an unchanged-mtime event through when it arrives close to the recorded mtime. That window is load-bearing: it covers a second save inside one mtime tick (1s granularity on gRPC-FUSE/NFS/WSL2 drvfs) and an edit landing between the build finishing and addPaths recording the already-new mtime. Both would drop a real edit, which is the very failure the dedupe exists to prevent. Also widen syncNfFileWatcher's second parameter to WatchSources, so an adapter that has to expand its cache passes the paths directly instead of a synthetic { keys() } object. Arrays expose keys() too and it yields indices, so Array.isArray has to be checked first. The JSDoc now names Angular's SourceFileCache, whose inputs live in typeScriptFileCache/ referencedFiles rather than the outer Map -- the shape that made the watch set silently empty in the first place.
Two more core-side staleness vectors behind angular-adapter#94, both surfaced once the watch list covers every compiled source rather than nothing. addPaths opened an fs.watch per file. That handle holds an inode, so an editor saving by rename-replace -- JetBrains "safe write", vim backupcopy=no -- replaces the file underneath it and every later edit goes unreported. The dev server then serves stale bundles again after exactly one working rebuild. It is the same inode problem pollWatch already exists for. Cover each tracked file with a non-recursive watch on its containing directory instead, filtered back down to the tracked set so the event surface is unchanged. This also collapses thousands of sources onto a few hundred handles and reports files created after they were added. Directories passed explicitly keep their recursive watch. WatchPort's non-recursive branch reported the watched path rather than the changed entry, which a directory watch cannot use. Have it deliver the entry filename like the recursive branch does; a single-file watch reports that file's own basename, which the caller already ignores. Add sharedMappingDirs(config): the source dir of every sharedMappings entry point, derived from config alone. It is coarser than a build's compiled inputs and covers what they cannot -- files added to a lib since the last build -- but does not follow imports out of the lib, so an adapter that can enumerate its build inputs should watch both. Until now only adapters able to expand their bundler cache could watch mappings at all, which left every non-Angular adapter with the bug.
Pair byte length with mtime as a path's recorded identity, so a second save inside one mtime tick that changed how much it wrote is settled on data rather than on a wall clock. Only a save that also left length untouched reaches replayGraceMs, whose 2000 default is twice the coarsest mtime granularity in play rather than a tuned constant. Measure that window from when the event arrived instead of from the debounced flush, so debounceMs is no longer subtracted from it. A negative age -- a network mount whose server clock runs ahead -- reads as recent and delivers, which is the safe direction; the opposite skew has no local remedy and is documented as a reason to raise replayGraceMs. Warn on the first watch that fails to open. Since 3ec2273 a watch covers a whole directory of sources, so swallowing the throw at debug level hides silent staleness with the same symptom as angular-adapter#94. Later failures stay at debug: descriptor exhaustion fails every subsequent directory too. Clear lastSeen in close(), or the seed in addPaths is skipped on re-add and a stale identity stands.
addPaths opened a recursive watch for a directory even when a file under it was already covered by a non-recursive watch on that same directory. That is the order syncNfFileWatcher itself uses -- files first, then linkedDirs -- and the order an adapter watching both a mapping dir and its compiled inputs hits, so every save in such a directory reported twice: two onChange wakes, and RebuildQueue.track aborts the in-flight build to start the duplicate. Nested directories overlapped the same way, since neither branch checked whether an existing watch already covered the path. Key both maps by one normalized posix directory, skip a directory an existing recursive watch already covers, and have a newly opened recursive watch supersede the narrower ones it covers. addPaths is now order-independent, and the recursive watch's event surface is strictly wider than what it closes, so no tracked file loses coverage. Keying on the normalized directory also drops the separate recursiveDirs array, whose posix entries could disagree with the raw path the watchers map was keyed by -- one directory spelled two ways opened two handles. Suppression and superseding are poll-aware in one direction: a polled watch survives the inode replacement a native one misses, which is why linkedDirs poll at all, so it may stand in for a native watch over the same tree but never the reverse. Without that asymmetry a native parent directory would silently close a polled linkedDirs handle. Reset watchFailures in close(), or the first failure after a re-add only reaches debug and loses the visibility ae5fccc added. memory-io's watch handle removed every listener registered on a path, so closing one of two handles on the same directory killed the other -- the superseding case was untestable until close() removed only its own listener, as real fs.watch handles do. Document how coarse sharedMappingDirs can get: an entry point that is not a lib barrel widens the watch to whatever directory it sits in, and with sharedMappings unset every tsconfig path becomes a mapping.
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.
Core-side counterpart to angular-adapter#94 / angular-adapter#96.
Context
With the dev server running, edits to
sharedMappingslibraries andexposessources never reached the emitted federation bundles. The adapter-side root cause is thatsyncNfFileWatcher(watcher, bundlerCache)was called with Angular'sSourceFileCache, which extendsMapbut keeps its inputs intypeScriptFileCache/referencedFiles— sokeys()was empty and the watch set never filled. That fix belongs in the adapter; it is Angular-private knowledge core cannot have.But the adapter fix leans on core behaviour that does not hold, and its much wider watch list (~0 → ~1,900 paths) surfaces work that belongs in the watcher core owns.
onChangecould not veto the dirty bufferdeliver()records before notifying — deliberately, per fc87599, because the adapter needs both channels:So an adapter-side
onChange: p => { if (!isRealChange(p)) return; ... }gates its rebuild loop but cannot keep the event out of the buffer. Downstream that meant replayed paths still becamemodifiedFilesforrebuildForFederation, bypassing themodifiedFiles.length === 0early-return inrebuildAffectedExternalsand clearing shared-bundle cache entries for files that never changed — and any consumer gating onwatcher.get().size === 0silently stopped working after the first replay.The replay filter is generic (
fs.watchbehaviour, zero framework content), so it moves here, ahead ofdirtyPaths.add.Changes
Replay dedupe in the watcher
dedupeReplays(default on). Events whose recorded identity — mtime and byte length — is unchanged since the last one seen for that path are dropped before reaching either channel. macOS FSEvents re-delivers 'changed' for recently edited files roughly every 30s; with a watch list of a few thousand sources that replay alone keeps a rebuild loop awake indefinitely.Length is paired with mtime so that the one genuinely ambiguous case — a second save landing inside a single mtime tick, on a filesystem with coarse granularity — is settled on data rather than on a clock. Only a save that also left byte length untouched reaches the time check at all, which is what keeps a wrong clock reading from swallowing the common case.
replayGraceMs(default 2000) is that time check: an identity-matching event still passes while the mtime is recent. The default is a derived bound, not a tuned constant. A filesystem truncating mtime to granularitygrecords a value up togbefore the write, so two writes can only collide in mtime within2g;gis 1s on the coarsest filesystems in play (gRPC-FUSE, NFS, WSL2 drvfs, HFS+). On ext4 and APFSgis nanoseconds, so a real save always advances mtime and the window is never reached.Age is measured from when the event arrived, not from when a debounced flush got round to it, so
debounceMsis not silently subtracted from the window. Two limits are documented rather than papered over: an event-loop stall delays thefs.watchcallback itself, which no local bookkeeping can correct; and on a network mount the age is a cross-clock subtraction, since mtime carries the server's clock. A server clock running ahead yields a negative age, which reads as recent and delivers — the safe direction, so it is left alone. Running behind shortens the window, and there the only remedy is a largerreplayGraceMsordedupeReplays: false.addPathsseeds each newly watched file's identity so the first replay after startup is already recognised as one. Directories are skipped — seeding a recursive watch would mean walking the tree — and their entries seed themselves on the event that first reports them.identityOffollows one symlink level, mirroringmaxMtimeinresolve-shared-dirs.ts;io.statis lstat-based, so without it every event for a symlinked source reads as a replay.File watches go through the directory
addPathsno longer opens a handle on a file. A per-filefs.watchholds an inode, and an editor that saves by rename-replace (JetBrains "safe write", vimbackupcopy=no) replaces it — the handle then reports nothing, so the second save onward is silently lost. Each tracked file is covered by a non-recursive watch on its containing directory instead, filtered back down throughtrackedFilesso the event surface is unchanged. This also collapses thousands of sources onto a few hundred handles, and reports files created after they were added. Directories passed explicitly keep their recursive watch.A failed watch is no longer silent
Now that one watch covers a whole directory of sources, swallowing the throw at debug level hides exactly the #94 symptom: paths that never report a change and a dev server serving the last bundle until restart. The first failure is a
logger.warnnaming the path and the consequence. Later ones stay at debug — descriptor exhaustion fails every subsequent directory too, and a line per path would flood.sharedMappingDirsThe source dir of every
config.sharedMappingsentry point — a framework-agnostic watch set for mappings, derived from config alone rather than from a bundler cache. Coarser than a build's compiled inputs and deliberately so: it covers files added to a lib since the last build, which a compiled-inputs watch set cannot know about yet. It does not follow imports out of the lib, so an adapter that can enumerate its build inputs should watch both.syncNfFileWatcheraccepts paths directlyThe second parameter widens to
WatchSources = Iterable<string> | { keys(): Iterable<string> }, so an adapter that has to expand its cache passes the resulting array instead of a synthetic{ keys: () => files[Symbol.iterator]() }. Backward compatible for cache-keyed-by-input-path consumers.One trap worth knowing: arrays expose
keys()too, and it yields indices — theArray.isArraycheck has to come first, and there is a test asserting real paths rather than0, 1.The JSDoc now names the
SourceFileCachecase, so the next adapter author meets it as documentation rather than as a dev server serving stale bundles.Compatibility
dedupeReplaysdefaults to on, which is a behaviour change for existing consumers.Map<string, unknown>keyed by input path, watched natively. On inotify a save always advances mtime, so the only events it loses are genuine no-ops. SetdedupeReplays: falseto opt out.syncNfFileWatchersignature change is additive — existing call sites keep working.StatInfogains a requiredsize. The io port is internal (not exported fromindex.tsorinternal.ts), so this affects no consumer.adapters/build-your-own.md,core/api-reference.md) stay accurate; they should gain a note about the dedupe, the widened parameter andsharedMappingDirsin a follow-up.Out of scope
watchers,fileDirWatchers,trackedFilesandlastSeenare never pruned during a session, only cleared onclose(). Linux inotify exhaustion is not a realistic blocker on modern distros (max_user_watchesis 247k on the machine this was measured on), and directory collapsing cut handle count by roughly an order of magnitude, but the maps still grow monotonically. Own issue.IN_Q_OVERFLOWundergit checkoutornpm installchurn, where events are lost wholesale andlastSeenstill holds the pre-checkout identity. Re-stat'ing the tracked set would catch every one and costs ~3.6 ms for 1,900 files (measured), but it needs a call-cadence decision:addPathsalready runs once per build and needs no new API, yet delivering from there firesonChangeinside the adapter's own post-build sync call and invites re-entrant rebuilds. Own issue — and note there that the motivation is queue overflow, not the dedupe. It cannot recover a wrongly-dropped ambiguous event, because in that case the recorded identity already equals the current one.Verification
pnpm test(440 tests / 44 files),pnpm typecheck,pnpm lint(0 errors; 5 warnings, all pre-existingno-consoleinlogger.ts),pnpm build,pnpm knipall pass.file-watcher.spec.tsgoes from 10 to 30 tests. Two existing tests needed a fixed clock plus an mtime advance to model a real save. New coverage: replay dropped from both channels; an identity-matching event inside the grace window passing; an aged same-mtime event whose length changed passing; a future mtime delivering; the grace window measured from event arrival rather than from the debounced flush; mtime advance re-arming; vanished file passing; first event for an unseeded directory entry passing;dedupeReplays: false; symlink target mtime; directory-not-file watch registration, neighbour filtering, post-add file creation, and no double watch under a recursive dir; warn-once on watch failure; and all fourWatchSourcesshapes.Still to do end-to-end, on a workspace where the issue reproduces: link
dist/into an angular-adapter#96 checkout and confirm asharedMappings.tssave reaches the emitted bundle in one rebuild, that an 80s idle window after it records zero further cycles, and that two saves inside one second both land.