feat(proxy): coalesce the duplicate session-start sidecar send (opt-in) - #337
feat(proxy): coalesce the duplicate session-start sidecar send (opt-in)#337Gunther-Schulz wants to merge 2 commits into
Conversation
…ated off CC issues one sidecar request twice, 6-25 ms apart on 47/47 measured pairs, with distinct upstream request-ids and two completed usage-log records -- both answered, both charged. Dropping the second is unavailable, since two client requests are in flight and each is owed a response, so the only safe shape is one upstream call serving both callers. Four conditions, all required, and the mid-session duplicate class -- where the second send is a legitimate retry -- fails on nMsg alone, which is the discriminator the row asked for. Fan-out sits at the RESPONSE WRITER, not at the upstream reader: the extension pass and the telemetry record run once, so both callers receive byte-identical post-pipeline output. Tee-ing the raw upstream would hand the follower unmutated bytes while the leader got the pipeline's, and fidelity outranks cache here. TWO ARMS WERE NOT DISCRIMINATING AND THE MUTATION PROOF IS WHAT SHOWED IT. Disabling the byte-identity compare left every arm green -- differing bodies produce a different key and never reach the compare, so the branch's only falsifying input is a sha256 collision. It is removed rather than kept as an unprovable predicate; the full-length key IS condition 3. Disabling the window left its arm green too, because that arm awaited both requests sequentially and the leader had already left the map. Rewritten to fire the second send while the first is still in flight but past the window, and it now goes red on exactly that mutation. Gated OFF (CACHE_FIX_COALESCE_SIDECAR). Enabling is a separate declared act: what a coalesced follower does to duplicate-billing's own measurement is a decision, not a detail -- with no outcome record the follower reads as the unanswered first send of a retry streak, which inverts the signal the mitigation is judged by. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011q6Zo7yKHCGokK4fT8LTPF
One self-contained helper, carried because the test in the previous commit imports it: every producer gets one run root per process, removed on exit, on throw, and on SIGINT/SIGTERM/SIGHUP. It deliberately never deletes anything it did not create, so it is a helper and not a reaper. Its own header states the two cases it cannot cover — SIGKILL and SIGABRT run no exit handlers — because a leftover run root then means a child died hard, which is a finding about that child rather than about this helper. We learned that the expensive way: three sessions hunted a leak that turned out to be a test's own deliberate out-of-memory crashes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q
…on because a gate said so PR cnighswonger#337 carries the coalescer itself, opt-in behind its env gate, with the measured population in the body — 144 pairs, 114 streaks, 55 billed twice — and the retry class stated as the design constraint rather than as a caveat, since suppressing those would leave a real request unanswered. The record half did not make the slice, and that was decided by the gates rather than by taste: the cherry-pick conflicted on fork-only paths that do not exist upstream, and slice-preflight then named five static imports in its test reaching the census and harvest stack. Ported anyway it would have widened this PR into the verification stack's topic. The PR body says so, so the omission is a stated boundary rather than something a reviewer discovers. Also filed cnighswonger#336, the one-file fix for upstream's hardcoded test port, proven as a controlled pair with the port deliberately held. Separate PR rather than a rider, because upstream asked for unrelated changes to be lifted out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q
|
Reviewed at f4ca4d1. Load-bearing proxy request-path change; opt-in via What's strong
Blocking1. Shared-proxy billing/auth leak — not documented. The coalesce key is
This is the auth-isolation concern that shared-proxy operators need called out explicitly. The mechanism ships with the right gate posture, but the README has no section for Two acceptable resolutions, either fine:
I don't have a preference between the two. Non-blocking suggestions2. 3. 4. Off-by-default merits a README section anyway. Even for opt-in mechanisms, the repo convention (image-retry, session-budget) is a full section with env-var table and detection conditions. A reader browsing the README won't find this feature without one. VerdictApplying Load-bearing per CLAUDE.md, so this will also need Codex review — I'll ask AITL to dispatch once the shared-proxy documentation lands. — Proxy Builder |
vsits-proxy-builder
Bot
added
the
changes-requested
1 participant
Claude Code sends the same session-start sidecar request twice, ~6–25 ms apart, byte-identical. Both are answered, and both are charged. This adds an opt-in coalescer that collapses the pair into one upstream call and fans the single response out to both waiting clients.
Off by default. It does nothing unless
CACHE_FIX_COALESCE_SIDECAR=1is set.The measurement
Not an inference from the code — this is what one day's traffic on one machine looks like, joined against the completion records by request id:
The session-start class specifically: haiku,
nMsg=1,max_tokens=32000, arriving at capture lines 3–5, 6–25 ms apart. 47 instances in one measurement window.The other half of that population is not this bug and must not be collapsed: mid-session duplicates are real client retries whose first attempt has no completion record. Suppressing those would leave a genuine request unanswered. That distinction is the whole design constraint here.
What it does
Four conditions must all hold before anything is coalesced, and they exist to keep the retry class out:
COALESCE_WINDOW_MS);The follower then waits on the leader's response and receives the same bytes. A miss on any condition forwards normally — the failure mode is "one extra upstream call", never "a dropped request".
Tests
test/duplicate-coalesce.test.mjs, 15 cases, green: the happy path, each of the four conditions removed singly (each must forward rather than coalesce), the fan-out delivering identical bytes to both waiters, and the error paths where the leader fails.npm teston this branch: 1796 tests, 1795 pass, 0 fail (one skip, pre-existing).Scope, and what deliberately is not here
Two files plus one helper. The second commit carries
tools/tmpdir.mjsonly because the test imports it — a per-process temp-run-root helper, self-contained, no other caller in this slice.Our fork also emits a telemetry record when a duplicate is suppressed, so a coalesced send cannot later read as an unanswered one. That change is not in this PR: its test depends on the capture/census tooling, which is a separate topic. Ported here it would have widened this PR into that one, so it rides with that work instead.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RMiYvNxKq6G9gfJMzArm4q