fix(acp): recover stale persistent sessions by structured resume-required code [AI-assisted]#93547
Conversation
openclaw-barnacle
Bot
added
the
proof: supplied
2244a98 to
0ae0761
Compare
|
@clawsweeper re-review The automated review doesn't appear to have triggered for this PR. CI is fully green and the branch is mergeable/clean — could you run the review? Thanks. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review The prior re-review failed waiting for Codex capacity (run 27612067178). CI is fully green and the branch is mergeable/clean — retrying now that capacity should have recovered. |
|
@clawsweeper re-review Two earlier attempts failed waiting for Codex capacity (last at 13:07 UTC, ~5h ago). Retrying now that capacity has had time to recover. CI is fully green (137 checks) and the branch is mergeable/clean. |
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 8:21 PM ET / 00:21 UTC. Summary PR surface: Source +29, Tests +41. Total +70 across 4 files. Reproducibility: yes. at source level: current main and Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the structured Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main and Is this the best way to solve the issue? Yes: preserving and matching acpx's structured AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1f6ae32cabb9. Label changesLabel justifications:
Evidence reviewedPR surface: Source +29, Tests +41. Total +70 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🦪 silver shellfish
…ired code
Persistent ACP threads died on the second turn for Kiro: when the backend
can no longer resume a stale session, acpx raises a SessionResumeRequiredError
whose reason text varies by backend ("Resource not found" for Claude,
"Internal error" / RequestError -32603 for Kiro). The recovery gate matched
the human reason text and required "resource not found", so Kiro's "Internal
error" never triggered the fresh-session retry and the thread produced no
reply (ACP_TURN_FAILED).
Recover by acpx's structured detail code instead of the reason text: acpx
tags every such failure with detailCode "SESSION_RESUME_REQUIRED"
(retryable), independent of wording. The two AcpRuntimeError construction
seams were discarding detailCode, so preserve it on AcpRuntimeError and match
it across the error and its cause chain. This fixes every backend's
resume-required failure and is more precise than the reason regex — a generic
"Internal error" without the code is still surfaced rather than silently
retried.
Fixes openclaw#87830. Reported by @chouzz.
0ae0761 to
762f574
Compare
|
@clawsweeper re-review Rebased onto current |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Retrying — the prior re-review on this SHA failed on Codex capacity, not on the change. Head is green (0 failures) and the diff is identical to the last verdict's commit. Please re-run when a slot is free. |
|
@clawsweeper re-review The previous run got a Codex slot and the review itself succeeded, but it failed at the "Publish event result and apply safe close" step — so the ack has been stuck at "Review in progress" since ~10:08 UTC with the verdict generated but never posted, and no reschedule followed (publish failures don't re-enter the capacity-retry loop). Re-triggering so the verdict can publish; capacity is free now, so this should complete rather than wait. |
|
@clawsweeper re-review Live Kiro backend proof added to the PR body (new "## Live Kiro backend proof" section), addressing the [P1] "needs real behavior proof before merge" gate:
|
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
rating: 🦞 diamond lobster
b843438
into
openclaw:main
…ired code (openclaw#93547) Persistent ACP threads died on the second turn for Kiro: when the backend can no longer resume a stale session, acpx raises a SessionResumeRequiredError whose reason text varies by backend ("Resource not found" for Claude, "Internal error" / RequestError -32603 for Kiro). The recovery gate matched the human reason text and required "resource not found", so Kiro's "Internal error" never triggered the fresh-session retry and the thread produced no reply (ACP_TURN_FAILED). Recover by acpx's structured detail code instead of the reason text: acpx tags every such failure with detailCode "SESSION_RESUME_REQUIRED" (retryable), independent of wording. The two AcpRuntimeError construction seams were discarding detailCode, so preserve it on AcpRuntimeError and match it across the error and its cause chain. This fixes every backend's resume-required failure and is more precise than the reason regex — a generic "Internal error" without the code is still surfaced rather than silently retried. Fixes openclaw#87830. Reported by @chouzz.
1 participant
Summary
Fixes #87830 — thread-bound persistent ACP sessions stop replying on the second turn for Kiro.
When a backend can no longer resume a stale persistent session, acpx raises a
SessionResumeRequiredErrorwhose reason text varies by backend: Claude reports"Resource not found", Kiro reports"Internal error"(RequestError -32603). OpenClaw's recovery gate (manager.runtime-resume-state.ts) matched the human reason text and required"resource not found", so Kiro's "Internal error" never triggered the fresh-session retry — the turn failed withACP_TURN_FAILEDand the thread produced no reply, while Claude (whose reason happened to match the regex) recovered.Root cause is deeper than the reason text: acpx tags every such failure with the structured contract
detailCode: "SESSION_RESUME_REQUIRED"(retryable: true), independent of wording. But the twoAcpRuntimeErrorconstruction seams (consumeAcpTurnEvents,errorFromTurnResult) were discardingdetailCode, leaving the gate no choice but to parse the message.The fix:
detailCodeonAcpRuntimeError(additive optional field) so structured failure data isn't thrown away at construction.detailCodethrough both seams.SESSION_RESUME_REQUIREDstructurally across the error and its cause chain, instead of the reason regex.This recovers every backend's resume-required failure (not just the two observed reasons) and is more precise than the old regex — a generic "Internal error" without the structured code is surfaced rather than silently retried.
Relationship to #93315
#93315 (@xialonglee) targets the same bug by extending the message regex to also match
SessionResumeRequiredError/session resume required. That approach cannot match the real failure: acpx builds the message as`Persistent ACP session ${id} could not be resumed: ${reason}`(acpx distlive-checkpoint,makeSessionResumeRequiredError), so theAcpRuntimeError.messagereaching the gate is"Persistent ACP session … could not be resumed: Internal error"— theSessionResumeRequiredErrorstring is the error's.name, never in the message (the seams usenormalizeText(event.message)/normalizeText(result.error.message), the raw message, not theformatAcpErrorChainrendering). #93315's retry test passes only because it manually concatenates" <- SessionResumeRequiredError: …"into the test event message — a string the real pipeline does not emit. Keying on the structureddetailCodeavoids depending on message wording entirely.AI-assisted (Claude Code), directed and reviewed by the PR author.
Real behavior proof
Behavior addressed: a thread-bound persistent ACP turn whose backend reports a resume-required failure with reason "Internal error" (Kiro /
RequestError -32603) now discards the stale persistent identity and retries with a fresh session, instead of failing the turn (#87830).Real environment tested: macOS (Darwin), Node 22, the real
AcpSessionManagerturn runner exercised end-to-end throughmanager.runTurnwith the actual recovery path (prepareFreshManagerRuntimeHandleRetry→ensureSession). The failure reproduces fully at the manager boundary: the recovery code is pure error-classification and makes no backend/Bot-API call, so a mocked runtime emitting the exact acpx error shape (detailCode: "SESSION_RESUME_REQUIRED", message"…could not be resumed: Internal error") drives the real production path.Exact steps or command run after this patch:
node scripts/run-vitest.mjs src/acp/control-plane/manager.turn-results.test.tsorigin/mainand re-run to confirm the new cases fail without the fix.Evidence after fix: copied live command output.
With the fix — all cases pass (the two resume-required reasons, the thrown-cause shape, and the negative precision case):
Reverting the source (matcher + seams + AcpRuntimeError) to
origin/main, the Kiro "Internal error" and thrown-cause cases fail while the happy-path and precision cases still pass — proving the new behavior:Dependency contract verified directly in
node_modules/acpxdist:SessionResumeRequiredError extends AcpxOperationalErrorwithdetailCode: "SESSION_RESUME_REQUIRED",retryable: true; the message is`Persistent ACP session ${id} could not be resumed: ${reason}`.Observed result after fix: the manager calls
prepareFreshSession, re-runsensureSessionwithresumeSessionIdundefined, the session identity is replaced with the fresh backend id, and the turn completes; session state transitions running→idle with no error.What was not tested: the full gateway/chat surface (channel → gateway → manager) end-to-end. The resume failure and recovery themselves were validated live against a real
kiro-clibackend through the real acpx runtime and the real manager seams (consumeAcpTurnStream→prepareFreshManagerRuntimeHandleRetry) — see "Live Kiro backend proof" below — not mocked.Live Kiro backend proof
Reproduced the failure and recovery against a real, logged-in
kiro-cli2.8.1 (free AWS Builder ID) — no mock backend.Backend (raw ACP over stdio). A session is created in one
kiro-cli acpprocess; that process is killed (gateway restart); a freshkiro-cli acpasked tosession/loadit rejects with the exact #87830 shape:Kiro advertises
agentCapabilities.loadSession: true, so acpx does attempt the reconnect — and the rejection is-32603 "Internal error", not Claude's "Resource not found".End-to-end (real acpx runtime + real manager seams). Turn 1 creates a persistent Kiro session with a real agent message (non-fallback-safe); a fresh acpx runtime sharing the same on-disk session store runs turn 2 → acpx reconnect → live Kiro
-32603→ realconsumeAcpTurnStream→ realprepareFreshManagerRuntimeHandleRetry:{ "kiroVersion": "kiro-cli 2.8.1", "turn2RuntimeError": [{ "type":"error", "code":"ACP_TURN_FAILED", "detailCode":"SESSION_RESUME_REQUIRED", "retryable":true, "message":"Persistent ACP session <id> could not be resumed: Internal error" }], "turn2ThrownError": { "instanceofAcpRuntimeError":true, "code":"ACP_TURN_FAILED", "detailCode":"SESSION_RESUME_REQUIRED" }, "recovery": { "retry":true, "identityDowngradedTo":"pending", "runtimeHandleClearedAfter":true }, "negativeControl": { "message":"…could not be resumed: Internal error", "detailCode":null, "retry":false } }Negative control: the same human message with
detailCodestripped →retry: false. Recovery fires on acpx's structuredSESSION_RESUME_REQUIRED, never the reason text — which is exactly why the old text-match missed Kiro. (Live harness available on request.)Verification
node scripts/run-vitest.mjs src/acp/control-plane/manager.turn-results.test.ts— 18 passed (4 new cases).manager.test,manager.failover,manager.backend-failover,manager.initialize-session,manager.startup-identity-reconcile,acp-core/runtime/errors,error-text).origin/main's source and pass with the fix (verified by reverting the three source files).kiro-cli2.8.1 (raw ACP-32603repro + real acpx runtime through the real manager seams; run locally, not in CI).tsgo:core+tsgo:core:testgreen; oxlint/oxfmt clean;pnpm buildgreen.Release-note context: persistent ACP threads (notably Kiro) no longer get permanently stuck after a stale-session resume failure; recovery now keys on acpx's structured resume-required code rather than the backend's reason wording. Reported by @chouzz in #87830.