feat(desktop): preserve bounded main-process recovery evidence - #3543
Conversation
Persist a redacted latest-log snapshot at a bounded cadence so the next launch can offer copyable diagnostics when the prior main process did not complete a clean shutdown. Keep the signal intentionally cause-agnostic, local-only, and disposable after the first recovery prompt. Generated-by: Codex
Treat the active journal file as the sole unclean-run marker, removing it synchronously on clean shutdown. Keep pending evidence when the native recovery dialog cannot be presented so a later launch can retry. Generated-by: Codex
d07b8d6 to
6d6d30d
Compare
Start the seven-day retention window when an interrupted run becomes pending, using one metadata update instead of a periodic heartbeat. Keep corrupt high-cardinality records bounded and centralize the present-before-discard policy so failed native prompts remain retryable. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Approving ff576a384d16056efd22ac92565f1fd12c8aa5fe. Both required checks are completed / success bound to that exact SHA. No P0–P3.
An independent line reviewed this with one question up front — "the PR title says bounded, so where is the bound, who enforces it, and what happens at the edge" — and produced the enforcement table before looking for defects. I then re-derived the load-bearing pieces myself at the gate rather than taking them on report. All four held:
boundedLogTailkeeps the newest, not the oldest. It walkslogsfrom the last index backwards, accumulating until the next entry would crossMAIN_PROCESS_RECOVERY_LOG_MAX_BYTES, then reverses. This is the one that had to be right — the last few lines before a crash are the whole point of the artifact, and a naiveslice(0, n)would have thrown away exactly the evidence worth keeping. The test pinningentry 399present andentry 0absent is the right assertion.- The write path is genuinely atomic and hostile-input aware:
writeFileSyncwithflag: 'wx'andmode: 0o600to a temp path, thenrenameSync, with the temp removed infinally. Reads go throughlstatSyncrejecting symlinks andO_NOFOLLOW, so a symlink planted at the evidence path is refused twice rather than followed. RUN_MAX_BYTESis headroom, not an unenforced limit. It only contributes toEVIDENCE_MAX_BYTES, and I checked why that is safe: every field onrecoveryRunSchemais aboundedStringSchemacapped at 1 KiB, so the run record cannot approach 16 KiB by construction. Worth knowing the reason, since "a constant that is never checked directly" usually is a finding.- The 7-day clock starts at discovery, not at the crash —
utimesSync(activePath, promotedAt, promotedAt)immediately before the rename to pending. That matches what the PR says, and it is the right choice: a crash that happened while the machine was off should not expire before anyone has seen it.
On the single pending slot: a newer crash overwrites an unviewed pending record. That is a deliberate design choice, not an oversight, and it is the correct one here — the alternative is unbounded growth in exactly the situation where the app is already failing repeatedly. Worth keeping in mind if crash-loop diagnosis ever becomes a goal, since only the most recent loop iteration survives.
On scope: this is one journal, not a second crash-collection system, and the -30 is mostly app.exit(0) → app.quit() so that a normal quit can mark itself clean. A failed write disables the journal for the remainder of the process rather than retrying into a broken disk, and appendUncaughtMainProcessError swallows its own errors so the evidence path cannot become the thing that kills the process. Diagnostic code that can take down the process it is diagnosing is the classic failure of this feature, and it is avoided here.
Seven distinct failure hypotheses were raised and each withdrawn against specific evidence — wrong-end truncation, write failure as a new crash source, torn JSON or symlink substitution, double-recording and replay, the age bound interacting with the slot, session-end listener target, and post-failure disabling. No P3s were manufactured to look thorough, which I appreciate more than a padded list.
Coverage disclosure: one independent review line plus my own verification at the gate. For a change of this size I would normally want a second independent line; I am approving on one because the line's claims were reproducible and I checked the load-bearing ones directly. Stated so the record is accurate rather than flattering.
Reviewed at 2026-08-23 12:50 UTC.
Summary
English
简体中文
Fixes #3491
Verification
npm run lintnpm run format:checknpm run buildnpm run typechecknpx knip --workspace apps/desktopAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with design analysis, implementation, tests, and review. The human contributor remains responsible for the contribution.
Checklist
Does this PR entail a change in behavior?