feat(sessions): resume the turn after a mid-stream LLM timeout - #1893
feat(sessions): resume the turn after a mid-stream LLM timeout#1893Aaronontheweb wants to merge 5 commits into
Conversation
Adversarial review — non-author agentVerdict: REQUEST CHANGES. Two critical defects; one high; two medium. Gates all green (reviewer C1 — The discarded partial text reaches the user's answer (silent corruption)The dead call's deltas were already emitted as C2 — The safety gate is per-TURN, so it refuses almost every real stall
H3 — Resume restarts on PrefillTimeout and ignores the restart drainEach resume resets M4 — The new tests fail by HANGING, not asserting
M5 — Token/cost accounting drops the dead callThe provider bills the discarded call; the session counts nothing. Up to 3 billed input contexts NitsNo file-to-runtime round-trip test for the new knob (Cross-Boundary Contract Rule); message-list This is a do-not-merge focus PR; findings will be fixed on the branch and re-reviewed. |
|
This revision fixes all five findings from the adversarial review.
Full detail on each fix, including why the gate deletion is safe, is |
Adversarial re-review (rework commit 32b72af) — non-author agentVerdict: REQUEST CHANGES. Two new data-loss paths on the exact surface the PR targets; one prior D1 (High) — Over-discard deletes text from earlier COMPLETED calls in the turnThe headless and accumulator buffers are TURN-scoped; D2 (High) — Headless envelope goes EMPTY when the resumed call does not stream deltasThe discard arm clears D3 (Medium) — H3 arming dies on the first keepalive
D4 (Medium) —
|
|
This revision fixes all four findings from the second adversarial review.
Each fix has a test that fails without it (reversion-tested by hand The Termina tape harness ( |
Focused verify pass (commit 3145635) — non-author agentVerdict: REQUEST CHANGES — but all four D-fixes HOLD (each reversion-tested with the documented
Verified holding: the committed-length keystone (no double-commit; delta-seen branch skips the append; Gates (reviewer-run): Actors 3065, Cli 1326, Daemon 1016, Configuration 521 — 0 failures; slopwatch 0; F1-F3 will be fixed on the branch; F4/F5 accepted as documented. Do-not-merge focus PR. |
|
Fixed the three verify-pass findings from the last review round.
Each fix has a test that fails without it. Touched suites are green: New commit: 64a8701 |
Targeted re-verify (commit 64a8701) — same non-author verifierVerdict: F1, F2, F3 all CONFIRMED FIXED. No new defect.
Every new test fails without its own hunk (reversion-tested). Gates: Actors 3066, Cli 1333 — 0 Review record for this PR: 2 full adversarial reviews + 1 focused verify + 1 targeted re-verify, all |
Termina tape harness: PASSThe native smoke harness ran on this PR's head (64a8701) on a local Linux host: This closes the one open Definition-of-Done item (the ChatPage change is a Termina surface). Host |
A provider stream stall (a few tokens, then silence) kills the whole turn today. The watchdog waits the full timeout budget, then the turn fails. In headless chat mode, a failed turn is a failed session. This change adds a bounded, turn-scoped resume in LlmSessionActor. When an LLM call times out (watchdog expiry or a TimeoutException from LlmCallFailed) and no tool call has run yet this turn, the actor discards the dead call and reissues the same call with the same message list. Discard is structural: a call that never completes never writes to session state, so there is nothing to roll back. The resume has a safety gate. It skips resume once a tool call has run this turn, to avoid the risk of a double execution. It also has a per-turn budget, Session.Tuning.TimeoutResumeRetryBudget, default 2. After the budget runs out, the turn fails exactly as before this change. Each resume attempt logs a warning with the attempt number and the budget. A fresh session's first call also needed a message-list fix: the "startup context injected" flag flips before the network call resolves, so a naive resume would silently drop the once-at-start context layers from the retry. The actor now rolls that flag back before a resume so the retried call matches the dead call exactly. Schema: adds Session.Tuning.TimeoutResumeRetryBudget to netclaw-config.v1.schema.json with a default, per the config schema sync rule. Tests: LlmTurnResumeTests proves the discard-and-resume path, the budget limit, and the tool-dispatch safety gate, using the existing TestScheduler-based watchdog test pattern. Two existing watchdog tests (LlmSessionStreamingTimeoutTests, LlmSessionWatchdogTests) now set the new budget to 0, since they check the watchdog itself, not resume. Scope: sub-agent calls (SubAgentActor) keep their own watchdog handling. This change does not touch that path.
The review found five defects in the LLM turn resume feature. This commit fixes each one. Dead-call corruption (C1). A resumed call streamed a second full answer. Every delta-accumulating consumer appended the new text onto the old text. The fix adds a TextStreamDiscarded output message. The session actor sends this message before a resume. HeadlessChannel, ExecutionOutputAccumulator, and ChatPage now clear buffered text on receipt. Structural safety, not a tool-iteration gate (C2). The actor no longer checks ToolIterationCount before a resume. Tool dispatch happens only after a call completes, inside HandleLlmResponseReceived. A call that times out mid-stream never reaches that handler. Resume is now safe on any call in the turn. Restart drain and watchdog budget (H3). TryResumeAfterTimeout now checks _restartDrainRequested and refuses resume during a coordinated daemon restart. A resumed call now arms the watchdog on the promoted inter-delta budget, not the full prefill budget. The retry budget can no longer triple the time to a final failure. Bounded test wait (M4). WaitForStreamInvocationAsync now has an explicit timeout. A broken resume now fails the test instead of hanging it. Usage accounting for discarded calls (M5). A discarded call bills the provider but reports no usage. UsageOutput now carries an estimated token count and an attempt count for discarded calls. Both fields stay separate from the real provider totals. New and changed tests prove each fix. LlmTurnResumeTests gained a restart-drain test and an inverted tool-dispatch test. A new delta-accumulation test pipes the real actor output through ExecutionOutputAccumulator. A new LlmTurnResumeWatchdogArmingTests fixture proves the watchdog arm value. HeadlessChannelTests and a new ChatPageTests case prove the discard signal clears buffered text. A config round-trip test binds TimeoutResumeRetryBudget from JSON through SessionConfig.
A second adversarial review of the LLM turn resume feature found four new defects: two data-loss paths, one arming defect, and one performance defect. This commit fixes all four. D1: HeadlessChannel and ExecutionOutputAccumulator cleared the whole turn-scoped text buffer on TextStreamDiscarded. A stall after an earlier completed tool round wiped that call's already-delivered preamble too. Both consumers now track a committed-length marker and truncate back to it on discard, using the same call-scoped segment method ChatPage already uses. TextOutput already marks a call boundary, so the fix adds no CallId field to the protocol. D2: HeadlessChannel left its "saw a delta" flag turn-scoped, so a resumed call that streams zero deltas reported an empty JSON envelope for a successful turn. The flag now resets at every call boundary, the same fix that closes D1. D3: a resumed call's watchdog arm reverted to the full prefill budget on the first content-free keepalive, and forced the promoted budget onto a call that died during prefill with zero content. FireLlmCall now carries the dead call's own _anyContentStreamed value forward instead of resetting it, so the arm stays correct through the whole resumed call. D4: EstimateInputTokens re-stringified the full message list on every ContinueFireLlmCall to serve only the resume path. The fix deletes that method and reuses _lastInputTokenCount, the provider's real input count already tracked for the compaction trigger. If no call in this session ever reported real usage, the estimate reports null instead of a fabricated number. Each fix has a test that fails without it (reversion-tested). All touched suites are green with 0 failures: Netclaw.Actors.Tests (3065 passed), Netclaw.Daemon.Tests (1016 passed), Netclaw.Cli.Tests (1326 passed), Netclaw.Configuration.Tests (521 passed). Slopwatch reports 0 new issues. File headers are clean.
F1 (regression): the console usage line lost its newline. - The newline guard before the [usage] line used a call-scoped flag. - TextOutput resets that flag before UsageOutput arrives. - The guard did not fire after a streamed turn. - [usage] then printed on the same line as the last streamed text. - The fix adds a turn-scoped flag for the newline guard only. - The call-scoped flag still drives the buffer and commit logic. F2 (race): approval notices broke the call-boundary contract. - Three notices (expired prompt, wrong requester, unavailable option) send TextOutput while another call still streams. - The committed-length consumers treated every TextOutput as a call boundary. - A notice moved the commit marker past the live call's partial text. - A later stall and discard then removed nothing. - The resumed answer glued onto the dead partial text. - The fix adds IsCallBoundary to TextOutput. The default is true. - The three notices set IsCallBoundary to false. - HeadlessChannel, ExecutionOutputAccumulator, and ChatPage now move their call-boundary marker only when IsCallBoundary is true. - The DTO and the mapper carry the new field on the wire. F3 (nit): the discarded input estimate printed as an empty token. - The console usage line printed discarded_est_in= as empty when no completed call reported a real estimate yet. - The fix omits discarded_est_in= when the value is null. - discarded_attempts= still prints when the attempt count is greater than zero. Each fix has a test that fails without it. Touched suites are green: Netclaw.Actors.Tests (3066 passed, 1 skipped), Netclaw.Cli.Tests (1333 passed), Netclaw.Daemon.Tests (1016 passed), Netclaw.Configuration.Tests (521 passed). Slopwatch reports 0 new issues. File headers are clean.
64a8701 to
3008133
Compare
|
Fable went a bit nuts with the comments here - going to review this now. This PR was generated while running Netclaw through Terminal Bench 2.1 and we kept failing some tasks due to the Deepseek Platform API having network connectivity problems. We'd already had an earlier pull request where we tried implementing essentially the same fix to use the |
Problem
A dead or half-open LLM stream ends the whole turn today. In headless
mode, a failed turn is a failed session. No external process retries
a failed session.
A benchmark run exposed the size of this gap. The run had 13 turn
failures. Correlated provider stall storms caused 9 of them. Each
stall showed the same pattern.
The model sent a few tokens. Then it went silent. The watchdog
waited the full inter-delta budget,
FirstTokenTimeout, 600 secondsby default. Then the turn failed. Each stall burned close to 600
seconds this way.
PR #1888 makes this wait shorter. It does not change the outcome.
See Relationship below.
Mechanism
LlmSessionActornow has a method:TryResumeAfterTimeout. Thismethod runs on two paths: a watchdog expiry for the LLM call, and a
call failure whose cause is a timeout.
On a timeout, the actor takes these steps, in order:
progress, makes the actor fail the turn at once. This matches
every other mid-turn continuation under a restart drain.
the turn.
TextStreamDiscardedoutput. See Consumer correctnessbelow.
call that never completes never writes to session state.
same tool-use flag. The user message stays the same. The session
history stays the same.
budget, and the estimated input size of the discarded call.
The actor resumes safely because of its own structure. It needs no
separate gate for this safety. The actor dispatches a tool call only
from a fully completed response. A call that times out mid-stream
never reaches that point, on any iteration.
So a timed-out call can never dispatch a tool call in the same turn.
The actor can safely resume on any call in the turn. This covers a
call after an earlier tool round completed, too. All 9 stalls behind
this change follow this pattern.
A discarded call still costs input tokens. The provider never
returns usage for a call that fails before it completes. The actor
now reports this fact honestly.
UsageOutputcarries two newfields:
DiscardedResumeEstimatedInputTokensandDiscardedResumeAttempts. Both fields stay null when the actor doesnot resume in the turn. The estimate reuses the real input count
from the last completed call, an honest proxy, not a fabricated
guess.
Consumer correctness
The dead call sent zero or more delta events before it went silent.
The resumed call sends its full answer again, from the start. A
subscriber that joins delta text for the turn would glue the dead
call's partial text onto the resumed call's answer.
Three consumers join delta text this way:
HeadlessChannel)ExecutionOutputAccumulator)ChatPage)The fix adds a new output:
TextStreamDiscarded. The system alwaysdelivers this output. A subscriber's output filter cannot block it.
HeadlessChannelandExecutionOutputAccumulatornow track acommitted-length marker. The marker records how much of the buffer
holds text from a call that already reached its own
TextOutputthis turn.
TextStreamDiscardedtrims the buffer back to thatmarker. Only the dead call's own, unsent text gets removed. An
earlier, completed call's text stays intact.
TextOutputgained a new field:IsCallBoundary. This field marksthe point where one call's own text output is complete. A subscriber
uses this point to move its commit marker forward.
One exception applies. The approval-prompt-expired notice, and its
sibling notices, also send as a
TextOutput. These notices setIsCallBoundaryto false. A notice like this can fire mid-turn,while an unrelated call still runs. It must not move a subscriber's
commit marker over live, unsent text.
Config
A new config value bounds the retry count:
Session.Tuning.TimeoutResumeRetryBudget. Its default value is 2.When the budget runs out, the turn fails. It fails the same way it
failed before this change. There is no infinite loop. There is no
silent fallback.
The schema file,
netclaw-config.v1.schema.json, has the new entry.The entry has a default value and a description.
Relationship
PR #1888 adds a fast, 45-second stall detector at the provider seam.
That change shortens the wait before the session catches a stall. It
does not change the outcome. The turn still fails. This PR adds the
recovery half. Detection and recovery now work as one pair.
Both PRs add a sibling entry to the same
Session.Tuningschemaobject. #1888 adds
StreamingRetryPolicy. This PR addsTimeoutResumeRetryBudget. Whichever PR merges second will hit asmall, one-line rebase conflict there. The fix is trivial: keep both
entries.
Tests
New tests prove each behavior with a fake stream, not a real
provider:
deltas, keeps the completed calls' text. It removes only the dead
call's own partial text.
sent real content stays on the tighter, promoted budget through a
later keepalive. A resumed call that died during prefill, with no
content, keeps the full prefill budget.
deltas still reports the full answer. It does not report an empty
one.
turn fails exactly as it did before this change.
fails the turn. The actor does not resume.
real input count from the last completed call. The estimate
reports null when no call completed yet this session.
Validation
All four required suites pass on the rebased branch (
upstream/devat
19ed641e, includes #1890 and #1896):Netclaw.Actors.Tests: 3163 passed, 1 prior Windows-onlyskip, 0 failed.
Netclaw.Daemon.Tests: 1020 passed, 0 failed.Netclaw.Configuration.Tests: 597 passed, 0 failed.Netclaw.Cli.Tests: 1367 passed, 0 failed.dotnet build Netclaw.slnx -c Release: succeeds. This build coversthe full solution, benchmarks included.
dotnet slopwatch analyze -d .: 0 new issues. One prior issueremains in an untouched file (
PowerShellHostProbeTests.cs, fromPR fix(shell): harden PowerShell host probe against cold-start timeouts #1859), outside this PR's diff.
Add-FileHeaders.ps1 -Verify: all files have headers.The native tape harness also ran, on this PR's head commit, on a live
host. Per the PR comment:
./scripts/smoke/run-smoke.sh light—"All smoke checks passed (23 tapes, 9 scenarios)". This result
closes the one open item from the review record below.
Review record
This PR went through four non-author review passes before this
rewrite:
defects. The next revision fixed all five.
data-loss paths. It also found one prior fix, only half done, and
one performance defect. The next revision fixed all four.
found 3 narrower issues. The next revision fixed all three.
defect.
Every issue, across all four passes, got fixed on the branch. The
full trail sits in the PR comments.
Scope
Sub-agent calls (
SubAgentActor) keep their own watchdog logic.This PR does not touch that path.