Skip to content

[codex] Fix stale active work session recovery#95778

Closed
moeedahmed wants to merge 5 commits into
openclaw:mainfrom
moeedahmed:codex/stale-active-work-recovery
Closed

[codex] Fix stale active work session recovery#95778
moeedahmed wants to merge 5 commits into
openclaw:mainfrom
moeedahmed:codex/stale-active-work-recovery

Conversation

@moeedahmed

@moeedahmed moeedahmed commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Telegram topic sessions could remain blocked behind stale active work even when the underlying model call, worker, or active run was no longer making progress. In that state, a lane could be classified as stalled but still fail to release because model-call recovery required an active embedded-run marker.

This meant a topic could stay silent behind a persisted busy/running state or queued work even though recovery should have cleared only that affected lane.

Why This Change Was Made

The diagnostic heartbeat already tracks meaningful progress and has per-lane recovery plumbing. The gap was that stalled model_call recovery was tied to hasActiveEmbeddedRun === true, so orphaned model work without that marker was observed but not actively released.

This change makes stalled model calls and orphaned active work recovery-eligible once their last meaningful progress age reaches the configured stuck-session abort threshold. It also aligns JSON task output with the operator inspection projection so stale/lost tasks do not look like real active work forever.

User Impact

  • A stale model/worker lane can be recovered per session/topic without a broad gateway restart.
  • One stuck Telegram topic should not keep unrelated topics behind stale active-work state.
  • Task JSON output is clearer: stale/lost records are projected as inspection state instead of appearing as live running work.

Evidence

Validated on a fresh worktree based on current origin/main:

node scripts/run-vitest.mjs src/logging/diagnostic.test.ts src/logging/diagnostic-session-attention.test.ts src/commands/tasks-json.test.ts

Result:

  • src/logging/diagnostic-session-attention.test.ts: 11 passed
  • src/logging/diagnostic.test.ts: 70 passed
  • src/commands/tasks-json.test.ts: 2 passed
  • git diff --check origin/main...HEAD: passed

Additional local verification after the task JSON summary fix:

pnpm format:check src/commands/tasks-json.ts src/commands/tasks-json.test.ts
node scripts/run-vitest.mjs src/commands/tasks-json.test.ts
pnpm check:test-types

Results: formatting, focused tasks JSON tests, and test type checks passed. Latest pushed commit: 8b893230e2d.

Additional local verification after the owner-boundary repair:

node scripts/run-vitest.mjs src/logging/diagnostic.test.ts src/logging/diagnostic-session-attention.test.ts src/commands/tasks-json.test.ts
pnpm format:check src/logging/diagnostic.ts src/logging/diagnostic.test.ts src/commands/tasks-json.ts src/commands/tasks-json.test.ts
pnpm check:test-types

Results: focused diagnostic/session-attention/tasks JSON tests passed, formatting passed, and test type checks passed. Latest pushed commit: 8b893230e2d.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: XS labels Jun 22, 2026
@moeedahmed
moeedahmed marked this pull request as ready for review June 22, 2026 14:23
@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 27, 2026, 4:44 AM ET / 08:44 UTC.

Summary
The branch adds a task-pressure summary object to task list JSON output, extends delivery-pressure summary tests, and renames one diagnostic owner-boundary test.

PR surface: Source +52, Tests +94. Total +146 across 6 files.

Reproducibility: yes. at source level: current main keeps model-call recovery gated on active embedded-run ownership, and the PR head computes the new routed JSON summary from raw task rows. No live Telegram/provider reproduction was supplied for this head.

Review metrics: 1 noteworthy metric.

  • JSON output surface: 1 field added. The additive summary field becomes part of openclaw tasks --json output, so its raw-versus-inspectable semantics need to be settled before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup 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 terminal/live logs or a recording showing the changed stale-lane behavior on this head, with private data removed.
  • [P1] Fix the routed task JSON summary to use the intended inspectable projection or explicitly document it as raw registry output.
  • Retitle and scope the PR to task JSON, or restore a bounded diagnostic recovery change with maintainer-approved owner semantics.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and follow-up comments provide focused tests, formatting, and type checks only, and the author explicitly says live Telegram/Mantis behavior proof is still missing. 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 current head does not change diagnostic recovery, so merging it would not fix the stale model-call/session recovery problem described in the PR body.
  • [P1] The new JSON summary is computed from raw task rows in the routed command path and can report stale missing-backed rows as active pressure instead of matching operator-inspection projection.
  • [P1] Contributor proof is limited to focused tests, formatting, and type checks; no redacted live logs, terminal output, or recording shows the stale-lane behavior after this head.

Maintainer options:

  1. Decide the mitigation before merge
    Either narrow this PR to a corrected task JSON summary that uses the intended inspectable projection, or restore a bounded diagnostic recovery fix with live proof and coordinate it with the canonical stale model-call issue.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Needs human review and contributor action because missing real behavior proof cannot be supplied by automation, and maintainers should decide whether to split the task JSON work from the stale recovery work.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes task/diagnostic source and tests only, with no dependency, workflow, secret, installer, or package-resolution surface changed.

Review findings

  • [P2] Summarize the inspectable task list — src/commands/tasks-json.ts:71
Review details

Best possible solution:

Either narrow this PR to a corrected task JSON summary that uses the intended inspectable projection, or restore a bounded diagnostic recovery fix with live proof and coordinate it with the canonical stale model-call issue.

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

Yes at source level: current main keeps model-call recovery gated on active embedded-run ownership, and the PR head computes the new routed JSON summary from raw task rows. No live Telegram/provider reproduction was supplied for this head.

Is this the best way to solve the issue?

No. The current head no longer implements stale active-work recovery, and the new JSON summary should summarize the same inspectable projection operators rely on before becoming a public output field.

Full review comments:

  • [P2] Summarize the inspectable task list — src/commands/tasks-json.ts:71
    The routed JSON path builds tasks from the raw store snapshot, while the operator list projects stale missing-backed active rows through reconcileInspectableTasks(). Adding summary here means stale rows can still be counted as active pressure in openclaw tasks --json, so compute the summary from the inspectable projection or explicitly scope this field as raw registry output.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • remove merge-risk: 🚨 session-state: Current PR review selected no merge-risk labels.
  • remove merge-risk: 🚨 availability: Current PR review selected no merge-risk labels.

Label justifications:

  • P1: The PR is tied to stuck session/topic recovery that can block real agent or channel workflows, even though the current head no longer implements the recovery change.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • 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 and follow-up comments provide focused tests, formatting, and type checks only, and the author explicitly says live Telegram/Mantis behavior proof is still missing. 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 +52, Tests +94. Total +146 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 3 53 1 +52
Tests 3 95 1 +94
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 148 2 +146

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; its deep PR review, real behavior proof, session-state, Codex-source, and maintainer-notes requirements affected this review. (AGENTS.md:1, c5d34c8376f8)
  • Maintainer notes checked: The Telegram maintainer note requires real Telegram proof for Telegram transport/topic changes, but this head does not touch Telegram code or visible Telegram rendering. (.agents/maintainer-notes/telegram.md:1, c5d34c8376f8)
  • Current PR diff: The live diff adds summary: summarizeTaskPressure(tasks) to task JSON list outputs; the only diagnostic source change left is a test-name rename, so stale recovery runtime behavior is not changed by this head. (src/commands/tasks-json.ts:67, 8b893230e2d1)
  • Routed JSON source is raw: The lean JSON task command intentionally reads listTaskRecords() and avoids maintenance reconciliation, so a new pressure summary here is computed from raw stored rows. (src/commands/tasks-json.ts:18, c5d34c8376f8)
  • Full task list uses inspectable projection: The human task list path filters reconcileInspectableTasks(), so its JSON output can differ from the routed raw JSON helper when stale rows are projected. (src/commands/tasks.ts:347, c5d34c8376f8)
  • Inspectable projection can mark lost: reconcileInspectableTasks() maps each stored task through operator-inspection reconciliation, which can project missing-backed active rows as lost. (src/tasks/task-registry.maintenance.ts:886, c5d34c8376f8)

Likely related people:

  • vincentkoc: Authored the merged task pressure and task maintenance/status command work that the new JSON pressure summary builds on. (role: task command contributor; confidence: high; commits: e6445c22aac7, c52fac836cc8, dce61dc920dc; files: src/commands/tasks.ts, src/tasks/task-registry.summary.ts, src/tasks/task-registry.maintenance.ts)
  • mbelinky: The original background task lifecycle commit was co-authored and reviewed under this handle, and that lifecycle is the owner boundary behind task registry summaries. (role: original background task lifecycle contributor/reviewer; confidence: medium; commits: 17c36b5093da; files: src/commands/tasks.ts, src/tasks/task-registry.maintenance.ts)
  • litang9: Authored the merged diagnostic behavior that preserves owned model-call handling while keeping ownerless stale activity from being hidden as long-running. (role: recent diagnostic feature contributor; confidence: high; commits: 0f71a665ed7a; files: src/logging/diagnostic.ts, src/logging/diagnostic-session-attention.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.

@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: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 23, 2026
…rk-recovery

# Conflicts:
#	src/commands/tasks-json.ts
@clawsweeper clawsweeper Bot added the mantis: telegram-visible-proof Mantis should capture Telegram visible proof. label Jun 26, 2026
@clawsweeper
clawsweeper Bot temporarily deployed to qa-live-shared June 26, 2026 17:42 Inactive
@moeedahmed

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Fixed the task JSON mismatch from the latest review: tasksListJsonCommand now emits the same task-pressure summary shape that its JSON tests expect, and the audit fixture now expects the existing stale_running error classification for a 40-minute running task.

Proof: pnpm format:check src/commands/tasks-json.ts src/commands/tasks-json.test.ts passed, node scripts/run-vitest.mjs src/commands/tasks-json.test.ts passed, and pnpm check:test-types passed.

@clawsweeper

clawsweeper Bot commented Jun 26, 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.

@moeedahmed

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the owner-boundary finding: ownerless processing-session model/tool markers are no longer active-abort eligible. Stale owned model calls still recover through the embedded-run owner path, while idle queued lanes blocked by stale orphaned markers continue to recover through stale-session-state handling.

Proof passed on PR head 8b893230e2d:

node scripts/run-vitest.mjs src/logging/diagnostic.test.ts src/logging/diagnostic-session-attention.test.ts src/commands/tasks-json.test.ts
pnpm format:check src/logging/diagnostic.ts src/logging/diagnostic.test.ts src/commands/tasks-json.ts src/commands/tasks-json.test.ts
pnpm check:test-types

Remaining proof gap: the live Telegram/Mantis behaviour proof requested in the previous review still needs maintainer/live-runtime capture.

@clawsweeper

clawsweeper Bot commented Jun 27, 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.

@clawsweeper clawsweeper Bot removed mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by the current focused recovery path.

The current head no longer carries the original stale active-work recovery fix in a form that should land independently, and the remaining task/status JSON pieces overlap the narrower recovery work being kept open, especially #96853. Keeping this PR open creates duplicate review overhead.

If a unique task-status output improvement is still needed, it should be folded into #96853 or reopened as a small task-status-only PR with current proof.

@moeedahmed moeedahmed closed this Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant