Skip to content

fix(gateway): persist chat state before aborting stream#98171

Closed
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix-chat-abort-persistence-leak
Closed

fix(gateway): persist chat state before aborting stream#98171
aniruddhaadak80 wants to merge 1 commit into
openclaw:mainfrom
aniruddhaadak80:fix-chat-abort-persistence-leak

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #97795

What Problem This Solves

Addresses swallowed terminal-persistence failures during chat stream aborts.

Why This Change Was Made

To ensure that errors encountered while persisting chat state during stream aborts are logged properly instead of being ignored, improving debuggability.

User Impact

Better visibility into persistence failures on the gateway side.

Evidence

Modified src/gateway/chat-abort.ts to log errors caught during terminal persistence.

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

clawsweeper Bot commented Jun 30, 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: the gateway persistence-logging problem is already owned by a stronger open canonical PR, while this branch both references the wrong already-fixed CLI issue and introduces an incorrect logger call on the cleanup failure path.

Root-cause cluster
Relationship: superseded
Canonical: #97839
Summary: The current PR is a duplicate, less-safe downstream attempt at the gateway terminal-persistence logging bug; the open canonical PR owns the same remaining work with proof and a safer owner-boundary implementation.

Members:

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

Canonical path: Close this PR and continue review on #97839 as the canonical owner-boundary, redaction-tested 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 PR and continue review on #97839 as the canonical owner-boundary, redaction-tested fix for #97795.

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

Yes, source-reproducible. Current main has silent terminal-persistence catch paths, and the PR defect is also source-proven by the logError RuntimeEnv parameter contract.

Is this the best way to solve the issue?

No. The maintainable fix is the canonical owner-boundary PR with formatForLog(err) and redaction proof; this branch is a downstream duplicate and uses the logger API incorrectly.

Security review:

Security review cleared: No concrete supply-chain or credentials regression was found; the blocker is a functional/session-state logger-contract bug, not a security flaw.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root review policy and gateway scoped guidance were read fully; gateway hot-path and owner-boundary review guidance applied to this PR. (AGENTS.md:1, 6cb82eaab865)
  • Current downstream cleanup catch is silent: Current main waits on projectSessionTerminalPersistence; the rejection handler deletes the matching controller and does not log, matching the issue symptom but at a downstream cleanup consumer. (src/gateway/chat-abort.ts:168, 6cb82eaab865)
  • Logger contract mismatch: logError accepts a RuntimeEnv as its second argument, so passing the caught Error as the second argument is not error metadata and can make the catch handler throw before cleanup runs. (src/logger.ts:77, 6cb82eaab865)
  • Current PR diff: The PR head changes the cleanup catch to call logError("Project session terminal persistence failed during abort", error) and adds no regression test or real behavior proof. (src/gateway/chat-abort.ts:176, 9ffa0e63d07f)
  • Owner-boundary catch remains the better fix point: Current main creates the terminal lifecycle persistence promise in server-chat and catches rejection only to broadcast fallback state, so the canonical owner-boundary fix belongs there rather than only in chat-abort cleanup. (src/gateway/server-chat.ts:714, 6cb82eaab865)
  • Canonical PR is open and proof-positive: fix: log terminal session persistence failures #97839 is open and mergeable, fixes the same gateway persistence-logging issue at src/gateway/server-chat.ts, includes live denied-write proof, and keeps formatForLog(err) redaction coverage. (src/gateway/server-chat.ts:719, f3abe1365048)

Likely related people:

  • ooiuuii: Authored the merged gateway restart/terminal ownership work that touched the affected persistence path across server-chat, chat-abort, and runtime subscriptions. (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)
  • Peter Steinberger: Authored adjacent gateway lifecycle snapshot handling work near the terminal session state and persistence surfaces. (role: adjacent lifecycle contributor; confidence: medium; commits: 2b61d38a455c; files: src/gateway/server-chat.ts, src/gateway/session-lifecycle-state.ts)
  • Jordan Baker: Authored the gateway log formatter/redaction work that the canonical PR uses to keep persistence error logs bounded and redacted. (role: adjacent formatter contributor; confidence: medium; commits: 86c1622a3a3c; files: src/gateway/ws-log.ts, src/gateway/ws-log.test.ts)

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

@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. P2 Normal backlog priority with limited blast radius. 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 30, 2026
This was referenced Jun 30, 2026
Closed
Merged
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 5, 2026
@aniruddhaadak80

aniruddhaadak80 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by canonical PR #97839, following maintainer review.

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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS 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

@aniruddhaadak80