Skip to content

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

Closed
hanZeng-08 wants to merge 2 commits into
openclaw:mainfrom
hanZeng-08:fix/97795-chat-abort-persistence-error-logging
Closed

fix(gateway): log terminal persistence failures in chat abort cleanup#97856
hanZeng-08 wants to merge 2 commits into
openclaw:mainfrom
hanZeng-08:fix/97795-chat-abort-persistence-error-logging

Conversation

@hanZeng-08

@hanZeng-08 hanZeng-08 commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #97795

Summary

Fix silent swallowing of terminal-state persistence failures across three Gateway boundaries:

  1. src/gateway/chat-abort.ts — cleanup catch block
  2. src/gateway/server-runtime-subscriptions.ts — delayed cleanup ordering (persistence attached after cleanup() was requested)
  3. src/gateway/server-chat.ts — owner-side terminal lifecycle persistence fallback catch

Real Behavior Proof

Environment: Linux, Node v22.23.1, pnpm 11.2.2, worktree SHA 1052652

Before fix (new regression test on old code):

$ pnpm test src/gateway/chat-abort.test.ts
AssertionError: expected "vi.fn()" to be called once, but got 0 times
Tests  4 failed | 124 passed (128)

After fix:

$ node scripts/run-vitest.mjs src/gateway/chat-abort.test.ts src/gateway/server-chat.agent-events.test.ts
Test Files  6 passed (6)
     Tests  326 passed (326)

Real behavior proof (chat-abort cleanup path):

$ tsx /tmp/proof-97795.mjs
[PROOF LOGGER] Failed to persist chat abort terminal state for run proof-run-1: simulated persistence failure {"error":{}}
Controllers remaining after cleanup: 0

The script creates a registerChatAbortController registration with a real log.error logger, attaches a rejecting projectSessionTerminalPersistence promise, calls cleanup(), and observes the diagnostic log plus correct controller removal.

Lint / Format checks (changed files):

$ oxlint src/gateway/chat-abort.ts src/gateway/chat-abort.test.ts src/gateway/server-methods/agent.ts src/gateway/server-methods/chat.ts src/gateway/server-chat.ts src/gateway/server-chat.agent-events.test.ts src/gateway/server-runtime-subscriptions.ts src/gateway/server.impl.ts
Found 0 warnings and 0 errors.

$ oxfmt --check ...
All matched files use the correct format.

Regression Test Plan

  • src/gateway/chat-abort.test.ts
    • Added logs an error and still removes the registration when terminal persistence fails
  • src/gateway/server-chat.agent-events.test.ts
    • Updated broadcasts a terminal fallback snapshot when persistence fails to assert log.error is called

Root Cause

Three separate .catch(() => { ... }) handlers discarded the rejection reason:

  • src/gateway/chat-abort.ts:176
  • src/gateway/server-runtime-subscriptions.ts:100
  • src/gateway/server-chat.ts:714

Implementation Plan Details

  1. Add optional log?: SubsystemLogger to registerChatAbortController;
  2. Add optional log?: SubsystemLogger to startGatewayEventSubscriptions and pass it to createAgentEventHandler;
  3. Add optional log?: SubsystemLogger to createAgentEventHandler;
  4. Pass context.logGateway from agent.ts / chat.ts to registerChatAbortController;
  5. Pass log from server.impl.ts to startGatewayEventSubscriptions;
  6. Call log?.error(...) in all three catch blocks, using formatErrorMessage(err);
  7. Add/update regression tests;
  8. Demonstrate real log output for the chat-abort path with a standalone script.

Change Details

  • src/gateway/chat-abort.ts

    • Import formatErrorMessage, SubsystemLogger
    • registerChatAbortController params gain log?: SubsystemLogger
    • Cleanup catch now logs the error

  • src/gateway/server-methods/agent.ts / chat.ts

    • registerChatAbortController(...) calls add log: context.logGateway

  • src/gateway/server-chat.ts

    • Import formatErrorMessage, SubsystemLogger
    • AgentEventHandlerOptions gains log?: SubsystemLogger
    • createAgentEventHandler destructures log
    • Terminal lifecycle persistence catch now logs the error

  • src/gateway/server-runtime-subscriptions.ts

    • Import formatErrorMessage, SubsystemLogger
    • startGatewayEventSubscriptions params gain log?: SubsystemLogger
    • Delayed cleanup catch now logs the error
    • Pass log to createAgentEventHandler

  • src/gateway/server.impl.ts

    • startGatewayEventSubscriptions(...) call adds log

  • src/gateway/chat-abort.test.ts

    • New regression test

  • src/gateway/server-chat.agent-events.test.ts

    • createHarness creates a mock logger and passes it to createAgentEventHandler
    • Updated persistence-failure test asserts log.error is called

Test Points

  • chat-abort new test fails before fix (RED) and passes after fix (GREEN)
  • server-chat existing persistence-failure test now asserts log.error and passes
  • All 326 relevant tests pass
  • Changed files pass oxlint / oxfmt
  • pnpm tsgo:core, pnpm tsgo:core:test, pnpm check:import-cycles pass

Next Steps

  1. Commit the updated changes;
  2. Force-push the updated branch (amended commit);
  3. Paste the updated PR body;
  4. Comment @clawsweeper re-review.

@hanZeng-08
fix(gateway): log terminal persistence failures in chat abort cleanup
2e7fdf1 
Previously, a rejected projectSessionTerminalPersistence promise was silently
swallowed, making it impossible for operators to detect terminal state
persistence failures. Add an optional SubsystemLogger to
registerChatAbortController and emit log.error before cleanup. Pass
context.logGateway from both chat-send and agent run registration paths.

Fixes openclaw#97795
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime size: S labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: this PR now covers the previously flagged paths, but it does so by adding multiple diagnostics for the same terminal-persistence promise, while #97839 is open, mergeable, proof-positive, and owns the cleaner single owner-boundary fix.

Canonical path: Land one formatted owner-boundary diagnostic via #97839 or an equivalent narrow branch, and keep downstream cleanup consumers focused on cleanup and recovery state.

So I’m closing this here and keeping the remaining discussion on #97839.

Review details

Best possible solution:

Land one formatted owner-boundary diagnostic via #97839 or an equivalent narrow branch, and keep downstream cleanup consumers focused on cleanup and recovery state.

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

Yes from source. Current main creates the terminal persistence promise in src/gateway/server-chat.ts, passes it to cleanup tracking, and catches rejection without an owner log.

Is this the best way to solve the issue?

No. The best fix is one owner-boundary diagnostic; this branch also logs in downstream cleanup consumers, while #97839 implements the cleaner canonical path with proof.

Security review:

Security review cleared: No concrete security or supply-chain regression was found; the diff adds no dependency, workflow, package, permission, or secret-handling surface, and uses existing redaction-aware error formatting/logging paths.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root and scoped gateway AGENTS.md files were read; the owner-boundary, deep-review, gateway hot-path, and proof guidance affected this review. (AGENTS.md:1, 6de357ad4772)
  • Current owner catch is the central persistence boundary: Current main creates one terminal lifecycle persistence promise, passes that same promise to tracking, then catches rejection only to broadcast fallback state; this is the owner boundary where a single diagnostic belongs. (src/gateway/server-chat.ts:672, 6de357ad4772)
  • Current cleanup consumers share the same promise: The runtime subscription tracking path stores the same persistence promise for cleanup and restart recovery, so logging there in addition to the owner catch can duplicate one failed write. (src/gateway/server-runtime-subscriptions.ts:99, 6de357ad4772)
  • This PR adds downstream and owner diagnostics: The PR head logs in the server-chat owner catch and also in downstream chat-abort/runtime cleanup catches, making the branch broader than the single owner-boundary fix. (src/gateway/server-runtime-subscriptions.ts:103, 9aa0b9251acc)
  • Canonical open replacement is viable: fix: log terminal session persistence failures #97839 is open, mergeable, labeled proof sufficient, and its body includes copied live output from a denied session-store write showing the formatted gateway error log plus fallback projection. (f3abe1365048)
  • Feature-history provenance: Gateway restart and terminal ownership tracking across the central files was introduced in the referenced history commit, with Peter Steinberger listed as co-author in the commit message. (src/gateway/server-chat.ts:672, d20fdf3b3842)

Likely related people:

  • ooiuuii: Authored the merged gateway restart/terminal ownership work that added the terminal persistence tracking and fallback structure across the reviewed files. (role: feature-history contributor; confidence: high; commits: d20fdf3b3842; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)
  • steipete: Listed as co-author on the gateway restart/terminal ownership commit and appears repeatedly in adjacent gateway lifecycle/history for these files. (role: co-author and adjacent gateway contributor; confidence: medium; commits: d20fdf3b3842, 5cf01ac7c1a4, c2e2b87f28f0; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)
  • Alix-007: Current checkout blame attributes the visible terminal-persistence lines to a recent repository import/base commit, though the commit subject is unrelated to this gateway behavior, so this is a weak routing signal. (role: current-line blame in this checkout; confidence: low; commits: 5f86c3a90f69; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)

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

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 29, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jun 29, 2026
This was referenced Jun 29, 2026
Merged
Closed
@hanZeng-08
fix(gateway): cover delayed and owner-side terminal persistence failu…
9aa0b92 
…re logs

Extends the previous chat-abort fix to also log terminal persistence
rejections in the remaining two boundaries ClawSweeper identified:

- src/gateway/server-runtime-subscriptions.ts: delayed cleanup ordering
  when persistence is attached after cleanup() was requested.
- src/gateway/server-chat.ts: owner-side terminal lifecycle persistence
  fallback catch.

Both boundaries receive the injected SubsystemLogger. The existing
server-chat.agent-events test now asserts log.error is called on
persistence failure.
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 29, 2026
@hanZeng-08

hanZeng-08 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

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 added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 29, 2026
@clawsweeper clawsweeper Bot closed this Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=9aa0b9251acc368ecdfb9f577d8d5ec951110cd1)

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

app: web-ui App: web-ui gateway Gateway runtime merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Unhandled persistence failures in chat-abort.ts silently swallow errors and risk memory leaks

1 participant

@hanZeng-08