Repository navigation
fix(quit): stop durable state writes from parking the main thread on quit - #11931
Conversation
will-quit ran stats.flush() and store.flush() synchronously, before preventDefault(). Both fsync and rename a multi-MB file on the profile directory. When that directory sits on a stalled network mount the syscall enters an uninterruptible wait: the app stops repainting and stops responding to Force Quit, because a process blocked in the kernel ignores SIGTERM and SIGKILL alike. The existing 20s teardown deadline could not bound this. Its timer runs on the very thread the syscall parked, so it never fires. The fix is to make the quit path awaitable rather than to try to bound it — a quit that is slow but responsive stays killable by the OS. - preventDefault() now runs first, so every teardown step is free to await - stats and state gain flushAsync() twins that use node:fs/promises - both join the existing teardown barrier, which can now actually bound them - the pass-2 will-quit re-entry returns early instead of re-running teardown - quitFlushStarted makes the quit flush the last write, so a teardown step touching the store cannot arm a debounce that races process exit Making the swap async cost the atomicity of check-generation-then-rename: a writer parked on await rename has already cleared the guard, so a later synchronous flush could be clobbered by stale state. Both async writers now claim their temp path, and the sync writers delete it, turning that swap into a swallowed ENOENT. Atomic temp+rename is unchanged, so a write cut short by the deadline leaves the previous file whole — bounded loss, never corruption.
📝 WalkthroughWalkthroughThe change adds serialized asynchronous persistence for active-view preferences, statistics, store state, terminal snapshots, and GitHub cache data. Synchronous flushes remove stale temporary files before writing newer state. New pending and final flush APIs drain current generations and support abort signals. Renderer unload handling now stages state before asynchronous persistence. Application shutdown prevents duplicate teardown and coordinates bounded asynchronous cleanup before re-entering quitting. Tests cover rename races, stalled writes, timeout handling, staging, and post-quit write suppression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/quit-path-durable-write-blocking.test.ts (1)
283-300: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the active-view sidecar after the quit flush.
This test passes
activeView: 'tasks', so it exercises thequitFlushStartedguard inActiveViewPreference.scheduleSaveas well. The assertions only coversyncCallsand the state file. An asynchronous active-view write that landed after the quit flush would issue no synchronous call and would not changeui.sidebarWidth, so it would pass undetected. Add an assertion on the sidecar contents.♻️ Proposed addition
expect(fsCalls.syncCalls).toEqual([]) expect(JSON.parse(readFileSync(dataFile(dir), 'utf-8')).ui.sidebarWidth).toBe(10) + expect(JSON.parse(readFileSync(activeViewFile(dir), 'utf-8')).activeView).not.toBe('tasks')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4deace0b-e5bf-4fda-8297-c2e9865c1456
📒 Files selected for processing (7)
src/main/active-view-preference-sync-flush-veto.test.tssrc/main/active-view-preference.tssrc/main/index.tssrc/main/persistence.tssrc/main/quit-path-durable-write-blocking.test.tssrc/main/stats/collector-async-save.test.tssrc/main/stats/collector.ts
Greptile SummaryThis PR fixes a hard freeze on quit caused by synchronous
Confidence Score: 5/5Safe to merge. The async quit path is correct and thoroughly tested; atomic temp+rename guarantees bounded loss and no corruption if the deadline fires mid-write. The core invariants — generation-guarded async renames, inFlightTmpFile veto for sync flush, quitFlushStarted gate to block late debounced writes — are each independently tested and correct. The daemonDisconnectDone / QuitTeardownStartGate interplay correctly handles re-entrant and concurrent will-quit events. The only finding is a minor logging inconsistency with no correctness impact. Files Needing Attention: No files require special attention; the complexity in persistence.ts and active-view-preference.ts is well-commented and covered by the new test suite.
|
| Filename | Overview |
|---|---|
| src/main/index.ts | will-quit handler refactored: preventDefault moved to top via QuitTeardownStartGate, daemonDisconnectDone guards the final-exit re-entry, stats/store flush captured as async promises before joining the teardown barrier. |
| src/main/persistence.ts | Added flushAsync()/flushPendingAsync()/flushPendingOrThrowAsync(); inFlightAsyncTmpFile guard vetoes stale renames during sync flush; quitFlushStarted blocks new scheduleSave/flushOrThrow calls after quit flush begins. |
| src/main/active-view-preference.ts | Added flushAsync()/flushPendingAsync()/drainPendingWrites() for async quit path; inFlightTmpFile allows flushOrThrow() to veto a parked async rename via unlinkSync; quitFlushStarted prevents new debounced writes after final flush begins. |
| src/main/stats/stats-snapshot-writer.ts | New class for async stats writes using writeFile+rename (consistent with pre-existing writeSync path); inFlightAsyncTmpFile guard mirrors the persistence.ts pattern for sync-flush veto. |
| src/main/stats/collector.ts | Added flushAsync() delegating to StatsSnapshotWriter; closeOutLiveAgents() remains synchronous; quitFlushStarted guard prevents late scheduleSave calls; errors are logged before swallowing. |
| src/main/quit-teardown-start-gate.ts | New guard class that calls preventDefault on every overlapping will-quit while teardown is in progress; returns false on re-entry so the handler exits early without repeating teardown. |
| src/main/durable-file-write.ts | Added renameDurable() for async rename + directory fsync; durableWriteTempPath() for consistent temp-file naming; removeStaleDurableWriteTempFiles() for orphan cleanup. writeFileDurableSync kept for crash/sync paths. |
| src/main/orca-profiles/profile-persistence-deadline.ts | New helper wrapping flushPendingOrThrowAsync in a 20-second AbortController-backed deadline for profile mutations; timeout always cleared in finally. |
| src/main/agent-auth-restart-preservation.ts | Switched store.flush() to store.flushPendingOrThrowAsync() for the restart path; now awaited within the existing 2-second lifecycle timeout. |
| src/main/quit-path-durable-write-blocking.test.ts | 10 new integration tests covering the async quit path: no-sync-syscall assertions, teardown deadline bounding against a stalled mount, rename-clobber guards, and behavior-preservation regression fencing. |
| src/main/active-view-preference-sync-flush-veto.test.ts | New test file verifying a parked async rename cannot overwrite a later synchronous flushOrThrow(), covering the unlink-veto, EBUSY recovery, and abort-signal coalescing paths. |
| src/main/terminal-scrollback-snapshot-async-migration.ts | New async migration helpers for terminal scrollback snapshots, offloading I/O from the main thread during session state writes. |
Sequence Diagram
sequenceDiagram
participant E as Electron
participant WQ as will-quit handler
participant Gate as QuitTeardownStartGate
participant Stats as StatsCollector
participant Store as Store
participant Deadline as settleTeardownWithinDeadline
participant FS as fs/promises
E->>WQ: will-quit (1st)
WQ->>Gate: tryStart(e) calls preventDefault
Gate-->>WQ: returns true (started)
WQ->>Stats: flushAsync closeOutLiveAgents sync set quitFlushStarted
Stats-->>WQ: statsFlush Promise
WQ->>Store: flushAsync set quitFlushStarted
Store-->>WQ: storeFlush Promise
WQ->>Deadline: settleTeardownWithinDeadline with daemon rpc stats state promises
par async background writes
Stats->>FS: writeFile tmpFile then rename to statsFile
Store->>FS: open tmpFile writeFile fsync renameDurable
end
Deadline-->>WQ: all settled or deadline reached
WQ->>E: "daemonDisconnectDone=true then app.quit()"
E->>WQ: will-quit (2nd)
WQ-->>E: "daemonDisconnectDone=true so return without preventDefault"
Reviews (3): Last reviewed commit: "fix(persistence): bound best-effort flus..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/stats/collector.ts (1)
228-238: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShorten the comment and remove the stale identifier references.
The comment names
prepareWritePayloadandwriteToDiskSync. Neither symbol exists after the extraction; the writer now exposespreparePayloadandwriteSync. The block also walks through implementation details across seven lines.As per coding guidelines: "Comments must be concise, limited to non-obvious information, and preferably one line; do not explain obvious code or walk through implementation details."
♻️ Proposed comment
this.writeTimer = setTimeout(() => { this.writeTimer = null - // Why: the debounced save is fun-stats telemetry, not crash-critical - // state, so it uses the async writer to move the ~900KB tmp-file write - // off the main thread (the stringify stays sync — see prepareWritePayload). - // A chatty multi-agent session re-arms this every 5s; a fully-sync write - // is a recurring main-thread stall. The quit path uses flushAsync(); the sync - // flush() remains for callers that cannot await, and writeToDiskSync keeps the - // two paths race-safe. + // Why async: a chatty session re-arms this every 5s, and a sync ~900KB write + // would stall the main thread each time. void this.enqueueWrite().catch((err) => {Source: Coding guidelines
src/main/window/attach-main-window-services.ts (1)
158-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound this flush with a deadline.
flushPendingAsync()is unbounded and cannot be aborted, becauseStore.flushPendingAsync()captures no signal at call time andStore.flushCurrentStateAsync()never checks anAbortSignalin its current code. The updateronBeforeQuitcan still hang indefinitely on a stalled disk write; avoid the duplicate store flush or callflushPendingOrThrowAsync({ signal })and race the flush against a timeout.
🧹 Nitpick comments (4)
src/main/stats/stats-snapshot-writer.ts (2)
23-37: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider restarting the drain if
writeRequestedis still set when the tracked promise settles.
drainWrites()exits whenwriteRequestedis false. The reset ofpendingWritehappens in a.finallymicrotask afterrunsettles. Ifwrite()runs in that gap, it setswriteRequested = true, observes a non-nullpendingWrite, and returns the settling promise. No drain then consumes the request, so the last snapshot is lost.The window is narrow because
write()is normally called from timer or IPC macrotasks. A guard in the reset closes it.♻️ Proposed guard
const run = this.drainWrites() const tracked = run.finally(() => { if (this.pendingWrite === tracked) { this.pendingWrite = null + if (this.writeRequested && this.pendingSerialize) { + void this.write(this.pendingSerialize).catch(() => {}) + } } })
68-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winResolve the target file once per write instead of twice.
writeToDiskAsync()capturesstatsFileat line 102, andpreparePayload()callsthis.resolveFile()again at line 75.getStatsFilereads a module-level path thatinitStatsPath()can change. If the path changes between the two calls, the temporary file and the rename target live in different directories, and the rename fails or writes to the wrong profile. Pass the resolved path intopreparePayload().♻️ Proposed change
- private preparePayload(serialize: () => string): { + private preparePayload( + finalPath: string, + serialize: () => string + ): { tmpFile: string json: string generation: number } { const generation = ++this.writeGeneration return { - tmpFile: durableWriteTempPath(this.resolveFile()), + tmpFile: durableWriteTempPath(finalPath), json: serialize(), generation } }Update both call sites to pass the already resolved
statsFile.src/main/stats/collector.ts (1)
248-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
STATS_SCHEMA_VERSIONis now declared in two files.
src/main/stats/stats-file-loader.tsdeclares its ownSTATS_SCHEMA_VERSION = 1at line 4, andserialize()writes the constant declared in this file. A future bump in one file leaves the other at the old value. Export the constant from a single module and import it in both places.src/main/ipc/orca-profiles.test.ts (1)
165-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that
onBeforeRelaunchran before comparing the order.The
?? Number.POSITIVE_INFINITYfallback makes this assertion pass whenonBeforeRelaunchwas never invoked. The test then proves only thatflushran, not that it ran first. Add an explicit call assertion so a regression that dropsonBeforeRelaunchfails here.♻️ Proposed change
+ expect(onBeforeRelaunch).toHaveBeenCalledTimes(1) expect(flush.mock.invocationCallOrder[0]).toBeLessThan( - onBeforeRelaunch.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY + onBeforeRelaunch.mock.invocationCallOrder[0] )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c6e4dde-4175-42ea-b458-ef36ee698719
📒 Files selected for processing (30)
src/main/active-view-preference-sync-flush-veto.test.tssrc/main/active-view-preference.tssrc/main/agent-auth-restart-preservation.test.tssrc/main/agent-auth-restart-preservation.tssrc/main/durable-file-write.tssrc/main/index.tssrc/main/ipc/orca-profiles.test.tssrc/main/ipc/orca-profiles.tssrc/main/ipc/renderer-shutdown-checkpoint.test.tssrc/main/ipc/renderer-shutdown-checkpoint.tssrc/main/orca-profiles/profile-persistence-deadline.tssrc/main/persistence-async-write-syscalls.test.tssrc/main/persistence.tssrc/main/quit-path-durable-write-blocking.test.tssrc/main/quit-teardown-start-gate.test.tssrc/main/quit-teardown-start-gate.tssrc/main/stats/collector-async-save.test.tssrc/main/stats/collector.tssrc/main/stats/stats-file-loader.tssrc/main/stats/stats-snapshot-writer.tssrc/main/terminal-scrollback-snapshot-async-migration.tssrc/main/terminal-scrollback-snapshots.tssrc/main/window/attach-main-window-services.test.tssrc/main/window/attach-main-window-services.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/App.tsxsrc/renderer/src/app-startup-routing.test.tssrc/renderer/src/web/web-preload-api.test.tssrc/renderer/src/web/web-preload-api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/index.ts
…quit (stablyai#11931) * fix(quit): stop durable state writes from parking the main thread will-quit ran stats.flush() and store.flush() synchronously, before preventDefault(). Both fsync and rename a multi-MB file on the profile directory. When that directory sits on a stalled network mount the syscall enters an uninterruptible wait: the app stops repainting and stops responding to Force Quit, because a process blocked in the kernel ignores SIGTERM and SIGKILL alike. The existing 20s teardown deadline could not bound this. Its timer runs on the very thread the syscall parked, so it never fires. The fix is to make the quit path awaitable rather than to try to bound it — a quit that is slow but responsive stays killable by the OS. - preventDefault() now runs first, so every teardown step is free to await - stats and state gain flushAsync() twins that use node:fs/promises - both join the existing teardown barrier, which can now actually bound them - the pass-2 will-quit re-entry returns early instead of re-running teardown - quitFlushStarted makes the quit flush the last write, so a teardown step touching the store cannot arm a debounce that races process exit Making the swap async cost the atomicity of check-generation-then-rename: a writer parked on await rename has already cleared the guard, so a later synchronous flush could be clobbered by stale state. Both async writers now claim their temp path, and the sync writers delete it, turning that swap into a swallowed ENOENT. Atomic temp+rename is unchanged, so a write cut short by the deadline leaves the previous file whole — bounded loss, never corruption. * fix(quit): harden async persistence finalization * fix(persistence): bound best-effort flushes (cherry picked from commit 8ab85c9)
1 participant
The freeze
will-quitranstats.flush()andstore.flush()synchronously, beforepreventDefault(). Bothfsyncandrenamea multi-MB file in the profile directory.When that directory sits on a stalled network mount (SMB/NFS profile redirection, a VPN drop mid-quit), the syscall enters an uninterruptible wait. A process blocked there ignores
SIGTERMandSIGKILL— so the app stops repainting and Force Quit stops working. That is the "app is frozen and won't even force-quit" report.The existing 20s teardown deadline could not bound this.
settleTeardownWithinDeadlineschedules asetTimeouton the very thread the syscall parked, so it never fires. No main-thread deadline can bound a main-thread block.So the fix is not to bound quit — it's to stop blocking. A quit that is slow but responsive stays killable by the OS; one blocked in the kernel does not.
The change
preventDefault()moved to the top of the handler, so every teardown step below is free toawait.StatsCollectorandStoregainflushAsync()twins built onnode:fs/promises. TheJSON.stringifystays synchronous (both writers need an untorn snapshot), but every syscall is async.will-quitre-entry returns early instead of re-running teardown against already-committed state.quitFlushStartedmakes the quit flush the final write. Without it, a teardown step touching the store arms a debounced write with nothing awaiting it, leaving arenameracing process exit.existsSyncprobe ahead ofmkdir— recursivemkdiris already a no-op when the directory exists, and the probe was itself a blockingaccess()on the stalled mount.Atomic temp+rename is unchanged, so a write cut short by the deadline leaves the previous file whole: bounded loss, never corruption.
One non-obvious consequence, and its guard
Making the swap async costs the atomicity of check-generation-then-rename. A writer parked on
await renamehas already cleared the generation guard, so nothing downstream can veto it — a later synchronous flush could be silently clobbered by stale state. This matters foractive-view-preference, whose syncflushOrThrow()runs from ~15 production sites (session checkpoints, renderer shutdown).Both async writers now claim their temp path; the sync writers delete it before writing. The stale swap becomes a swallowed
ENOENT. Two tests pin this at the rename specifically, since the pre-existing tests only ever parked a writer before its temp write.Tests
src/main/quit-path-durable-write-blocking.test.ts(10 new) plus 2 clobber tests.Being explicit about what each one proves — I reverted the fix and re-ran:
Fail without the fix (4): the two "issues no synchronous fs syscalls" tests, the sidecar test (active-view + github-cache also move off the thread), and — the key one — "the quit teardown deadline can now bound a wedged state flush", which drives the real
settleTeardownWithinDeadlineagainst a mount that never responds and asserts it returns['state']instead of hanging.Fail without their specific guard (2): the two rename-clobber tests. Removing only the temp-path removal flips
active-view.jsonback to the stale view and loses 8000ms of agent time from the stats flush.Pass either way, by design (6): behavior-preservation guards — live-agent closeout still happens before
killAllPty(), the flush still drains an in-flight debounced write, it resolves rather than throwing when the mount rejects writes, etc. They're regression fencing, not evidence.Deliberate scope decisions
flush()/flushOrThrow()are kept. They have non-quit callers that genuinely cannot await (session checkpoints, crash paths). Only the quit path moved.statsandstateflush concurrently with the other teardown joiners, not after them. Serializing after would let a wedged transport ([Bug]: macOS app freezes on the "phone session ended" resize modal after waking from sleep, and can't be quit except via Force Quit #9447) eat the entire window and starve the state write. Verified neitherdisconnectDaemon()norshutdownWatchersOnce()mutates the store.fsyncfailure (as opposed to a hang) the writer'sfinallyremoves the temp file, losing that write. Degrading to an unsynced commit would preserve it. Different failure mode; worth a separate change.Verification
tsc --noEmitclean ·oxlint src/mainclean ·oxfmt --checkclean · fullsrc/mainsuite 17,969 passed / 57 skipped, 0 failures.One unrelated flake appeared in a single run and did not reproduce across two subsequent full runs; it's in a file this PR doesn't touch.
Screenshots
No visual change.
AI Review Report
Security Audit
Notes