Skip to content

fix(recovery): reset command lane when abort leaves pre-existing task blocked#92303

Closed
hanZeng-08 wants to merge 1 commit into
openclaw:mainfrom
hanZeng-08:fix/92270-stuck-session-recovery-lane-release
Closed

fix(recovery): reset command lane when abort leaves pre-existing task blocked#92303
hanZeng-08 wants to merge 1 commit into
openclaw:mainfrom
hanZeng-08:fix/92270-stuck-session-recovery-lane-release

Conversation

@hanZeng-08

@hanZeng-08 hanZeng-08 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

fix #92270

PR Comment for Issue #92270

Branch: fix/92270-stuck-session-recovery-lane-release
Files Changed:

  • src/logging/diagnostic-stuck-session-recovery.runtime.ts
  • src/logging/diagnostic-stuck-session-recovery.runtime.test.ts

1. Solution Overview

This fix addresses Issue #92270 where stuck-session recovery reports status=aborted with released=0 while the wedge persists for hours.

Root Cause

recoverStuckDiagnosticSession decided whether to call resetCommandLane solely based on queuedCount and queueDepth after a successful abort+drain, ignoring the possibility that the command lane still has an underlying active task (e.g., a run stuck in waitForCompactionRetry or in-flight tool execution). This left activeCount > 0 unhandled, blocking queued work forever.

Fix Strategy

After the abort completes, additionally check postAbortLaneSnapshot.activeCount. If the lane has not started a new fresh task but activeCount > 0, force resetCommandLane to unblock the lane so queued work can drain.

2. Changes Detail

File Change Description
src/logging/diagnostic-stuck-session-recovery.runtime.ts 1. Read postAbortLaneSnapshot after abort (previously only queuedCount was read).
2. Introduce laneStillBlockedByPreExistingTask: when !laneStartedFreshTask && postAbortLaneSnapshot.activeCount > 0, include resetCommandLane in the release logic.
3. Ensure mutual exclusion with existing laneStartedFreshTask to avoid killing healthy fresh tasks.
src/logging/diagnostic-stuck-session-recovery.runtime.test.ts Add new test case: mock a scenario where abort+drain succeeds with queueDepth=0 but lane activeCount=1. Verify resetCommandLane is called and outcome has released=1.

3. Test Coverage

New Test

"releases the session lane when abort+drain succeeds but the lane task is still executing"

  • Input: allowActiveAbort=true, abortEmbeddedAgentRun returns true, waitForEmbeddedAgentRunEnd returns true (simulating successful abort+drain).
  • Lane state: getCommandLaneActiveTaskIds always returns [101] (no fresh task); getCommandLaneSnapshot returns activeCount=1, queuedCount=0.
  • Assertions: resetCommandLane is called once; outcome is { status: "aborted", released: 1 }.

Regression Tests

All 22 existing tests pass, ensuring no breakage for:

  • laneStartedFreshTask === true (lane already pumped fresh work during abort) — lane is preserved.
  • queuedCount > 0 or hasQueuedSessionWork — lane reset works as before.
  • forceClear and noop paths remain unchanged.

Verification Command

node scripts/run-vitest.mjs src/logging/diagnostic-stuck-session-recovery.runtime.test.ts
# Result: 23 passed (22 existing + 1 new)

4. Next Steps

  1. Code Review: Please focus on the mutual exclusion between laneStillBlockedByPreExistingTask and laneStartedFreshTask to ensure we never reset a healthy fresh task.
  2. Root-cause follow-up (optional): This fix addresses the recovery-layer symptom. Long-term, the underlying reason why waitForCompactionRetry and similar paths do not respect abort signals should still be investigated (see related Issues Embedded run never exits after abort: waitForCompactionRetry() not tied to abort signal, session stays active and locked #12085, [Bug]: Embedded run timeout fails to clean up session/lane state when compaction is in-flight (waitForCompactionRetry not abort-aware) #13341, [Bug]: Embedded run timeout fails to clean up session/lane state when tool execution is in-flight #14228, Session lane stays stuck after embedded compaction run timeout #16331) so that drained=true after abort truly means the task ended.
  3. Stale target binding: If sessionId=unknown combined with aborting the wrong session still occurs, consider adding explicit mismatch detection and lane-level escalation in the activeSessionId resolution logic inside recoverStuckDiagnosticSession.

Comment generated for PR targeting Issue #92270.

@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 12, 2026
This was referenced Jun 12, 2026
Closed
Closed
Closed
Open
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 15, 2026
@clawsweeper

clawsweeper Bot commented Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 22, 2026, 6:58 PM ET / 22:58 UTC.

Summary
The PR changes stuck-session recovery to reset a session command lane when abort/drain reports success but the lane still has a pre-existing active task, and adds one regression test for that case.

PR surface: Source +14, Tests +34. Total +48 across 2 files.

Reproducibility: yes. at source level. Current main can reach active-abort recovery with queueDepth=0, abort/drain can report true after the embedded-run registry clears, and the existing release gate ignores a still-active command-lane task unless another reset arm is true; I did not run a live gateway reproduction in this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted after-fix runtime evidence from a real stuck-session recovery or a realistic gateway scenario showing the lane drains after abort.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only includes a local focused test command; it does not show after-fix behavior from a real stuck-session setup, terminal log, recording, or artifact. The contributor should add redacted proof to the PR body, which should trigger a fresh ClawSweeper review automatically; otherwise a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR lacks after-fix real behavior proof from an installed or realistic gateway recovery path; unit regression coverage alone does not prove the user-visible stuck-lane incident is fixed.
  • [P1] This recovery path deliberately clears command-lane ownership for a still-executing old task, so maintainers should keep the merge-risk review focused on session-state and message-delivery behavior.

Maintainer options:

  1. Require real recovery proof before merge (recommended)
    Ask for a redacted terminal/log artifact or realistic gateway scenario showing a clean abort/drain with a still-active lane being reset and later same-session work proceeding.
  2. Accept as narrow mitigation
    Maintainers may accept the source-backed queue contract and focused regression as enough for this P1 recovery mitigation despite missing real-environment proof.
  3. Hold for broader recovery direction
    Pause this draft if maintainers want the linked stuck-session cluster resolved through a broader recovery redesign instead of a focused lane reset patch.

Next step before merge

  • [P1] The remaining blocker is contributor proof or maintainer override for a draft external PR, not a narrow code defect that ClawSweeper should rewrite automatically.

Security
Cleared: The diff only changes recovery runtime logic and a focused test; I found no new dependency, secrets, CI, workflow, package, or supply-chain surface.

Review details

Best possible solution:

Land the narrow active-count recovery mitigation only after real behavior proof is added or a maintainer explicitly overrides that proof gate; keep ghost-target binding, ineffective-recovery logging, and empty-queue observability as separate follow-ups if they still reproduce.

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

Yes, at source level. Current main can reach active-abort recovery with queueDepth=0, abort/drain can report true after the embedded-run registry clears, and the existing release gate ignores a still-active command-lane task unless another reset arm is true; I did not run a live gateway reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the primary release defect: this is an acceptable narrow mitigation because it reuses the existing command-lane generation reset contract and preserves the fresh-task guard. It is not the full solution for stale target binding, ineffective-recovery logging, or all abort-aware tool/compaction cleanup.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 89de454f822c.

Label changes

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only includes a local focused test command; it does not show after-fix behavior from a real stuck-session setup, terminal log, recording, or artifact. The contributor should add redacted proof to the PR body, which should trigger a fresh ClawSweeper review automatically; otherwise a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The PR targets a high-impact stuck-session recovery path where queued channel work can remain blocked for real users until reset or restart.
  • merge-risk: 🚨 session-state: The changed recovery branch can reset a session command lane while a previous task is still executing, which affects ownership of session runtime state.
  • merge-risk: 🚨 message-delivery: The affected command lane gates queued channel/session work, so a wrong reset or missed reset can suppress, duplicate, or misorder follow-up message processing.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only includes a local focused test command; it does not show after-fix behavior from a real stuck-session setup, terminal log, recording, or artifact. The contributor should add redacted proof to the PR body, which should trigger a fresh ClawSweeper review automatically; otherwise a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +14, Tests +34. Total +48 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 16 2 +14
Tests 1 34 0 +34
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 50 2 +48

What I checked:

Likely related people:

  • openperf: Authored the closely related merged PR that added session-queue-aware lane release and the fresh-task guard in the same recovery runtime and tests. (role: recent recovery contributor; confidence: high; commits: 54748bf92d31, f194be19e261; files: src/logging/diagnostic-stuck-session-recovery.runtime.ts, src/logging/diagnostic-stuck-session-recovery.runtime.test.ts, src/logging/diagnostic-stuck-session-recovery.integration.test.ts)
  • vincentkoc: Current-main history shows recent work on the recovery runtime and command queue, including the v2026.6.9 release commit that carried the current gate. (role: recent area contributor; confidence: medium; commits: c645ec4555c0, 470a0f80b6d6; files: src/logging/diagnostic-stuck-session-recovery.runtime.ts, src/process/command-queue.ts)
  • LiLan0125: Authored the earlier stuck-session recovery change that reset the session command lane when abort/drain left queued lane work stranded. (role: prior recovery contributor; confidence: medium; commits: ad9f7f9a59ae; files: src/logging/diagnostic-stuck-session-recovery.runtime.ts, src/logging/diagnostic-stuck-session-recovery.runtime.test.ts)
  • steipete: Authored broader stuck diagnostic session recovery work and multiple command-queue hardening commits that shape the lane reset behavior this PR relies on. (role: adjacent diagnostics and queue contributor; confidence: medium; commits: 761e668acf8a, c397a02c9ad5, 13be898c07e3; files: src/logging/diagnostic-stuck-session-recovery.runtime.ts, src/logging/diagnostic.ts, src/process/command-queue.ts)
  • Joseph Krug: Introduced the generation-based stale active task reset pattern after SIGUSR1 restarts, which is the core safety mechanism used by resetCommandLane. (role: command lane reset introducer; confidence: medium; commits: 4e9f933e88e4; files: src/process/command-queue.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

This was referenced Jun 19, 2026
Closed
Open
@hanZeng-08
hanZeng-08 marked this pull request as draft June 22, 2026 09:03
@clawsweeper clawsweeper Bot removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 22, 2026
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 22, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 22, 2026
@hanZeng-08 hanZeng-08 closed this Jun 23, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 26, 2026
Closed
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

merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

1 participant

@hanZeng-08