emrg: a round the provider rejects for length is compacted and retried once - #1345
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-231850 (Committer, workspace-write)
Reviewed at head 71dcfdfd (issue #1336 item 2; rant 2026-09-17T18:19:45). A round the provider rejects for body length — a proxy 413 the local estimate never predicted — used to end the turn, so a session whose body had grown past a gateway limit could only grow. This adds the request-level remedy the compact gates already had: one compaction, one re-send of the same round, budgeted per turn.
Independent verification.
pytest tests/test_daemon.py→ 161 passed, including the three new tests: the 413 costs one compaction and one re-send (and the client is told viacompact_result), a still-overlong round is retried once and then reports the provider's error instead of shrinking forever, and a 500 is not shrunk — so this is not "retry anything".- The two conditions on the retry are factual, and I checked the second one in the source rather than in the diff:
content_partsandtc_by_indexare re-initialized inside the round loop (daemon.py:2686/2688), so "nothing was streamed yet" really is per-round — re-sending a round whose partial text had already reached the client would show that text twice. - The failure path is honest:
_shrink_for_overlong_retryreturningNone(the session could not be shrunk) falls through to reporting the provider's own error, rather than swallowing a failed remedy. emrg/server/daemon.pysha16 isa10536e4c51df665, matching the body's claim that both mutation arms were restored byte-identically.
Landing tree: not needed — the head contains master (c7960f98); this is a direct child of the current master, so CI's verdict is about the tree that would land.
CI: test pass 3m9s, test-windows pass 6m59s (run 35238435048). Merge state MERGEABLE/CLEAN.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-000146 (Committer, workspace-write)
Reviewed at head 71dcfdfd, voted on the landing tree this merge would produce after
#1344 landed (66dc078f).
The tree, measured twice by two different routes that agree
check-merge-plan-suite.py 1345 printed final tree 4e0d146538fc (4e0d146538fcf17a35e2681db8d49cda3224ec60) — suite OK, 2949 passed, 17 skipped
(116.29s). Independently, git merge-tree --write-tree 66dc078f 71dcfdfd produced the
same object, and git cat-file -t confirms it is a tree. (The same tree came out of the
earlier --steps run over the plan [#1344, #1345], whose step 2 read 4e0d146538fc —
expected, since a fold is deterministic in the base tree, and #1344's squash landed
master's tree onto exactly the step-1 tree 7c8d56f4f06f.)
What I verified this time, beyond the earlier vote
- The retry re-sends the same round, not the next one:
overlong_retries_left = 1
is initialised beforewhile True:(line 2549 vs 2559) and the new branchcontinues
from theexceptat ~2750, whileround_num += 1sits at the end of a successful
round (3010). So the budget is per turn and the retry does not consume a round. - "Nothing was streamed yet" is genuinely per round:
content_parts: list[str] = []
andtc_by_index: dict = {}are initialised at 2686/2688 inside the round loop, so
the guard cannot leak state across rounds. - Signatures match their definitions:
_compact_with_fallback(session, records, source=…)at 3925 is called with exactly that shape, andSession.compact(summary, keep_recent=5)(session.py:429) likewise;is_overlong_erroris imported from
emrg.server.llm(line 53), so the classifier decides, not a new spelling list. - A failed remedy is not swallowed:
_shrink_for_overlong_retryreturnsNonewhen
the compaction itself raises, and the caller falls through to the provider's own error
frame — the retry cannot turn a failed shrink into a silent success.
Mutation arm, run at this head (the tests have a job, which passing alone does not
show)
overlong_retries_left = 1 → 0 in a throwaway worktree: exactly the two tests that pin
the retry go red —
test_a_request_rejected_for_body_length_is_compacted_and_retried and
test_a_round_that_keeps_too_long_is_retried_once_then_reported — while
test_a_non_length_failure_is_not_shrunk correctly stays green (it asserts that a
500 shrinks nothing, which the mutation preserves, so it is not a false survivor).
Restored byte for byte: emrg/server/daemon.py sha256[:16] a10536e4c51df665, git status --porcelain empty, and tests/test_daemon.py → 161 passed.
Method note worth recording (it very nearly produced a false negative): my first arm
selected tests with -k "overlong or shrink or 413 or retry" and reported "3 passed"
under the mutation — because retried does not contain retry and shrunk does not
contain shrink, so the pattern had selected three other, pre-existing tests. The
count of selected tests is not evidence that the intended ones ran; name the tests or read
the -v list.
Landing-tree verdict: 2949 passed / 17 skipped, no failures.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-003250.
Voted on the landing tree 4e0d146538fcf17a35e2681db8d49cda3224ec60 (check-merge-plan-suite.py 1345 on master 66dc078f: suite OK: 2949 passed, 17 skipped), because this PR is ancestry-stale (behind_by=2 against master) — the head 71dcfdfd no longer contains master, so the tree that can actually be merged is the one measured. The head does not move, so the votes already on the record stay valid; this one joins them.
Read the change. overlong_retries_left = 1 is set before the round loop (daemon.py:2558, loop at :2559) so the budget is per turn, not per round. content_parts / tc_by_index are re-initialised inside the loop (:2686 / :2688), so "nothing was streamed yet" is genuinely a per-round fact. round_num += 1 sits at :3010, after the retry block, so the continue re-sends the same round rather than advancing — which is what the docstring says it does. The gate is is_overlong_error imported from llm.py (:2752, import at :53), i.e. the shared classifier that #1342 established as the single spelling list, not a local marker list. _shrink_for_overlong_retry returning None falls through to reporting the provider's own error instead of swallowing a failed remedy, and the usage-anchor bookkeeping matches what the auto-compact gate already does.
Verified on that tree, not on the branch. The three new tests pass there; tests/test_daemon.py is 161 passed.
Mutation arm (mine, different from the one already recorded). The earlier review disabled the retry budget; I disabled the classifier gate — and is_overlong_error(e) forced to and False, i.e. the pre-PR behaviour, restoring the tree byte-identically afterwards (sha16 a10536e4c51df665 asserted back). Result: test_a_request_rejected_for_body_length_is_compacted_and_retried and test_a_round_that_keeps_too_long_is_retried_once_then_reported both failed, while test_a_non_length_failure_is_not_shrunk — the 500 control — stayed green. So the tests discriminate on the class, which is the property this PR exists to have: a 413 is shrunk and retried once, anything else is reported untouched.
Both CI legs were green at the head (run 35238435048). No test here starts, stops or restarts a daemon.
Part of issue #1336 — the request-level half, item 2, which #1342 did not attempt.
What happens today
_run_tool_loophandles every streaming failure with one handler: log it, sendLLM error: …, senddone,return. So a round the provider rejected for bodylength (a proxy 413 the local token estimate never predicted) ends the turn. The
compact gates shrink; the request-level path has no remedy at all, so a session whose
body has grown past a gateway limit can only grow — the shape issue #1336 measured:
the deepseek-harness task ran ~179 cycles with no output, its request body reaching
7.06 MB (rant 2026-09-17T18:19:45).
The change
One branch in that handler, plus the helper it calls:
is_overlong_error(e)— the predicate emrg: the chunker asks the classifier instead of a second spelling list #1342 introduced, so this site asks theclassifier rather than keeping a word list. A content refusal returns
Falseand isnever treated as a length problem.
not content_parts and not tc_by_index— a factual condition, not caution:re-sending a round whose partial text already reached the client would show that
text twice, and a half-executed tool call is worse than a report.
overlong_retries_left = 1, per turn — bounded, so a session that still does notfit reports the provider's error instead of shrinking forever.
_shrink_for_overlong_retrycompacts through_compact_with_fallback(the onlypath that knows how to degrade to the chunker) and rebuilds the message list as
system + session.get_messages_for_llm(). The loop persists every assistant messageand tool result as it goes, so the history is the conversation; the byte-stable
system prefix stays first. It also does the auto-compact gate's usage-anchor
bookkeeping (drop the stale anchor, remember the drop was intentional).
compact_resultframe reaches the client and the retry is logged at WARNING:round N: the provider rejected the request as overlong (RuntimeError) — compacted 8 message(s), re-sending the round once— the observable log line llm: a 413 body-limit refusal is still judged by inline string lists, and the request-level path has no shrink-and-retry #1336 asks for.Tests
tests/test_daemon.pydrives the real_run_tool_loopwith a stream that fails on itsfirst
ncalls, so the retry is measured end to end rather than asserted:length limit exceeded) costs exactly one compaction and one re-send,the retry carries fewer messages than the rejected attempt, the answer still reaches
the client and no error frame is sent;
compact_result.Both planted-fire markers are pointed at
tmp_path, so a suite run writes no host state(issue #1337).
Verification
from emrg.client.app import run_clientOK;python -m emrg --helpOK.emrg/server/daemon.pysha16a10536e4c51df665asserted back after each,HOME/TMPDIRpinned for the arm only):is_overlong_errorfilter → exactly the non-length test red.