Skip to content

fix(agents): clear poisoned claude cli sessions#81247

Merged
giodl73-repo merged 5 commits into
mainfrom
fix-claude-cli-session-invalidation
May 17, 2026
Merged

fix(agents): clear poisoned claude cli sessions#81247
giodl73-repo merged 5 commits into
mainfrom
fix-claude-cli-session-invalidation

Conversation

@giodl73-repo

@giodl73-repo giodl73-repo commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #78785.

When a reused claude-cli session fails with AbortError or a non-session_expired FailoverError, OpenClaw currently keeps the stored CLI session id. The next turn can then resume the same poisoned Claude CLI transcript/session and wedge again.

This PR keeps the existing session_expired clear-and-retry path unchanged, and adds a separate clear-only path for failed reused Claude CLI turns:

  • AbortError clears the stored claude-cli binding before rethrowing.
  • non-session_expired FailoverError clears the stored claude-cli binding before rethrowing.
  • no retry is added for these new paths; callers still see the original failure.
  • the behavior is scoped to reused claude-cli sessions.

Motivation

The issue has live logs showing claude live session reuse, no output for the watchdog window, then FailoverError(reason=timeout). Source inspection also showed the old code only cleared stored CLI sessions for session_expired, leaving aborts and timeout failovers eligible for reuse on the next turn.

Clearing the stored id after a failed reused turn is the smallest defensive fix: the next turn starts fresh instead of resuming the session id that just failed.

What changed

  • Added a small error classifier in runAgentAttempt for failed reused Claude CLI turns.
  • Reused the existing clearCliSessionInStore helper so cliSessionBindings, cliSessionIds, and legacy claudeCliSessionId are cleared the same way as existing session cleanup.
  • Added regression coverage for:
    • existing session_expired clear-and-retry behavior;
    • AbortError clear-only behavior;
    • FailoverError(reason=timeout) clear-only behavior.

Real behavior proof

  • Behavior or issue addressed: a reused claude-cli session id should be invalidated after AbortError or non-session_expired FailoverError failures, so the next turn cannot resume the poisoned Claude CLI session. The existing session_expired clear-and-retry path must remain unchanged.
  • Real environment tested: WSL Ubuntu-24.04 source worktrees created from upstream/main and actual PR head 8cec105b7017ae9f1c39e4a58672220becb2479c.
  • Exact steps or command run after this patch: created clean detached worktrees for upstream/main and the actual fix(agents): clear poisoned claude cli sessions #81247 head; in each worktree ran node ./probe-claude-cli-session-invalidation.mjs. The probe inspects the OpenClaw command/session-store seam and regression coverage: AbortError classification, non-expired FailoverError classification, preservation of the existing session_expired clear-and-retry path, clear-only rethrow behavior, and tests that seed reusable Claude CLI transcripts, assert one failed CLI call, and assert all stored claude-cli bindings are cleared.
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output):

PR #81247 Claude CLI session invalidation before/after proof

Fresh proof artifact: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/pr-81247-claude-cli-session-invalidation-before-after-proof.png
Proof summary: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/summary.txt

BEFORE upstream/main:
AbortError classifier=false
non-expired Failover clear=false
session_expired retry kept=true
clear-only log path=false
throws original error=false
seeds Claude transcript=false
AbortError regression=false
timeout regression=false
asserts no retry=true
asserts store cleared=true
ok=false

AFTER PR #81247 actual head:
AbortError classifier=true
non-expired Failover clear=true
session_expired retry kept=true
clear-only log path=true
throws original error=true
seeds Claude transcript=true
AbortError regression=true
timeout regression=true
asserts no retry=true
asserts store cleared=true
ok=true
  • Observed result after fix: upstream/main only has the existing session_expired clear-and-retry path; it does not classify AbortError or non-expired FailoverError for clear-only invalidation. PR head adds the clear-only path for reused Claude CLI failures, preserves session_expired retry behavior, rethrows the original error, and adds regressions proving AbortError/timeout failures clear cliSessionBindings, cliSessionIds, and legacy claudeCliSessionId without retrying.
  • What was not tested: a live Claude CLI timeout against a real Claude account was not forced. The proof covers the command/session-store boundary where OpenClaw owns persisted CLI session ids.

Risk

Low. The change is scoped to reused claude-cli sessions and uses the existing session-store clear helper. It does not alter API backends, fresh CLI starts, or the existing session_expired retry path.

@giodl73-repo

Copy link
Copy Markdown
Contributor Author

/review

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels May 13, 2026
@clawsweeper

clawsweeper Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper

clawsweeper Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

Summary
The branch adds clear-only invalidation of reused claude-cli session bindings after AbortError or non-session_expired FailoverError, plus focused regression tests and changelog text.

Reproducibility: yes. for a source-level path: current main can reuse a stored claude-cli binding, timeout failures are non-expired FailoverErrors, and only session_expired clears today. The issue and latest proof comment also provide live-log evidence of the reused Claude CLI failover path, though I did not run a local live repro in this read-only review.

Real behavior proof
Sufficient (logs): The latest PR comment includes copied live workflow logs from the Docker live CLI backend showing a reused Claude CLI session failover and all stored session bindings cleared after the patch.

Next step before merge
A repair lane can mechanically remove the unrelated duplicate CHANGELOG.md additions while preserving the agent-session code and tests.

Security
Cleared: The diff is limited to agent session cleanup logic, focused tests, and changelog text; I found no concrete security or supply-chain regression.

Review findings

  • [P3] Remove unrelated duplicate changelog entries — CHANGELOG.md:577-593
Review details

Best possible solution:

Land the scoped clear-only Claude CLI session invalidation after trimming the changelog diff and explicitly choosing it over the broader retry behavior.

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

Yes for a source-level path: current main can reuse a stored claude-cli binding, timeout failures are non-expired FailoverErrors, and only session_expired clears today. The issue and latest proof comment also provide live-log evidence of the reused Claude CLI failover path, though I did not run a local live repro in this read-only review.

Is this the best way to solve the issue?

Yes for the scoped PR: clearing only active reused claude-cli bindings through the existing store helper preserves the session_expired retry path and rethrows the original error. The broader retry PR remains a separate maintainer choice, not a reason to reject this narrow fix.

Full review comments:

  • [P3] Remove unrelated duplicate changelog entries — CHANGELOG.md:577-593
    The PR adds a block of changelog bullets, but only the Claude CLI entry belongs to this change and the other entries already exist on current main around CHANGELOG.md:928-943. Merging this as-is would duplicate unrelated release notes; trim the changelog change to the Claude CLI entry or leave changelog handling to landing.
    Confidence: 0.9

Overall correctness: patch is correct
Overall confidence: 0.88

Acceptance criteria:

  • git diff --check
  • Confirm the PR diff no longer adds unrelated duplicate CHANGELOG.md bullets
  • If code files are touched, run node scripts/run-vitest.mjs src/agents/command/attempt-execution.cli.test.ts src/agents/command/session-store.test.ts

What I checked:

  • Current main cleanup gap: Current main only clears and retries reused CLI sessions for FailoverError with reason === "session_expired"; other errors are rethrown without clearing the active reused binding. (src/agents/command/attempt-execution.ts:542, 5a350431bdae)
  • Timeouts are non-expired failovers: The CLI runner throws FailoverError with reason: "timeout" for no-output and overall timeout paths, so timeout wedges do not hit the existing session_expired cleanup branch on current main. (src/agents/cli-runner/execute.ts:665, 5a350431bdae)
  • Existing helper clears all stored Claude CLI forms: clearCliSessionInStore delegates to clearCliSession, which removes provider-scoped cliSessionBindings, cliSessionIds, and legacy claudeCliSessionId for claude-cli. (src/agents/cli-session.ts:104, 5a350431bdae)
  • PR adds scoped clear-only path: The PR adds an error classifier for AbortError and non-expired FailoverError, then clears only active reused claude-cli bindings before rethrowing the original failure. (src/agents/command/attempt-execution.ts:602, f1ee4d5cf63e)
  • PR regression coverage: The PR tests seed reusable Claude CLI transcripts and assert AbortError and timeout FailoverError clear stored bindings without retrying. (src/agents/command/attempt-execution.cli.test.ts:298, f1ee4d5cf63e)
  • Live proof now present: The latest PR comment reports a secret-bearing live CLI backend workflow on proof commit 568af5d, showing useResume=true, a real Claude CLI FailoverError, the clear log, and bindingCleared, idCleared, and legacyCleared all true; the linked job completed successfully. (568af5d58e52)

Likely related people:

  • Peter Steinberger: Current-line blame and local history for the central CLI session cleanup path, clearCliSession, clearCliSessionInStore, and adjacent tests point to recent work in commit d887eb8. (role: recent area contributor; confidence: medium; commits: d887eb8dc2cc; files: src/agents/command/attempt-execution.ts, src/agents/cli-session.ts, src/agents/command/session-store.ts)
  • galiniliev: Provided the maintainer review on this PR, confirmed the implementation shape, ran targeted checks, and identified the original live-proof gap that the latest comment addresses. (role: reviewer / likely follow-up reviewer; confidence: medium; files: src/agents/command/attempt-execution.ts, src/agents/command/attempt-execution.cli.test.ts)
  • draix: Authored the related broader recovery PR for the same poisoned Claude CLI resume/failover family, useful context for comparing clear-only versus retry behavior. (role: adjacent proposed fix owner; confidence: low; files: src/agents/cli-runner.ts, src/agents/command/attempt-execution.ts)

Remaining risk / open question:

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

@giodl73-repo

Copy link
Copy Markdown
Contributor Author

/review

@clawsweeper

clawsweeper Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@giodl73-repo
giodl73-repo marked this pull request as draft May 13, 2026 03:53
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

Applying the maintainer-proof pass from Galin's review style: this PR is mechanically narrow and CI is green, but the current proof is still seam-level/test-output proof rather than a real reused Claude CLI failure path. I converted it back to draft until we can add redacted runtime proof or a maintainer explicitly accepts the seam proof as sufficient for this failure mode.

@giodl73-repo
giodl73-repo force-pushed the fix-claude-cli-session-invalidation branch from 0c75041 to d9125c9 Compare May 13, 2026 04:58
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

Rebased on current origin/main, pushed to the upstream PR branch, and reran focused verification.\n\nChecks:\n- oxfmt --check changed files\n- node scripts/test-projects.mjs src/agents/command/attempt-execution.cli.test.ts --reporter=verbose\n- node scripts/check-changed.mjs\n- git diff --check\n\nStill draft pending the requested real behavior proof for the Claude CLI poisoned-session case.\n\n@clawsweeper re-review

@galiniliev

Copy link
Copy Markdown
Contributor

Maintainer review: not merge-ready yet, but I do not see a blocking code defect in the patch itself.

This PR clears stored claude-cli session bindings after a reused Claude CLI turn fails with AbortError or a non-session_expired FailoverError, so the next turn starts fresh instead of resuming a poisoned CLI transcript. The linked issue has strong before-fix live evidence for the wedge, and the code change is scoped to the right runtime path.

Findings:

  • No blocking code findings.

Merge readiness:

  • The PR is currently draft.
  • GitHub currently reports the branch as conflicting.
  • The remaining proof gap is real after-fix runtime evidence for the reused Claude CLI failure path. The focused mocked regression tests are good, but they do not prove the live Claude CLI poison-session path after the fix.

Best-fix verdict:

  • The implementation is the right shape: src/agents/command/attempt-execution.ts classifies abort and non-expired failover failures, then clears only active reused claude-cli bindings through the existing clearCliSessionInStore helper before rethrowing.
  • This preserves the existing session_expired clear-and-retry behavior and avoids broad cleanup for unrelated providers.

Verification I ran against PR head d9125c98ed44389915936937b1a4ad3ae552df2e:

git diff --check HEAD^..HEAD
pnpm exec oxfmt --check --threads=1 CHANGELOG.md src/agents/command/attempt-execution.ts src/agents/command/attempt-execution.cli.test.ts
pnpm test src/agents/command/attempt-execution.cli.test.ts src/agents/command/session-store.test.ts -- --reporter=verbose

Result: targeted formatting passed; 2 test files and 49 tests passed.

@giodl73-repo
giodl73-repo force-pushed the fix-claude-cli-session-invalidation branch from ade345a to 18d8e8d Compare May 16, 2026 14:16
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

OpenClaw QA Artifacts

Summary

Before/after Crabbox proof for PR #81247: fix(agents): clear poisoned claude cli sessions

Before shows the origin/main baseline in a fresh Crabbox checkout. After shows the PR head, diff summary, and patch excerpts from the same fresh checkout.

Before / After

Before

before

After

after

Evidence

@clawsweeper clawsweeper Bot added the P1 High-priority user-facing bug, regression, or broken workflow. label May 16, 2026
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

Updated #81247 with a fresh deterministic before/after proof image and replaced the old text-only proof with a raw GitHub artifact.

The proof compares upstream/main against actual PR head 8cec105b7017ae9f1c39e4a58672220becb2479c. It shows upstream only has the existing session_expired clear-and-retry path, while PR head adds clear-only invalidation for reused Claude CLI AbortError and non-expired FailoverError failures, preserves the retry path, rethrows the original error, and includes regressions that assert one failed call and cleared stored claude-cli bindings.

Fresh artifact: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/pr-81247-claude-cli-session-invalidation-before-after-proof.png
Summary: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/summary.txt

@clawsweeper clawsweeper Bot added impact:crash-loop Crash, hang, restart loop, or process-level availability failure. impact:session-state Session, memory, transcript, context, or agent state can drift or corrupt. labels May 17, 2026
@giodl73-repo

Copy link
Copy Markdown
Contributor Author

@galiniliev thanks for the maintainer review here — direct follow-up so this does not get lost.

You were right about the remaining merge-readiness gap. Since your comment, I refreshed the branch/proof and the PR is now clean/mergeable, with a fresh deterministic before/after artifact in the PR body. That proof shows upstream/main only has the existing session_expired clear-and-retry path, while PR head adds clear-only invalidation for reused Claude CLI AbortError and non-expired FailoverError, preserves the existing retry behavior, rethrows the original error, and includes the focused regressions.

Fresh proof image: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/pr-81247-claude-cli-session-invalidation-before-after-proof.png
Summary: https://raw.githubusercontent.com/giodl73-repo/openclaw/proof-artifacts/pr-81247-fresh/pr-81247/summary.txt

That said, this is still deterministic source-seam/runtime-path proof, not a live after-fix reused Claude CLI poisoned-session capture. I am not claiming that it satisfies the exact live-runtime proof gap you called out unless you or another maintainer explicitly accept this proof level for this failure mode. Otherwise the remaining next step is still a redacted live Claude CLI runtime capture showing the poisoned reused binding is cleared and the following turn starts fresh.

@giodl73-repo

Copy link
Copy Markdown
Contributor Author

@galiniliev I got the live Claude CLI reused-session proof now.

This was run through the repo's secret-bearing OpenClaw Live And E2E Checks (Reusable) workflow against a proof-only branch/commit (568af5d58e5268e2857d120034a86630b64a80f7) based on this PR head. The only proof-branch change is a live-test harness that seeds a stored claude-cli binding plus matching Claude transcript; the production fix is the same as this PR.

Run: https://github.com/openclaw/openclaw/actions/runs/25981733389
CLI backend job: https://github.com/openclaw/openclaw/actions/runs/25981733389/job/76371756681

Relevant log markers:

HEAD is now at 568af5d5 test: seed claude transcript under runtime home
OPENCLAW_LIVE_CLI_BACKEND_MODEL: claude-cli/claude-sonnet-4-6
==> CLI backend binary: /home/node/.npm-global/bin/claude
[gateway-cli-live] poison-proof:seeded {"sessionKey":"agent:dev:live-cli-backend", ...}
[agent/cli-backend] cli exec: provider=claude-cli model=sonnet promptChars=125 trigger=user useResume=true session=present resumeSession=2fa688bc1cea reuse=reusable historyPrompt=none
[agent/cli-backend] claude live session start: provider=claude-cli model=claude-sonnet-4-6 activeSessions=1
[agent/cli-backend] claude live session turn failed: provider=claude-cli model=claude-sonnet-4-6 durationMs=703 error=FailoverError
[agents/agent-command] CLI session cleared after failed reused turn: provider=claude-cli sessionKey=agent:dev:live-cli-backend reason=unknown
[gateway-cli-live] poison-proof:after {"sessionKey":"agent:dev:live-cli-backend","error":"GatewayClientRequestError: FailoverError: Claude CLI failed.","bindingCleared":true,"idCleared":true,"legacyCleared":true}
Test Files  1 passed (1)
Tests       1 passed (1)

So the live path exercised a reused Claude CLI session (useResume=true / reuse=reusable), hit a real Claude CLI FailoverError, and verified the stored cliSessionBindings, cliSessionIds, and legacy claudeCliSessionId entries were all cleared after that failed reused turn.

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 17, 2026
@giodl73-repo
giodl73-repo merged commit 5817e47 into main May 17, 2026
117 checks passed
@giodl73-repo
giodl73-repo deleted the fix-claude-cli-session-invalidation branch May 17, 2026 05:25
galiniliev pushed a commit to galiniliev/openclaw that referenced this pull request May 20, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 24, 2026
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 24, 2026
galiniliev pushed a commit to galiniliev/openclaw that referenced this pull request May 25, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
SebTardif pushed a commit to SebTardif/openclaw that referenced this pull request May 26, 2026
jameslcowan pushed a commit to jameslcowan/openclaw that referenced this pull request Jun 2, 2026
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling impact:crash-loop Crash, hang, restart loop, or process-level availability failure. impact:session-state Session, memory, transcript, context, or agent state can drift or corrupt. maintainer Maintainer-authored PR P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[claude-cli backend] Auto-invalidate stored CLI session id after AbortError / FailoverError

2 participants