Skip to content

Fix confirmed findings from the #389 adversarial review - #414

Merged
Juliusolsson05 merged 2 commits into
mainfrom
fix/incident-forensics-review-fixes
Jul 6, 2026
Merged

Juliusolsson05 merged 2 commits into
mainfrom
fix/incident-forensics-review-fixes

Conversation

@Juliusolsson05

Copy link
Copy Markdown
Owner

Follow-up to #389 (incident forensics hardening). The multi-agent adversarial review of that PR confirmed 15 findings; this fixes all 6 with behavioral impact plus the load-bearing comment rot. One PR, per "no micro-PRs" convention.

Behavioral fixes

src/main/storage/feedDebugLog.ts — 5 fixes to the per-file cap

  1. Pre-existing over-cap files never got a tombstone. Priming tombstoneWritten from startingBytes >= cap routed the first post-upgrade batch into the already-capped short-circuit, which never writes a marker — so the 60–300 MiB pre-cap-era files that motivated the cap (Incident forensics hardening: catch fast heap bursts, cap feed-debug, journal retention, detect prior-run OOMs #388) had their appends dropped with zero on-disk trace. Now primes false; the first batch flows through the cap branch, writes the tombstone, then latches.

  2. Tombstone write failure silently latched as "capped". The bare catch {} still set tombstoneWritten = true, so an ENOSPC — disk-full being the exact pressure the cap exists for — produced a file with no marker that the code believed was cleanly marked. Now latches only on success; on failure the next batch retries (one small append attempt per batch).

  3. stat() failures failed OPEN at 0 bytes. Any non-ENOENT error (EACCES/EIO/EMFILE — fd exhaustion is likeliest under exactly the load the cap targets) primed the counter at 0 and let an already-300 MiB file grow by another full cap's worth. loadInitialFileBytes now returns null for unknown-size; the caller fails CLOSED — unprimes and returns without advancing the cursor, so the renderer resends and the next batch re-stats.

  4. forgetFeedDebugSession racing the stat() await leaked map entries. get → await → set meant a forget firing during the await deleted a not-yet-existing key (no-op), then the continuation inserted cap state for a dead session — a permanent leak since session UUIDs never recur. The placeholder now lands in the map BEFORE the await, and an identity re-check after it detects the forget and bails.

  5. On-disk droppedEntriesSoFar froze at the first batch's count while capped sessions kept flooding for hours. A refreshed tombstone row is now appended each time the total doubles since the last row — log-bounded (~20 rows for a million drops), and the last row is always within 2× of the truth.

  6. Batch drops were all-or-nothing. A 40 MiB batch whose last bytes crossed the cap discarded all 40 MiB even though most entries fit. Entries are now serialized per-line and the cap cuts within the batch; the admitted prefix is written in the same writeFile append as the tombstone (JSONL prefixes are always valid, so no torn interleaving is possible).

src/main/incident/previousRunClassifier.ts — minidump regression

main_oom_suspected dropped minidumpPath from evidence. V8 fatal aborts raise SIGABRT, so Crashpad almost always writes a minidump for the same crash the diagnostic report describes. The new OOM branch returned before the minidump lookup ran — losing native/minidumpPath for exactly the crash class the branch was added to improve (pre-#389 code surfaced both). The OOM branch now runs findRecentMinidump too.

Comment/structure fixes

  • previousRunClassifier.ts — both "trigger is a top-level string" comments were factually wrong (trigger lives under header). Comments now describe the real shape and explicitly warn against "simplifying" the flat regex scan to JSON.parse(prefix).trigger, which returns undefined and would silently kill every OOM classification.
  • heapWatchdog.ts — the claim that the ~1.2 GB abort class "was already covered by the ratio" was algebraically false (ratio arm resolves to ~3.0 GiB on the 2026-05-11 host; both old and new trips sit above 1.2 GB). Replaced with an honest coverage note: low-peak aborts are attributed post-hoc via reportOnFatalError + the classifier; polling only buys a pre-crash snapshot for climbs crossing ~1.5 GiB.
  • index.ts — setDebugRetentionJournal(appRunJournal) moved BEFORE appRunJournal.start(). start() fires the run's first prune (incident-run-start), and with the sink wired after the await the first prune's journal entry was race-free only by accident (the prune's statfs() I/O losing to the synchronous continuation). Any future await inserted between them would have silently dropped the breadcrumb. Wiring first makes the ordering structural instead of incidental; the old comment (which also misidentified 'startup' as the first prune) is gone.

Verification

Applied to a clean main checkout: tsc -b tsconfig.node.json exit 0, tsc --noEmit -p tsconfig.web.json 0 errors. No new tests per repo convention.

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits July 6, 2026 18:07
The multi-agent review of PR #389 (incident forensics hardening)
confirmed 15 findings, 6 with real behavioral impact. All are
addressed here, plus the load-bearing comment rot it flagged.

feedDebugLog.ts (5 behavioral fixes):

* Pre-existing over-cap files never got a tombstone. Priming
  tombstoneWritten from `startingBytes >= cap` routed the first
  post-upgrade batch into the already-capped short-circuit, which
  never writes a marker — so the 60-300 MiB pre-cap-era files that
  MOTIVATED the cap (#388) had appends dropped silently. Now primes
  false; the first batch flows through the cap branch, writes the
  tombstone, then latches.

* Tombstone write failure silently latched as "capped". The bare
  catch around the tombstone append still set tombstoneWritten=true,
  so an ENOSPC (the exact pressure the cap exists for) produced a
  file with no marker that the code believed was cleanly marked.
  Now latches only on success; on failure the next batch retries the
  tombstone (one small append attempt per batch).

* stat() failures failed OPEN at 0 bytes. Any non-ENOENT error
  (EACCES, EIO, EMFILE — fd exhaustion is likeliest under exactly
  the load the cap targets) primed the counter at 0 and let an
  already-300 MiB file grow by another full cap. loadInitialFileBytes
  now returns null for unknown-size; the caller fails CLOSED —
  unprimes and returns without advancing the cursor so the renderer
  resends and the next batch re-stats.

* forgetFeedDebugSession racing the stat() await leaked map entries.
  get → await → set meant a forget during the await deleted a
  not-yet-existing key, then the continuation inserted state for a
  dead session — permanent leak since session UUIDs never recur.
  The placeholder now lands in the map BEFORE the await; an identity
  re-check after it detects the forget and bails.

* On-disk droppedEntriesSoFar froze at the first batch's count.
  Capped sessions kept flooding for hours while the tombstone
  reported the first few hundred drops. A refreshed tombstone row is
  now appended each time total drops DOUBLE since the last row —
  log-bounded (~20 rows for a million drops) and the last row is
  always within 2x of the truth.

* Batch drops were all-or-nothing. A 40 MiB batch whose LAST bytes
  crossed the cap discarded all 40 MiB even though most entries fit.
  Entries are now serialized per-line and the cap cuts WITHIN the
  batch: the admitted prefix is written in the same append as the
  tombstone (JSONL prefixes are always valid; no torn interleaving).

previousRunClassifier.ts:

* main_oom_suspected dropped minidumpPath — a regression. V8 fatal
  aborts raise SIGABRT, so Crashpad writes a minidump for the same
  crash the diagnostic report describes. The new OOM branch returned
  before the minidump lookup, so evidence lost `native`/`minidumpPath`
  for exactly the crash class the branch was added to improve, and
  downstream symbolication read undefined. The OOM branch now runs
  findRecentMinidump too.

* Fixed both "trigger is a top-level string" comments: trigger lives
  under `header` in Node diagnostic reports. The regex works because
  it is a flat text scan; the comments now say so explicitly and warn
  against "simplifying" to JSON.parse(prefix).trigger, which returns
  undefined and would silently kill every OOM classification.

heapWatchdog.ts:

* The threshold comment claimed the ~1.2 GB abort class "was already
  covered by the ratio" — algebraically false on the 2026-05-11 host
  (ratio arm resolves to ~3.0 GiB; both old and new trips sit above
  1.2 GB). Replaced with an honest coverage note: low-peak aborts are
  attributed post-hoc by reportOnFatalError + the prior-run
  classifier; polling only buys a pre-crash snapshot for climbs
  crossing ~1.5 GiB, and why the trip is not lowered further.

index.ts:

* setDebugRetentionJournal(appRunJournal) moved BEFORE
  appRunJournal.start(). start() fires the run's first prune
  (incident-run-start), and with the sink wired after the await the
  first prune's journal entry was race-free only because the prune's
  statfs() I/O happened to lose to the synchronous continuation —
  any future await inserted between them would silently drop the
  breadcrumb. Wiring first makes the ordering structural. The old
  comment (which misdescribed 'startup' as the first prune) is gone.

Verification: applied to a clean main checkout — `tsc -b
tsconfig.node.json` exit 0, `tsc --noEmit -p tsconfig.web.json`
0 errors. No new tests per repo convention.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The adversarial review's PLAUSIBLE verdicts were initially set aside;
auditing all 16 against the confirmed-tier fixes found four real gaps
that cost a few lines each:

* AppRunJournal.start(): set process.report.directory BEFORE
  reportOnFatalError. In the old order, a throw from the directory
  setter left the flag armed with Node's default directory (CWD), so
  a real OOM wrote its report where the prior-run classifier never
  looks — silently reverting to force_quit_or_power_loss. Directory-
  first means a throw leaves the flag off entirely; the minidump
  path still attributes the crash.

* AppRunJournal.stop(): latch started=false. record() kept accepting
  events after stop(), and the debug-retention prune chain holds
  this journal as its sink for the process lifetime — a late .then
  in the will-quit window pushed events into `pending` with every
  drain timer already cleared. Dead weight, never written.

* previousRunClassifier: match Node's real report name shape
  (report.<date>.<time>.<pid>.<tid>.<seq>.json) instead of loose
  report.*.json. A future artifact named e.g. report.summary.json
  sorted lexically last, won the newest-pick, had no trigger — and
  shadowed the genuine fatal-error report sitting next to it.

* previousRunClassifier: slice the prefix buffer to readSync's
  actual return value instead of stringifying the whole zero-filled
  allocation on a short read.

Remaining PLAUSIBLE findings intentionally NOT acted on: the regex-
anchoring concerns (guarded by JSON escaping + header-first ordering
in every known Node version; now documented at the regex), the
missing prune call in the already-capped branch (no new bytes → no
new information, and the 5-minute cooldown makes it moot), and the
trigger-unreadable fallthrough (nodeReportPath still lands in
evidence, so triage sees the report even when classification stays
conservative).

Verification: full branch diff applied to a clean main checkout —
tsc -b node exit 0, tsc web 0 errors.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Juliusolsson05
Juliusolsson05 merged commit 74b80f0 into main Jul 6, 2026
@Juliusolsson05
Juliusolsson05 deleted the fix/incident-forensics-review-fixes branch July 6, 2026 16:11
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.

1 participant