Skip to content

fix(proxy): bound the events mirror and take it off the synchronous path - #56

Merged
Juliusolsson05 merged 5 commits into
mainfrom
fix/proxy-mirror-bound
Sep 27, 2026
Merged

Juliusolsson05 merged 5 commits into
mainfrom
fix/proxy-mirror-bound

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Refs Juliusolsson05/agent-code#372. App side: Juliusolsson05/agent-code#1332 (pointer bump plus the shared, rotation-safe debug-bundle reader).

Plan: agent-code docs/plans/2026-09-26-codex-proxy-mirror-bound.md.

Problem

ResponsesProxy.emit ran JSON.stringify and then appendFileSync for every event, before the listeners, with no cap and no drop policy.

  • A real 3 h Codex session wrote 103.5 MB. response-chunk lines were 91.3% of it.
  • Replaying the recorded median chunk 13,890 times through the old path took 1.9 s in total, with a worst single append of 74 ms, all on the Electron main process. Review C's reruns measured maxima of 4.8 ms and 9.5 ms, so a single maximum depends on the machine and its load; the synchronous blocking itself reproduces.
  • With about 15 sessions live, proxy/ was at 2.3 GB, and nothing rotated.

Change

src/proxy/eventsMirror.ts, EventsMirror:

  • Async and ordered. One fs.WriteStream per file; emit only serialises and queues.
  • Synchronous open, once per file. The file is opened with openSync and sized with fstatSync on that fd, so a stream is never pending and a rotation never waits for an open. Earlier, deferring the rotation let a startup burst or a restart at the cap push the file past its cap (review A3).
  • An exact per-file bound, decided before any rename. Each write is an optional drop or rotation marker plus the line, and it must fit within eventsFileMaxBytes (default 64 MiB). The rotation marker is deterministic, so fit is measured first, and a line that fits no file never rotates away kept events. When it would not fit, the file is renamed to <name>.1.jsonl and a fresh file starts with {kind:'mirror-rotated', rotatedBytes, rotations, droppedEvents, droppedBytes}. A line that cannot fit even in a fresh file is dropped and counted. A run holds at most two files.
    • Why rotate rather than follow Claude's omit-past-budget: on Codex the chunks are the bulk, and a debug bundle needs the newest ones.
  • An aggregate queue bound. Past 16 MiB of unwritten bytes, counted across every open stream and including markers, lines are dropped and counted. A mirror-dropped marker precedes the next written line, or is written at close.
  • Per-stream accounting. A rotated-out stream that fails gives back only its own share and never hangs flush() or stop(). Only the current file failing turns the mirror off, and it stays off. The error listener means a failure never crashes the main process.
  • stop() closes the mirror after the server teardown. flushMirror() is added. API.md is updated.

Tests

responsesProxy.mirrorBound.test.ts (18 tests), on the recorded Codex 0.157 chunk (testing/fixtures/proxy-mirror/models-chunk-2865.json):

  • emit does not write on the emitting call;
  • rotation keeps the newest events complete and byte-exact, each file within the cap, and rotatedBytes equals .1's size;
  • a start-of-run burst rotates within the cap;
  • a restart at the cap rotates on its first event;
  • an oversized line is dropped and marked;
  • drops are marked mid-run and at close, the close-time marker stays within the queue bound, and a cap smaller than a marker is never exceeded;
  • the queue is bounded across a rotation;
  • a failing rotated-out stream settles flush() and close(), and gives back only its own share of pending writes;
  • a line that fits no file never evicts kept events;
  • the rotation marker counts against both the file cap and the queue;
  • rotation happens at the 64 MiB default (a sparse pre-sized file, so no 64 MB is written);
  • an unwritable path stays off;
  • stop() closes (a later event is not written).

responsesProxy.mirror.test.ts is ported to await flushMirror(), with its assertions unchanged.

Review

  • Round 1: three Codex reviewers, all FIX-BEFORE-MERGE; every finding fixed in 7575ae3.
  • Round 2 (verification): B MERGE-READY. A and C found the fit-order eviction and two test gaps, fixed in 3e7e35e.
  • Round 3 (final verification by A and C): every round-2 fix was confirmed. One new minor (the close-time marker over the queue bound) is fixed in ceefb9c, and steering q62's tiny-cap edge in 6b8ca32. Both came after the final pass and are not re-verified.

The dispositions are in the PR comment.

Verification

  • tsc --noEmit is clean.
  • src/proxy: 59 tests. tsc --noEmit is clean. The mutant setting the default to 65 MiB is killed. The full package suite is run in the background at each head; src/transcript/* tests time out only under heavy machine load. Those files are untouched here, and they pass on CI.
  • Boundary: no app launch.

Residual

  • Codex re-fetches /v1/models (about 523 KB, every ~6.5 min). Rotation bounds it, but it shortens the kept window.
  • The latest prompt body can fall outside a 5 MiB bundle tail: agent-code#1336.

🤖 Generated with Claude Code

The mirror was one appendFileSync per emitted event, before any listener
ran, with no cap. A real 3 h Codex session wrote 103.5 MB (91% chunk
lines), and the recorded median chunk benchmarked at a worst single append
of 74 ms on the main process. With ~15 sessions live the proxy directory
grew by gigabytes a day.

EventsMirror writes through one ordered stream, drops and counts lines past
a 16 MiB queue (marking the gap in the file), and rotates to <name>.1.jsonl
at eventsFileMaxBytes (default 64 MiB), so a run holds at most two files
and always keeps its newest traffic. Rotation, not Claude's omit-past-budget,
because on Codex the chunks are the bulk and a debug bundle tails the newest
5 MiB. stop() flushes and closes the mirror.

Refs Juliusolsson05/agent-code#372

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Juliusolsson05 and others added 2 commits September 26, 2026 18:23
…tream accounting

Review A/B round 1:
- A1: a rotated-out stream's error zeroed the shared in-flight counter; the
  new stream's callbacks drove it negative and flush()/stop() hung. Writes
  are now accounted per stream, and only the current file failing turns the
  mirror off.
- A2: the queue bound was per stream, so a rotation doubled it. It is now on
  the total unwritten bytes across every open stream.
- A3/A4/B2: the per-file cap leaked through a pending async open (restart at
  the cap, startup burst), uncounted markers, and oversized lines. The file
  is opened synchronously (one syscall per file) and sized from that fd; a
  write is marker plus line and must fit; anything that cannot fit even a
  fresh file is dropped and counted.
- B1: drops at the end of a run are marked on close.
- B3/B4: tests pin the 64 MiB default, close-not-flush on stop, the marker's
  rotatedBytes, and that a failed mirror stays off.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…count against the queue

- C1: append() rotated first and only then found the line could not fit
  even a fresh file, so rejected lines evicted accepted events (a one-line
  cap left only markers). The rotation marker is deterministic, so the fresh
  file's first write is measured before anything is renamed, and a line
  that fits no file is dropped without rotating.
- A2: the queue check now includes the drop or rotation marker's bytes.
- A1/A3: tests pin per-stream accounting (the failure is delivered while
  the live write is certainly pending) and the marker in the cap check.
- The separate oversized-line queue guard was redundant with the aggregate
  check and is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner Author

Review disposition (final head 6b8ca32)

Two rounds with three Codex reviewers. Round 1 was FIX-BEFORE-MERGE from all three. Round-2 verification: B MERGE-READY; A and C raised fit-order and test gaps, all fixed. Round 3 was the final verification by A and C: both confirmed every round-2 fix and found one new minor, fixed below after the final pass, so it is not re-verified. That is the review cap.

Round 1, all fixed in 7575ae3

  • A1, major (a rotated-out stream's error zeroed the shared counter, so flush()/stop() hung): per-stream accounting.
  • A2, major (the queue bound was per stream): the bound is now on the aggregate unwritten bytes.
  • A3 / A4 / B2 / C2 (the cap leaked through a pending async open, uncounted markers, and oversized lines): a synchronous fd open sized from that fd, and every write is marker plus line and must fit.
  • B1 / C1 (drops at the end of a run left no marker): close() writes it.
  • B3 / B4 (the default cap, stop-closes, rotatedBytes, and fail-stays-off were unpinned): tests added.

Round 2

  • C1, major (rejected lines rotated away kept events): fixed in 3e7e35e. The rotation marker is deterministic, so fit is decided before any rename, and a line that fits no file is dropped without rotating. The test uses the reviewer's one-line-cap sequence.
  • A2, minor (the rotation marker bypassed the queue check): fixed. The marker counts against the queue.
  • A1 and A3, test gaps: fixed. Per-stream accounting is pinned with the failure delivered synchronously while the live write is pending, and the marker-in-cap boundary is pinned. Both reviewer mutants are killed.
  • Error handler as a no-op, surviving mutant: accepted as defense in depth. Node settles a destroyed stream's pending write callbacks itself, so the handler matters only for making the current file's failure turn the mirror off, which the unwritable-path test covers through the synchronous open. Kept.
  • close() without flush(), surviving mutant: equivalent, per reviewer C: endStream drains before close. The flush is kept for clarity.
  • B, declined to judge (the final drop marker can sit 65 bytes over the queue budget at close): accepted. It is at most one marker line, written once at teardown.
  • src/transcript/* timeouts: seen locally at load average ~140, in code this PR does not touch. The package CI, on a clean runner, passes them.

Round 3 (final pass: A and C)

  • A and C, the same minor (the close-time drop marker skipped the queue check, since a marker-only write has no line bytes, so one marker could sit over the bound): fixed in ceefb9c. close() drains the queue before writing the marker; a test pins the peak queue at close. Red on 3e7e35e. Not re-verified, because the step-7 cap was reached.
  • Test design fix: the 64 MiB default test wrote about 70 MB and timed out once under load. It now pre-sizes the file sparsely to just under the default, and checks that one recorded event rotates it and one event less does not. The mutant setting the default to 65 MiB is killed.
  • Surviving mutants restated by A and C: the error handler as a no-op (defense in depth; Node settles a destroyed stream's pending callbacks) and close() without its first flush() (equivalent, since endStream drains). Both accepted.

Steering q62 (after round 3)

  • A marker-only write skipped the queue check (bytes === 0), so a configured cap smaller than one marker line was still exceeded: fixed in 6b8ca32. Every write, markers included, now fits the queue or is not written, and the drops stay counted in stats(). Pinned with a 1-byte cap; the mutant restoring the bytes > 0 exception is killed. Not re-verified by the reviewers (cap reached); the narrow arithmetic and test are listed here for the manager's inspection.

🤖 Generated with Claude Code

Juliusolsson05 and others added 2 commits September 26, 2026 19:02
…ker; a light 64 MiB default test

- A/C round 3: close() wrote its drop marker onto a possibly full queue (a
  marker-only write has no line bytes to check), so one marker could sit over
  the queue bound. close() now drains first; on an empty queue the marker is
  the only thing queued.
- The 64 MiB default test wrote ~70 MB of real events and timed out under
  load. It now pre-sizes the file sparsely to just under the default, as a
  restart finds it, and checks that one recorded event rotates it and one
  event less does not.

Fixed after the final verification pass (step-7 cap): not re-verified by the
reviewers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
close() drains before its drop marker, but a marker-only append skipped the
queue check (bytes === 0), so a configured cap smaller than one marker line
was still exceeded. Every write, markers included, now fits the queue or is
not written; a line is counted as dropped, a marker that cannot fit leaves
the drops in stats(). Pinned with a 1-byte cap; the bytes>0 exception mutant
is killed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Final addendum (manager). Round 3 was the last review pass. Reviewers A and C each found one minor issue: the close-time drop marker could exceed maxQueuedBytes. Fixed in ceefb9c (close drains before the marker). Steering q62 then found that a cap smaller than a marker line could still be exceeded; fixed in 6b8ca329: a marker that does not fit is not written, the drop stays counted in stats(), and the test pins maxQueuedBytes: 1. Under the final-pass cap, the manager verified this arithmetic by inspection, as q62 allows. Exact-head CI at 6b8ca329 is green. Merging.

@Juliusolsson05
Juliusolsson05 merged commit f100f05 into main Sep 27, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working provider:codex Affects Codex integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant