fix(gateway): log chat-abort terminal persistence failures instead of swallowing by guocn · Pull Request #97852 · openclaw/openclaw

guocn · 2026-06-29T15:32:28Z

Summary

When a chat run's terminal session-store persistence failed, registerChatAbortController's cleanup path caught the rejection with an empty handler — silently dropping the error and any operator signal while still deleting the abort controller.

In production this meant terminal states could fail to persist (DB / file-system issues) with zero trace in logs or telemetry, and the swallowed error masked infrastructure health problems.

Root cause

src/gateway/chat-abort.ts, in the cleanup closure of registerChatAbortController:

.catch(() => {            // ← err dropped, nothing logged
  if (params.chatAbortControllers.get(params.runId)?.controller === controller) {
    params.chatAbortControllers.delete(params.runId);
  }
});

The rejection handler swallowed the error entirely. The controller cleanup (correctly) remained, but there was no way for an operator to learn that persistence had failed.

Fix

Surface the failure through the project's standard subsystem logger (gateway/chat-abort), at error level, including the run id and underlying error — while preserving the existing controller cleanup so idempotent retry cleanup is not blocked:

.catch((err) => {
  log.error(
    `chat-abort: failed to persist terminal session state for run ${params.runId}: ${err}`,
  );
  if (params.chatAbortControllers.get(params.runId)?.controller === controller) {
    params.chatAbortControllers.delete(params.runId);
  }
});

Note: the issue suggested log.error?.(...), but SubsystemLogger.error is a required (non-optional) method per src/logging/subsystem.ts, so this uses the plain log.error(...) call matching the existing channel-health-monitor.ts convention.

Test

Added a regression test in chat-abort.test.ts alongside the existing success-path test. It asserts that, on terminal-persistence rejection:

the abort-controller entry is still cleaned up (no leak), and
the failure is logged (run id + error message present) rather than silently swallowed.

The subsystem logger is mocked (createSubsystemLogger) following the existing pattern in gateway-processes.test.ts.

Verification

vitest run src/gateway/chat-abort.test.ts → 96/96 passing (3 projects × 32 tests, including the new case)
Regression test confirmed meaningful: reverting only the source fix makes the new test fail; restoring it makes it pass.
oxlint on changed files → 0 warnings, 0 errors
oxfmt --check on changed files → format OK
vitest --typecheck on the target file → OK

… swallowing The terminal session-store persistence promise in registerChatAbortController's cleanup path caught rejections with an empty handler, silently dropping both the error and any operator signal while still deleting the abort controller. In production, terminal states could fail to persist with zero trace in logs or telemetry, while the swallowed error masked database / file-system health issues. Surfaces the failure via the gateway/chat-abort subsystem logger (error level), including the run id and the underlying error, and preserves the existing controller cleanup so idempotent retry cleanup is not blocked. Fixes #97795

clawsweeper · 2026-06-29T15:40:37Z

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 11:44 AM ET / 15:44 UTC.

Summary
The PR adds a subsystem logger call in registerChatAbortController's terminal-persistence rejection cleanup path and a Vitest regression for that direct cleanup branch.

PR surface: Source +11, Tests +58. Total +69 across 2 files.

Reproducibility: yes. Source inspection shows a rejected persistGatewaySessionLifecycleEvent can reach current-main fallback and cleanup consumers without any diagnostic log.

Review metrics: 1 noteworthy metric.

Logging boundary: 1 cleanup-consumer log added. The new diagnostic is downstream of the persistence owner catch, so maintainers should review placement before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97795
Summary: This PR is one candidate fix for the open gateway terminal-persistence observability issue, with two overlapping open PRs exploring nearby logging sites.

Members:

canonical: [Bug]: Unhandled persistence failures in chat-abort.ts silently swallow errors and risk memory leaks #97795 - The PR body links this issue, and the issue describes the same rejected terminal persistence logging gap.
fixed_by_candidate: fix: log terminal session persistence failures #97839 - This open PR targets the same issue at the server-chat.ts owner catch, but it still had review/proof blockers in the inspected discussion.
same_root_cause: fix(gateway): log errors when terminal persistence rejects in chat-abort cleanup #97813 - This open PR targets the same terminal-persistence observability gap from cleanup-consumer paths and remains an overlapping candidate rather than a merged superseder.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

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:

Move the diagnostic to the server-chat.ts owner catch and use formatForLog(err).
[P1] Add redacted terminal output or logs from a real gateway/runtime setup showing a rejected terminal persistence write emits the new diagnostic.
[P1] Coordinate with the overlapping open PRs so only one canonical owner-boundary fix lands.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body lists Vitest, lint, format, and typecheck output, but no redacted live gateway log, terminal output, or runtime artifact showing the real persistence-failure path after the change. 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

[P2] The branch logs only one downstream cleanup branch; delayed tracking and owner fallback persistence rejections can remain silent.
[P1] The new diagnostic interpolates raw unknown error text instead of using the gateway formatForLog(err) helper.
[P1] Contributor proof is limited to Vitest/lint/typecheck output, with no redacted real gateway runtime log showing the changed failure path.
[P1] Multiple open PRs target the same canonical issue, so landing cleanup-consumer logging alongside an owner-boundary fix could double-report one failed write.

Maintainer options:

Move and format the diagnostic (recommended)
Log the rejected persistence in the server-chat.ts owner catch with formatForLog(err), keep fallback projection unchanged, and update focused coverage for that path.
Pause in favor of one canonical PR
Maintainers can pause or close this cleanup-consumer branch after choosing a single owner-boundary candidate that resolves formatting and proof blockers.

Next step before merge

[P1] The remaining work needs contributor-supplied real behavior proof and maintainer selection of the owner-boundary fix, so it should stay in human review rather than automated repair.

Security
Needs attention: The diff introduces a low-severity log-safety concern by interpolating an unknown persistence error directly instead of using the gateway formatter.

Review findings

[P2] Log the persistence owner rejection instead — src/gateway/chat-abort.ts:185-187

Review details

Best possible solution:

Log the rejected terminal lifecycle persistence once at the server-chat.ts owner catch using gateway-safe error formatting, preserve fallback projection and cleanup semantics, and keep one canonical PR for the linked issue.

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

Yes. Source inspection shows a rejected persistGatewaySessionLifecycleEvent can reach current-main fallback and cleanup consumers without any diagnostic log.

Is this the best way to solve the issue?

No. The branch is a plausible partial mitigation, but the maintainable fix is to log once where server-chat.ts owns the persistence promise and fallback projection.

Full review comments:

[P2] Log the persistence owner rejection instead — src/gateway/chat-abort.ts:185-187
This log only runs when cleanup sees projectSessionTerminalPersistence already set. If caller cleanup happened while projectSessionTerminalPending was true, production still goes through startGatewayEventSubscriptions, whose delayed cleanup consumes the same rejection with catch(() => undefined), while the server-chat.ts owner catch only broadcasts fallback. Move the diagnostic to the server-chat.ts owner catch and render the error with formatForLog(err) so every rejected terminal write is visible without downstream duplicate logs.
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 1052652a7168.

Label changes

Label changes:

add P2: This is a normal-priority gateway observability bugfix with limited blast radius and no evidence of an active outage.
add merge-risk: 🚨 security-boundary: The new log path can write raw unknown error text instead of using the existing gateway-safe formatter.
add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
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 lists Vitest, lint, format, and typecheck output, but no redacted live gateway log, terminal output, or runtime artifact showing the real persistence-failure path after the change. 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:

P2: This is a normal-priority gateway observability bugfix with limited blast radius and no evidence of an active outage.
merge-risk: 🚨 security-boundary: The new log path can write raw unknown error text instead of using the existing gateway-safe formatter.
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 lists Vitest, lint, format, and typecheck output, but no redacted live gateway log, terminal output, or runtime artifact showing the real persistence-failure path after the change. 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 +11, Tests +58. Total +69 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	12	1	+11
Tests	1	58	0	+58
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	70	1	+69

Security concerns:

[low] Raw unknown error text in gateway logs — src/gateway/chat-abort.ts:186
The new log message renders err with template interpolation; gateway logging paths use formatForLog(err) to redact and cap arbitrary error values before operator-visible logs.
Confidence: 0.78

What I checked:

Repository policy read: Read the full root policy and scoped gateway policy; the gateway hot-path and deep-review rules shaped the owner-boundary and proof assessment. (AGENTS.md:1, 1052652a7168)
PR diff logs only direct cleanup: The PR head adds log.error only in the registerChatAbortController catch that runs when cleanup already sees projectSessionTerminalPersistence. (src/gateway/chat-abort.ts:185, 962ba790900b)
Current main owner catch is silent: Current main creates the terminal persistence promise in createAgentEventHandler and catches rejection only to broadcast a fallback session snapshot, without logging the failed write. (src/gateway/server-chat.ts:714, 1052652a7168)
Current main delayed cleanup is silent: When cleanup was requested before persistence is attached, startGatewayEventSubscriptions still consumes the rejection with catch(() => undefined) before deleting the entry. (src/gateway/server-runtime-subscriptions.ts:101, 1052652a7168)
PR test covers the narrower branch: The added test manually sets projectSessionTerminalPersistence before calling cleanup, so it does not prove the production owner/fallback or delayed-tracking rejection paths log. (src/gateway/chat-abort.test.ts:286, 962ba790900b)
Related canonical issue and overlapping PRs: The linked issue remains open, and live search found overlapping open candidate PRs targeting the same terminal persistence observability gap.

Likely related people:

ooiuuii: Authored recent merged gateway restart/active-run lifecycle work that modified all three central terminal persistence tracking files. (role: recent area contributor; confidence: high; commits: d20fdf3b3842; files: src/gateway/chat-abort.ts, src/gateway/server-chat.ts, src/gateway/server-runtime-subscriptions.ts)
steipete: Authored adjacent server-chat.ts lifecycle guard work and co-authored the recent restart/active-run lifecycle change that touched this surface. (role: recent area contributor; confidence: medium; commits: 2b61d38a455c, d20fdf3b3842; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)
scotthuang: Recent merged active-run projection and plugin-finalization work touched the same abort/session tracking files and subscription path. (role: adjacent feature contributor; confidence: medium; commits: 22e8cd2a1d25, b3dc27403498; files: src/gateway/chat-abort.ts, src/gateway/server-chat.ts, src/gateway/server-runtime-subscriptions.ts)
vincentkoc: Co-authored recent gateway active-run preservation work touching server-runtime-subscriptions.ts, adjacent to the delayed cleanup path. (role: adjacent area contributor; confidence: medium; commits: b3dc27403498; files: src/gateway/server-runtime-subscriptions.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.

guocn · 2026-06-29T15:47:21Z

Closing in favor of the overlapping owner-boundary candidates for #97795 (#97839 / #97813). ClawSweeper flagged that landing cleanup-consumer logging alongside an owner-boundary fix would double-report the same failed persistence write, so I'll defer to a single canonical fix rather than compete here. Thanks for the review.

mdjahid11978-design

openclaw-barnacle Bot added gateway Gateway runtime size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 29, 2026

guocn closed this Jun 29, 2026

This was referenced Jun 29, 2026

fix: log terminal session persistence failures #97839

Merged

fix(gateway): log terminal persistence failures in chat abort cleanup #97856

Closed

mdjahid11978-design reviewed Jun 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(gateway): log chat-abort terminal persistence failures instead of swallowing#97852

fix(gateway): log chat-abort terminal persistence failures instead of swallowing#97852
guocn wants to merge 1 commit into
openclaw:mainfrom
guocn:fix/chat-abort-log-persistence-failure-97795

guocn commented Jun 29, 2026

Uh oh!

clawsweeper Bot commented Jun 29, 2026 •

edited

Loading

Uh oh!

guocn commented Jun 29, 2026

Uh oh!

mdjahid11978-design left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

guocn commented Jun 29, 2026

Summary

Root cause

Fix

Test

Verification

Uh oh!

clawsweeper Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guocn commented Jun 29, 2026

Uh oh!

mdjahid11978-design left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 29, 2026 •

edited

Loading