fix(project): Cancel pending settle timer before watcher recovery - #1528
Draft
matz3 wants to merge 13 commits into
Draft
fix(project): Cancel pending settle timer before watcher recovery#1528matz3 wants to merge 13 commits into
matz3 wants to merge 13 commits into
Conversation
matz3
enabled auto-merge (rebase)
August 18, 2026 10:00
d3xter666
reviewed
Aug 18, 2026
Comment on lines
+233
to
+236
| // Cancel any pending settle timer before tearing down the subscriptions. On Windows, | ||
| // ReadDirectoryChangesW holds a raw HANDLE; a timer callback firing into a closed handle | ||
| // after recovery causes an access violation (0xC0000005). The timer is re-armed naturally | ||
| // by the first event that arrives on the new subscriptions. |
Member
There was a problem hiding this comment.
Do we need all the resoning here. I think there's much detail that needs a deeper look to understand the root cause.
From my perspective, it's completely resonable and expected to clear timeouts before releasing the property. Otherwise, side effects (like the one above) can be expected
Member
Author
There was a problem hiding this comment.
Yeah, thats right. Thanks for pointing that out. I removed it and refactored the code.
On Windows, a settle timer callback firing into a closed ReadDirectoryChangesW handle after recovery causes an access violation (0xC0000005). Cancel any pending timer before tearing down the subscriptions; it is re-armed by the first event on the new set. Extract the timer-cancel and subscription-drain logic shared by #recoverWatcher and destroy into #cancelSettleTimer and #drainSubscriptions helpers to remove the duplication.
matz3
force-pushed
the
fix/project-watcher-cancel-settle-timer
branch
from
August 18, 2026 12:35
fe7fc7e to
9744c52
Compare
Member
Author
|
The CI ran into a timeout on Windows, so this PR is most likely not solving all the current issues of flaky tests. I will continue investigating tomorrow. |
…exclusive lock Introduce #lockTail / #runExclusive so destroy()'s field teardown and #swap never run concurrently. destroy() fires its synchronous head (state flip, abort, timer/relay cleanup) immediately, then queues teardown behind any in-flight swap; the swap therefore always finishes adopting #stack / #definitionWatcher before destroy reads and nulls them, closing the race that orphaned a @parcel/watcher subscription and the node:sqlite database on Windows. Adds two regression tests: one for the concurrent-swap leak and one confirming destroy() unblocks promptly when a recovery is parked in the settle wait.
matz3
marked this pull request as draft
August 19, 2026 08:52
auto-merge was automatically disabled
August 19, 2026 08:52
Pull request was converted to draft
Member
Author
|
Moving back to draft. I will also test on Windows to verify the changes. No need to review right now. |
Tests that exercise error and build paths leaked log lines into the test output because @ui5/logger writes to process.stderr whenever its process events have no listener attached, obscuring real failures. Attach no-op listeners to every logger event with a stderr fallback via an AVA --import setup module, and stub the raw stderr password prompt in the sslUtil test through file-level hooks.
The reinitialize.js test exits with 0xC0000005 (access violation) on Windows, a native crash in the forked AVA worker rather than a JS test failure. Add standalone probes that drive each native subsystem the test exercises in isolation — node:sqlite (single and overlapping WAL+mmap handles), the @parcel/watcher subscribe/unsubscribe cycle, and the full serve/reinitialize/ close lifecycle without AVA — so the crash origin can be attributed on Windows before changing the teardown logic further. See test/probes/README.md.
import() of an absolute path throws ERR_UNSUPPORTED_ESM_URL_SCHEME on Windows, where the default ESM loader requires a file:// URL rather than a bare drive path (e.g. 'c:\...'). Wrap each probe's absolute-path dynamic import in pathToFileURL so the probes run on the Windows machine they target.
The four component probes all exit clean on Windows, including the full serve/reinitialize/close lifecycle — so the 0xC0000005 is not a native subsystem in isolation. Add two probes that layer on what the AVA worker adds: supertest request sockets, and three serve/reinitialize/close cycles in one long-lived process (mirroring the three serial subtests). Document the narrowed hypothesis and the -m subtest bisection in README.
The reinitialize.js 0xC0000005 on Windows is flaky and the deterministic probes do not reproduce it, so it is a timing-dependent race in native teardown/exit (@parcel/watcher unsubscribe or node:sqlite close). Add a UI5_TEARDOWN_TRACE=1 tracer that writes each teardown step to fd 2 with writeSync — unbuffered, so the last line survives a hard segfault and names the native call in flight. Wired through Supervisor.destroy, BuildServer.destroy, the definition/source watcher destroys, drainSubscriptions (per unsubscribe), and CacheManager/ BuildCacheStorage close (WAL checkpoint vs db.close). No-op and zero-cost when the env var is unset. See packages/server/test/probes/README.md for the loop- until-crash procedure.
AVA wraps the worker's native 0xC0000005 and reports its own (non-zero) exit code rather than the raw crash code, so the loop must break on any non-zero exit, not on -1073741819. Also tag each iteration in the log so the crashing run is easy to locate.
…race drainSubscriptions fired every @parcel/watcher unsubscribe() concurrently via Promise.allSettled. @parcel/watcher has a data race on its global shared-backend registry between subscribe and unsubscribe (parcel-bundler/watcher#259); on Windows, where all subscriptions share one backend thread, the overlapping teardown access-violated (0xC0000005), crashing the AVA worker in test/lib/server/reinitialize.js. Synchronous teardown tracing pinpointed the crash to a watcher unsubscribe in flight (the final subscription started with no matching completion) while its siblings were being torn down in parallel — not to node:sqlite, whose db.close() always completed cleanly. Drain the subscriptions one at a time instead. The list is small (one subscription per watched directory) and this only runs at teardown, so the lost concurrency is irrelevant. Every unsubscribe() is still attempted and all failures are still collected.
The serialized-unsubscribe fix was necessary but not sufficient: reinitialize.js still crashed (0xC0000005) on Windows, now after the first subtest fully passes rather than mid-unsubscribe. That points at a second native teardown/exit site. Restore the env-gated synchronous tracer (UI5_TEARDOWN_TRACE=1) over the fix so the next Windows loop shows the new last-surviving operation, and add a trace to the native subscribe path so a fresh subscribe racing a prior teardown is visible too. Silent no-op when the env var is unset.
…wide Serializing drainSubscriptions was necessary but not sufficient: the Windows 0xC0000005 persisted, now during a teardown unsubscribe that raced a fresh subscribe from a reinitialize() swap. The teardown trace confirmed a native unsubscribe in flight (no matching completion) right after subscribe cycles. @parcel/watcher mutates a process-global backend registry from both the JS thread (subscribe: find/emplace/rehash) and a libuv worker thread (unsubscribe of the last subscriber: erase/rehash) with no lock guarding that static map (parcel-bundler/watcher#259). Any overlap of subscribe with an in-flight unsubscribe corrupts the registry and access-violates on Windows. Funnel every native subscribe and unsubscribe through one process-wide promise chain in fileWatcher, so the process never has two registry mutations in flight at once: each waits for the previous to fully settle. The chain is process-wide because the registry it protects is process-global, and it only orders rare watcher-lifecycle calls, so it costs nothing on the hot path. subscribe now returns a thin wrapper whose unsubscribe routes through the same chain.
Process-wide subscribe/unsubscribe serialization was still not enough: the Windows 0xC0000005 persisted, always in the second reinitialize.js subtest, which fully tears down its server (registry drops to zero subscribers) before the next subtest subscribes. The trace showed a lone, non-overlapping native unsubscribe crashing — proving the corruption is not from our concurrent JS calls but from @parcel/watcher's own libuv worker thread. Per parcel-bundler/watcher#259, the destructive operation is the empty transition: when a backend's last subscriber is removed, removeShared() runs erase() + rehash(0) on a worker thread, and that rehash(0) races a concurrent subscribe from the JS thread. Our unsubscribe promise can resolve before the worker-thread erase completes, so JS-side serialization cannot fence it. Hold one never-unsubscribed keep-alive subscription for the life of the process so the backend registry never empties. rehash(0) only fires at size zero, so keeping one subscriber alive means the destructive transition never happens. The target is os.tmpdir() (always present) with an ignore-everything glob, so it delivers no events and does no work beyond existing; the OS reclaims it at process exit. The serialization chain stays as defense in depth against same-thread overlap.
The process-lifetime keep-alive killed the 0xC0000005 crash, but because it was never unsubscribed it kept @parcel/watcher's native handle alive, so AVA's test worker could not drain its event loop and reported "Failed to exit". Production exits via process.exit on SIGINT/SIGTERM so it was unaffected, but the leaked handle is still wrong. Make the keep-alive a refcounted, session-scoped pin instead. fileWatcher exposes pinBackend()/unpinBackend(); the Supervisor pins in #init before any watcher subscribes and releases in destroy() after every watcher this session owns is torn down. The keep-alive is established once per active-session set and released only at the last unpin — a quiescent moment where nothing is serving, so no concurrent subscribe can race the final registry-empty. That still covers the risky swap churn (the pin spans all reinitialize() swaps of a session) while letting the process, and the test worker, exit cleanly.
|
coverage: 94.16% (+0.1%) from 94.035% — fix/project-watcher-cancel-settle-timer into main |
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
3 participants
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.
On Windows, a settle timer callback firing into a closed ReadDirectoryChangesW handle after recovery causes an access violation (0xC0000005). Cancel any pending timer before tearing down the subscriptions; it is re-armed by the first event on the new set.
Extract the timer-cancel and subscription-drain logic shared by #recoverWatcher and destroy into #cancelSettleTimer and #drainSubscriptions helpers to remove the duplication.
This should resolve the flaky test observed via #1524 (comment).