Skip to content

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

Closed
luoyanglang wants to merge 2 commits into
openclaw:mainfrom
luoyanglang:wolf/chat-abort-terminal-persistence-logging
Closed

fix(gateway): log terminal session persistence failures in chat abort cleanup#97863
luoyanglang wants to merge 2 commits into
openclaw:mainfrom
luoyanglang:wolf/chat-abort-terminal-persistence-logging

Conversation

@luoyanglang

@luoyanglang luoyanglang commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #97795.

In the gateway, terminal session-store persistence is awaited in the chat-abort
cleanup path and in the runtime-subscription terminal handler. When that
persistence promise rejects, the existing .catch() blocks either release
the abort controller or fall back to restart recovery without recording the
error. A failed terminal-state write (e.g. database or file-system health
problem) therefore leaves no trace in logs or telemetry, so operators cannot see
that terminal state failed to persist.

Affected sites (all consume the same projectSessionTerminalPersistence
promise):

  • src/gateway/chat-abort.ts cleanup .catch(() => {}) — releases the controller silently.
  • src/gateway/server-runtime-subscriptions.ts cleanup .catch(() => undefined) — releases the entry silently.
  • src/gateway/server-runtime-subscriptions.ts fallback .catch(() => { restartRecoveryCandidates.set(...) }) — falls back to restart recovery silently.

Why This Change Was Made

The terminal persistence rejection is a real, operator-relevant signal (storage
health). Swallowing it hides production failures. This logs the rejection at
each site before the existing cleanup / restart-recovery behavior runs, so
the control-flow and fallback semantics are unchanged — only observability is
added.

The single-site fix the issue links would leave the sibling
runtime-subscription sites silent. #97813 takes that single-site approach: it
only touches src/gateway/chat-abort.ts and misses the two
src/gateway/server-runtime-subscriptions.ts consumers of the same
projectSessionTerminalPersistence promise — so a rejected terminal write in
the runtime-subscription cleanup or the restart-recovery fallback would still be
swallowed silently there. ClawSweeper's review explicitly names both the
"clean up" and "broadcast fallback state" paths, so all three sites are the
minimum complete fix unit; this PR covers them with no product-policy change.

User Impact

No behavior change for end users. Operators now get a gateway/chat-abort /
gateway/runtime-subscriptions error log line when terminal session
persistence fails, instead of a silent swallow. No new config or surface.

Evidence

  • New regression src/gateway/chat-abort.terminal-persistence-logging.test.ts:
    a rejected projectSessionTerminalPersistence now logs error (with run id +
    reason) and still releases the controller. RED/GREEN confirmed — the test
    fails on current main (no log) and passes with the fix.
  • Local validation (Mac, Node 22.22.3):
    • node scripts/run-vitest.mjs run src/gateway/chat-abort.terminal-persistence-logging.test.ts src/gateway/chat-abort.test.ts — 128 tests pass
    • pnpm tsgo:core and pnpm tsgo:core:test
    • pnpm exec oxfmt --check on the 3 touched files
    • git diff --check origin/main...HEAD
    • gitleaks protect --staged --redact — no leaks
  • Net diff: 3 files, +84/-3.

@luoyanglang
fix(gateway): log terminal session persistence failures in chat abort…
9154c1f 
… cleanup

Rejected projectSessionTerminalPersistence promises in chat-abort cleanup and
runtime-subscription terminal handling released the controller or fell back to
restart recovery without recording the error, hiding DB/file-system health
failures from logs. Log the rejection at all three sites before the existing
cleanup/fallback runs. Adds a regression covering the chat-abort cleanup path.

Fixes openclaw#97795
@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S labels Jun 29, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@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: the PR targets a real gateway observability bug, but it is superseded by the open, mergeable owner-boundary implementation in #97839, which formats unknown errors and includes stronger real-runtime proof.

Root-cause cluster
Relationship: superseded
Canonical: #97839
Summary: Several PRs address the same rejected terminal persistence logging gap; the open canonical sibling is the safer owner-boundary landing candidate because it uses gateway formatting and has real-runtime proof.

Members:

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

Canonical path: Close this branch and review or land #97839 as the canonical formatted owner-boundary fix for #97795.

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

Review details

Best possible solution:

Close this branch and review or land #97839 as the canonical formatted owner-boundary fix for #97795.

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

Yes. Source inspection shows rejected terminal lifecycle persistence reaches current-main catch blocks that clean up or broadcast fallback state without logging the failed write.

Is this the best way to solve the issue?

No. This branch is a plausible partial/downstream observability fix, but the best solution is one formatted diagnostic at the persistence owner catch, as implemented by #97839.

Security review:

Security review needs attention: The diff adds operator-visible logging for arbitrary rejected persistence values without the gateway formatter and redaction proof used by the canonical sibling.

  • [low] Unknown rejection bypasses gateway formatter — src/gateway/server-runtime-subscriptions.ts:107
    The new log messages render unknown rejected values with String(err) instead of formatForLog(err), weakening the gateway-specific redaction and length-capping boundary for persistence failures.
    Confidence: 0.86

AGENTS.md: found and applied where relevant.

What I checked:

Likely related people:

  • Alix-007: Local blame attributes the current terminal persistence owner and cleanup catch blocks to commit 5f86c3a; confidence is limited because the checkout history is grafted around a broad imported commit. (role: current line provenance; confidence: low; commits: 5f86c3a90f69; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)
  • steipete: Recent local history shows Peter Steinberger authored gateway suite and formatter-related changes touching the same gateway area and formatForLog path. (role: recent adjacent gateway contributor; confidence: medium; commits: 5cf01ac7c1a4, c2e2b87f28f0, 50b7607f7781; 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. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 29, 2026
@luoyanglang

luoyanglang commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #97839 as the canonical fix for #97795.

After comparing, #97839 is the better landing: it logs at the owner boundary in src/gateway/server-chat.ts where the terminal-persistence promise is created, so the rejection is recorded once at the source instead of at each downstream consumer, and it uses formatForLog(err) for redaction rather than a raw String(err). That owner-boundary + redaction approach supersedes this consumer-side patch (and addresses the merge-risk: security-boundary flag raised on raw error interpolation here).

This PR's only remaining unique surface was the two server-runtime-subscriptions.ts consumer catches; those become redundant once the owner logs the rejection. No action needed here — #97839 covers the issue.

Thanks @LZY3538.

@luoyanglang
luoyanglang deleted the wolf/chat-abort-terminal-persistence-logging branch June 29, 2026 16:31
This was referenced Jun 29, 2026
Closed
Merged
Closed
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

gateway Gateway runtime merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

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

@luoyanglang