fix: log terminal session persistence failures#97839
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 5:05 PM ET / 21:05 UTC. Summary PR surface: Source +4, Tests +12. Total +16 across 2 files. Reproducibility: yes. source-reproducible. Current main catches rejected terminal lifecycle persistence only to broadcast fallback state, and the PR turns that rejected promise into an asserted log call. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this owner-boundary logging fix once required CI is green, then let it close #97795. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible. Current main catches rejected terminal lifecycle persistence only to broadcast fallback state, and the PR turns that rejected promise into an asserted log call. Is this the best way to solve the issue? Yes. Logging once in createAgentEventHandler at the persistence owner boundary with formatForLog(err) is narrower and safer than adding downstream chat-abort cleanup logs that could miss or duplicate the shared promise failure. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1755a9dc5bce. Label changesLabel justifications:
Evidence reviewedPR surface: Source +4, Tests +12. Total +16 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review I added the requested real logger proof to the PR Evidence section. It shows the gateway subsystem logger emitting the exact terminal lifecycle persistence failure message, paired with the focused handler test that drives the rejected persistence path and asserts the same |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Addressed the latest review:
Latest local proof:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
f3abe13 to
f6960df
Compare
f6960df to
715a994
Compare
|
Land-ready verification:
Known gap: I did not recreate the Windows filesystem lock locally; the contributor's production trace covers that platform-specific trigger, while local tests and the real logger cover the changed owner and output path. |
|
Merged via squash.
|
* fix: log terminal session persistence failures * fix: format terminal persistence error logs * fix(gateway): identify failed terminal persistence --------- Co-authored-by: Peter Steinberger <[email protected]>
The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since openclaw#97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection.
The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since #97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection.
The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since #97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection.
* fix(cron): reject sub-millisecond durations * fix(skill-workshop): preserve proposal terminal newline Preserve proposal_content exactly at the agent tool boundary and make renderProposalMarkdown defensively emit a terminal newline. Add focused regressions for the tool write path and markdown renderer. * fix(skill-workshop): reject blank raw proposal content * fix: treat empty-string optional integer tool params as unset Optional positive-integer tool params (e.g. Telegram replyTo/threadId) threw ToolInputError when a tool-calling model populated them with an empty-string or whitespace-only default. Those defaults carry no value, so readPositiveIntegerParam/readNonNegativeIntegerParam now treat a blank string as unset (undefined) instead of throwing, while still rejecting genuinely invalid present values (0, "42.5", "-3"). This prevents silent message-delivery failures when models emit empty routing-param defaults. Adds unit tests covering blank vs invalid. * fix(gateway): log start session persistence failures The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since #97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection. * fix(memory): report close-time pending work failures * fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice sliceUtf16Safe silently swapped reversed bounds (to < from) instead of returning "" like String.prototype.slice, creating a subtle footgun for callers with dynamic start/end pairs. Caller scan across src/, extensions/, and packages/ confirmed no production code relies on the old swap behavior — all callers use (text, 0, N) or (text, -N) forms only. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(plugins): require plugin manifest in npm verifier * fix(android): propagate CancellationException in CameraHandler catch blocks Catch (err: Throwable) swallows kotlinx.coroutines.CancellationException, breaking structured concurrency when the coroutine scope is cancelled during camera operations (handleList/handleSnap/handleClip). Add CancellationException rethrow before each Throwable catch to match the existing pattern used in GatewaySession and TalkModeManager. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix(android): propagate CancellationException past GatewaySession invoke boundary * chore: sync native i18n inventory after gateway session line shift * fix(agents): prevent native hook relay bridge race condition on renew and registration Remove synchronous bridge record write after server.listen() that races before the TCP server binds, and guard renew handler with server.listening check to prevent stale relay registrations. Closes #98650 * fix: harden small reliability fixes Co-authored-by: cxbAsDev <[email protected]> * docs(agents): explain buffered LSP spawn failures * docs(agents): clarify LSP spawn timeout invariant --------- Co-authored-by: qingminlong <[email protected]> Co-authored-by: anyech <[email protected]> Co-authored-by: snotty <[email protected]> Co-authored-by: masatohoshino <[email protected]> Co-authored-by: lin-hongkuan <[email protected]> Co-authored-by: simon-w <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: 宇宙熊Yzx <[email protected]> Co-authored-by: xialonglee <[email protected]> Co-authored-by: nankingjing <[email protected]> Co-authored-by: cxbAsDev <[email protected]>
* fix(cron): reject sub-millisecond durations * fix(skill-workshop): preserve proposal terminal newline Preserve proposal_content exactly at the agent tool boundary and make renderProposalMarkdown defensively emit a terminal newline. Add focused regressions for the tool write path and markdown renderer. * fix(skill-workshop): reject blank raw proposal content * fix: treat empty-string optional integer tool params as unset Optional positive-integer tool params (e.g. Telegram replyTo/threadId) threw ToolInputError when a tool-calling model populated them with an empty-string or whitespace-only default. Those defaults carry no value, so readPositiveIntegerParam/readNonNegativeIntegerParam now treat a blank string as unset (undefined) instead of throwing, while still rejecting genuinely invalid present values (0, "42.5", "-3"). This prevents silent message-delivery failures when models emit empty routing-param defaults. Adds unit tests covering blank vs invalid. * fix(gateway): log start session persistence failures The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since openclaw#97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection. * fix(memory): report close-time pending work failures * fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice sliceUtf16Safe silently swapped reversed bounds (to < from) instead of returning "" like String.prototype.slice, creating a subtle footgun for callers with dynamic start/end pairs. Caller scan across src/, extensions/, and packages/ confirmed no production code relies on the old swap behavior — all callers use (text, 0, N) or (text, -N) forms only. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(plugins): require plugin manifest in npm verifier * fix(android): propagate CancellationException in CameraHandler catch blocks Catch (err: Throwable) swallows kotlinx.coroutines.CancellationException, breaking structured concurrency when the coroutine scope is cancelled during camera operations (handleList/handleSnap/handleClip). Add CancellationException rethrow before each Throwable catch to match the existing pattern used in GatewaySession and TalkModeManager. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix(android): propagate CancellationException past GatewaySession invoke boundary * chore: sync native i18n inventory after gateway session line shift * fix(agents): prevent native hook relay bridge race condition on renew and registration Remove synchronous bridge record write after server.listen() that races before the TCP server binds, and guard renew handler with server.listening check to prevent stale relay registrations. Closes openclaw#98650 * fix: harden small reliability fixes Co-authored-by: cxbAsDev <[email protected]> * docs(agents): explain buffered LSP spawn failures * docs(agents): clarify LSP spawn timeout invariant --------- Co-authored-by: qingminlong <[email protected]> Co-authored-by: anyech <[email protected]> Co-authored-by: snotty <[email protected]> Co-authored-by: masatohoshino <[email protected]> Co-authored-by: lin-hongkuan <[email protected]> Co-authored-by: simon-w <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: 宇宙熊Yzx <[email protected]> Co-authored-by: xialonglee <[email protected]> Co-authored-by: nankingjing <[email protected]> Co-authored-by: cxbAsDev <[email protected]>
Fixes #97795
What Problem This Solves
Fixes an issue where operators would not see any diagnostic output when a terminal session lifecycle persistence write failed during gateway run completion. The gateway still broadcast a fallback terminal snapshot, but the persistence failure itself was silent.
Why This Change Was Made
The terminal lifecycle persistence failure path now logs the rejected write at the gateway event handling boundary before preserving the existing fallback broadcast behavior. The rejection is formatted through the existing gateway
formatForLog(err)helper so arbitrary error text is redacted and capped before it reaches operator-visible logs.User Impact
Operators get an actionable gateway error log when terminal session persistence fails, making session-state storage problems easier to diagnose without changing normal chat or abort behavior.
Evidence
RED:
node scripts/run-vitest.mjs run --config test/vitest/vitest.gateway.config.ts src/gateway/server-chat.agent-events.test.tsfailed before the implementation becauselogErrorwas not called.GREEN:
Focused related proof after the formatter update:
git diff --checkpassed.Real runtime persistence rejection proof after the formatter update:
A temporary
node --import tsxscript seeded a realOPENCLAW_STATE_DIRwithstate/agents/main/sessions/sessions.json, denied writes to that sessions directory withicacls, then invokedcreateAgentEventHandlerwith the realpersistGatewaySessionLifecycleEventpath. The script was not committed and used synthetic ids only.Formatter safety proof:
The focused handler test now rejects with
Error("disk full sk-abcdefghijklmnopqrstuvwxyz123456")and asserts the gateway log contains the formatted/redacted valuesk-abc…3456, proving the new catch usesformatForLog(err)instead of rawString(err).Autoreview note: attempted
python .agents\skills\autoreview\scripts\autoreview --mode local, but the helper failed before review on Windows path handling withNotADirectoryError: [WinError 267]in this non-ASCII worktree path, even when retried through asubstdrive.