Skip to content

fix(agent): replace hard agent deadline with pi-session-aware silence watchdog - #842

Open
cokesprite wants to merge 2 commits into
kunchenguid:mainfrom
cokesprite:fm/no-mistakes-pi-session-liveness-fix-20260826
Open

fix(agent): replace hard agent deadline with pi-session-aware silence watchdog#842
cokesprite wants to merge 2 commits into
kunchenguid:mainfrom
cokesprite:fm/no-mistakes-pi-session-liveness-fix-20260826

Conversation

@cokesprite

Copy link
Copy Markdown

Defect (incident-proven)

The daemon's agent timeout was a fixed 30-minute deadline whose diagnostic read "agent silent for 30m0s", but Pi in --mode json buffers stdout when piped, so a healthy turn shows zero stdout bytes. Two failure classes, both proven by session forensics (see the linked diagnosis below):

  1. Healthy fixer killed mid-work — run 01M0WRW741G2YPQFXYE5TG6DZD, fixer pid 55993: its pi session JSONL kept advancing until 23 seconds before SIGTERM, yet the wrapper killed it as "silent 30m".
  2. New invocation inherits the previous turn's spent budget — run 01M0WF7T6DEKM614JT3DM4ZT2F, reviewer pid 2574: the review step installed one budget at the fix turn's start, so the rereview launched 20 minutes later received only ~10 minutes before being killed.

Fix

Agent timeouts are now silence budgets, not hard wall-clock caps. One monotonic last-activity clock per invocation (internal/pipeline/liveness.go), fed by three evidence kinds (agent.ActivityKind):

  • stdout bytes — wired once at the shared startNativeAgentCommand choke point, covering every native adapter with no per-adapter logic change;
  • native process lifecycle — a process start re-arms the clock, which is what gives every newly launched agent a fresh full budget (failure class 2);
  • bound pi-session JSONL advancement — the exact session file of the launched process (internal/agent/pi_liveness.go).

The watchdog terminates the whole process tree only after the full configured budget passes with no activity from any source, so a genuinely frozen agent is still killed on the same budget (shellenv process-group kill path unchanged — verified in the journey below).

The pi session binding is deliberately narrow and fail-closed:

  • --no-session turns persist nothing and stay stdout/lifecycle-only;
  • a resumed session binds only to the one existing _<uuid>.jsonl in the launched cwd's session dir (pi's --<cwd>-- encoding, resolved physical like pi's process.cwd(); PI_CODING_AGENT_DIR honored);
  • a fresh durable session binds only when exactly one new session file appears after launch, and unbinds if the late stdout session header names a different id;
  • relocated session storage (--session-dir / PI_CODING_AGENT_SESSION_DIR), multiple matches, or an unresolvable dir all mean no binding — the invocation keeps the conservative stdout behavior instead of crediting unrelated activity. The watcher never reads session content and never credits broad filesystem changes.

Review and Test now pass their configured budgets per agent turn through the shared seam (RunAgentBudget/RunAgentSessionBudget) instead of pre-installing one hard context, so the fix turn and the rereview turn each get a fresh full review_agent_timeout. Nested seams (RunAgent outside, the executor timeoutAgent backstop inside) share one clock via a context-carried owner; a caller-supplied hard deadline is still honored unchanged.

Diagnostics: timeout errors now name per-kind last-activity ages — e.g. agent fix timed out after 20s (review agent silent for 20s: last activity: pi session events 21.995s ago, process lifecycle 24s ago) — with no prompt content, session content, or paths. Bind/unbind transitions also land in the step log as pi liveness: notes.

Unchanged: managed-server adapters (opencode, rovodev) report no activity and behave exactly as the legacy fixed deadline; non-pi native adapters change only through the shared seam/pipe owner.

Test evidence

New committed coverage (all in go test -race):

  • internal/pipeline/liveness_test.go — watchdog re-arm/fire/parent-cancel, evidence rendering, nested-seam single owner, existing-deadline precedence.
  • internal/agent/pi_liveness_test.go — session-dir encoding (verified against pi's dist/core/session-manager.js), agent-dir resolution, resume binding, fresh-session unique-new-file binding, ambiguity → credit nothing, header-mismatch unbind, relocated storage → no binding, watcher shutdown, stdout/lifecycle pipe activity.
  • internal/pipeline/agent_run_pi_test.goprocess-level: a real piAgent driving a fake pi subprocess (temp PI_CODING_AGENT_DIR, no real ~/.pi contact): (a) quiet-stdout invocation with an advancing session survives >2× the budget and returns successfully; (b) frozen session is killed after the full budget with session evidence in the error; (c) --no-session quiet invocation is killed at budget with lifecycle-only evidence.
  • TestReviewStep_EachAgentTurnGetsItsOwnFreshBudget — the rereview turn measurably gets its own full budget after a long fix turn (failure class 2 regression).

Full-suite results on this machine: go test -race ./... green except three pre-existing, environment-only failures that fail identically on the base commit (TestCIStep_CommitAndPush_CommitsLocallyWithoutPushing — local git hook injects a ticket prefix into commit subjects; TestCIWorkflow_WindowsHangSurfacesAsGoTimeoutNotJobCancellation + internal/e2edaemon TestReapAll_* — the documented macOS fork-pre-exec git flake / process-reaping environment). make lint green (skill unchanged, vet clean). gofmt clean.

Tagged e2e (via scripts/e2e.sh, fake claude/codex journeys through the modified seam): TestUserJourney, TestAxiAgentJourney, TestReviewPipelineOwnedPRCriterionDoesNotPark, TestAxiRunReattachesAfterManagedFix — all pass (240s).

Built-binary journey (real daemon in temp NM_HOME/HOME, fake pi on PATH, agent_timeout/review_agent_timeout: 20s, no real ~/.no-mistakes or ~/.pi touched):

  • Active session survives: review step (initial review → 60s quiet-stdout fixer with appending session → rereview) completed in 60.9s with a 20s budget; review.log shows pi liveness: watching session 019ff2f3-…. Previously this was SIGTERM at exactly 30m/20s.
  • Frozen session killed: fixer wrote 2 session events then froze; run failed at ~24s with agent fix timed out after 20s (review agent silent for 20s: last activity: pi session events 21.995s ago, process lifecycle 24s ago); zero leftover processes.

User-visible semantics changes (owned in docs/.../reference/global-config.md)

  • agent_timeout, review_agent_timeout, test_agent_timeout are now silence budgets (kill after this long with no activity) instead of hard wall-clock caps. A healthy long turn may now run past the configured value; a frozen one dies on the same value as before.
  • review_agent_timeout is now per agent turn (fix turn and rereview turn each get a fresh budget) instead of one shared per-round budget.
  • Timeout diagnostics gained the per-kind activity evidence suffix.

Risks

  • Active-but-unproductive agents can run longer than the configured budget. This is the intended semantics (the old message already claimed "silent"); genuinely hung agents still die after budget-of-silence. Managed adapters (opencode/rovodev) are unchanged.
  • Mis-attributed session activity would keep an agent alive wrongly; prevented by the fail-closed binding (unique new file / exact resume id / late-header cross-check / relocation opt-out) with conservative fallback.
  • Pi session-layout drift (future pi changes the dir encoding) degrades to the old stdout-only behavior, never to wrong crediting.
  • Retry backoff gaps count as silence; with production budgets (minutes) this is immaterial, and each new attempt's process start re-arms the clock.

Rollback

Revert this commit. All legacy behavior is restorable: the watchdog is confined to internal/pipeline/liveness.go + agent_run.go, the pi watcher is additive in internal/agent/pi_liveness.go, and adapter edits are one-line argument additions.


Note on process: this PR was raised directly (not via the no-mistakes pipeline), so it deliberately carries no pipeline-attestation marker and the no-mistakes / required check is expected to fail — the validator being repaired here cannot safely self-certify this fix. Diagnosis report: omp-deepseek-qwen-speed-quick-eval-20260825 (firstmate scout, 2026-08-26), which code-level proved both failure classes against runs 01M0WRW741G2YPQFXYE5TG6DZD and 01M0WF7T6DEKM614JT3DM4ZT2F.

… watchdog

The daemon killed healthy Pi agents as "silent 30m" because it used a
fixed per-context deadline while pi in --mode json buffers stdout when
piped, so a healthy turn showed zero stdout bytes. Forensics on run
01M0WRW741G2YPQFXYE5TG6DZD proved the fixer's session JSONL advanced
until 23s before SIGTERM. And one review-step budget spanned the fix
turn and the rereview turn, so run 01M0WF7T6DEKM614JT3DM4ZT2F's new
reviewer inherited ~20m of spent budget and died ~10 minutes into its
own turn.

Fold all liveness evidence into one monotonic last-activity clock per
invocation (internal/pipeline/liveness.go): stdout bytes from the shared
native-command pipe, native process lifecycle (a start re-arms the
clock, giving every new process a fresh full budget), and bound
pi-session JSONL advancement. The watchdog cancels with the whole
process tree only after the full configured budget passes with no
activity from any source, so the real timeout is preserved unchanged for
a genuinely frozen agent while a healthy long turn is never cut off.

The pi session binding (internal/agent/pi_liveness.go) is narrow and
fail-closed: resume binds only the one existing _<uuid>.jsonl in the
launched cwd's session dir (pi's encoding, resolved physical like pi's
process.cwd()); a fresh durable session binds only when exactly one new
file appears after launch and unbinds if the late stdout header names a
different id; --no-session turns, relocated session storage, ambiguity,
and unresolvable dirs all keep the conservative stdout/lifecycle-only
behavior.

Review and Test now pass their configured budgets per agent turn through
the seam (review_agent_timeout, test_agent_timeout) instead of
pre-installing one hard context, so every turn gets its own fresh
silence budget. Timeout diagnostics name per-kind last-activity ages
("pi session events 21.995s ago, process lifecycle 24s ago") with no
content or paths, and bind/unbind transitions land in the step log as
liveness notes. Managed-server adapters (opencode, rovodev) report no
activity and behave exactly as the legacy fixed deadline.

Docs: global-config.md owns the changed agent_timeout /
review_agent_timeout / test_agent_timeout semantics (silence budget, not
wall-clock cap; review is per-turn, no longer per-round).
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "fix(agent): order liveness evidence dete..." | Re-trigger Greptile

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Fork CI on HEAD 5d3f29e0 reviewed and approved (no workflow/secret/install retarget). Waiting on those checks now — not waiting on you for the first-time fork approval.

VISION.md per-rule (from PR body + docs/config patches; full code review of liveness/pi binding continues as CI runs):

  • R1 one gate, one meaning: aligns. Still fails closed on silence; does not skip core validation.
  • R2 never lose work: aligns. Intent is to stop killing healthy mid-work agents; process-tree kill on genuine freeze kept.
  • R3 judgment stays human: aligns. Timeout mechanics, not judgment transfer.
  • R4 independent validation: aligns. Fail-closed session binding; ambiguous/relocated → stdout/lifecycle only.
  • R5 evidence: aligns. Diagnostics name per-kind last-activity ages.
  • R6 agent-agnostic: aligns. Shared seam; pi-specific evidence is additive; managed adapters unchanged.
  • R7 local-tool scope: aligns.

Not auto-merging even if CI goes green. This changes default product semantics of agent_timeout / review_agent_timeout / test_agent_timeout from hard wall-clock caps to silence budgets (and review from per-round to per-turn). That is a captain decision once CI is actually green — not something for you to chase, and not a captain flag yet while checks are still running.

The expected PR must be raised via no-mistakes fail (no attestation; self-certifying the timeout validator) is not a ruleset-required check on main.

evidence() sorted per-kind activity with an unstable sort keyed only on
the observation timestamp, so two kinds stamped within one clock tick
(the Windows CI timer granularity) rendered in Go map iteration order:
the "most recent first" diagnostic contract was nondeterministic at
exact ties, which is what failed TestLivenessEvidence_NamesEveryKindSeen
on the windows-core leg with both ages rendering as 0s.

Fold a per-invocation record sequence into the sort: most recent first,
exact clock ties break in record order. Records are mutex-serialized, so
the later record is the genuinely more recent activity and the ordering
is a deterministic total order on every platform. Names, ages, and the
no-content/no-paths diagnostic contract are unchanged, as are the
watchdog budget, pi session binding, process-tree termination,
per-invocation clock reset, and conservative fallback.

Regression: TestLivenessEvidence_OrdersByRecencyAndBreaksClockTiesInRecordOrder
constructs an exact tie through the recordAt seam (a coarse timer can no
longer make the case vacuous) alongside a real ordering difference, and
TestLivenessEvidence_NamesEveryKindSeen is now stable on every platform.
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

New HEAD 2e9a8019 — fork CI (CI, docs, Guard generated files) reviewed and approved again (no workflow/secret/install retarget). Waiting on those checks.

Still not auto-merging even if they go green: default product semantics of agent_timeout / review_agent_timeout / test_agent_timeout (wall-clock → silence budgets; review per-turn). That is a captain decision, not waiting on you, once CI is actually green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants