Repository navigation
fix(engine): charge the carried conversation from the map that holds it - #2278
Merged
Merged
Conversation
`admit_interpreted_generate_request` gated the carried prefix length on `workflow_sessions` (engine-side, keyed by `SessionId`) while reading its value from `workflow.worker.session_state` (pipeline-side, keyed by `(String, cell)`). `session_prepended_prompt_len` is already total: it returns 0 for a session it does not know. So when the two maps agree the gate is redundant -- the callee returns the same 0 the `else` arm supplies -- and when they diverge the gate discards a correct non-zero count and forces `carried = 0`. There is no input for which it improves the outcome; its only reachable effect is to under-reserve, which is precisely what the function's own comment warns against. Read it unconditionally instead. The only caller is the workflow path, so no non-workflow hot path pays for the removal. This was latent, not live: `close_session` maintains both sides, and the caller-side hazard is already closed. But the gate's correctness depended on an invariant maintained in a different module from the one reading it, and nothing asserted that invariant -- so a future close/reset/eviction path that skipped `forget_session` would have silently under-reserved the KV budget on the longest conversations, failing as an admission that should have been a refusal rather than as an error. The agreeing case cannot test a redundant gate, which is why removing it left the suite green. The new test builds the divergence directly and fails without this change: a turn needing 5 tokens of budget was admitted against a budget of 3. Each arm uses its own engine, since `admit_*` takes a reservation that only `complete()` returns and a shared engine would let the admitted arm pay for the other arm's refusal. Closes #2251 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 27, 2026 10:05
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2278 +/- ##
==========================================
+ Coverage 80.75% 81.10% +0.34%
==========================================
Files 434 434
Lines 222170 222170
Branches 222170 222170
==========================================
+ Hits 179416 180188 +772
+ Misses 36824 36050 -774
- Partials 5930 5932 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
Closes #2251.
The defect
admit_interpreted_generate_requestgated the carried prefix length on one map and read its value from another:session_prepended_prompt_lenis already total —let Some(cell) = … else { return 0 }and.unwrap_or(0), so it answers0for a session it does not know. That makes the gate'selsearm identical to the callee's own answer:carried = 0.No input exists for which the gate improves the outcome. Its only reachable effect is to under-reserve — exactly what the function's own comment warns about:
The fix
Read it unconditionally. The sole caller is the workflow path, so no non-workflow hot path pays for the removal. Production change is 6 lines out, 3 lines in.
Why this needed a new test rather than an existing one
This is latent, not live —
close_sessionmaintains both sides, and the caller-side hazard is already closed. So the agreeing case cannot distinguish the two versions, which is precisely why deleting the gate leaves the rest of the suite green. A change that no test can fail is a change with no evidence.The new test builds the divergence directly: create a session, seed its conversation, then drop it from the engine-side map only — the state any future close/reset/eviction path that skipped
forget_sessionwould leave behind.Confirmed failing before the fix, on the same commit:
A turn needing 5 tokens of budget was admitted against a budget of 3. The failure mode is silent by nature — an admission that should have been a refusal, not an error.
Two controls make the arms mean something, both asserted rather than assumed:
workflow_sessions.remove(...).is_some()—create_sessionis what inserted the id, so the removal is real. A silently-failed removal would send both arms down the agreeing path and pass while proving nothing.session_prepended_prompt_len(...) == seed.len()— the conversation is still readable from the map the carry is read from, per arm.Each arm gets its own engine.
admit_*takes a reservation that onlycomplete()returns, so a shared engine would let the admitted arm's leftover reservation pay for the other arm's refusal — and the test would pass with the carry ignored.Verification
1 passed; 668 filtered outa_continuing_turn_is_admitted_for_the_conversation_it_carries(#1982, the agreeing case)cargo fmt --checkcargo clippy --all-targets -D warningscargo checkat default features (#1973 lane)Both targeted runs used
--exactand report1 passed / 668 filtered out, so the filter demonstrably matched — per the empty-filter hazardscripts/test_step.shguards against.Limitations
The divergence is constructed by the test, not reachable through any current public path. This does not fix a live bug; it removes a gate whose correctness depended on an invariant maintained in a different module from the one reading it, with nothing asserting that invariant — and it adds the assertion.