fix(gateway): log chat-abort terminal persistence failures#98104
fix(gateway): log chat-abort terminal persistence failures#98104ly-wang19 wants to merge 3 commits into
Conversation
The catch on the terminal persistence promise silently removed the abort controller without recording why, masking DB/filesystem failures during run cleanup. Log the rejection via the gateway/chat-abort subsystem logger before removing the controller so operators can see the underlying health failure. The controller entry is still removed either way so memory does not leak. Closes openclaw#97795.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the same linked gateway persistence-logging bug is already owned by an open, mergeable, proof-positive canonical PR that logs at the persistence owner boundary with formatter-backed redaction coverage. Root-cause cluster 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 fix. So I’m closing this here and keeping the remaining discussion on #97839. Review detailsBest possible solution: Close this PR and continue review on #97839 as the canonical owner-boundary fix. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main catches rejected terminal lifecycle persistence in server-chat and downstream cleanup paths without logging; I did not execute tests in this read-only review. Is this the best way to solve the issue? No. This branch is a partial downstream mitigation; the better solution is the open canonical owner-boundary PR with formatForLog(err), redaction coverage, and real behavior proof. Security review: Security review needs attention: The patch adds an operator-visible arbitrary-error log path without using the canonical gateway formatter and redaction regression proof.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 738b2be4b49b. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
Satisfies oxlint use-unknown-in-catch-callback-variable after the prior commit introduced the typed catch parameter.
Scan changed paths failed to fetch opengrep from GitHub (infra). checks-node-compact-large-1 had disposeSession timing flake. Both unrelated to chat-abort change.
1 participant
What Problem This Solves
Fixes #97795.
When the project session terminal persistence promise inside
registerChatAbortControllerrejected, the.catch()block removed the abort controller without ever recording why. In production this silently masks DB or filesystem health failures during run cleanup — operators get no trace in telemetry or logs that a terminal state failed to persist.Why This Change Was Made
The bug is a missing telemetry signal, not a logic flaw. The existing behavior (remove the controller either way so memory does not leak) is correct; the only gap is observability. The fix adds a
gateway/chat-abortsubsystemerrorlog on rejection carryingrunIdand the underlying error message, then keeps the existing cleanup behavior unchanged.A subsystem logger is used (matching
src/gateway/boot.tsandsrc/gateway/channel-health-monitor.ts) rather than the barelogErrorhelper so the failure is attributable to this code path in structured logs.User Impact
Evidence
src/gateway/chat-abort.test.ts: "logs and still removes the controller when terminal persistence rejects". It registers a controller, attaches a rejectingprojectSessionTerminalPersistence, callscleanup(), and asserts both that the entry is removed and that the subsystem logger receivederrorwith the expected message and{ runId, error }metadata.node scripts/run-vitest.mjs src/gateway/chat-abort.test.tsexits 0.node scripts/run-tsgo.mjs -p tsconfig.core.jsonexits 0.