Test flake: request-save-script-transports afterEach rmSync races the fire-and-forget session-event-log write (ENOTEMPTY)

[thymikee]

Evidence

Surfaced once by the coverage-shard lane on PR #1996 (CI consolidation): src/daemon/__tests__/request-save-script-transports.test.ts failed with ENOTEMPTY inside its own afterEach cleanup, under shard load. It passed on re-run and passes repeatedly in isolation. PR #1996's description records it under "Known pre-existing flake" — the CI change only exposed it; the race predates it.

The race, precisely

The test's cleanup removes each per-test temp root recursively:

// src/daemon/__tests__/request-save-script-transports.test.ts:56
afterEach(() => {
  for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true });
});

That root contains sessions/<session>/events.ndjson, and the daemon request path writes to it asynchronously, fire-and-forget:

• Every request handled by createRequestHandler (both the socket and HTTP transports in this file, including the rejected --save-script requests) appends request.started / request.finished events via sessionStore.recordEvent(...) (src/daemon/request-execution-scope.ts:153, :257, :271, :416).
• appendSessionEvent → queueEventLogWrite (src/daemon/session-event-log.ts:84) chains an unawaited promise per log path: mkdir -p the session dir → rotation check → fs.promises.appendFile(events.ndjson). The daemon response resolves without awaiting this chain, so the test body's await send(...) returning does not mean the write has landed.

Under a loaded runner the queued append can still be in flight when afterEach runs. fs.rmSync(root, { recursive: true }) unlinks the directory's entries and then rmdirs each directory; if the pending chain re-creates sessions/<session> (its mkdir -p) and/or events.ndjson (append creates the file) between the unlink sweep and the rmdir, the rmdir hits a non-empty directory → ENOTEMPTY. In the quiet case the write happens to land before cleanup, which is why the test is green in isolation and on re-run.

Note the PR description's shorthand "async daemon-log write" is slightly off: writes to daemon.log itself (the diagnostics ndjson) are synchronous (appendFileSync in src/utils/diagnostics.ts:270). The only async writer in this path is the session event log under sessions/<session>/.

Fix direction

The quiesce hook already exists: flushSessionEventLogWrites() (src/daemon/session-event-log.ts:77) awaits all pending event-log writes (also surfaced as SessionStore.flushEvents()). The fix is a one-liner in the test's cleanup:

afterEach(async () => {
  await flushSessionEventLogWrites();
  for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true });
});

Alternative (weaker): retry-on-ENOTEMPTY in the cleanup, e.g. fs.rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }). Draining the writer is preferable — it removes the race instead of tolerating it, and maxRetries alone can still lose if the append lands after the last retry's sweep.

Worth a quick sweep while fixing: any other daemon test that drives requests through createRequestHandler (or SessionStore.recordEvent/recordAction) and then rmSyncs its temp root in cleanup has the same latent race.

Links

• Exposed by (and documented in) PR ci: consolidate CI workflow from 15 jobs to 8 #1996.

[thymikee]

Triaged: #1999 fixes this at the owning synchronization seam by awaiting the existing session-event-log flush before cleanup, and is reviewed clean. Keep this issue open until that PR merges; its Closes #1998 link will close it automatically.