perf(engine): stop splitting a truncation the thinking budget caused - #368
Conversation
A truncated review call is normally a payload problem: one call was asked to cover more than one response could hold, so the engine halves the batch and reviews the pieces. That is the wrong reaction when the ceiling went on *reasoning* — a smaller payload does not shrink a thinking budget, and issue #348 recorded a fifteen-line diff truncating at the same ceiling as a large one. Splitting there re-spends the whole ceiling on every piece and fails identically: pure added latency on a review that is already slow. The adapter already measured the diagnosis and then formatted it into prose and threw the number away. ProviderTruncated now carries `reasoning_tokens` (None, not 0, when the route reports no breakdown) and `output_tokens` — the ceiling actually reached — so the engine decides from data, not from our own sentence. When reasoning accounts for >= 90% of the ceiling the split is skipped and the failure names `reasoning_effort`, the one lever that bounds thinking. Measured on one self-review: five calls truncated having spent 25,963-35,463 tokens reasoning against a 32,768 ceiling, while the output-length truncation the split exists for sits near zero on that ratio — the two populations are nowhere near 90% from either side. The salvage (findings finished before the cut) is kept on this path exactly as on the others. Second: the pieces of a split now run concurrently. They ran serially, from inside a fan-out worker that was already holding a pool slot, so a splitting lens cost one full model latency per piece — on a run where most lenses split, the biggest wall-clock multiplier in the system. They run in an executor of their own rather than back in the global pool: a pool worker that submits to its own pool and blocks on the result deadlocks once the pool is saturated. Width is the backend's concurrency (`_concurrency_cap`, split out of `_resolve_workers` so the fan-out's task-count clamp does not shrink it), so a single-stream server still gets its pieces one at a time. The soft review deadline still bounds the split — each piece re-checks it at execution, and running them together shortens rather than extends the overshoot. Fixes #348 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
⏱️ 1 batch was too big for one call (timed out, or ran past the 3 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2 |
![lgtmaybe[bot]](https://avatars.githubusercontent.com/in/3987976?s=60&v=4){kind=small}
There was a problem hiding this comment.
max_tokens ceiling (32868 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.
⏱️ 1 batch was too big for one call (timed out, or ran past the max_tokens ceiling) and was reviewed in smaller pieces instead. Consider a lower max_input_tokens, a higher max_tokens, or a faster model.
0 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2
Incremental review of the changes since 212aea3 — earlier findings stay open until fixed.
…rlap check
Three gaps in the tests from the previous commit.
The concurrency test proved overlap with a 0.05s sleep and a max-in-flight
counter. On a loaded runner the second worker can be scheduled more than 50ms
after the first, so that test could fail against correct code — a flake shipped
into a suite that runs on every PR, Windows included. The pieces now rendezvous
on a `threading.Barrier`: each blocks until the other has entered the provider,
so overlap is established rather than timed. The barrier carries an explicit
5s timeout — an unbounded `wait()` would turn a regression to serial pieces
into a hung job with no output, where the timeout makes it a recorded failure
the assertion can name ("the split's pieces ran serially"). The sleep-based fake
stays for the test asserting pieces do NOT overlap: there a clock that is too
short can only miss a detection, never fail spuriously.
`_concurrency_cap`'s explicit-cap branch was uncovered — both split tests used a
provider default. It is checked before the single-stream default, so an explicit
`max_concurrency` raises ollama's 1, matching what the fan-out has always done
(`test_explicit_max_concurrency_wins_everywhere`); ollama's 1 is a default for
the usual one-slot deployment, not a property of the backend, and honouring the
setting in only one of the two pools would make it mean two different things.
Covered with the ollama case, which is the branch that has something to prove.
And a reasoning-dominant truncation on an already-split *piece* was asserted
nowhere, though the check is deliberately placed before the "already a piece"
branch so a piece reports `reasoning_effort` too. A regression there would fall
through to the generic "raise `max_tokens`" — the one knob that does not move
this — after the split has already been paid for.
Both new tests characterise code from the previous commit rather than driving
it, so each was verified non-vacuous by mutation: reordering `_concurrency_cap`
fails the first (in 5.6s, legibly, not a hang), and moving the reasoning check
after the piece branch fails the second and only the second. Pinning the split
back to width 1 fails the rendezvous test, which is what it is for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
⏱️ 1 batch was too big for one call (timed out, or ran past the 💬 3 earlier lgtmaybe conversations are still unresolved on this PR — this run's count covers what it reviewed now, not those. 0 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2 Incremental review of the changes since 36cb069 — earlier findings stay open until fixed. |
…truncation-3j8akh # Conflicts: # src/lgtmaybe/engine/engine.py
|
⏱️ 1 batch was too big for one call (timed out, or ran past the 0 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2 Incremental review of the changes since 212aea3 — earlier findings stay open until fixed. |
2 participants
What
Two changes to how a truncated review call is handled, both aimed at the failure in #348.
1. The truncation diagnosis is structured, and a split that cannot help is not attempted.
ProviderTruncatednow carriesreasoning_tokens(None— not0— when the route reports no breakdown) andoutput_tokens, the ceiling actually reached. The adapter already computed both and then formatted them into a sentence and discarded the numbers; the engine now reads the diagnosis as data, per the "structured output only, never parse prose" rule.When reasoning accounts for ≥ 90% of the ceiling, the batch is not split. Splitting is right for a wall timeout and for a truncation caused by output length. It is wrong for one caused by reasoning exhaustion: a smaller payload does not shrink a thinking budget, so every piece re-spends the full ceiling and fails identically — pure added latency on an already-slow review. The failure instead names
reasoning_effort, the lever that bounds thinking. Salvage (_salvage_truncated) is preserved on this path exactly as on the others.2. The pieces of a split run concurrently.
_review_splitlooped its pieces serially, from inside a worker already holding a slot in the global fan-out pool — so a splitting lens cost one full model latency per piece. On a run where most lenses truncate (#348: 5+ of 9), that was the single biggest wall-clock multiplier in the system.Why 90%
From the measurements in #348 and
.lgtmaybe.yml, on one self-review against a 32,768-token ceiling:Five of nine calls spent the entire ceiling on thought before writing a finding — 0.79 to over 1.0 of the cap, clustering at 0.98+. The failure mode the split exists for is the opposite shape: little thinking, then an answer that runs long, where the ratio sits near zero. The two populations are nowhere near 90% from either side, so the threshold is not delicately placed. At 90%, at most a tenth of the ceiling was left for the answer, so halving the diff cannot buy a complete one.
#348 also records the case that proves shrinking is not the lever at all: a fifteen-line diff still truncated at 16,384.
It is deliberately not configurable — it is a diagnosis, not a preference.
Avoiding the deadlock
The pieces are not resubmitted into the global
ThreadPoolExecutor. A pool worker that submits to its own pool and blocks on the result deadlocks the moment the pool is saturated: every worker waits on work only a free worker could start. They run in a private, bounded executor instead, so that is impossible by construction rather than by argument.Width is the backend's concurrency, not the piece count.
_resolve_workerswas split into_concurrency_cap(cfg)(explicitmax_concurrency, else 1 for single-stream providers, else 8) and the fan-out's task-count clamp on top of it — otherwise a one-lens review, whose pool is sized to one task, would have bounded its split to one too. A single ollama instance still gets its pieces one at a time; at width 1 the executor runs them in submission order, exactly as the old loop did. A splitting worker is blocked rather than calling, so calls in flight peak at fan-out width × piece count, which the adapter's backoff absorbs.Semantics preserved:
pool.mapyields in submission order, so findings and the reported error stay deterministic;self._split_batches.add(batch_num)still fires only when at least one piece answered; and any failed piece is still a failed call.The deadline
max_review_secondsstill bounds the split correctly, and slightly better than before. Each piece goes through_review_lens, which re-checks the deadline and token budget at execution — so a split beginning past a ceiling costs nothing. Concurrent pieces are both in flight at once rather than sequential, so the overshoot a split in flight can add is still one call's latency, over a shorter wall clock. Guarded by a new test.Tests (TDD, red first)
tests/providers/test_retry.py—test_the_counts_travel_as_numbers_not_only_as_prose,test_a_route_without_reasoning_detail_carries_no_reasoning_counttests/engine/test_truncation_split.py—test_a_reasoning_dominated_truncation_is_not_split,test_a_reasoning_dominated_truncation_keeps_its_salvage,test_a_truncation_that_spent_its_ceiling_on_findings_is_still_split,test_a_truncation_with_no_reasoning_breakdown_is_still_splittests/engine/test_timeout_split.py—test_the_pieces_of_a_split_batch_are_reviewed_concurrently,test_a_single_stream_provider_still_reviews_its_pieces_one_at_a_time,test_a_split_starting_past_the_deadline_costs_nothingSpecs & docs
provider-gateway/provider.truncation: the counts travel as data; a route reporting no breakdown carriesNone, never0.review-pipeline/engine.timeout-split: pieces are reviewed concurrently.review-pipeline/ newengine.reasoning-ceiling(+anchors.ymlrule on_reasoning_exhausted_reason): a split is only attempted when covering less can help. The old "the failure says nothing about size" scenario moved into it, keeping both sections under the 40-line cap.docs/how-to/reduce-review-cost.mdupdated (andllms-full.txtregenerated). NoReviewConfigfield added, so the generated config reference is unchanged.Gate:
ruff check,ruff format --check,mypy,pytest -q(1,836 passed),pytest tests/specs -q, andopenspec validate --specsall green.Fixes #348
🤖 Generated with Claude Code
Generated by Claude Code