Skip to content

fix(runtime): retry worker and widget-dev cleanup with the promise form of rm - #631

Merged
mrgoonie merged 2 commits into
mainfrom
fix/626-retrying-runtime-cleanup
Oct 8, 2026
Merged

mrgoonie merged 2 commits into
mainfrom
fix/626-retrying-runtime-cleanup

Conversation

@mrgoonie

@mrgoonie mrgoonie commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #626. Refs #563, #622.

Why

On Windows, rmSync(path, { recursive: true, force: true, maxRetries }) reports a held directory as EBUSY (Node 22) or EPERM (Node 24) at once and never runs its retries. #622 measured this, and it reproduces here (Windows 11, Node v24.11.0): with a child process holding the directory as its working directory, rmSync(..., { maxRetries: 5, retryDelay: 100 }) threw EPERM after 0 ms, while await rm(...) from node:fs/promises with the same options removed it after 637 ms, once the holder let go 300 ms in.

Two production call sites relied on the broken retry:

Site Before After
apps/runtime/src/worker-process.ts (runWorkerProcess's finally) rmSync(directory, { ..., maxRetries: 5, retryDelay: 100 }) await rm(directory, { ..., maxRetries: 5, retryDelay: 100 })
apps/runtime/src/application/widget-dev-sessions.ts (prune) rmSync(path, { ..., maxRetries: 2 }) await rm(path, { ..., maxRetries: 5, retryDelay: 100 })

What changes

  • Worker run directory. runWorkerProcess is already async, so its finally now awaits rm. A failure is still swallowed as before (the directory holds the brief, never a key). The run's result or error now waits for cleanup: at most about 1.5 s, and only while the directory is held.
  • Widget-dev snapshots. prune becomes async, and both callers await it. Both callers (follow and activate) are already async, and they run only on the session's serial chain, so nothing else of that session interleaves with the removal. A snapshot that still cannot be removed after the retries stays on the session's list and is tried again after the next install, as before.
    • The retry budget goes from 2 attempts (about 0.3 s, which never ran) to 5 attempts with retryDelay: 100 (about 1.5 s), the same as the worker. The wait now lands on one session's chain, not on the event loop.
  • No exit-handler or synchronous caller is involved. Neither site runs inside a process exit handler, and no synchronous caller had to become async. createWidgetDevSessions().close() does not wait for the chains today, and it still does not.
  • This uses rm from node:fs/promises directly, as task-browser.ts and package-fetch.ts already do. No new helper is added.

Tests

apps/runtime/test/hold-directory.ts is a new helper. It holds a directory by running a child process with that directory as its working directory, and it reports when the hold is really in place. On Windows, that makes removal fail with EBUSY or EPERM until the child exits. On other platforms the hold changes nothing, so the tests pass there but only tell the two forms apart on Windows.

Each spec wraps rmSync and rm so that it can see when the removal starts, without changing what the removal does. It releases the hold 300 ms later. Only a removal that really retries finds the directory free.

  • worker-process.spec.ts: removes its run directory even when the directory is still held for a moment after the worker exits. The hold starts when the run directory is made, and the test asserts that the hold was in place when the removal started, so it cannot pass without proving anything.
  • widget-dev-sessions.spec.ts: removes a superseded snapshot that is held for a moment, as a file still open on Windows holds it. The first build's snapshot is held. The second build keeps it as the rollback target, and the third prunes it. The snapshot must be gone, and off the session's list.

Both new tests fail against main's source (Windows 11, Node v24.11.0). Each was run with only the source file reverted to origin/main. Each fails at expect(existsSync(...)).toBe(false), because the directory is still there.

Verification (Windows 11, Node v24.11.0)

Check Result
worker-process.spec.ts + widget-dev-sessions.spec.ts, 3 runs in a row 38/38 each run
The new worker test against main's worker-process.ts fails: the run directory remains
The new widget-dev test against main's widget-dev-sessions.ts fails: the snapshot remains
pnpm typecheck exit 0
pnpm invariants 14/14 pass
eslint on the changed files exit 0
pnpm verify invariants, typecheck and lint pass. Tests: 569 files passed, 1 failed, 1 skipped; 7701 tests passed. The one failed file is packs/browser-playwright/test/close-during-launch.spec.ts, whose afterAll threw EPERM from rmSync(dir, { ..., maxRetries: 10 }). That is the test-cleanup bug #622 fixes, in a file this PR does not touch. Run again on its own, it passed 1/1.

Overlap with open PRs

Overlap correction

#622 also edits apps/runtime/test/widget-dev-sessions.spec.ts. A trial merge conflicts on one import line only; keeping both imports passes invariants (15/15) and the affected specs. Whichever of #612 or #614 lands after this PR must keep await prune(...) at both call sites, because lint does not flag floating promises.

…rm of rm

On Windows, rmSync reports a held directory as EBUSY or EPERM at once and
never runs its maxRetries, so the worker's run directory and a superseded
widget-dev snapshot were left behind whenever something still held them.
Both callers are already async, so they now await rm from node:fs/promises,
which does retry, without blocking the event loop. The widget-dev prune
runs on the session's chain, so nothing else of that session interleaves.

Tests hold each directory with a child process's working directory until
300 ms after its removal starts; they fail against the old rmSync calls.
@mrgoonie

mrgoonie commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at 97d59406f6544a6a9be7065dce449daa6f6d41d8, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie enabled auto-merge (squash) October 7, 2026 22:16
@mrgoonie

mrgoonie commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Review attestation: ready to merge at fe20644cd5f08aaa88b9d3f4e14ddb0ce821d181, reviewed by agent:code-reviewer.

A push to this PR makes this attestation stale; the new head needs its own review.

@mrgoonie
mrgoonie merged commit 7908946 into main Oct 8, 2026
21 of 22 checks passed
@mrgoonie
mrgoonie deleted the fix/626-retrying-runtime-cleanup branch October 8, 2026 01:58
mrgoonie added a commit that referenced this pull request Oct 8, 2026
… a bound (#646)

`WidgetDevSessions.close()` now returns a promise that waits for every
session's queued work, including a superseded snapshot still being removed
with retries, for at most two seconds. The node's shutdown starts it with the
other stops and awaits it before the database closes.

The spec awaits `close()` and removes its folder with the async cleanup
helper. The held-snapshot test no longer races a 300 ms timer against the
retry budget: one attempt meets the hold, the holder exits, and then the
runtime's own removal runs, whose retry options are asserted.

Refs #626, #631
Fixes #639
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(runtime): production cleanup uses rmSync with maxRetries, which never retries on Windows

1 participant