Repository navigation
fix(sse): shallow per-target copy for the combo attempt body (#7847) - #8553
diegosouzapw merged 2 commits into
Conversation
b43168b to
b13d8e8
Compare
|
Reviewed #8553 by checking out the PR branch in an isolated worktree against origin/release/v3.8.49 and running the new test suite both ways. On unmodified combo.ts (test file added, production code untouched), 2 of the 6 new tests fail as expected: the round-robin cross-target leak reproduces exactly as described (target N+1 receives model "mutated-by-" instead of the original), and the shallow-copy-shape test correctly fails since the base still deep-clones. With your fix applied, all 6 pass. typecheck:core, check:any-budget:t11, and check:file-size (combo.ts is at 3641/3642 lines, under the frozen cap) are all clean on the PR branch. The file-size violations I do see (providers/page.tsx, tokenHealthCheck.ts) and the check:db-rules reds are all in unrelated files — confirmed pre-existing on the base tip, not introduced here. One item to close before merge: your new test file exercises circuitBreaker.ts (it resets the breaker in beforeEach), and check:mutation-test-coverage --strict is strict about that mapping. I re-measured on the pristine tip: it reports 3 drift entries there (accountFallback.ts, error.ts, comboPredicates.ts) and 4 on your branch — so the circuitBreaker.ts entry is new drift from this PR. Since #8538 is about to bring that gate to zero, could you add tests/unit/combo-attempt-body-isolation-7847.test.ts to stryker.conf.json's tap.testFiles so this doesn't immediately re-red it? One heads-up on merge ordering: #8555 and #8558 each independently reformat the exact same computeCompatRejectedTargets(...) call (byte-identical wrap) that you also reformat here, so whichever of the three merges last will hit a small mechanical conflict at that spot — trivial to resolve, no semantic risk since it's pure formatting. Good catch finding the round-robin leak while working the shallow-copy change — that's a real production bug (Background Task Redirection bleeding into the next target's model) that the deep clone was masking. |
…uzapw#7847) combo.ts deep-cloned the request body for every target. On a 3.05 MiB agent request that is 9.53 MiB at 3 targets, and it scales linearly: 3 targets 9.53 MiB (3.12x wire) -> ~0.001 MiB 5 targets 15.89 MiB (5.19x wire) -> ~0.000 MiB 10 targets 31.78 MiB (10.39x wire) -> ~0.001 MiB The isolation it bought only ever needed to contain TOP-LEVEL SCALAR writes. The full mutation surface on this path is two assignments: combo.ts bodyRecord.max_tokens = ... (reasoning buffer) chatCore.ts body.model = model (Background Task Redirection T41) Nothing mutates the nested payload; applyCompression and injectUniversalHandoffBody both return new objects (verified empirically -- neither touches its input, and both tolerate a frozen one). So a fresh top-level object per target gives identical isolation while sharing the expensive messages/tools arrays. Also fixes a REAL cross-target leak in handleRoundRobinCombo. It already used a shallow copy, but took it only when the reasoning buffer actually changed max_tokens -- every other attempt shared the caller's object outright. The new test reproduces it on the unmodified code: target 2 received model "mutated-by-openai/gpt-4o-mini". In production that is a Background Task Redirection on one round-robin target rewriting body.model for the next. The copy is now unconditional. The invariant is pinned by tests rather than by a comment listing mutation sites, so the clone strategy can change again without anyone re-deriving them by hand: - a target's in-place write must not leak into the next (priority / fill-first / round-robin; the stub reproduces chatCore's body.model write) - the caller's body is never mutated - the per-target copy stays shallow (targets share one messages array) - freeze probe: combo's own body handling performs no in-place writes
…tap.testFiles The new test resets the circuit breaker in beforeEach, so it counts as a covering test for src/shared/utils/circuitBreaker.ts. Without registering it, check:mutation-test-coverage --strict reported a 4th drift entry that was not there on the pristine tip -- new drift introduced by this PR. Registered in sorted position; the gate is back to the 3 pre-existing entries (accountFallback.ts, error.ts, comboPredicates.ts) that diegosouzapw#8538 addresses.
aed8bfb to
e480c70
Compare
|
Both items are handled. stryker: Merge ordering: the Also rebased onto |
|
Correction to my comment above: I measured it on pristine detached checkouts with an empty working tree: The gate is in |
|
Thanks @MumuTW — merged into |
…uzapw#7847) (diegosouzapw#8553) * fix(sse): shallow per-target copy for the combo attempt body (diegosouzapw#7847) combo.ts deep-cloned the request body for every target. On a 3.05 MiB agent request that is 9.53 MiB at 3 targets, and it scales linearly: 3 targets 9.53 MiB (3.12x wire) -> ~0.001 MiB 5 targets 15.89 MiB (5.19x wire) -> ~0.000 MiB 10 targets 31.78 MiB (10.39x wire) -> ~0.001 MiB The isolation it bought only ever needed to contain TOP-LEVEL SCALAR writes. The full mutation surface on this path is two assignments: combo.ts bodyRecord.max_tokens = ... (reasoning buffer) chatCore.ts body.model = model (Background Task Redirection T41) Nothing mutates the nested payload; applyCompression and injectUniversalHandoffBody both return new objects (verified empirically -- neither touches its input, and both tolerate a frozen one). So a fresh top-level object per target gives identical isolation while sharing the expensive messages/tools arrays. Also fixes a REAL cross-target leak in handleRoundRobinCombo. It already used a shallow copy, but took it only when the reasoning buffer actually changed max_tokens -- every other attempt shared the caller's object outright. The new test reproduces it on the unmodified code: target 2 received model "mutated-by-openai/gpt-4o-mini". In production that is a Background Task Redirection on one round-robin target rewriting body.model for the next. The copy is now unconditional. The invariant is pinned by tests rather than by a comment listing mutation sites, so the clone strategy can change again without anyone re-deriving them by hand: - a target's in-place write must not leak into the next (priority / fill-first / round-robin; the stub reproduces chatCore's body.model write) - the caller's body is never mutated - the per-target copy stays shallow (targets share one messages array) - freeze probe: combo's own body handling performs no in-place writes * test(sse): register the combo attempt-body isolation test in stryker tap.testFiles The new test resets the circuit breaker in beforeEach, so it counts as a covering test for src/shared/utils/circuitBreaker.ts. Without registering it, check:mutation-test-coverage --strict reported a 4th drift entry that was not there on the pristine tip -- new drift introduced by this PR. Registered in sorted position; the gate is back to the 3 pre-existing entries (accountFallback.ts, error.ts, comboPredicates.ts) that diegosouzapw#8538 addresses.
…uzapw#7847) (diegosouzapw#8553) * fix(sse): shallow per-target copy for the combo attempt body (diegosouzapw#7847) combo.ts deep-cloned the request body for every target. On a 3.05 MiB agent request that is 9.53 MiB at 3 targets, and it scales linearly: 3 targets 9.53 MiB (3.12x wire) -> ~0.001 MiB 5 targets 15.89 MiB (5.19x wire) -> ~0.000 MiB 10 targets 31.78 MiB (10.39x wire) -> ~0.001 MiB The isolation it bought only ever needed to contain TOP-LEVEL SCALAR writes. The full mutation surface on this path is two assignments: combo.ts bodyRecord.max_tokens = ... (reasoning buffer) chatCore.ts body.model = model (Background Task Redirection T41) Nothing mutates the nested payload; applyCompression and injectUniversalHandoffBody both return new objects (verified empirically -- neither touches its input, and both tolerate a frozen one). So a fresh top-level object per target gives identical isolation while sharing the expensive messages/tools arrays. Also fixes a REAL cross-target leak in handleRoundRobinCombo. It already used a shallow copy, but took it only when the reasoning buffer actually changed max_tokens -- every other attempt shared the caller's object outright. The new test reproduces it on the unmodified code: target 2 received model "mutated-by-openai/gpt-4o-mini". In production that is a Background Task Redirection on one round-robin target rewriting body.model for the next. The copy is now unconditional. The invariant is pinned by tests rather than by a comment listing mutation sites, so the clone strategy can change again without anyone re-deriving them by hand: - a target's in-place write must not leak into the next (priority / fill-first / round-robin; the stub reproduces chatCore's body.model write) - the caller's body is never mutated - the per-target copy stays shallow (targets share one messages array) - freeze probe: combo's own body handling performs no in-place writes * test(sse): register the combo attempt-body isolation test in stryker tap.testFiles The new test resets the circuit breaker in beforeEach, so it counts as a covering test for src/shared/utils/circuitBreaker.ts. Without registering it, check:mutation-test-coverage --strict reported a 4th drift entry that was not there on the pristine tip -- new drift introduced by this PR. Registered in sorted position; the gate is back to the 3 pre-existing entries (accountFallback.ts, error.ts, comboPredicates.ts) that diegosouzapw#8538 addresses.
2 participants
The largest of the #7847 reductions, plus a real cross-target leak found on the way. Pairs with #8549 (benchmark) and #8550 (entry clone).
Numbers
combo.tsdeep-cloned the request body for every target. On a 3.05 MiB agent request:The deep clone scaled linearly with the target count; the shallow copy is constant and effectively free.
Why a shallow copy is sufficient
The isolation the clone bought only ever needed to contain top-level scalar writes. The complete mutation surface on this path is two assignments:
Nothing mutates the nested payload.
applyCompressionandinjectUniversalHandoffBodyboth return new objects — verified empirically, not by reading: probed with a before/after deep-compare and a frozen input, neither touches its argument. So a fresh top-level object per target gives identical isolation while sharing the expensivemessages/toolsarrays.The leak this found — round-robin was already broken
handleRoundRobinComboalready used a shallow copy, but took it only when the reasoning buffer actually changedmax_tokens:Every other attempt shared the callers object outright, so
chatCoresbody.model = modelon one target rewrote the model for the next. The new test reproduces it on the unmodified code:priorityandfill-firstpassed, because the deep clone was covering for them. In production this is a Background Task Redirection firing on one round-robin target and the next target's upstream request carrying the degraded model name. The copy is now unconditional.I raised this as an unconfirmed risk during assessment; the test settled it. Fixing it here rather than separately because it is the same defect (per-target copy discipline), the same file, and the same test file covers all strategies.
Tests pin the invariant, not the implementation
Deliberately written so the clone strategy can change again without anyone re-deriving mutation sites by grep:
body.modelwrite, which is the real downstream mutation this exists to containhandleComboChattreatsbodyas read-onlymessagesarray, and the message objects are still the caller'shandleSingleModelis stubbed, so this covers combo's own body handling, not chatCore or the executors — test 1 is what covers a mutating downstream.Verification
typecheck:core,check:cycles,check:any-budget:t11,check-file-sizePre-existing failures confirmed red on
upstream/release/v3.8.49withcombo.tsuntouched, so not from this PR:tests/unit/autoCombo/provider-family-combos,tests/unit/autoCombo/tieredRotation(vitest-authored, they fail under node:test), andlive repo: no NEW unexported db modules beyond the frozen allowlist.Note the
check-file-sizegate is shrink-only andcombo.tsis frozen at 3642 lines — the explanatory comments had to be trimmed to fit. That is why they are terser than the reasoning warrants; the test file carries the full rationale.Inherited base-red (not from this PR)
release/v3.8.49is red on lint (stale suppression, fixed by #8544) and file-size (providers/page.tsx,tokenHealthCheck.ts— fixed by #8532 / #8524).