fix(tracing): keep the end of a long session's trace, not the start - #397
Open
plombeer31 wants to merge 2 commits into
Open
fix(tracing): keep the end of a long session's trace, not the start#397plombeer31 wants to merge 2 commits into
plombeer31 wants to merge 2 commits into
Conversation
The per-session NDJSON cap was enforced from the wrong end. On reaching `tracing.trace.maxBytesPerSession` (10 MB by default) the sink appended one `trace_truncated` marker, set `overflown`, and dropped every later event for that session — permanently, and across restarts, because `resolveState` re-derived `overflown` from the file size. A long session therefore kept a pristine record of its opening minutes and nothing at all about its end, which is where "what went wrong" lives. The cap is now honoured by dropping the OLDEST events instead of refusing new ones. When an append would cross it the sink rewrites the file keeping its tail: whole leading lines go until the file is at half the cap, a `trace_truncated` marker is left at the seam carrying `droppedEvents` / `droppedBytes`, and writing continues. A leading `session_started` row is preserved so `trace list` and `trace replay` still find the header. There is still exactly one file per session, so `trace show/export`, the debug bundle, the issue report and the eval harness need no changes. The rewrite goes through a temp file in the same directory and an atomic rename, so the trace is never missing or half-written; a trim that cannot be completed degrades to the old stop-writing behaviour and never throws into the caller. Halving on each trim bounds the cost to an amortised two bytes rewritten per byte traced, and a file already at the target is never rewritten — a single event bigger than the cap is dropped on its own instead of triggering a rewrite per event.
Review follow-ups on the head-trimming sink. The cost bound the trim relies on was not enforced. `bytesWritten <= target` skips a rewrite only when the file can reach the target, and a trim cannot always get there: a preserved `session_started` plus the marker is irreducible. When that floor sits above the target the guard never fires and every single event pays a whole-file read + write + rename. Measured on a 1.4 KB header at a 2 KB cap: 54 rewrites over 59 events; at a 256-byte cap, 40 over 40. Two changes fix it — the header is only preserved while it is at most half the target (it exists to introduce a tail, not to crowd it out), and the size a trim actually produced is remembered so a file already at its floor is left alone. Same measurements after: 8 over 59, and 2 over 40 — with more of the session on disk, not less. Also: - flush the temp file before renaming it. `rename` is atomic for the directory entry, not for the data behind it, so a power cut after the rename could leave the trace pointing at an unwritten extent — losing the whole file rather than half of it. - sweep temp files a crashed trim stranded. A process killed between the write and the rename leaks up to half a cap of unredacted trace content under a name no reader lists, one per crash, forever. Only temps whose owning pid is gone are removed. - count a file's unterminated last line as a dropped event, so `droppedEvents` still adds up on a trace another process left mid-append. Tests. Every cap test ran at 600 bytes, where the target (300) is below `MARKER_BUDGET_BYTES` — the trim degenerates to "wipe everything but the header" and no surviving tail is ever cut, so the newline rounding was never executed. Cutting mid-line passed the whole suite. Added coverage at caps where a real tail survives (asserting each surviving row is byte-identical to the event that was emitted), for the rewrite bounds above, for the temp sweep, and for the marker's seq and the unterminated-line count. The `trace_truncated` marker no longer means "the trace stops here", so the eval postmortem's rendered line and two stale comments that still described the old cap behaviour are corrected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The report
From Discord
#feedback-and-bugs,thegreatteacher, 2026-09-09:The reporter said themselves they were not sure of the cause, and I have no reproduction of their exact session. What follows is the defect an audit of the trace sink turned up — it matches the symptom exactly, but treat it as "this is a real bug that produces that symptom", not as a confirmed reproduction of their case.
The mechanism
createNdjsonTraceSink(src/tracing/trace/trace-sink.ts) capped a session's NDJSON file attracing.trace.maxBytesPerSession— 10 MB by default (src/config/config-schema.ts). On reaching the cap it appended onetrace_truncatedmarker, setoverflown, and then silently dropped every later event for that session.resolveStatere-derivedoverflownfrom the existing file size, so the state survived restarts: once a session's trace hit the cap it was mute for good.The cap was therefore enforced from the wrong end. A long session kept a pristine record of its opening minutes and lost everything after — and the end is where "what went wrong" lives. An agent asked to diagnose its own long session would read a trace that stops well before the failure, which is exactly the complaint.
The fix
The cap is now honoured by dropping the oldest events instead of refusing new ones.
trace_truncatedmarker is left at the seam carrying new optionaldroppedEvents/droppedBytesfields, so no reader mistakes the first surviving row for the start of the session. The counts are cumulative and are read back off the previous marker rather than from memory, so they stay right across restarts and across repeated trims. The marker takes theseqandtsof the last dropped event, so the file stays ordered by both.session_startedrow is preserved when the file has one, sotrace liststill shows the start time andtrace replaystill finds the working directory after a trim.overflownis gone. A resumed session whose file is already over the cap — including one an older build left stranded at the cap — trims on its next event and keeps recording.One file per session, unchanged.
traceFilePathstill resolves to<dir>/<sessionId>.ndjsonand nothing rotates into sibling files. I checked all four readers —src/cli/trace-command.ts(show/replay/export),src/tui/debug-bundle/write-debug-zip.ts,src/tui/issue-report/write-report-zip.tsandeval-memory/harness/postmortem-trace.ts— and none needed a change: they read the whole file, skip unparsable lines, tolerate a missingsession_started, and none depend onseqstarting at 0 or being unique.redactTraceNdjsonpasses the new fields through, andtrace_truncatedis already in itserrors-level allowlist.trace-formatter.tsnow prints the two counts. TheTraceEventunion gained only two optional fields, so no exhaustive switch moved.Crash safety. The new content is written to a temp file in the same directory and
renamed over the original, so the trace is never missing or half-written if the process dies mid-rewrite. A trim that cannot be completed logs once and degrades to the old behaviour (stop writing) rather than losing the file; nothing throws into the caller.Cost
The trim is O(file size) and runs on the append path, so it is bounded two ways:
capbytes buyscap/2bytes of appends — an amortised ≤2 bytes rewritten per byte traced, no matter how long the session runs. At the 10 MB default: a ~10 MB read plus a ~5 MB write plus a rename, once per 5 MB of trace.Worst case is therefore one
cap-sized read +cap/2-sized write percap/2bytes of trace appended, synchronous on the emitting thread — the same thread that was already doing a synchronousappendFileSyncper event.Tests
Extended
src/tracing/trace/trace-sink.test.ts(2 cap tests replaced by 9):dropped + still-on-disk == every event ever handed to the sink;seq;session_startedsurvives a trim, with the marker right behind it;Vacuity check: stashing only the three
srcchanges (trace-sink.ts,trace-event.ts,trace-formatter.ts) and keeping the new test file gives 5 failed | 6 passed — the five that fail onorigin/mainare trims the head at the cap instead of going mute, states the loss in the marker and keeps it growing across trims, preserves session_started so trace list/replay still find the header, resumes writing into a file that is already over the cap, and drops a single event larger than the cap without rewriting. The four that pass on both are deliberate: two are the pre-existing suite, one is the byte-identical no-regression guard, and two are property guards (file stays under the cap, file stays parseable and ordered) that the old code also satisfied by never writing at all.Also updated
AGENTS.md§"Traceability and replay", which documentedtrace_truncatedas a final marker after which events are dropped.