Skip to content

fix(agents): keep finite LLM idle watchdog under unlimited run budgets#101986

Closed
obviyus wants to merge 5 commits into
mainfrom
fix/llm-idle-timeout-semantics
Closed

fix(agents): keep finite LLM idle watchdog under unlimited run budgets#101986
obviyus wants to merge 5 commits into
mainfrom
fix/llm-idle-timeout-semantics

Conversation

@obviyus

@obviyus obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Track B3 of #101863. Three coupled timeout-semantics defects, all verified in source and one proven live (the wedge in #101910's E2E was built on exactly this coupling):

  1. An unlimited run budget (runTimeoutMs >= MAX_TIMER_TIMEOUT_MS) made resolveLlmIdleTimeoutMs return 0 — disabling mid-stream stall detection entirely. Users who configure "no timeout" for long tasks silently trade away all liveness: one TCP half-open or provider stall wedges the turn forever.
  2. Self-hosted providers (vLLM et al., [Bug]: Agent stall detector hard-coded 120s threshold kills legitimate long model calls on local vLLM #85826) got 300s for the first event but only the generic 120s for mid-stream gaps — legitimate batching/scheduling pauses over two minutes killed the call unless the baseUrl happened to be local.
  3. agents.defaults.timeoutSeconds: 0 clamped to 1 second (Math.max(seconds, 1)), while the per-run override path defines 0 as unlimited — the same key with opposite semantics depending on position.

Why This Change Was Made

  • The unlimited-budget sentinel now falls through to provider-class idle/first-event defaults instead of 0: an unlimited budget bounds total cost, not liveness. Explicit models.providers.<id>.timeoutSeconds still wins and may exceed the defaults; genuinely local baseUrls keep their existing idle opt-out (prompt-eval silence is normal there).
  • New SELF_HOSTED_LLM_IDLE_TIMEOUT_MS = 300_000 aligns self-hosted mid-stream tolerance with the existing 300s first-event allowance. Cloud stays 120s; cron clamps unchanged.
  • Config timeoutSeconds: 0 now resolves to the same unlimited sentinel as the override path. Safe because of the first change: unlimited budget no longer implies zero stall detection. The prior behavior (0 silently becoming a 1s budget that kills every turn) is a footgun, not a contract anyone can be relying on.
  • Docs: docs/concepts/agent-loop.md timeout table updated (unlimited-budget semantics, self-hosted idle default).

User Impact

  • The "long-task config" (timeoutSeconds: 0) becomes safe by default: unlimited run budget with a finite per-class stream watchdog — the exact hang mode reported as "agent hangs mid-task forever" can no longer be configured accidentally.
  • Self-hosted/vLLM users stop losing calls to legitimate >120s mid-stream pauses.
  • No change for anyone with explicit provider timeouts or local endpoints.

Evidence

  • Resolver matrix traced case-by-case against current main (sentinel × explicit/implicit budget × cloud/self-hosted/local × cron) — precedence identical except the three intended changes.
  • Tests: extended resolver tables cover sentinel → class defaults (not 0), self-hosted 300s vs cloud 120s, explicit provider timeout precedence, local opt-out unchanged, and timeoutSeconds config table (0 → unlimited, finite, negative, NaN). Composition test: config 0 + cloud provider = unlimited budget AND 120s idle watchdog.
  • Validation: resolver + import-sweep suites (314 tests), tsgo core/core-test lanes, oxlint/oxfmt — green on Testbox.

Part of #101863 (Track B3). Fixes #85826; closes the timeoutSeconds: 0 footgun identified in the #101863 audit (root cause 2).

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Jul 8, 2026
@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 8:21 AM ET / 12:21 UTC.

Summary
The branch changes agent timeout resolution, config validation, docs, and tests so unlimited run budgets keep finite LLM stream watchdogs, timeoutSeconds: 0 is accepted as unlimited, and self-hosted providers get a 300s idle tier.

PR surface: Source +54, Tests +201, Docs 0. Total +255 across 8 files.

Reproducibility: yes. source-reproducible. Current main and v2026.6.11 still show the no-timeout sentinel disabling idle watchdog behavior and the schema rejecting timeoutSeconds: 0, while PR-head tests and live output cover the after-fix paths.

Review metrics: 1 noteworthy metric.

  • Timeout/config semantics: 1 existing config field broadened; 2 watchdog defaults or guards changed. The PR changes timeoutSeconds: 0 handling plus self-hosted idle and local creation guarding, so maintainers should explicitly review upgrade and availability tradeoffs.

Root-cause cluster
Relationship: superseded
Canonical: #102160
Summary: The atomic integration PR is the canonical landing path for this timeout-semantics track; this PR remains a per-track review record until that path lands or is rejected.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Risk before merge

  • [P1] The branch broadens an existing config contract: agents.defaults.timeoutSeconds: 0 changes from rejected or effectively clamped behavior to an unlimited run budget.
  • [P2] The self-hosted implicit idle watchdog moves to 300s, which fixes vLLM-style false positives but delays failure or fallback for genuinely silent self-hosted streams.
  • [P1] The PR discussion says this track should land atomically through fix(agents): run-lifecycle reliability — bounded release, evidence-based liveness, watchdog semantics #102160; direct merge or cleanup before that lands would bypass the recorded integration plan.

Maintainer options:

  1. Use the atomic integration PR (recommended)
    Keep this draft open as the review record and let fix(agents): run-lifecycle reliability — bounded release, evidence-based liveness, watchdog semantics #102160 carry the final timeout semantics with the rest of the coupled liveness stack.
  2. Merge this track directly
    Maintainers could land this branch alone, but that accepts the risk of putting an intermediate run-lifecycle state on main outside the recorded atomic plan.
  3. Pause for more soak time
    Keep both this review record and the integration draft open until the production soak and final full-gate decision are complete.

Next step before merge

  • [P2] Manual maintainer handling remains because this is a protected draft review record whose remaining action is the atomic integration/soak decision, not a narrow repair on this branch.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes TypeScript agent/config runtime code, tests, and docs without dependencies, workflows, permissions, secrets, lockfiles, or package-resolution changes.

Review details

Best possible solution:

Keep this PR as the Track B3 review record, land the identical changes through the atomic integration PR after maintainer soak/acceptance, then close this branch from that merged path.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible. Current main and v2026.6.11 still show the no-timeout sentinel disabling idle watchdog behavior and the schema rejecting timeoutSeconds: 0, while PR-head tests and live output cover the after-fix paths.

Is this the best way to solve the issue?

Yes for this per-track branch. The resolver/config boundary is the right fix location and the prior explicit self-hosted run-cap gap is covered; the safest merge vehicle is the atomic integration PR rather than direct landing.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4bf70be01a21.

Label changes

Label justifications:

  • P1: The PR targets agent liveness hangs and self-hosted model timeout failures that can break active agent/channel workflows for real users.
  • merge-risk: 🚨 compatibility: The diff changes existing timeout config semantics and implicit provider-class timeout defaults that existing operators may observe after upgrade.
  • merge-risk: 🚨 availability: The timeout resolver changes when stalled streams are detected, retried, or left unpoliced, affecting whether agent turns hang, fail over, or wait longer.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Contributor-supplied live E2E output shows the timeoutSeconds: 0 black-hole provider path after the fix, including watchdog fires around 300s, retry/failover, schema acceptance, and lane recovery.
  • proof: sufficient: Contributor real behavior proof is sufficient. Contributor-supplied live E2E output shows the timeoutSeconds: 0 black-hole provider path after the fix, including watchdog fires around 300s, retry/failover, schema acceptance, and lane recovery.
Evidence reviewed

PR surface:

Source +54, Tests +201, Docs 0. Total +255 across 8 files.

View PR surface stats
Area Files Added Removed Net
Source 4 75 21 +54
Tests 3 205 4 +201
Docs 1 7 7 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 8 287 32 +255

What I checked:

  • Repository policy read: Root AGENTS.md plus scoped src/agents, embedded-runner, and docs AGENTS.md were read; compatibility-sensitive config/default guidance and protected-label cleanup policy shaped the keep-open verdict. (AGENTS.md:16, 4bf70be01a21)
  • Live PR state: Live GitHub state shows this PR is open, draft, assigned to obviyus, labeled maintainer, proof: sufficient, P1, and ready for maintainer look, so conservative cleanup must not close it automatically. (d2a3cc9ba760)
  • PR-head resolver implementation: PR head routes finite non-cron run budgets and agent defaults through a provider-class idle ceiling, preserving local opt-out, self-hosted 300s, and cloud 120s behavior. (src/agents/embedded-agent-runner/run/llm-idle-timeout.ts:281, d2a3cc9ba760)
  • Runtime caller path: runEmbeddedAttempt passes resolved run timeout, provider request timeout, base URL, model id, and provider id into both idle and first-event timeout resolvers, so the resolver change is on the embedded agent runtime path. (src/agents/embedded-agent-runner/run/attempt.ts:3219, d2a3cc9ba760)
  • Config contract change: PR head broadens the existing agents.defaults.timeoutSeconds schema from positive to nonnegative and documents 0 as unlimited while stream liveness still applies. (src/config/zod-schema.agent-defaults.ts:232, d2a3cc9ba760)
  • Regression coverage: PR-head tests cover large agent defaults, explicit run budgets above provider-class tiers, and below-tier self-hosted caps, including the prior review blocker for explicit self-hosted run caps. (src/agents/embedded-agent-runner/run/llm-idle-timeout.test.ts:523, d2a3cc9ba760)

Likely related people:

  • obviyus: They authored this track and the atomic integration stack, posted the live E2E proof, recorded the accepted timeout semantics decision, and are the live assignee. (role: proposal owner and likely follow-up owner; confidence: high; commits: ee1888f2dfe7, 5cbfd99a85d9, d2a3cc9ba760; files: src/agents/embedded-agent-runner/run/llm-idle-timeout.ts, src/agents/embedded-agent-runner/run/attempt.ts, src/agents/timeout.ts)
  • vincentkoc: Current-main blame for the timeout resolver, agent timeout resolver, and agent defaults schema points to recent merged work by Vincent Koc, and he merged adjacent cron idle-watchdog work. (role: recent area contributor and merger signal; confidence: high; commits: cc083e5f4fe0, e085fa1a3ffd; files: src/agents/embedded-agent-runner/run/llm-idle-timeout.ts, src/agents/timeout.ts, src/config/zod-schema.agent-defaults.ts)
  • tyler6204: History for src/agents/timeout.ts shows prior work treating timeout 0 as no-timeout in the agent run resolver, which this PR aligns config defaults with. (role: introduced adjacent timeout semantics; confidence: medium; commits: 8d2f98fb01a6, 191da1feb52e; files: src/agents/timeout.ts)
  • lanzhi-lee: They authored the merged PR that previously repaired provider timeout handling for no-timeout LLM idle paths, adjacent to this PR's resolver behavior. (role: prior idle-watchdog contributor; confidence: medium; files: src/agents/embedded-agent-runner/run/llm-idle-timeout.ts)
  • yujiawei: They authored the merged provider-timeout PR that made models.providers.<id>.timeoutSeconds affect the LLM idle watchdog, a central contract this PR preserves. (role: prior idle-watchdog contributor; confidence: medium; files: src/agents/embedded-agent-runner/run/llm-idle-timeout.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (7 earlier review cycles)
  • reviewed 2026-07-08T03:11:35.038Z sha ee1888f :: needs real behavior proof before merge. :: [P1] Align the config schema with timeoutSeconds: 0
  • reviewed 2026-07-08T06:40:58.920Z sha 5cbfd99 :: found issues before merge. :: [P3] Fix cron self-hosted timeout wording
  • reviewed 2026-07-08T07:41:19.223Z sha f677617 :: needs changes before merge. :: [P2] Apply self-hosted idle defaults before the agent cap
  • reviewed 2026-07-08T07:58:00.772Z sha f677617 :: needs changes before merge. :: [P2] Apply provider-class idle defaults before caps
  • reviewed 2026-07-08T09:57:02.373Z sha 02eb663 :: needs changes before merge. :: [P2] Apply self-hosted defaults to explicit run caps
  • reviewed 2026-07-08T10:05:36.590Z sha 02eb663 :: needs changes before merge. :: [P2] Apply self-hosted defaults to explicit run caps
  • reviewed 2026-07-08T12:11:00.428Z sha d2a3cc9 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 8, 2026
@obviyus

obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Live E2E validation — and two gaps it caught (both fixed in 5cbfd99)

Re-ran the #101910 wedge harness (black-hole model server, scratch gateway profile) against this branch with the previously-lethal config: agents.defaults.timeoutSeconds: 0, no explicit provider timeoutSeconds, stuck-session recovery neutralized so the watchdogs themselves are measured.

Results

scenario result
Headers arrive, then eternal silence PASS — first-event watchdog fired at 300.7s ([llm-idle-timeout] blackhole/wedge-model produced no reply before the idle watchdog; retrying same model), one bounded same-model retry fired again at 300.1s, then loud failover. Three independent fires all in the 300–302s band.
Lane free after watchdog failure PASS — message 2 admitted ~1s after the failover, fresh request hit the black hole.
timeoutSeconds: 0 accepted from openclaw.json FAIL as shipped in the first commit — the resolver change was unreachable: the config schema still required positive(), so both old and new builds rejected the config at startup.
Headers never arrive (raw black hole) FAIL as shipped in the first commit — 556.6s observed with zero watchdog action (ended only by killing the server). OpenAI stream creation awaits response headers; the transport first-event guard only arms after creation; and the idle wrapper that does bound creation is not installed when local idle resolves to 0.

Fixes (second commit, 5cbfd99a85d)

  1. Schema: agents.defaults.timeoutSeconds now nonnegative() — 0 is accepted and means unlimited budget (table-driven schema tests added). Per-agent entries never had this key; unchanged.
  2. Creation deadline for local providers: streamWithIdleTimeout gains scope: "creation-only"; when idle resolves to 0 (local opt-out) but a finite first-event timeout exists, stream creation is bounded by the first-event budget while iterator gaps stay unpoliced (local providers own their pacing). A headers-never-arrive request now fails at the first-event deadline instead of wedging until the run budget. Test covers both halves (creation bounded, gaps not).

With these, the accidental-wedge matrix is closed end-to-end: every default path has a finite creation + first-event guard, and "unlimited" only ever refers to the run budget. Full evidence timeline in the harness report (timestamps, request ledger, log lines) retained locally; happy to attach if wanted.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 8, 2026
@obviyus obviyus mentioned this pull request Jul 8, 2026
Closed
@obviyus

obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Docs finding fixed in f677617: the cron sentence now distinguishes genuinely-local endpoints (loopback/private baseUrl, idle opt-out unchanged) from self-hosted providers on network baseUrls (300s implicit watchdog), and states the explicit-cron-timeout cap. That was the only code/docs-level finding; the remaining item is the maintainer decision on the config-zero + self-hosted-default semantics, which will be recorded here explicitly.

@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 8, 2026
@obviyus

obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Maintainer decision (@obviyus): accept both semantics changes.

  1. agents.defaults.timeoutSeconds: 0 = unlimited run budget. The prior state was not a contract anyone could rely on: the schema rejected 0 outright, and had it passed, the resolver clamped it to a 1-second budget. Zero-as-unlimited matches the existing per-run override semantics for the same concept, and it is safe because this PR also guarantees an unlimited budget never disables stream liveness (live-proven in the E2E comment above: the previously-lethal config now fails loudly at the ~300s watchdog and frees the lane).

  2. Self-hosted implicit idle watchdog 120s → 300s. Aligns mid-stream tolerance with the 300s first-event allowance these providers already get; the 3-minute delay in detecting a genuinely dead self-hosted stream is an acceptable trade against killing legitimate vLLM batching pauses ([Bug]: Agent stall detector hard-coded 120s threshold kills legitimate long model calls on local vLLM #85826). Explicit models.providers.<id>.timeoutSeconds remains the precise control in both directions.

Docs finding fixed in f677617. No remaining blockers from this review.

@obviyus obviyus self-assigned this Jul 8, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Closed
@obviyus obviyus mentioned this pull request Jul 8, 2026
Merged
@obviyus

obviyus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Landing plan update: this track lands atomically with its siblings via the integration PR #102160 (all commits preserved, rebase merge) — sequential landing would put untested intermediate states on main (e.g. the stale takeover without the quiet-tool floor). This PR stays open as the per-track review record and closes when #102160 lands.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@obviyus obviyus

Labels

agents Agent runtime and tooling docs Improvements or additions to documentation maintainer Maintainer-authored PR merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Agent stall detector hard-coded 120s threshold kills legitimate long model calls on local vLLM

1 participant

@obviyus