fix(providers): detect a mid-stream LLM stall within seconds - #1888
fix(providers): detect a mid-stream LLM stall within seconds#1888Aaronontheweb wants to merge 3 commits into
Conversation
Adversarial review — non-author agentVerdict: REQUEST CHANGES. A non-author agent reviewed this change in an isolated worktree and re-ran the gates itself: slopwatch Good news first: the highest-risk trap is SAFE. A stream that emits tokens then stalls is NOT retried Blocking findingsF1 — The guard duplicates a more-capable watchdog Netclaw already owns, and it arms on the wrong F2 — The inactivity timer stays armed across the F3 (discussable) — Verified soundPartial-output-on-retry (no double emission); decorator ordering; timer lifetime (uses NitsStale "Logging -> Retry" comment ( This PR is a do-not-merge focus PR. Core insight for the upstream decision: Netclaw already owns the |
Adversarial re-review — non-author agent (rework, commit ced240d)Verdict: REQUEST CHANGES — but the core mechanism is verified SOLID. Both findings are design/doc, Verified solid (revert experiments + own gate runs)
FindingsF-A [design/coupling] — F-B [doc/behavior mismatch] — the comments deny a real behavior. The comments say a reasoning-only NitsA 45s-deadline timer TOCTOU (astronomically unlikely, retryable, self-heals); the Decision for this skunkworks focus PRThe core fix is correct and adversarially verified. Both findings are design/doc, not correctness, and |
Adversarial review follow-upThis commit fixes both findings from the adversarial review. Finding 1: narrow the InternalsVisibleTo grant
Fix: the predicate now lives in a new public class, Finding 2: fix the doc/behavior mismatchDoc comments in Fix: the comments now state the true rule. The predicate and the Verification
|
A dead or half-open LLM connection can send a few tokens, then go silent. No error appears. The connection stays open. The old per-call watchdog catches this stall only after minutes. A caller then has little time left in its own retry budget. Add StreamStallGuardChatClient. It arms an inactivity timer after the first update in a stream. It cancels the read and throws a TimeoutException when no update arrives within the timeout. The timer does not run before the first update. A self-hosted backend can stay silent for minutes during a cold prefill. The old per-call watchdog still covers that case. Add RetryPolicy.StreamInactivityTimeout. The default value is 45 seconds. Set it to zero to turn off the new guard. The guard sits below RetryingChatClient in the pipeline. The RetryPolicy.ShouldRetry rule already treats a TimeoutException as retryable. No new retry path exists. This change revives the idea in closed PR netclaw-dev#1272. That PR added TCP keepalive to SocketsHttpHandler. The author closed it: keepalive only proves liveness between Netclaw and its reverse proxy. It does not detect a stuck upstream stream behind Caddy. This fix uses an application-layer signal instead.
An adversarial review of this pull request found two bugs in StreamStallGuardChatClient. Bug 1: the guard armed its tight timer on any update. A keepalive or a reasoning-only delta could arm it. A legitimate cold prefill could then abort too early. Bug 2: the timer stayed armed across each yield return. A slow consumer could hold an update past the window. The guard could then throw a false timeout. Fixes: - Arm the timer only after the first substantive update. Reuse StreamingResponseReader.IsSubstantiveUpdate. ProcessingWatchdog already uses this same rule. Netclaw.Actors now exposes its internals to netclawd for this reuse. - Disarm the timer right after each update arrives. Arm it again just before the next read. The timer now measures provider silence only. - Add schema coverage for Session.Tuning.StreamingRetryPolicy. An operator can now set StreamInactivityTimeout through netclaw.json. - Fix a stale comment. The pipeline order is Logging, then Retry, then StreamStallGuard, then the leaf client. Tests: - Two new tests prove the arm-on-keepalive bug and the disarm-on-yield bug stay fixed. - One test proves the new schema keys pass doctor validation. - One test proves the new config keys bind to the runtime RetryPolicy.
An adversarial review found two problems in this branch. Problem 1: Netclaw.Actors granted InternalsVisibleTo to netclawd. This was the first production-to-production internal grant in the repo. The grant existed only so StreamStallGuardChatClient could call StreamingResponseReader.IsSubstantiveUpdate. Fix: extract the predicate into a new public class, ChatStreamUpdateClassifier, in Netclaw.Actors. StreamingResponseReader and StreamStallGuardChatClient both call this one public method now. The logic stays the same. The InternalsVisibleTo grant to netclawd is removed. The three grants that pre-date this branch stay. Problem 2: two doc comments claimed a reasoning-only delta never arms the stall guard's tight timer. This claim is false. A reasoning delta with text is substantive, so it arms the timer, the same as a text or tool-call delta. Only a content-free keepalive leaves the timer unarmed. Fix: correct the doc comments in StreamStallGuardChatClient.cs and RetryPolicy.cs. The predicate and the guard timing do not change. Two test comments also named the old internal method; they now name ChatStreamUpdateClassifier. Tests: Netclaw.Daemon.Tests (1024), Netclaw.Configuration.Tests (520), Netclaw.Actors.Tests (3052 passed, 1 pre-existing skip), and the ConfigSchemaDoctorCheck filter in Netclaw.Cli.Tests (24) all pass. dotnet slopwatch analyze finds 0 issues. Add-FileHeaders.ps1 -Verify passes.
102bfde to
566eb29
Compare
The macOS test failure is a pre-existing flake, not this PRTwo specialist analyses (Akka.NET + .NET concurrency, independent verification) root-caused the
The test landed in #1904 (2026-08-12) and flaked the same day. This is also a small production |
|
This fix is stupid and is going to create problems with self-hosters who have a long TFT window. Closing it. |
Problem
A dead or half-open LLM connection can send a few tokens, then stop. No
error appears. The connection stays open. A benchmark run hit this case.
A stream stalled for about 605 seconds. The stall lost a race against an
outer 900-second kill. The run lost most of its time budget.
Netclaw catches this stall today with one coarse per-call watchdog
(
FirstTokenTimeout, 600 seconds by default). This bound must staygenerous, because a self-hosted backend can stay silent for minutes
during cold prefill.
Five sidecar LLM call paths have no inter-delta stall guard at all:
Each sidecar path has only a flat absolute deadline (10 to 450 seconds).
A stall on a sidecar path can run to the full deadline before Netclaw
detects it.
Prior art: closed PR #1272
PR #1272 proposed TCP keepalive on the LLM
HttpClient. The authorclosed the PR after this finding. TCP keepalive proves only that the
socket between Netclaw and its reverse proxy (Caddy) stays alive. TCP
keepalive does not detect a stuck upstream stream. A self-hosted backend
can hold an established connection open, send zero bytes, and still
answer keepalive probes at the TCP layer.
The fix needs a signal from the application layer, not the transport
layer.
Mechanism
This PR adds
StreamStallGuardChatClient, a newIChatClientdecoratorat
PipelineChatClientFactory.Compose. This seam wraps every LLM callpath: the main session, the sub-agent path, and all five sidecar paths.
The guard arms an inactivity timer only after the first substantive
update in a stream.
ChatStreamUpdateClassifier.IsSubstantiveUpdateisthe one public predicate that decides "substantive."
StreamingResponseReaderand this guard both call the same predicate, so the two components share
one rule.
Note: a reasoning-only delta with text counts as substantive. It arms
the timer, the same as a text or tool-call delta. This behavior matches
the watchdog rule used elsewhere in the codebase. Only a content-free
keepalive leaves the timer unarmed.
Once armed, every later update resets the timer. Only a stream that
produces no more output at all trips it.
The timer measures provider silence only. The guard disarms the timer
right after each update arrives, and arms it again just before the next
read. A slow downstream consumer that holds an update past the window
does not trigger a false abort.
On expiry the guard raises a
TimeoutException.RetryPolicy.ShouldRetryclassifies this exception as retryable, the same as any other transient
failure. No new retry path exists. By the time this guard catches a
stall, the stream already sent at least one chunk. The transport layer
cannot silently retry a partial stream.
Config
The new timeout lives on
RetryPolicy:StreamInactivityTimeout. Thedefault value is 45 seconds. An operator can set this value to zero to
turn the guard off.
src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.jsonnowcovers
Session.Tuning.StreamingRetryPolicy, so an operator can setthis value through
netclaw.json.Relationship to other work
This PR does not remove or change
ProcessingWatchdog. The main sessionpath and the sub-agent path keep
ProcessingWatchdog. On those twopaths,
StreamStallGuardChatClientruns as a uniform guard under thecurrent watchdog.
PR #1893 adds a separate, later half: turn-level resume after a caught
stall. This PR only shortens the time to catch a stall. PR #1893
changes what happens after Netclaw catches one.
Tests
StreamStallGuardChatClientTests.csuses aFakeTimeProvider. No testuses a real
Task.DelayorThread.Sleepwait. The tests prove:the inactivity window.
no abort.
this guard.
until a substantive update arrives.
an abort.
PipelineChatClientFactoryTests.csadds two end-to-end tests. Thesetests prove the same behavior through the full composed pipeline:
Logging → Retry → StreamStallGuard → leaf.ConfigSchemaDoctorCheckTests.csadds one test. This test proves aconfig that sets
Session.Tuning.StreamingRetryPolicypasses schemavalidation.
SessionConfigDefaultsTests.csadds one test. This test proves the newconfig keys bind to the runtime
RetryPolicy.Validation
This branch is rebased onto the current
devbranch (upstream/devatb1832884). The rebase had no conflicts.dotnet build Netclaw.slnx— clean, 0 warnings, 0 errorsdotnet test src/Netclaw.Daemon.Tests— 1027 passeddotnet test src/Netclaw.Configuration.Tests— 596 passeddotnet test src/Netclaw.Cli.Tests --filter ConfigSchemaDoctorCheck—24 passed
dotnet test src/Netclaw.Actors.Tests— 3149 passed, 1 skipped (aWindows-only test)
dotnet slopwatch analyze— 0 issuesAdd-FileHeaders.ps1 -Verify— all files have headersReview record
Two adversarial review cycles checked this branch. A non-author agent
ran each cycle in an isolated worktree, and re-ran the gates itself.
The first cycle found three issues:
A follow-up commit fixed all three.
The second cycle found two more issues:
InternalsVisibleTogrant was wider thanthe guard needed.
claim was false.
A follow-up commit narrowed the grant to one public predicate
(
ChatStreamUpdateClassifier). The same commit fixed the comment.All five findings are fixed on this branch. The full review trail is in
the PR comments.