Skip to content

fix(agents): clear stale CLI session on any FailoverError, not only session_expired#77141

Closed
hclsys wants to merge 3 commits into
openclaw:mainfrom
hclsys:fix/77089-cli-session-failover-retry-any-reason
Closed

fix(agents): clear stale CLI session on any FailoverError, not only session_expired#77141
hclsys wants to merge 3 commits into
openclaw:mainfrom
hclsys:fix/77089-cli-session-failover-retry-any-reason

Conversation

@hclsys

@hclsys hclsys commented May 4, 2026

Copy link
Copy Markdown

Problem

The stale-session recovery path in runPreparedCliAgent only cleared the
binding and retried fresh when err.reason === "session_expired". Failures
with reason "timeout" (watchdog kill) or "unknown" (generic CLI crash)
would rethrow immediately, leaving the dead session binding in place. Every
subsequent turn then tried to resume the same dead session, failed in
milliseconds, and cascaded to API fallback (Opus → Sonnet).

The incident in #77089 shows a gateway that degraded for 4+ hours because a
single timeout failure left a stale binding that nothing cleared until a
gateway restart.

Fix

Remove the err.reason === "session_expired" narrowing. Any FailoverError
from an active reused CLI session now clears the binding and retries once
fresh. If the fresh retry also fails, it falls through to the API fallback
chain as before. Add a cliBackendLog.warn so operators can see when a
non-session_expired reason triggered the recovery.

Changes

  • src/agents/cli-runner.ts — remove reason === "session_expired" guard,
    add warn log, import cliBackendLog
  • src/agents/cli-runner.reliability.test.ts — two new regression tests:
    • FailoverError reason timeout → retry succeeds with fresh session
    • FailoverError reason unknown → retry succeeds with fresh session

Tests

38/38 reliability tests pass. pnpm oxlint clean. pnpm tsc --noEmit clean.

Fixes #77089.

Co-Authored-By: Claude Sonnet 4.6 [email protected]

@hclsys @claude
fix(agents): clear stale CLI session on any FailoverError, not only s…
22db847 
…ession_expired

Previously the stale-binding recovery branch in runPreparedCliAgent only
triggered when err.reason === "session_expired". Failures with reason
"timeout" or "unknown" would rethrow immediately, leaving the dead session
binding in place and cascading every subsequent turn to API fallbacks.

Widen the condition to retry fresh for any FailoverError from an active
reused CLI session. Add regression tests for both timeout and unknown
failure reasons. Fixes openclaw#77089.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S labels May 4, 2026
@clawsweeper

clawsweeper Bot commented May 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

Summary
The PR broadens CLI stale-session recovery for resumed sessions, adds timeout/unknown regression coverage at runner and attempt-execution levels, and updates CHANGELOG.md.

Reproducibility: yes. Source inspection on current main shows timeout and unknown CLI FailoverError paths bypass the session_expired-only stale-binding clear; the PR’s remaining blocker can be reproduced by a stale-session timeout followed by a fresh retry that fails with a different FailoverError reason.

Next step before merge
There is a narrow, actionable PR-branch repair for the remaining stale-binding path plus a rebase requirement.

Security
Cleared: The diff touches agent runtime recovery, tests, and changelog only; the raw session-id log was changed to a last-8 fingerprint and no CI, dependency, permission, release, or secret-handling paths are modified.

Review findings

  • [P2] Clear stale bindings before retrying fresh — src/agents/cli-runner.ts:331-334
Review details

Best possible solution:

Move the persisted binding clear into the store-aware path before starting the fresh retry, then retry once and persist any replacement binding while preserving existing fallback behavior.

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

Yes. Source inspection on current main shows timeout and unknown CLI FailoverError paths bypass the session_expired-only stale-binding clear; the PR’s remaining blocker can be reproduced by a stale-session timeout followed by a fresh retry that fails with a different FailoverError reason.

Is this the best way to solve the issue?

No, not as-is. Widening recovery is the right direction, but the stored binding must be cleared based on the original reused-session failure before the fresh retry can fail with another reason.

Full review comments:

  • [P2] Clear stale bindings before retrying fresh — src/agents/cli-runner.ts:331-334
    This retry happens before the store-aware caller clears the persisted binding. If a resumed session fails with timeout/unknown and the fresh retry then throws a different FailoverError such as auth, rate_limit, or billing, the caller only sees that fresh reason and will not enter the widened timeout/unknown clear branch, so the next turn can resume the same dead stored session. Clear the binding before starting the fresh retry, or pass a store-clear callback into this branch, and cover stale timeout followed by a non-timeout fresh failure.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

Acceptance criteria:

  • pnpm test src/agents/command/attempt-execution.cli.test.ts src/agents/cli-runner.reliability.test.ts
  • pnpm exec oxfmt --check --threads=1 src/agents/cli-runner.ts src/agents/cli-runner.reliability.test.ts src/agents/command/attempt-execution.ts src/agents/command/attempt-execution.cli.test.ts CHANGELOG.md
  • pnpm check:changed

What I checked:

  • Current-main runner retry gate: runPreparedCliAgent currently retries fresh only for reused-session FailoverError reason session_expired. (src/agents/cli-runner.ts:331, 89a15fddaf84)
  • Current-main store clear gate: runAgentAttempt is the store-aware path, but current main clears the persisted CLI binding only when err.reason is session_expired. (src/agents/command/attempt-execution.ts:512, 89a15fddaf84)
  • Timeout and unknown sources: The CLI executor throws FailoverError reason timeout for watchdog/overall timeout and reason unknown for unclassified CLI failures, so the reported bug is source-reproducible. (src/agents/cli-runner/execute.ts:557, 89a15fddaf84)
  • Retry error becomes caller-visible: The runner's fresh-retry catch calls toCliRunFailure, which rethrows FailoverError values; after the PR, the caller can see only the fresh retry's reason, not the original stale-session timeout/unknown reason. (src/agents/cli-runner.ts:357, 89a15fddaf84)
  • PR diff after fixup: Head 8c370a8 widens the runner retry to any FailoverError and widens the store-aware caller branch only to session_expired, timeout, and unknown, with new tests for timeout/unknown success paths. (src/agents/command/attempt-execution.ts:510, 8c370a8e4471)
  • Merge state: GitHub's pull API reports mergeable=false, rebaseable=false, mergeable_state=dirty for head 8c370a8. (8c370a8e4471)

Likely related people:

  • steipete: Current-main blame for the runner retry branch, store-aware clear branch, and session-store helper points to 585ce38, and earlier CLI session preservation/refactor commits also sit in this path. (role: current-main behavior owner / recent maintainer; confidence: high; commits: 585ce38015ef, 12100719b8a9, 3f54076d3736; files: src/agents/cli-runner.ts, src/agents/command/attempt-execution.ts, src/agents/command/session-store.ts)
  • frankekn: Commit ed86252 introduced the CLI session-expired recovery behavior that this PR is broadening. (role: original session-expired recovery contributor; confidence: medium; commits: ed86252aa5a5; files: src/agents/cli-runner.ts, src/agents/command/attempt-execution.ts)
  • vincentkoc: Commit 4d183af touched CLI watchdog behavior and session-scoped watchdog wakeups, which are part of the timeout failure source for this bug. (role: adjacent CLI reliability contributor; confidence: medium; commits: 4d183af0cf73; files: src/agents/cli-runner/execute.ts, src/agents/cli-runner.ts)

Remaining risk / open question:

  • The PR branch is dirty/unrebaseable against current main per the GitHub pull API and needs a rebase after the code repair.
  • No tests were run in this read-only review; the targeted regression suite and changed gate still need CI/Testbox proof.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 89a15fddaf84.

@hclsys @claude
fix(cli-runner): redact session id in recovery warn; fix changelog ref
2df3444 
Log last-8 chars of session ID instead of raw value to avoid exposing
host-local resume identifiers in support bundles. Also fix changelog
PR reference (openclaw#77140 → openclaw#77141).

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@hclsys

This comment was marked as low quality.

Sign in to view
@hclsys @claude
fix(agents): widen attempt-execution store clear to timeout/unknown F…
8c370a8 
…ailoverErrors

The cli-runner already retried fresh for any FailoverError, but the store-clear
in attempt-execution only fired for session_expired. Timeout and unknown failures
left the stale binding in the store, causing the next turn to fail again.

Also adds regression tests for timeout and unknown failure reasons at the
attempt-execution caller level.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@hclsys

This comment was marked as low quality.

Sign in to view
@clawsweeper clawsweeper Bot mentioned this pull request May 4, 2026
Closed
@hclsys

This comment was marked as low quality.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling size: M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CLI backend: retry fresh session on any FailoverError, not just session_expired

1 participant

@hclsys