Skip to content

fix: log failed terminal session persistence#97884

Closed
aditya-vithaldas wants to merge 1 commit into
openclaw:mainfrom
aditya-vithaldas:codex/log-chat-abort-persistence-failures
Closed

fix: log failed terminal session persistence#97884
aditya-vithaldas wants to merge 1 commit into
openclaw:mainfrom
aditya-vithaldas:codex/log-chat-abort-persistence-failures

Conversation

@aditya-vithaldas

@aditya-vithaldas aditya-vithaldas commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Closes #97795

What Problem This Solves

Fixes an issue where operators would lose visibility into terminal session persistence failures when an agent run finished and the session-store write rejected. The gateway still broadcast a terminal fallback projection, but the underlying write error was swallowed, making storage or filesystem problems much harder to diagnose.

Why This Change Was Made

The fix keeps the existing cleanup, restart-recovery, and fallback broadcast behavior intact, but threads the gateway logger into the agent-event handler and records the rejected terminal persistence promise once at the persistence owner boundary.

User Impact

Operators now get a gateway error log with run, client run, session key, and sanitized error context when terminal session persistence fails. Users keep the same terminal fallback behavior instead of seeing stalled session state.

Evidence

  • node scripts/run-vitest.mjs src/gateway/server-chat.agent-events.test.ts src/gateway/chat-abort.test.ts -- --reporter=dot — 322 passed
  • node scripts/run-vitest.mjs src/gateway/server-chat.agent-events.test.ts -- --reporter=dot — 198 passed after final trim
  • pnpm exec oxfmt --check src/gateway/server-chat.ts src/gateway/server-runtime-subscriptions.ts src/gateway/server.impl.ts src/gateway/server-chat.agent-events.test.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --codex-bin .artifacts/codex-flex --no-web-search — clean, patch correct 0.88

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: XS 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 applies a plausible owner-boundary logging fix, but the same central gateway persistence diagnostic is already covered by the open, mergeable, proof-positive canonical PR.

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 landing candidate because it is mergeable, uses formatted gateway logging, 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 from source. Current main catches rejected terminal lifecycle persistence only to broadcast fallback sessions.changed state and does not log the failed write.

Is this the best way to solve the issue?

No. This branch is a reasonable implementation, but the best landing path is the already-open proof-positive canonical sibling that covers the same owner-boundary fix.

Security review:

Security review cleared: The diff adds no dependency, workflow, lockfile, package, or code-execution surface, and formats arbitrary rejection content through the existing gateway formatter before logging.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root and scoped Gateway AGENTS.md were read fully; the review applied the owner-boundary, proof, duplicate-PR, and Gateway hot-path guidance. (AGENTS.md:1, 6de357ad4772)
  • Current main still has the silent owner catch: Current main creates the terminal lifecycle persistence promise and catches rejection only to broadcast fallback session state, with no diagnostic log. (src/gateway/server-chat.ts:714, 6de357ad4772)
  • This PR uses the right owner boundary: The diff changes the same owner catch to log terminal session persistence failed with run, client run, session key, and formatForLog(err) before preserving fallback broadcast behavior. (src/gateway/server-chat.ts:719, b81d7ef91d8a)
  • Canonical sibling is open and proof-positive: Live PR data shows fix: log terminal session persistence failures #97839 is open, mergeable, labeled proof: sufficient, and its body includes copied real runtime EPERM output plus fallback sessions.changed output for the same persistence-rejection path. (src/gateway/server-chat.ts:717, f3abe1365048)
  • Duplicate cluster search: GitHub search found multiple PRs for the same linked issue; prior overlapping attempts were closed in favor of fix: log terminal session persistence failures #97839, leaving this PR and that canonical sibling as the live overlap.
  • Formatter contract checked: formatForLog is the gateway helper that redacts and caps arbitrary values before they are written to gateway logs, which both this PR and the canonical sibling rely on. (src/gateway/ws-log.ts:114, 6de357ad4772)

Likely related people:

  • ooiuuii: Authored the recent merged gateway restart/terminal ownership work that heavily changed server-chat.ts, chat-abort.ts, and server-runtime-subscriptions.ts. (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: Recent path history shows adjacent gateway lifecycle and state-helper work, including co-authorship on the restart/terminal ownership change. (role: adjacent gateway lifecycle contributor; confidence: high; commits: 2b61d38a455c, d20fdf3b3842, 437a5a71ae20; files: src/gateway/server-chat.ts, src/gateway/chat-abort.ts, src/gateway/server-runtime-subscriptions.ts)
  • hexsprite: Authored nearby gateway/runtime error-detail formatting work relevant to the formatForLog(err) logging boundary. (role: adjacent formatter contributor; confidence: medium; commits: 86c1622a3a3c; files: src/gateway/ws-log.ts, src/gateway/ws-log.test.ts)
  • vincentkoc: Recent path history shows gateway refactors and chat-abort/server-chat touches near the affected abort and lifecycle surfaces. (role: recent adjacent gateway contributor; confidence: medium; commits: 2b75806197ab, ada70ece6fbb, 45e0545e823b; 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: 🧂 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. labels Jun 29, 2026
@aditya-vithaldas

aditya-vithaldas commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing this as superseded by the canonical fix in #97839 for #97795. This branch went green (63 success, 1 neutral, 34 skipped), but #97839 already covers the same gateway owner-boundary persistence diagnostic with proof and is the cleaner landing path.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Merged
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 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

@aditya-vithaldas