Skip to content

feat(llm-reach): the in-pass split-and-retry for dropped batches — a malformed batch re-issued once as two halves, the recovery carrying its own provenance (#558) - #575

Merged
gadievron merged 1 commit into
masterfrom
fix/issue558-split-retry
Sep 11, 2026

Conversation

@gadievron

Copy link
Copy Markdown
Collaborator

Summary

The post-lift measurement (this issue's step 1, run 2026-09-09, receipts in the issue comment): 4 malformed batches in ~95 at the DEFAULT_MAX_TOKENS cap — all the end_turn broken-JSON finish class, 0 max_tokens — the cap lift eliminated the truncation-at-cap class; the residual is the model's own structurally-broken finishes, a fresh-roll class. The close-out rule failed → the in-pass recovery proceeds.

The implementation (this issue's direction 3 — split-and-retry, never the JSON corrector: it cannot recover signals the model never emitted, and on the truncation class it would freeze partial batches as reviewed, defeating absence-as-retry):

  • a dropped or truncated batch of ≥2 units re-issues once as two halves — smaller outputs (less likely to break mid-structure) and a fresh roll; bounded to one split level, no recursion — a still-dropped half stays dropped (its units re-run on resume via absence-as-retry);
  • the recovery carries its own provenance (direction 4): batches_split_recovered/batches_split_lost — distinct counters, surfaced in the step summary; the original drop's counters are revised by the attempt's exact applied deltas (the adversarial review's catch: an inferred-shape subtraction drove batches_truncated negative on the max_tokens+prose shape);
  • the usage window spans the whole recovery (the review's second catch: per-attempt tracking reset left the records carrying only the last half's spend — the window now starts once per original batch, the recovered units' records carrying the original + halves' true cost);
  • the recovered halves' units persist checkpoint records; the still-dropped leave none. A provider-exception batch (failed) is deliberately not split — not output-size-shaped; the llm-reachability: provider-exception batches are never counted in batches_dropped/units_not_reviewed — the step report can read success/0/0 for a pass that reviewed nothing #541 counters + the resume own it.

Closes #558.

Test plan

7 new tests (the #558 class): full recovery via halves (the counters revised to 0, the provenance counted); the recovered units persist; a 1-unit batch never splits; the mixed half-recovered/half-lost shape (the coverage truth exact); the max_tokens+prose no-brace shape never drives a counter negative (the deltas pin); the usage spans the recovery; plus the 3 old-behavior pins updated (a still-dropped batch needs the halves to also fail).

Verification evidence

check command result
new + family pytest tests/test_issue532_llr_resume.py tests/test_issue294_reach_batch_diagnostics.py tests/test_llm_reachability.py -q 74 passed
full suite pytest tests/ -q 4022 passed, 2 failed — both pre-existing (SDK pin drift)
static analysis ruff / Semgrep clean / 0
adversarial review combined wave+refute 5 findings, all folded: the negative-counter catch (the deltas mechanism), the usage-window catch, the provenance surfacing, the recovery stderr line, the failed-class note
the measurement the issue comment's receipts (4 malformed in ~95 at the 20000 cap, 0 max_tokens) the decision rule's answer, posted

…malformed batch re-issued once as two halves, the recovery carrying its own provenance (#558)

The post-lift measurement (the issue's step 1, run 2026-09-09): 4
malformed batches in ~95 at the DEFAULT_MAX_TOKENS cap — ALL the
end_turn broken-JSON finish class (0 max_tokens; the cap lift
eliminated the truncation-at-cap class). The close-out rule failed:
the residual is the model's own structurally-broken finishes, a
fresh-roll class — re-generation usually recovers.

The implementation (the issue's direction 3 — split-and-retry, NEVER
the JSON corrector: it cannot recover signals the model never emitted,
and on the truncation class it would freeze partial batches as
reviewed, defeating absence-as-retry):
- a dropped or truncated batch of >= 2 units re-issues ONCE as two
  halves — smaller outputs (less likely to exhaust a cap or break
  mid-structure) AND a fresh roll. Bounded: one split level, no
  recursion; a still-dropped half stays dropped (its units re-run on
  the next resume via absence-as-retry);
- the ORIGINAL drop's counters are REVISED (subtracted) so the halves
  count their own outcomes — the coverage truth never double-counted;
- the recovery's own provenance counters (batches_split_recovered /
  batches_split_lost, direction 4 — a recovered batch never silently
  overwrites the units_not_reviewed count);
- the recovered halves' units persist checkpoint records (they were
  reviewed this pass); the still-dropped leave none.

The batch loop is restructured into a _attempt() helper (call+parse+
counters per sub-batch) — the same behavior for the non-split paths,
with the split at the loop level.

@gadievron gadievron left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification review — recommend-merge. (all evidence pinned to head e291bac; re-verify on any new head)

The fix: _attempt returns (signals, outcome, deltas) where deltas are THIS attempt's applied counter changes; a dropped or truncated batch of ≥2 units is re-issued ONCE as two halves with the original drop's counters subtracted EXACTLY (a prior refutation drove batches_truncated negative when the shape was inferred — this closes that class); ONE split level, no recursion; provider-exception (failed) batches are deliberately not split; the recovery carries its own provenance counters (batches_split_recovered/batches_split_lost) surfaced in scanner stats; the usage-tracking window spans the original + halves so per-unit cost carries the recovery; per-unit records persist for OK units only — dropped halves re-run on resume via absence-as-retry.

Verification (run at the pinned head): RED was assertion-level — 6 tests failed against the base tree for the stated reasons (including test_truncated_no_brace_never_negative); GREEN — 44 passed in the two touched suites; full suite — 3889 passed, 168 skipped, 0 FAIL; CI at e291bac: success. The review round (six independent reviewers) traced every counter path — split, partial-recovery, both-halves-lost, odd sizes, the max_tokens/no-brace shape — and found no defect; the counter arithmetic was re-derived case-by-case against the tests.

Cross-PR: the combined tree of all six open PRs passed the full suite; the same-file merge with #572 (disjoint regions of llm_reachability.py) is clean; no retry stacking with #571 (LLR's _attempt failures are never split at a raised cap — they go to batches_failed).

Known semantics (disclosed): batches_dropped now counts attempts (a fully-lost split = 2) while len(batches) remains the source-batch denominator — consumers gate on >0 only, but the counter's unit changed; a partial recovery (one OK half, one lost) counts batches_split_recovered — a counter-only reader over-reads coverage.

Follow-ups (named, none blocking): no test drops an odd-size (≥3) batch (mid = (len+1)//2 is correct by inspection but untested); the split also fires on prose-refusal drops while the comment argues only output-size-shaped failures should split — either state the isolation rationale or gate on shape; the cheap composition with #571: have _attempt's exception arm return "truncated" when it sees is_budget_exhausted_error, so the split covers the empty-length-stop class in LLR too.

Approve + merge are code-owner actions; never on red CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant