feat(runner): detect stalled agent runs via event inactivity - #6595
feat(runner): detect stalled agent runs via event inactivity#6595guyoron1 wants to merge 9 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://0ddee7a2-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ffe01bf to
0339139
Compare
PR Summary by QodoDetect stalled agent runs from event-stream inactivity
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // StallTimeout terminates the run when the event stream stays silent for | ||
| // this long. Timeout is wall-clock and cannot tell a wedged agent from a | ||
| // thinking one, so without this a wedge is billed for the full window. | ||
| // Zero disables the watchdog. The CLI resolves it from |
There was a problem hiding this comment.
1. Runtime guide not consulted 📘 Rule violation ⛨ Security
The PR changes the runtime.Runtime execution contract by adding RunParams.StallTimeout and watchdog integration, but neither updates docs/contributing/runtime-implementation.md nor states in the PR description that the guide was consulted. The new event-inactivity and cancellation requirements are therefore undocumented for runtime implementers.
Agent Prompt
## Issue description
The runtime implementation guide does not document the new stall-watchdog contract, and the PR description does not state that the guide was consulted.
## Issue Context
`RunParams` now carries an event-inactivity timeout, and streaming runtime implementations must reset the watchdog from normalized events, disarm it when the stream ends, and use the `ExecStreamReader` cancellation path. Update the guide accordingly and amend the PR description to explicitly reference `docs/contributing/runtime-implementation.md`.
## Fix Focus Areas
- internal/runtime/runtime.go[49-55]
- internal/runtime/claude.go[110-135]
- internal/runtime/pi_run.go[488-528]
- docs/contributing/runtime-implementation.md[120-131]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 0339139 |
d81237a to
2b1e60b
Compare
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 2b1e60b |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 957481a |
|
Any way we can fold this with the heartbeat? Or the other way around: any way to fold the heart beat into this? They serve similar purposes and the watchdog could be reporting "agent working: x seconds since last event" each 30 seconds (as the heartbeat interval is 30 seconds). |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep — 5 findings posted inline (no approval/request-changes; comment only):
- HIGH
internal/runtime/stall.go:46— the stall kill releases the local openshell client; nothing signals the agent inside the sandbox (verified against OpenShell v0.0.116 source) - HIGH
internal/runtime/pi_run.go:528— liveness counted at the AgentEvent level, so streaming tool output (pitool_execution_update, Claudeusertool_result) reads as silence - HIGH
internal/cli/run_overrides.go:115— default 10m equals Claude Code'sBASH_MAX_TIMEOUT_MSceiling; previously-successful runs now fail; not marked breaking - MEDIUM
docs/runtimes.md:88—FULLSEND_STALL_TIMEOUTdocumented as a CI repo-variable override but not in thesetup-agent-env.shallowlist - MEDIUM
internal/cli/run.go:1852— stall timeout not bounded by the run timeout; a no-op fortimeout_minutes <= 10harnesses (triage, prioritize)
| // agent is alive, so a wedged process is indistinguishable from a thinking | ||
| // one until the global timeout expires — and gets billed for the difference. | ||
| // Every event the stream parser emits is proof of life: note() records it, | ||
| // half a timeout of silence logs a warning, and a full timeout of silence |
There was a problem hiding this comment.
[HIGH] Stall kill releases the local openshell client, but nothing signals the agent inside the sandbox
The design claim here (stall.go:46-48, claude.go:110-112, pi_run.go:488-490, the PR body's "the existing kill path... No second termination mechanism", and docs/cli/run.md:63 "terminates the sandbox command — the same kill the global timeout uses") does not hold against the OpenShell source at the pinned v0.0.116.
cancel() from sandbox.ExecStreamReader (internal/sandbox/sandbox.go:1222-1226) cancels an exec.CommandContext, which SIGKILLs only the local openshell sandbox exec client. Server side, handle_exec_sandbox (crates/openshell-server/src/grpc/sandbox.rs:1189-1256) runs the exec in a detached tokio::spawn; stream_exec_over_relay -> run_exec_with_russh (sandbox.rs:2295) writes with let _ = tx.send(...) and leaves its loop only on ChannelMsg::Close/ExitStatus — the only tx.closed() check in that file is in the watch handler (sandbox.rs:1090), not the exec path. In the supervisor, spawn_pipe_exec (crates/openshell-supervisor-process/src/ssh.rs:1360) hands the std::process::Child to a wait() thread with no kill_on_drop, and channel_close (ssh.rs:534) / SshHandler::drop (ssh.rs:460) abort only main_output_task. The server-side timeout_seconds wrapper does not signal the child either — on expiry it drops the russh future and reports exit 124. So neither cancel path terminates the in-sandbox sh -c claude|pi ...; in both cases the process actually dies only when the deferred sandbox.Delete at internal/cli/run.go:1565-1581 tears the sandbox down.
Consequences for the stall path specifically: after a "stalled" verdict runAgent returns (run.go:2030) and collectOpenshellLogs plus the post-failure workspace download run against a still-live agent that is still writing the workspace, running hooks and spending tokens; under --keep-sandbox (run.go:1569) the agent keeps running in the kept sandbox with nothing left to stop it. Practical exposure in the normal path is bounded to the seconds before teardown, but the documented "no in-sandbox process survives the kill" property is inverted and the docs promise it. The PR body's non-goals (per-dimension timeouts, heartbeat, global timeout) don't defer this.
Suggestion: have the watchdog's kill terminate the process inside the sandbox first, then cancel the client. origin/main already has the primitive: killStrayProcesses/clearStrayProcesses in internal/runtime/stray_processes.go TERM->KILLs the sandbox user's processes via sandbox.Exec while sparing the keep-alive (mind its documented sandboxMu serialization; on main it only runs from ClearIterationArtifacts, i.e. the next iteration, which a stalled run never reaches). Alternatively send TERM to the exec'd process group via a short sandbox.Exec. Then reword stall.go:46-48, claude.go:110-112, pi_run.go:488-490, the PR body and docs/cli/run.md:63 to state what actually happens (client released; in-sandbox process killed by the sweep / torn down with the sandbox) — and note the same caveat applies to the global timeout today.
There was a problem hiding this comment.
You're right that the cancel SIGKILLs only the local exec client — verified. What terminates the agent is the deferred sandbox.Delete registered before the agent loop (run.go:1565-1582), which runs on every return path including the stall one, so teardown is prompt; 815582c documents that chain in stall.go and run.md instead of adding new kill machinery (stray_processes.go doesn't exist on this branch or main). Residual: --keep-sandbox skips Delete by design and leaves the agent running — identical to the global-timeout path today; happy to address that as a follow-up if you want it.
| var lastResult *ResultEvent | ||
| innerHandler := handler | ||
| handler = func(evt AgentEvent) { | ||
| stall.note() |
There was a problem hiding this comment.
[HIGH] Liveness is counted at the AgentEvent level, so actively streaming tool output is treated as silence
stall.note() is called only from the normalized-event handler (here and claude.go:135), not per stream line. On pi, tool_execution_update lines — emitted continuously while a tool streams output — are explicitly discarded by parsePiStream (internal/runtime/pi_progress.go:625: "Lifecycle / intermediate events — no AgentEvent mapping", alongside turn_start/turn_end), and pi's bash tool has no command timeout (docs/contributing/runtime-implementation.md:448). So a code-role run whose test suite streams output for 10+ minutes is killed with "no runtime events" while raw JSON lines are flowing, and the whole run fails (run.go:2030 returns; no retry iteration).
On Claude, parseClaudeStream (internal/runtime/claude_progress.go:149-283) has cases only for system, stream_event, result and assistant — user/tool_result lines produce nothing — and fullsend passes --verbose --output-format stream-json without --include-partial-messages (claude.go:324-325), so the silent window for one tool call is tool runtime + result round-trip + the entire next model turn including thinking.
docs/cli/run.md:63 ("without a single agent event") and the warning text therefore misdescribe these cases: the process is demonstrably alive and the watchdog reports it as wedged.
Suggestion: count liveness at the parser level, not the AgentEvent level: give both parsers a per-line liveness callback (e.g. onLine func() or a LivenessEvent{} AgentEvent the renderer/metrics ignore) invoked for every successfully unmarshalled line — including pi tool_execution_update/turn_* and Claude user tool_result messages — and call stall.note() from it, keeping the semantic events unchanged. Add tests that a stream of pi tool_execution_update lines and Claude user tool_result lines keeps the watchdog quiet, and fix the run.md wording.
There was a problem hiding this comment.
Fixed in 815582c at the root — both stream parsers now invoke a per-line hook after every successful envelope unmarshal (including pi tool_execution_update/turn_* and Claude tool_result lines) and Run passes stall.note, so any well-formed stream line resets the clock. Garbage/blank lines don't count; tests cover both runtimes.
| // a slow clone) is legitimately silent for minutes. The default is | ||
| // deliberately generous; repos that know their event cadence can tune it | ||
| // down with FULLSEND_STALL_TIMEOUT. | ||
| const defaultStallTimeout = 10 * time.Minute |
There was a problem hiding this comment.
[HIGH] Default-on 10m stall timeout equals Claude Code's bash ceiling and kills previously-successful runs; PR is not marked breaking
defaultStallTimeout = 10 * time.Minute is enabled by default, and the justifying comment ("a single long tool call ... is legitimately silent for minutes") never checks how long a tool call may legitimately run. Per the Claude Code environment-variables reference, BASH_MAX_TIMEOUT_MS ("Maximum timeout the model can set for long-running bash commands") defaults to 600000 ms — exactly 10 minutes — and the model routinely requests it for test suites; pi's bash tool has no timeout at all (docs/contributing/runtime-implementation.md:448).
Because tool completion is not counted as liveness (see the pi_run.go:528 thread), the observed silence for such a call is tool runtime + result round-trip + the next assistant turn, which always exceeds the default; the 30s poll cadence adds at most 30s of grace. A healthy run that uses the documented bash ceiling is therefore killed as stalled with the shipped default — the same run that succeeded before this change now fails with ErrStalled.
COMMITS.md ("Breaking changes") lists "Default values change in ways that alter existing behavior" as breaking and AGENTS.md:16 makes a missing ! an important-severity review finding, yet the title is a plain feat(runner) with no BREAKING CHANGE: trailer.
To be clear, the PR is right that there is no --include-partial-messages so events are per assistant message, and that system/api_retry is mapped to RetryEvent (claude_progress.go:161) so API backoff keeps the watchdog fed.
Suggestion: either (a) make tool completion / streaming output count as liveness (the parser-level fix) so the silent window is bounded by tool runtime alone, and document the relationship to BASH_MAX_TIMEOUT_MS in docs/cli/run.md, or (b) keep AgentEvent liveness but choose a default with headroom above the bash ceiling (e.g. 15m) and state the derivation in the comment. In either case mention BASH_MAX_TIMEOUT_MS in the run.md tuning paragraph so a repo that raises it knows to raise the watchdog, and if the default stays at/near 10m mark the PR feat(runner)! with a BREAKING CHANGE: trailer naming FULLSEND_STALL_TIMEOUT=0 as the opt-out.
There was a problem hiding this comment.
Fixed in 815582c — default raised to 15m with the BASH_MAX_TIMEOUT_MS 600000ms ceiling named as the derivation in the comment and docs. With line-level liveness (other thread) streaming tools never look silent; the 15m floor covers genuinely quiet calls above the bash ceiling.
| | Runtime | `--runtime` | `FULLSEND_RUNTIME` | `runtime:` on the agent's `agents:` entry | `runtime:` in `.fullsend/config.yaml` (repo default) | | ||
| | Model | `--model` | `FULLSEND_MODEL` (`FULLSEND_PI_MODEL` is a lower-precedence alias on pi) | `model:` on the agent's `agents:` entry | harness `model:`, then agent frontmatter `model:` | | ||
| | Effort | `--effort` | `FULLSEND_EFFORT` | `effort:` on the agent's `agents:` entry | harness `effort:` | | ||
| | Stall timeout | — | `FULLSEND_STALL_TIMEOUT` (default `10m`, `0` disables) | — | — | |
There was a problem hiding this comment.
[MEDIUM] This row presents FULLSEND_STALL_TIMEOUT as a CI repository variable, but the passthrough allowlist does not include it
This row sits directly above the sentence on line 91, "In CI these are repository variables of the same name, plain or role-prefixed (TRIAGE_FULLSEND_MODEL)". That passthrough is FULLSEND_REPO_VARS: ${{ toJSON(vars) }} in .github/workflows/reusable-dispatch.yml (lines 667/794/929/1209) feeding internal/scaffold/fullsend-repo/.github/scripts/setup-agent-env.sh, whose override_keys allowlist at line 45 is FULLSEND_RUNTIME FULLSEND_MODEL FULLSEND_EFFORT FULLSEND_FALLBACK_MODELS FULLSEND_PI_PROVIDER FULLSEND_PI_MODEL on both this branch and origin/main; only allowlisted keys reach GITHUB_ENV. A repo that sets a FULLSEND_STALL_TIMEOUT or CODE_FULLSEND_STALL_TIMEOUT Actions variable silently keeps the 10m default with no warning.
The script is scaffold-shipped from this repo (internal/scaffold/scaffold.go:125 and vendorcontent.go:136 special-case it; ADR 0035 lists setup-agent-env.sh as "upstream infrastructure ... referenced directly from upstream"), and fullsend-ai/agents has no setup-agent-env.sh under .github/scripts, so the fix is an in-repo edit.
Suggestion: add FULLSEND_STALL_TIMEOUT to override_keys in setup-agent-env.sh:45 (the value regex ^[A-Za-z0-9._/@:,-]+$ already admits Go durations such as 10m, 90s, 0), extend setup-agent-env-test.sh and the key list in internal/scaffold/scaffold_test.go:753 — or, if that is out of scope for this PR, move the row out of the override table / add a sentence that the stall timeout is currently process-env only and not yet a repository-variable override.
There was a problem hiding this comment.
Fixed in 815582c — FULLSEND_STALL_TIMEOUT added to override_keys (role-prefixed variants work via the generic per-key handling); setup-agent-env-test.sh and scaffold_test.go extended.
| timeout = 30 * time.Minute | ||
| } | ||
|
|
||
| stallTimeout, stallErr := resolveStallTimeout(os.Getenv) |
There was a problem hiding this comment.
[MEDIUM] Stall timeout is not bounded by the run's own timeout, so the watchdog is a no-op for harnesses with timeout_minutes <= 10
timeout is derived from h.TimeoutMinutes at 1847-1850 and stallTimeout from resolveStallTimeout here, with no relationship between them. ExecStreamReader wraps the command in context.WithTimeout(ctx, timeout) (sandbox.go:1222), so when stallTimeout >= timeout the global context fires first, the stream ends, stall.stop() runs before Wait (claude.go:162, pi_run.go:554) and the run reports a plain timeout, never ErrStalled — the watchdog can only ever emit its half-way warning.
This is not hypothetical for the fleet: fullsend-ai/agents harness/triage.yaml and harness/prioritize.yaml both set timeout_minutes: 10, equal to the default, so for those two roles the kill can never fire before the global timeout — precisely the "wedged run burns its entire global timeout" case the PR body describes. Nothing in docs/cli/run.md tells a repo that FULLSEND_STALL_TIMEOUT must be strictly shorter than timeout_minutes to have any effect.
Suggestion: because equality is still a no-op (the ctx timer wins the race), a plain min(stallTimeout, timeout) is not enough: clamp the effective stall timeout to a fraction of timeout (e.g. timeout/2) when the configured value is not strictly shorter, or emit a startup StepWarn when stallTimeout >= timeout and document in run.md that the stall timeout must be shorter than timeout_minutes for the kill to ever fire.
There was a problem hiding this comment.
Fixed in 815582c — when the stall timeout is not strictly below the run timeout the watchdog is not armed and a StepInfo says so; no clamping, existing configs change only by the log line. Relationship documented in runtimes.md and run.md.
|
/agentic_review |
| if stallErr != nil { | ||
| printer.StepWarn(fmt.Sprintf("Stall watchdog: %v; using %s", stallErr, stallTimeout)) | ||
| } | ||
| if stallTimeout >= timeout { |
There was a problem hiding this comment.
1. stalltimeout guard lacks tests 📘 Rule violation ▣ Testability
The new branch that disables the watchdog when its timeout meets or exceeds the global run timeout has no corresponding behavioral test. Regressions could silently leave runs unprotected or disable valid watchdog configurations.
Agent Prompt
## Issue description
Add behavioral tests for the new stall-timeout deactivation condition.
## Issue Context
Verify that a stall timeout equal to or greater than the run timeout is disabled, while a smaller positive timeout remains enabled. Assert the resulting runtime parameters or observable behavior rather than only the log text.
## Fix Focus Areas
- internal/cli/run.go[1852-1864]
- internal/cli/run_test.go[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 53ae715 — the decision is extracted to effectiveStallTimeout() and TestEffectiveStallTimeout asserts parameter behavior across below/equal/above/zero/default; no log-text assertions.
|
Code review by qodo was updated up to the latest commit 815582c |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 53ae715 |
53ae715 to
f02a4cf
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit f02a4cf |
|
@rh-hemartin They solve different problems, so I left them separate: the heartbeat ( Swapping the heartbeat's line from "time since start" to "time since last event" (using the watchdog's |
|
I understand the difference, I don't think that with your addition the heartbeat has any value. I would bring to discussion if we want the heartbeat after we merge this stall detection. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep — 3 findings (no approval/request-changes; comment only).
[HIGH] Codex runtime is completely excluded from stall-watchdog coverage (internal/runtime/codex_run.go:483, not touched by this PR's diff, so posted here instead of inline)
Verified against PR head f02a4cf: CodexRuntime.Run (codex_run.go:413-537) uses the identical shape as ClaudeRuntime.Run/PiRuntime.Run — it calls sandbox.ExecStreamReader to get stdout/execCmd/cancel, wraps a handler, and drains via parseCodexStream(reader, handler) — but never calls startStallWatchdog and never references params.StallTimeout anywhere in the file (confirmed via grep: only claude.go and pi_run.go call startStallWatchdog(params.StallTimeout, ...)). RunParams' doc comment in runtime.go:54 says "Runtimes that stream no events ignore it", which is inaccurate for codex since it is architecturally a third streaming runtime with the same NDJSON-over-ExecStreamReader shape as pi — a wedged codex run still burns the full global timeout with no ErrStalled signal while claude/pi runs of the same scenario are now caught early.
Suggestion: either wire the same startStallWatchdog/stall.note()/stalledErr() pattern into CodexRuntime.Run (mirroring pi_run.go/claude.go), or explicitly scope codex out in the PR description's non-goals and correct the RunParams/runtime.go doc comment so the gap is documented instead of silently implied not to exist.
Two other findings are posted as inline comments on internal/cli/run.go:2175 and internal/cli/run_overrides.go:168.
| if runErr != nil { | ||
| attachIterationContent("error") | ||
| finalizeAgentSpan(agentSpan, runErr, iteration, exitCode, rt.System(), rt.Name(), &metrics, "") | ||
| if errors.Is(runErr, agentruntime.ErrStalled) { |
There was a problem hiding this comment.
[MEDIUM] run.go's stall-timeout integration wiring has no test coverage
Verified against PR head f02a4cf: grep for ErrStalled/Stalled in internal/cli/*_test.go shows only TestAggregateMetrics_StalledOmittedWhenFalse (a JSON-marshal/omitempty test) and the pure-function tests TestResolveStallTimeout/TestEffectiveStallTimeout in run_overrides_test.go. Nothing exercises the actual wiring inside runAgent itself: resolving FULLSEND_STALL_TIMEOUT via os.Getenv at run.go:2000, the StepInfo "watchdog inactive" branch at 2004-2009, passing StallTimeout into RunParams at 2146, or the errors.Is(runErr, agentruntime.ErrStalled) branch at 2175-2181 that sets aggMetrics.Stalled and prints the stall-specific StepFail message. This is distinct from the already-resolved thread on run.go:1856 (which was about testing the effectiveStallTimeout decision function in isolation, fixed via TestEffectiveStallTimeout) — the integration-level branches in runAgent remain unexercised by any test.
Suggestion: add a fake/dummy runtime that returns agentruntime.ErrStalled from Run() and assert runAgent sets aggMetrics.Stalled and emits the stall-specific message, to get direct coverage of the new branches in run.go rather than relying only on the pure-function unit tests.
| // timeout's context, so a stall timeout at or above it always loses the race | ||
| // to the global deadline. Below it, the configured value stands unchanged — | ||
| // no clamping or deriving, existing configs keep their behavior. | ||
| func effectiveStallTimeout(stall, run time.Duration) time.Duration { |
There was a problem hiding this comment.
[MEDIUM] effectiveStallTimeout's disable check ignores the watchdog's own poll-interval detection latency
Verified against PR head f02a4cf: stall.go's own comment (lines 21-26) states detection lands within roughly 5% of the threshold, capped by a 30s poll interval (stallMaxPoll = 30 * time.Second). effectiveStallTimeout (run_overrides.go:168-173) only disarms when stall >= run, not when stall + (poll latency) >= run. So a configuration where stall is just under run (e.g. stall=14m50s, run=15m) stays "armed" per this check, but the watchdog may not actually detect and fire until up to stallMaxPoll after the stall threshold is crossed — potentially after the global context deadline already won the race. In that near-boundary zone the watchdog appears active but often provides no real protection, which is inconsistent with the PR's own stated detection-latency model.
Suggestion: change the disable condition to account for detection latency, e.g. stall + stallMaxPoll >= run (or an equivalent margin), so "inactive" determination matches the watchdog's actual worst-case detection time rather than the nominal threshold.
The heartbeat prints "Agent running (Xs elapsed)" whether or not the agent is alive, so a wedged process is indistinguishable from a thinking one and burns the entire global timeout before anyone learns it was dead. The global timeout is wall-clock; nothing watched the event stream. The runtime now runs a watchdog seated on the normalized event stream: every event is proof of life, half a timeout of silence warns once per stall episode (::warning:: in CI, the printer everywhere), and a full timeout of silence terminates the sandbox command through the cancel ExecStreamReader already returns -- the same kill the global timeout uses, not a second mechanism -- and fails the run with ErrStalled. runAgent maps that sentinel to a specific failure line and records "stalled": true in metrics.json. FULLSEND_STALL_TIMEOUT (Go duration, default 10m, 0 disables) is resolved by the CLI and handed to the runtime in RunParams, since runtimes do not read env themselves (fullsend-ai#6526). 10m rather than a Cloudflare-style 60s because fullsend does not request partial messages: Claude Code's stream-json emits one event per assistant message and per completed tool call, so a single long tool call is legitimately silent for minutes. The watchdog derives no context of its own and never rebinds the caller's ctx -- a body-scope `ctx, cancel := context.WithCancel(ctx)` is what once made every successful run report as "cancelled" -- and a source-reading regression test pins that for both streaming runtimes. Non-goals: per-dimension timeouts (dimensions run inside one CLI process the runner cannot see into), and no change to the heartbeat or the global timeout. Signed-off-by: guy oron <goron@redhat.com>
…c time stop() and the ticker's fire branch both wrote fired/lastEvent from separate atomics with no ordering between them, so a tick already past the select when stop() closed the channel could still set fired=true after disarm — misclassifying a healthy or just-finished run as stalled. Replaced fired+stopOnce with a single state (armed/stopped/ fired) that stop() and the fire branch both reach only via CompareAndSwap from armed, so exactly one wins. Also swapped the UnixNano/time.Unix round-trip for time.Since(start), which keeps Go's monotonic clock reading instead of discarding it, so a wall-clock step can no longer perturb the silence calculation. Signed-off-by: guy oron <goron@redhat.com>
The select in watch() can draw an already-queued tick even when the stopped channel is closed, and only the kill path was protected by the state CAS — so a run that had just completed normally could still emit a misleading inactivity warning or CI annotation. A bare state check before warning would shrink the window but keep a check-then-warn race, so stop()'s disarm and the warning's check+emit now serialize on a mutex: once stop() returns, no warning can follow. Signed-off-by: guy oron <goron@redhat.com>
- re-check lastEvent under mu before the half-timeout warning, so an event arriving after the tick computed its silence suppresses the stale warning - count liveness per well-formed stream line (parseClaudeStreamLines / parsePiStreamLines feed stall.note), so streaming tool output and lifecycle lines with no AgentEvent mapping keep the watchdog quiet - raise the default stall timeout to 15m: Claude Code's bash ceiling (BASH_MAX_TIMEOUT_MS, 600000ms) makes a 10m tool call legitimate, so the default needs headroom above it - allowlist FULLSEND_STALL_TIMEOUT in setup-agent-env.sh so the documented CI repository variable actually reaches the runner - skip arming the watchdog (with a log line) when the stall timeout is not below the run timeout, where the global deadline always fires first and the watchdog could never act - document the real kill chain: the cancel kills the local openshell exec client; the in-sandbox agent dies when the deferred sandbox teardown deletes the sandbox, and survives under --keep-sandbox Signed-off-by: guy oron <goron@redhat.com>
- count a fully consumed oversized stream line (> streamBufSize) as liveness in both parsers: it is excluded from semantic parsing, but a runtime writing megabytes is alive and must not be killed as stalled - extract the stall-vs-run-timeout disable decision into effectiveStallTimeout and cover it with a behavioral test (disabled at or above the run timeout, untouched below, 0 stays 0) - validate FULLSEND_STALL_TIMEOUT repository variables with a duration-shaped pattern in setup-agent-env.sh: the shared charset rejected valid Go durations (+5m, 1µs, 1μs), silently dropping the override; µ/μ are matched as literal alternations so the check stays byte-safe in any locale, and injection protection is preserved Signed-off-by: guy oron <goron@redhat.com>
- the stall kill only released the local `openshell sandbox exec` client, which is all the global timeout does and signals nothing inside the sandbox: the wedged agent kept writing the workspace and spending tokens until teardown, and indefinitely under --keep-sandbox. stallKill now runs the stray-process sweep over a second exec channel first (OpenShell exposes no signal API), then cancels; a failed sweep is reported and still releases the client - arm the watchdog in CodexRuntime.Run: it has the same ExecStreamReader -> handler -> parse shape as the other two streaming runtimes but never read params.StallTimeout, so a wedged codex run burned its whole global timeout with no ErrStalled. parseCodexStreamLines gives it the same per-line liveness hook, so item.started/item.updated progress lines are not mistaken for silence - derive the file list in the wiring tests from the ExecStreamReader call sites, so a fourth streaming runtime cannot ship unguarded the way codex did - export StallDetectionLatency (the poll interval watch() ticks at) as the one source of truth for how late the kill can land - correct the RunParams doc comment: it said runtimes that stream no events ignore StallTimeout, which read as "no streaming runtime is missing" Signed-off-by: guy oron <goron@redhat.com>
- effectiveStallTimeout compared the threshold, not the kill: the watchdog polls, so a stall just under the run timeout (14m50s against 15m) was reported as armed while the global deadline usually won the race. Compare against stall + StallDetectionLatency instead, and name the interval in the "inactive" line - cover the run.go wiring: fold the resolve/warn/disarm decision into runStallTimeout and the ErrStalled verdict into noteStalledRun, both exercised directly — the branches sat inside runAgent, which no test reaches past sandbox creation - test the case the fleet actually hits: harnesses with timeout_minutes: 10 get no watchdog at the 15m default Signed-off-by: guy oron <goron@redhat.com>
The kill was documented as leaving the in-sandbox agent running until teardown; it now terminates it. Name codex as a covered runtime, and say that the stall timeout must clear timeout_minutes by more than the poll interval — so a harness at 10 minutes or less has no stall protection at the default. Signed-off-by: guy oron <goron@redhat.com>
The unit tests cover the watchdog, stallKill and the line hooks in isolation; nothing showed they compose. Drive the real ClaudeRuntime.Run against a fake openshell on PATH -- the stub shape claude_test.go already uses -- and assert that a stream which goes quiet ends with the sandbox swept exactly once and ErrStalled returned. Deleting the sweep from stallKill, or the stalledErr check from claude.go, fails it. TestStreamingRuntimesArmTheWatchdog stays, now labelled for what it is: a source-shape guard covering all three runtimes at once, not a behavioural test. Its cost is a rename or a reflowed call breaking it; what it buys is catching a fourth streaming runtime shipped unguarded, which is how codex shipped unguarded here. Also: the malformed-value warning goes through the printer, which run.go points at stdout, so run.md saying "reported on stderr" was wrong. Signed-off-by: guy oron <goron@redhat.com>
f02a4cf to
811acf1
Compare
|
@waynesun09 @rh-hemartin rebased onto main (conflicts:
Deferred: the heartbeat, separate output and PR. |
3 participants
Heyaa : )
This one came out of watching a wedged run burn its entire global timeout while the heartbeat cheerfully printed "agent running" the whole time — the process was dead and nothing could tell.
The runtime now watches the normalized event stream instead: every
AgentEventis proof of life, half a timeout of silence warns once per stall episode (::warning::in CI, printer everywhere), a full timeout kills and fails distinctly withErrStalled+"stalled": trueinmetrics.json. Config:FULLSEND_STALL_TIMEOUT(Go duration, default10m,0disables), resolved by the CLI and handed over inRunParams— runtimes don't read env themselves (#6526). A malformed value is reported and the default applies.Design points:
cancelthatsandbox.ExecStreamReaderalready returns. No second termination mechanism.claude,pi);dummy/opencodestream no events and ignore the field.ctx(a body-scope rebind once made every successful run report "cancelled"); a source-reading regression test pins that for both runtimes.Tests: the full watchdog matrix (flowing events never kill / silence kills exactly once /
0disables / warn-once-per-episode, rearmed by the next event / no annotations outside CI),resolveStallTimeoutcases,stalled,omitemptymarshalling.go build,go vet, and the touched packages pass.Non-goals: per-dimension timeouts (dimensions run inside one CLI process the runner can't see into), no change to the heartbeat or the global timeout.