Repository navigation
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: failed turns with no persisted tail still reload with a different context reading.
I found one blocking persistence gap. I reviewed the full diff and the relevant loop persistence path, RPC/lane propagation, both frontend consumers, schema/generated types, and the history that introduced stored context readings. I also checked the repository rules and domain terms, backward compatibility, architecture boundaries, and that the added tests were not weakened.
Verification:
uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_usage_sink.py tests/test_rpc_session.py tests/test_rpc_spine.py tests/test_spine_events.py tests/test_spine_scheduler_lane.py-> 393 passed.- A real-loop reproduction with a first-call empty response measured
context_used=7000, but persisted both the user row and failure marker withoutcontext_used. - The focused web and TUI tests could not start because this worktree has no installed Node dependencies (
vitest: not found;esbuild: not found). git diff --check github/main...HEADpassed.
|
Windows compatibility review — PR #1126 @ Scope: full base Result: no Windows-specific issue. The change is token accounting and event plumbing: integers on message dicts and event payloads, no path, filesystem, subprocess, signal, encoding, or platform primitive. Verification (native Windows 11, CPython 3.12.13, isolated worktree Limitations: no live provider or gateway; the TUI side was not run (the change there is the generated client only). |
50891ec to
e1bcea5
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The reported persistence gap is fixed and the thread is resolved. I reviewed the new commit and refreshed the full PR diff against the current github/main, including the loop persistence and rerun callers, broken-turn rescue paths, RPC/UI behavior, backward compatibility, repository rules and domain terms, architecture boundaries, history, and the new tests for whether they genuinely exercise the failure.
Verification: uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_usage_sink.py tests/test_agent_loop_broken_turn.py tests/test_agent_loop_max_iter_synthesis.py tests/test_rpc_session.py tests/test_rpc_spine.py tests/test_spine_events.py tests/test_spine_scheduler_lane.py -> 434 passed. git diff --check github/main...HEAD also passed.
|
Windows compatibility re-review — PR #1126 @ Scope: the head moved from the previously reviewed Result: no Windows-specific issue. The additions are integer window readings on message dicts and event payloads plus a transient-key filter; no path, filesystem, subprocess, signal, encoding, or platform primitive. Verification (native Windows 11, CPython 3.12.13, isolated worktree Limitations: no live provider or gateway; the TUI side was not run (generated client only). |
|
Not a blocker -- smaller points from the same pass, separate from the kernel thread. Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
|
|
Thanks. All six points are addressed in this push.
One more thing, found while wiring the TUI side: the failed-turn change in the earlier push sat in |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the three-commit delta and refreshed the full PR diff against current github/main. The failed/stopped usage channel now stays inside RPC, is keyed by turn rather than lane, is shared by both server wiring paths, and is removed on every terminal event. Cancellation captures the live sink before unwind, both clients apply failure/stop usage before their early exits, zero/no-window reports preserve the prior ring, and upstream transport failures no longer overwrite a real window reading. I also rechecked the persistence callers, schema/generated shapes, backward compatibility, repository rules and domain vocabulary, architecture boundaries, history, and whether the replacement tests exercise the reported failures.
Verification:
uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_usage_sink.py tests/test_agent_loop_broken_turn.py tests/test_agent_loop_max_iter_synthesis.py tests/test_rpc_session.py tests/test_rpc_spine.py tests/test_rpc_turn_cancel.py tests/test_rpc_bootstrap.py tests/test_cli_tui_commands.py tests/test_spine_events.py tests/test_spine_scheduler_lane.py-> 574 passed.git diff --check github/main...HEADpassed.- The focused web and TUI tests could not start because Node dependencies are not installed in this worktree (
vitest: not found;esbuild: not found).
|
Windows compatibility re-review — PR #1126 @ Scope: the head moved from the previously reviewed Result: no Windows-specific issue. The additions are integer window readings and dict slots passed through existing in-process wiring; no path, filesystem, subprocess, signal, encoding, or platform primitive. Verification (native Windows 11, CPython 3.12.13, isolated worktree Limitations: no live provider or gateway; the TUI side was not run (generated client only). |
There was a problem hiding this comment.
Blocking: the "a call that failed in transport reads no window" rule (124bc78b5) never fires on the streaming path, the only path a gauge has in production. On that path this PR now sends the failed call's reading to the client and files it, so after a failed TUI/web turn both the live ring and a resume show about 0%. On the tip, a resume of the same turn showed the real reading.
Mechanism.
_reads_no_window(raven/agent/loop/_shared.py:190-200) keys onerror_classification.category == "upstream_transport_failure". Only the non-streaming exit sets that:LiteLLMProvider._parse_response,litellm_provider.py:1126-1138, reached fromchatonly.- The streaming exit asks the same verdict, but only to reconnect. Once the reconnects are spent it returns the empty response with its usage and no classification (
raven/providers/streaming.py:508-522, "No verdict here"). RpcTurnRunner.runalways streams (raven/rpc/spine.py:266,stream=True, usage_sink=...). So on the TUI / served page / ACP,_call_windowtakes the failed call's 1-2 tokens. That number then reaches:- the gauge and the sink (
turn_path.py:1374-1414); - the kept-row stamp;
- the failure marker (
_save_broken_turn(context_used=...)); - the new error usage (
reported_usage,rpc/spine.py:693), which both clients apply.
- the gauge and the sink (
Reproduced (lane probe, one fault, two exits; the streaming case is driven the way run_turn(stream=True) drives it):
head, streaming : sink 2, marker 2, resume 2, error_event.usage.context_used 2
head, non_streaming : 6200 everywhere
tip, streaming : sink 2, marker None, resume 6200, error usage not sent
With an honest control (6100+0, no verdict), every reading is 6100 on both exits. So the non-streaming exit, which has no gauge in production, behaves as the commit says, and the streaming exit does not. The test test_a_call_that_failed_in_transport_reads_no_window gives a usage sink to a non-streaming turn, a pairing no production caller makes, so it passes without reaching the real path.
Why blocking. This is the exact fault 124bc78b5 says it removes, and the PR's other commits newly expose it: the error usage and the marker / kept-row reading are new here. A failed streamed turn now leaves a worse ring and a worse resume than the tip did.
Fix direction: carry the streaming verdict on the response stream_llm_call returns once reconnects are spent (attach the same classification, keeping the empty-response recovery as the owner of what happens next), so the one rule in _reads_no_window sees it on both exits. Then pin it with a streaming-turn test that has a usage sink.
There was a problem hiding this comment.
Agreed, and reproduced. On the reviewed head, a streamed turn whose call after a tool call came back empty and billed at one token put 2 on the sink, the failure marker and a resume, where main resumed at 6200. As you say, test_a_call_that_failed_in_transport_reads_no_window gave the sink to a non-streaming turn, a pairing production never makes, so it never reached this exit.
Fixed in 7ab0ba4, the way you suggest. Once its reconnects are spent, stream_llm_call returns the empty response with the classification the non-streaming exit gives the same call, both taken from one transport_failure_verdict(), so _reads_no_window sees it on either exit. The finish reason stays stop, so the loop's empty-response recovery still owns what happens next: every reader that retries or falls back on a classification checks finish_reason == "error" first.
Tests, red on the reviewed head and green now:
test_a_stream_that_stays_empty_carries_the_verdict_the_other_exit_gives(tests/test_transport_failure_paths.py): the spent stream carries the classification_parse_responsegives the same call (it carried none);test_a_streamed_call_that_failed_in_transport_reads_no_window[failed|answered](tests/test_agent_loop_usage_sink.py): a streamed turn with a usage sink, the pairingRpcTurnRunnermakes; the sink and a resume read 6200 (the sink read 2).
test_a_silent_stream_with_honest_usage_carries_no_verdict is your honest control: an empty stream billed at a size its prompt can have carries no verdict. It passes on both heads.
In a real browser, through raven web against a scripted upstream that has the turn call a tool and then sends empty replies billed at one token until the loop gives up, the ring read Context 6.2k/40k 16% after the failure and after a reload, with context_used: 6200 on the error frame. On the reviewed head the same run read Context 2/40k 0% on both.
@0xKT could you take another look, and resolve the thread if this settles it?
There was a problem hiding this comment.
A follow-up after rebasing onto main: #1092 merged with its own fix for this gap. transport_failure_response is what both exits now return once the verdict stands, so on the streaming exit a call the upstream failed comes back, once its reconnects are spent, as finish_reason="error" with the upstream_transport_failure classification, the same as the non-streaming exit. I dropped transport_failure_verdict and the classified stop response in favour of it, so what I wrote above about the finish reason staying stop no longer holds: the error ladder owns that call now, as #1092 decided.
_reads_no_window reads the same classification, so the gauge keeps the reading of the call before it on that path too. The branch keeps the streamed-turn test, test_a_streamed_call_that_failed_in_transport_reads_no_window[failed|answered], now in 0b02cb2: on the rebased head the sink and a resume read 6200, and it fails with the reading rule removed. In a real browser on the rebased head the same run still reads Context 6.2k/40k 16% after the failure and after a reload.
|
Not a blocker - non-blocking items from the delta acceptance on Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
Tests here on the merged tree: the 6 changed Python test files via the board runner gave 469 passed. ui-web |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the streaming transport-failure path must preserve the prior real window reading.
The newly demonstrated streaming-path gap invalidates my prior clean stance. I verified the mechanism in raven/providers/streaming.py: once reconnects are exhausted, the streaming response returns without the upstream_transport_failure classification that _reads_no_window requires, while the production RPC runner always uses stream=True. The open finding from @0xKT owns the requested correction; I am not duplicating or re-grading that thread.
Verification: uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_stream.py tests/test_agent_loop_usage_sink.py tests/test_transport_failure_paths.py -> 152 passed. Those passing tests do not cover the reported production streaming pairing, which is the gap the open thread identifies.
|
@0xKT thanks for both notes; they are in 1cc9bb6.
@gloryfromca the streaming Blocking in your latest review is fixed in 7ab0ba4, with the details on the thread above. If nothing else stands in your way, could you approve? |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The streaming-path blocker that invalidated my prior stance is fixed on this revision. Once reconnects are spent, the streaming exit now carries the shared upstream_transport_failure verdict while still returning an empty response for the loop's recovery; _call_window therefore preserves the prior real reading on the production RPC path. The usage sink now advances its context reading only when the persisted reading advances, so all-zero/no-window calls keep wire, marker, and resume aligned while cost still moves. I reviewed the two-commit delta, its production streaming callers, persistence/RPC consumers, shared classification contract, backward compatibility, architecture boundaries, repository rules, and the replacement regression tests. I did not resolve or re-grade the other reviewer's thread.
Verification: uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_stream.py tests/test_agent_loop_usage_sink.py tests/test_transport_failure_paths.py tests/test_rpc_spine.py tests/test_rpc_turn_cancel.py -> 290 passed. git diff --check github/main...HEAD also passed.
|
Windows compatibility re-review — PR #1126 @ Scope: the delta since the previously reviewed head Result: no Windows-specific defect. The change is accounting and classification logic over token counts and Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun for this delta; the provider paths here are exercised with stub streams, not live vendor calls. |
|
Merge conflict: @LivXue, this PR no longer merges onto the current
The judge's verdict on |
1cc9bb6 to
07342fe
Compare
|
@0xKT rebased onto |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the rebased head 07342fe80541. The range-diff shows that the branch dropped its duplicate provider patch and now relies on main's shared transport_failure_response; both streaming and non-streaming failures retain the upstream_transport_failure classification, so the PR's no-window rule still preserves the prior real reading. The turn_path.py and web stage conflict resolutions keep both main's empty-recovery/silent-turn behavior and this PR's reading carrier. The final test-only commit filters Raven's underscore bookkeeping before comparing requests, which matches the providers' request-message sanitization and does not relax the wire-level behavior being tested.
I covered the repository rules and context documents, the full github/main...HEAD diff, the rebase range-diff and relevant history, runtime/RPC/UI callers, wire compatibility, test-strength changes, and the kernel/RPC architecture constraint. The kernel remains unchanged; the live channel remains RPC-local and turn-keyed.
Verification: git merge-tree --write-tree github/main HEAD and git diff --check github/main...HEAD passed. uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_stream.py tests/test_agent_loop_usage_sink.py tests/test_transport_failure_paths.py tests/test_rpc_spine.py tests/test_rpc_turn_cancel.py tests/test_agent_loop_empty_recovery.py tests/test_rpc_session.py tests/test_rpc_bootstrap.py tests/test_cli_tui_commands.py passed (1052 tests). The targeted web test command could not start because vitest is not installed; the targeted TUI command could not start because esbuild is not installed.
|
Windows compatibility re-review — PR #1126 @ Scope: the branch is rebased onto current main; the PR's own diff at this head is Result: no Windows-specific defect. The change is usage/context-reading plumbing across the loop, RPC methods, spine events, and the ui-web/ui-tui context display; it touches no path, subprocess, signal, socket, file, encoding, or permission surface. Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun; no live RPC/UI session was driven end to end. |
|
Merge conflict: @LivXue, this PR no longer merges onto the current
The judge's verdict on |
A turn answered through the message tool can end on a call whose reply is empty: recovery stands scaffolding in for it and then gives up, or recovery is off and the empty row is dropped on save. The live meter showed that call's window, but no stored row carried it, so a resume showed the turn's earlier tool-call reading (6200 against a live 7400 in the regression test). An empty reply's reading now goes on the newest row the turn keeps, skipping the transient rows dropped before persistence; that row ends the window the call measured. A call that reports no usage leaves the reading before it in place.
A failed turn never moved the page's context ring: its error event carried no usage, so the ring stayed on the turn before while the failed turn's own stored record, which session.resume reads, had moved on (3100 live against 7000 restored after a turn that called a tool and then never answered). The RPC runner now sets what the turn measured on the exception it lets out, the lane copies it onto TurnFailed.turn_usage the way it copies full_detail, and the error event carries it as an optional usage in message.complete's shape; the page moves the ring and the TUI its context bar when one is present. It rides the failure rather than the per-lane usage register, which a queued turn shares with the turn that owns the lane. The test that pinned TurnFailed as carrying no usage now pins that it carries no outcome Usage, the type TurnEnded accounts with, and lists turn_usage beside full_detail.
A stored context_used past float range raised OverflowError on the way to a percentage in session.resume, which then handed the reader a freshly minted empty session carrying the 401-digit reading; one under that range but past any window drew an absurd ring. A reading over a billion tokens now counts as no reading, so the newest real one stands. Two notes are corrected as well. The _window_reading docstring named a transport_failure.never_processed that does not exist and called its add-back the same as one that adds cache counts unconditionally. The resume fallback's comment named loop/main.py as a get_history caller, which it is not.
A turn whose calls each came back empty or as a provider error filed no row of its own, and lost its window reading. The stamp for an unfiled reply went on the newest kept row, which was the turn's question; that row is filed before the first call, so the stamp never reached disk. The error event showed the live reading (7000) while a resume fell back to an estimate (22). The turn now holds the newest reading its calls measured, across attempts, and the marker _save_broken_turn files carries it on all three rescue paths: answerless, stopped and crashed. The marker is the turn's newest row, so session.resume reads the window from it. A turn that answered through the message tool and then ended on a provider error kept the reading of the call before it (6200 against a live 7100): the error exit left before that call's reading went on the newest kept row. It now puts it there, as an empty reply's exit does.
A failed turn's reading reached its error event through the kernel: the runner set it on the exception, the lane copied it onto TurnFailed.turn_usage, and the RPC sink read it back. Its only writer and its only reader both live in raven/rpc/spine.py, so the kernel field and turn_usage_of are gone, and raven/spine, its tests and CONTEXT.md are back to what main has. The runner now keeps each streamed turn's usage sink under its turn id in live_usages, a map build_rpc_spine shares with turn.cancel. A failed turn's end takes its entry onto the error event, and every end lets go of it. It is keyed by turn rather than by lane, because a queued turn shares its lane with the turn that is running. A stopped turn's cancel now carries the same reading. turn.cancel takes the running turn's sink before cancelling it and attaches what the turn measured, so a client's context ring moves with the record the stopped turn files. The map reaches turn.cancel from both entrances, bootstrap and the TUI's own server, the way direct_targets does. Either event sends the usage only when it read a window: a last call that reported zeros, or a sink with no reading, leaves the ring where it was instead of emptying it.
A stopped turn's cancel now carries what the turn measured, and both clients take it. The page applies an error's usage before a stop's own early exits, through applyUsage in state/session/ctxReport.ts. A completion and a resumed session's usage go through it too, so the three reports read the fields one way. A completion still falls back to its prompt count when it measured no window, and a resumed session's zero still draws an empty ring. The TUI applies the usage in chatStream's onError, for a failed turn and a stopped one alike. A turn's frames arrive on the typed turn stream, so the earlier change, made in the gateway event handler, never saw a turn's error.
A response the provider flagged as an upstream transport failure carries the upstream's account of a call that failed, and that account can be what flagged it: a prompt billed at a size the request cannot have been. It was still taken as a window reading: shown on the live gauge, stamped on the newest kept row, and fed to the proactive compaction trigger. Such a call now gives no reading. The gauge stays on the call before it, the stored record keeps the earlier stamp, and only the turn's cost moves on, being a flow rather than a gauge.
The rule that a call failed in transport gives no window reading was pinned on a non-streaming turn only, a pairing production never makes: the RPC runner, the one caller with a usage sink, always streams. The streaming exit returns such a call with the failure and classification the other exit gives it, and this drives that path the way the runner does: a streamed turn whose call after a tool call the upstream failed keeps the reading of the call before it, 6200 on the sink and on a resume, where the failed call's bill read 2.
The loop rewrote the usage sink on every call that reported usage, while the reading a failed turn files moves only on a reading above zero. A last call that reported a block of zeros emptied the sink, so the error carried no usage and the ring stayed on the turn before, while the failure marker and a resume read 6200. The sink now keeps the reading it holds through a call that read no window, a block of zeros or a transport failure alike, and only the turn's cost moves on; a sink that holds no reading yet is still filled whole, since an answered turn's message.complete sends it as it is. The RpcTurnRunner and _stopped_usage docstrings now say why the usage a failed or stopped turn reports matches the reading it files.
An empty reply's reading now goes on the newest row the turn keeps, which on a turn whose first reply only thought is its question. The fallback test compared the question it asked again with the first request dict for dict, and failed on that bookkeeping key, which no provider sends: each sends only its allowed message keys. It now compares the two requests as they are sent, and still fails when the switch leaves the prefill on.
07342fe to
fd9a155
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the rebase onto current main. The range-diff leaves every substantive PR commit patch-equivalent to the previously clean revision; the sole conflict resolution keeps main's new Callable import beside this PR's Any import in raven/cli/tui_commands.py. Main's new find_node(accept=...) path and this PR's RPC live_usages wiring are independent and both remain intact.
For this revision I rechecked the repository/context rules, the complete github/main...HEAD diff, callers and history through the range-diff, backward compatibility, test strength, and the RPC-local architecture constraint. I found no new defect. git merge-tree --write-tree github/main HEAD and git diff --check github/main...HEAD passed. The focused backend suite passed: 1052 tests via uv run --python 3.12 --all-extras pytest -q -n 0 tests/test_agent_loop_stream.py tests/test_agent_loop_usage_sink.py tests/test_transport_failure_paths.py tests/test_rpc_spine.py tests/test_rpc_turn_cancel.py tests/test_agent_loop_empty_recovery.py tests/test_rpc_session.py tests/test_rpc_bootstrap.py tests/test_cli_tui_commands.py.
|
Windows compatibility re-review — PR #1126 @ Scope: the branch is rebased onto current main; the PR's own diff at this head is Result: no Windows-specific defect. The change is usage/context-reading plumbing through the loop, the RPC methods/spine, and the ui-tui/ui-web display; it adds no path, subprocess, signal, socket, file, encoding, or permission surface. Re-ran the affected suites on Windows at the rebased head. Verification (native Windows 11; detached worktrees
Coverage gaps: the full suite was not rerun; ui-web's No Windows-specific issue found at this head. |
Summary
A follow-up to #1119, which stores the window each reply's call measured and has
session.resumeread it back. Some turns still left the context ring on a number the reader never saw live, and three notes were left on #1119 after it merged. This resolves all of them.The branch is rebased onto the current
main. The only conflict was one import line inraven/cli/tui_commands.py, wheremain'sCallableand this branch'sAnymerged into one; a range-diff against the pre-rebase branch shows every commit's change unchanged apart from that line.A turn that ends on an empty reply. A turn answered through the
messagetool can end on a call whose reply is empty: recovery stands scaffolding in for it and then gives up, or recovery is off and the empty row is dropped on save. The live meter showed that call's window, but no stored row carried it, so a resume showed the turn's earlier tool-call reading (6200 against a live 7400). The reading of a call whose reply is not filed now goes on the newest row the turn keeps (_stamp_context_used_on_kept, skipping the_TURN_TRANSIENT_KEYSrows dropped before persistence); that row ends the window the call measured. A call that reports no usage leaves the reading before it in place.A turn that fails before any of its calls filed a row (its first reply empty with recovery off, every retry empty, or a provider error) had nowhere to keep the reading: the newest row it keeps is its question, filed before the first call, so the stamp never reached disk and a resume fell back to an estimate (22 against a live 7000). The turn now holds the newest reading its calls measured, across attempts, and the marker
_save_broken_turnfiles carries it, whether the turn was answerless, stopped or crashed; that marker is the turn's newest row.A turn that answered through the
messagetool and then ended on a provider error kept the reading of the call before it (6200 against a live 7100): the error exit left before that call's reading went on the newest kept row. It now puts it there, as an empty reply's exit does.A turn that fails or is stopped. A failed turn's
errorevent carried no usage, so the page's ring stayed on the turn before (3100) while a resume read the turn's own stored rows (6200 on the code fix(*): restore the context ring from the window the last call measured #1119 merged, 7000 with the first change here), and a stopped turn's cancel had the same gap (3100 live, 6200 after a reload). The RPC layer now keeps each streamed turn's usage sink under its turn id inlive_usages, a mapbuild_rpc_spineshares withturn.cancel: a failed turn'serrorand a stopped turn'scancelled_by_clienterror both carry what the turn measured, as an optionalusageinmessage.complete's shape, and every end of a turn lets go of its entry. The map is keyed by turn rather than by lane, because a queued turn shares its lane with the turn that is running, and it stays inraven/rpc/spine.py, where its only writer and readers are, so the kernel'sTurnFailedis unchanged. The usage is sent only when it read a window (reported_usage), and the loop moves the sink's reading only on a call that read one, on the terms the turn's filed reading moves on: a last call that reported zeros leaves the ring on the turn's reading instead of emptying it. Both entrances,bootstrapand the TUI's own server, hand the spine and the turn methods the same map. The page applies the usage ahead of a stop's own early exits, and it now reads every report the ring is drawn from, a completion, an error and a resumed session's usage, through one helper (applyUsageinui-web/src/state/session/ctxReport.ts); a completion keeps its prompt-count fallback and a resumed session's zero still draws an empty ring. The TUI applies it inchatStream'sonError, the typed turn stream a turn's frames arrive on, for a failed turn and a stopped one. Added torpc-schema/openrpc.json,raven/rpc/models.py, both generated clients and the TUI's hand-writtengatewayTypes.ts.A call that failed in transport. A response the provider flagged as an upstream transport failure (
upstream_transport_failure) carries the upstream's account of a call that failed, and that account can be what flagged it, a prompt billed at a size the request cannot have been. It was taken as a window reading: shown live, stamped on the newest kept row and fed to the proactive compaction trigger. Such a call now gives no reading (_reads_no_window); the gauge stays on the call before it and only the turn's cost moves on.The notes left on fix(*): restore the context ring from the window the last call measured #1119 after it merged:
context_usedpast float range raisedOverflowErroron the way to a percentage, andsession.resumethen handed the reader a freshly minted empty session carrying the 401-digit reading; one under that range but past any window drew an absurd ring. A reading over a billion tokens now counts as no reading, so the newest real one stands._window_readingdocstring named atransport_failure.never_processedthat does not exist and called its add-back the same as one that adds cache counts unconditionally. It now states its own rule: add the cached counts back only where the provider reports them apart fromprompt_tokens.loop/main.pyas aget_historycaller, which it is not. It now names the two that are.Type
Verification
All on this head, on top of the current
origin/main, Python 3.12.13:Each change went red first for its reason, then green:
pytest tests/test_agent_loop_usage_sink.py -k ends_on_an_empty_reply(live 7400 or 7000, restored 6200) -> 3 passed;pytest tests/test_agent_loop_usage_sink.py -k "failed_turn or cut_short"(the failure marker carried no reading, so a resume read an estimate of 22, 35, 35 and 24 against a live 7000, 7400, 7000 and 7100, and a stopped or crashed turn's marker carried none either) -> 7 passed, one of them the guard that a turn which measured nothing files nothing, green on both sides;pytest tests/test_agent_loop_usage_sink.py -k answered_turn_that_ends_on_a_provider_error(restored 6200 against a live 7100) -> 1 passed;test_a_failed_turn_hands_its_measured_usage_to_its_errorwent red (a last call that read no window still sent its zeros);pytest tests/test_rpc_spine.py -k "measured_usage_to_its_error or keeps_what_a_turn_measured or lets_go or non_owning"-> 10 passed, the map's own cases written with it; the failed case ofvitest run src/__tests__/chatStream.test.tsin ui-tui (the bar did not move on the typed turn stream) -> 58 passed;pytest tests/test_rpc_turn_cancel.py -k stopped_turns_cancel(the cancel carried no usage) -> 4 passed; the stop cases ofvitest run src/state/session/pipeline.events.test.tsin ui-web (the arm left before the ring) -> 42 passed, and ofchatStream.test.tsin ui-tui (the bar did not move);pytest tests/test_agent_loop_usage_sink.py -k failed_in_transport(the live gauge read the failed call's 5 tokens instead of 6200, and on a streamed turn, the pathRpcTurnRunnertakes, the sink, the failure marker and a resume read the 2 tokens of the call the upstream failed) -> 4 passed. The streaming exit's own verdict ismain's since feat(*): align empty-reply recovery with Hermes and let a turn end in silence #1092 (transport_failure_response, which both exits return), so this branch no longer changes the providers;pytest tests/test_agent_loop_usage_sink.py -k "last_call_that_read_no_window or whole_usage"(a block of zeros emptied the sink, so the error carried no usage while the marker and a resume read 6200) -> 2 passed, one of them the guard that an answered turn whose calls read no window still sends a wholeTurnUsage, green on both sides;pytest tests/test_rpc_session.py -k no_token_count(a reading past any window was used, and one past float range turned the resume into a fresh empty session) -> 7 passed.pytest tests/test_rpc_bootstrap.py tests/test_cli_tui_commands.py -k share_one_map(both entrances hand the spine and the turn methods one map) -> 2 passed, andvitest run src/state/session/ctxReport.test.tsin ui-web (the one helper) -> 4 passed.Rebased onto
main, which carries feat(*): align empty-reply recovery with Hermes and let a turn end in silence #1092:test_a_switch_after_a_streamed_error_asks_the_fallback_the_original_questioncompared the question a fallback model is asked with the first request dict for dict, and failed on the_context_usedkey the empty-reply stamp puts on the question when the first reply only thought. No provider sends that key (each sends only its allowed message keys), so the test now compares the two requests as they are sent; it still fails when the switch leaves the prefill on -> 446 passed intests/test_agent_loop_empty_recovery.py.Mutation: 40 in-place mutants at this head, all killed, each turning red the case it targets: for the empty reply, drop the kept-row stamp, stamp a transient row, walk from the oldest row, or drop the zero-usage guard, and for the provider error, drop its stamp; for the failure marker, the loop records no reading, the attempt is handed no holder for it, any one of the three rescue paths passes none, the marker files none, or the holder keeps the oldest reading; for a failed or stopped turn's usage, the runner registers nothing, a failure reads its entry without letting go, a completion keeps its entry, the failure or either cancel path sends none, the rule accepts any usage or the cancel skips it,
emit_errordrops it, or any of the four hand-offs of the map drops it; for the transport failure, it counts as a reading, it moves no cost, or the window ignores the check; for the live reading, a call that read none overwrites it, so does a reading of zero, or such a call leaves a sink that holds no reading without its counts; on the page, the error arm draws nothing, the helper ignores the fallback or takes it for a resumed zero, a completion passes none, or the helper draws without a report; in the TUI,onErrorignores the usage or reads it only off a failure, and the gateway handler ignores it; and drop the reading ceiling. The files were restored byte for byte.Related files:
pytest -q -n 2over 33 files (agent loop usage sink, iteration hooks, stream, overflow, part durations, empty recovery, broken turn, max-iteration synthesis, failure text, transport failure paths; agent hook contract; message tool turn-local state and RPC route; rpc session, session naming, init bundle, spine, turn send, turn cancel, turn subscribe, bootstrap, contract shapes, schema match, turn wire shape; spine events, scheduler and lane, kernel budget; session manager; acp updates; the TUI command server; the TUI in-flight cancel integration) -> 2459 passed; and over 15 provider and streaming files (litellm provider attribution, model names, model overrides, reasoning, stream, stream end and timeout; provider stream fallback, stream idle budget and protocol; LLM error patience, reply room, call record, the Mirasim ACP stream, tool name normalization) -> 658 passed.Full suite:
uv run --frozen --python 3.12 --all-extras pytest -q -n 4-> 37247 passed, 9 failed, 255 skipped. All nine also fail on a cleanorigin/mainworktree under the same interpreter, in the same files: fivetests/test_config_update_providers.pycases that pick up the dev box's HTTP proxy, one terminal-style case intests/test_cli_theme.py, one LibreOffice conversion, onetests/test_install_script.pycase that finds a node elsewhere on PATH, and onetests/test_subagent_node_runtime.pycase about what cannot be read. So 0 introduced.Front ends:
vitest run src/state/in ui-web (736 passed),tsc --noEmitin ui-web (with--listFilesshowing this tree'sctxReport.ts) and ui-tui, eslint on the touched client files, prettier on the touched ui-tui files and the two new ui-web ones (the edited ui-web files are not prettier-formatted onmaineither and are left as they were), the TUI'schatStream,createGatewayEventHandlerandgatewayTypesDrifttests (101 passed), and both generators'--check: all clean. The full ui-tui suite: 10 failed, 2270 passed; nine are the render cases that also fail on a cleanorigin/mainworktree (which showed 11 in the same run, with different extra ones), and the tenth,modelPicker.test.tsx's OAuth-providers case, passes 26 of 26 on its own in three runs.ruff check,ruff format --check,ty checkon the touched modules,lint-imports(10 kept, 0 broken),scripts/check_commit_messages.py, commitlint over the range,scripts/check_large_files.py,scripts/check_source_language.pyandgit diff --check: all clean.Real browser, at this head: an isolated
raven webagainst a scripted OpenAI-compatible upstream, four runs of an answer followed by a turn that ends without one. Where that turn calls a tool and then gets empty replies until the loop gives up, and where its first reply is already empty, the ring readContext 7.0k/40k 18%after the failure and again after a reload, theerrorframe carryingcontext_used: 7000; the first run on the code fix(*): restore the context ring from the window the last call measured #1119 merged keptContext 3.1k/40k 8%after the failure and readContext 6.2k/40k 16%after the reload, and the second without the failure-marker change fell back to the turn before's 3.1k on the reload. Where the empty replies after the tool call are billed at one token each, as an upstream that fails a call bills it, the ring readContext 6.2k/40k 16%after the failure and again after a reload, theerrorframe carryingcontext_used: 6200; on the head the review was on, before the streaming exit carried the verdict, the same run readContext 2/40k 0%on both. Where the reader stops the turn while the call after its tool call hangs, the ring readContext 6.2k/40k 16%right after Stop and again after a reload, the cancel frame carryingcontext_used: 6200; the same run without the cancel change stayed atContext 3.1k/40k 8%after Stop and readContext 6.2k/40k 16%only after the reload.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Left unchecked as not applicable: no docs page describes the ring or the
errorevent's fields.Risk
Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Security: the usage on the
errorand cancel events is integers the runtime computed from the provider's usage reports. It is kept in an RPC-layer map keyed by turn id that each turn's end clears, and nothing new reaches a provider payload.Compatibility: the
errorevent, a failed turn's and now a stopped turn's, gains an optionalusage, which a client that does not read it ignores. A call flagged as an upstream transport failure no longer counts as a window reading, so it no longer feeds the proactive compaction trigger either. A turn that ends on an empty reply leaves its reading on the newest kept row, which can be a tool row, about 25 bytes more.Rollback: revert the squash commit.
Related Issues
#1119 (the pull request this follows; nothing to close)