fix: persist pending followup queues across gateway restart#51623
fix: persist pending followup queues across gateway restart#51623tomsalphaclawbot wants to merge 2 commits into
Conversation
Before this change, in-memory followup queue items (FollowupQueueState) were silently lost when the gateway restarted. Messages arriving during the drain window hit GatewayDrainingError and were dropped with no persistence or replay mechanism. This adds: 1. Queue persistence (persist.ts): Before shutdown, serialize non-empty followup queues to state/pending-messages.json. Strips non-portable fields (config, skillsSnapshot) and includes a version marker. 2. Post-restart replay (server-pending-messages.ts): On startup, read the persisted file, inject the missed messages as system events into their respective sessions, and delete the file. Includes a 5-minute staleness guard to avoid replaying ancient messages. 3. Close handler integration: persistFollowupQueues() is called before chatRunState.clear() in server-close.ts so queue state is captured while still available. 4. Startup integration: replayPersistedPendingMessages() runs 1.5s after startup (after channels initialize) alongside the existing restart sentinel wake. Tests: 16 new tests covering persist, consume, round-trip, staleness, corruption handling, and system event injection. Closes openclaw#51620
Greptile SummaryThis PR implements Phase 1 of crash-safe followup-queue persistence: on graceful shutdown the in-memory The architecture is clean and fits well with the existing Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/queue/persist.ts
Line: 43-47
Comment:
**`config: undefined as never` silently drops a required field**
Setting `config: undefined as never` suppresses the TypeScript error at compile time, but the JSON round-trip will silently omit the `config` key entirely (since `JSON.stringify` drops `undefined` values). Any Phase 2 code that tries to re-enqueue a replayed `FollowupRun` and access `item.run.config` will get a runtime crash rather than a type error.
A safer approach is to give the persisted form an explicit type that omits the stripped fields, rather than using `as never` to paper over the type mismatch:
```ts
export type PersistedFollowupRun = Omit<FollowupRun, 'run'> & {
run: Omit<FollowupRun['run'], 'config' | 'skillsSnapshot'>;
};
```
Then use `PersistedFollowupRun[]` inside `PersistedQueueEntry.items` so callers cannot accidentally treat replayed entries as full `FollowupRun` values without re-attaching a live `config`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/queue/persist.ts
Line: 105-117
Comment:
**`persistedAt` type not validated — staleness guard can be bypassed**
`parsed.persistedAt` is cast with `as PersistedQueueFile` but never validated to be an actual number. If the field is absent or is a non-numeric value (e.g., a string), `Date.now() - parsed.persistedAt` produces `NaN`, and `NaN > MAX_AGE_MS` evaluates to `false`, silently skipping the staleness check and potentially replaying arbitrarily old messages.
Consider adding a type guard alongside the existing structural check:
```suggestion
if (!parsed || parsed.version !== 1 || !Array.isArray(parsed.entries) || typeof parsed.persistedAt !== "number") {
log.warn("pending-messages.json has unexpected format; discarding");
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: persist pending..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ...item.run, | ||
| // Config object is too large and non-portable across restarts; | ||
| // it will be re-resolved from the live config on replay. | ||
| config: undefined as never, | ||
| skillsSnapshot: undefined, |
There was a problem hiding this comment.
config: undefined as never silently drops a required field
Setting config: undefined as never suppresses the TypeScript error at compile time, but the JSON round-trip will silently omit the config key entirely (since JSON.stringify drops undefined values). Any Phase 2 code that tries to re-enqueue a replayed FollowupRun and access item.run.config will get a runtime crash rather than a type error.
A safer approach is to give the persisted form an explicit type that omits the stripped fields, rather than using as never to paper over the type mismatch:
export type PersistedFollowupRun = Omit<FollowupRun, 'run'> & {
run: Omit<FollowupRun['run'], 'config' | 'skillsSnapshot'>;
};Then use PersistedFollowupRun[] inside PersistedQueueEntry.items so callers cannot accidentally treat replayed entries as full FollowupRun values without re-attaching a live config.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/queue/persist.ts
Line: 43-47
Comment:
**`config: undefined as never` silently drops a required field**
Setting `config: undefined as never` suppresses the TypeScript error at compile time, but the JSON round-trip will silently omit the `config` key entirely (since `JSON.stringify` drops `undefined` values). Any Phase 2 code that tries to re-enqueue a replayed `FollowupRun` and access `item.run.config` will get a runtime crash rather than a type error.
A safer approach is to give the persisted form an explicit type that omits the stripped fields, rather than using `as never` to paper over the type mismatch:
```ts
export type PersistedFollowupRun = Omit<FollowupRun, 'run'> & {
run: Omit<FollowupRun['run'], 'config' | 'skillsSnapshot'>;
};
```
Then use `PersistedFollowupRun[]` inside `PersistedQueueEntry.items` so callers cannot accidentally treat replayed entries as full `FollowupRun` values without re-attaching a live `config`.
How can I resolve this? If you propose a fix, please make it concise.| if (!parsed || parsed.version !== 1 || !Array.isArray(parsed.entries)) { | ||
| log.warn("pending-messages.json has unexpected format; discarding"); | ||
| return null; | ||
| } | ||
|
|
||
| // Discard entries older than 5 minutes (messages may be stale) | ||
| const MAX_AGE_MS = 5 * 60 * 1000; | ||
| if (Date.now() - parsed.persistedAt > MAX_AGE_MS) { | ||
| log.info( | ||
| `pending-messages.json is ${Math.round((Date.now() - parsed.persistedAt) / 1000)}s old; discarding as stale`, | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
persistedAt type not validated — staleness guard can be bypassed
parsed.persistedAt is cast with as PersistedQueueFile but never validated to be an actual number. If the field is absent or is a non-numeric value (e.g., a string), Date.now() - parsed.persistedAt produces NaN, and NaN > MAX_AGE_MS evaluates to false, silently skipping the staleness check and potentially replaying arbitrarily old messages.
Consider adding a type guard alongside the existing structural check:
| if (!parsed || parsed.version !== 1 || !Array.isArray(parsed.entries)) { | |
| log.warn("pending-messages.json has unexpected format; discarding"); | |
| return null; | |
| } | |
| // Discard entries older than 5 minutes (messages may be stale) | |
| const MAX_AGE_MS = 5 * 60 * 1000; | |
| if (Date.now() - parsed.persistedAt > MAX_AGE_MS) { | |
| log.info( | |
| `pending-messages.json is ${Math.round((Date.now() - parsed.persistedAt) / 1000)}s old; discarding as stale`, | |
| ); | |
| return null; | |
| } | |
| if (!parsed || parsed.version !== 1 || !Array.isArray(parsed.entries) || typeof parsed.persistedAt !== "number") { | |
| log.warn("pending-messages.json has unexpected format; discarding"); | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/queue/persist.ts
Line: 105-117
Comment:
**`persistedAt` type not validated — staleness guard can be bypassed**
`parsed.persistedAt` is cast with `as PersistedQueueFile` but never validated to be an actual number. If the field is absent or is a non-numeric value (e.g., a string), `Date.now() - parsed.persistedAt` produces `NaN`, and `NaN > MAX_AGE_MS` evaluates to `false`, silently skipping the staleness check and potentially replaying arbitrarily old messages.
Consider adding a type guard alongside the existing structural check:
```suggestion
if (!parsed || parsed.version !== 1 || !Array.isArray(parsed.entries) || typeof parsed.persistedAt !== "number") {
log.warn("pending-messages.json has unexpected format; discarding");
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c979792f40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // so queued messages survive restart and can be replayed on next startup. | ||
| // This is best-effort — if persistence fails, messages are still lost | ||
| // (same as today) but now we at least attempt to save them. | ||
| await persistFollowupQueues(FOLLOWUP_QUEUES).catch(() => {}); |
There was a problem hiding this comment.
After persisting FOLLOWUP_QUEUES, shutdown leaves the in-memory map intact. In the in-process restart path (src/cli/gateway-cli/run-loop.ts falls back to restart without process exit), those same queue items remain live and can still drain, while startup also replays the just-written snapshot from disk, so the same pending messages can be processed/reported twice. Clear the followup queue state after a successful persist to avoid duplicate recovery behavior on in-process restarts.
Useful? React with 👍 / 👎.
| const MAX_AGE_MS = 5 * 60 * 1000; | ||
| if (Date.now() - parsed.persistedAt > MAX_AGE_MS) { | ||
| log.info( |
There was a problem hiding this comment.
The replay loader unconditionally discards persisted queues older than 5 minutes, which reintroduces message loss whenever restart downtime exceeds that window (for example, manual downtime, slow reboot, or repeated failed restarts). Since this feature is intended to preserve queued messages across restarts, this fixed TTL makes recovery unreliable and should be removed or made configurable with a much safer default.
Useful? React with 👍 / 👎.
- test/restart-persistence.test.ts: 3 integration tests that exercise the full persist → consume → replay pipeline with realistic multi-session multi-channel scenarios, staleness guard, and corruption handling - test/prove-restart-persistence.sh: standalone bash/node script that proves the mechanism end-to-end against the compiled build, with human-readable step-by-step output
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2324f0d2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const text = item.prompt.length > 500 ? `${item.prompt.slice(0, 500)}…` : item.prompt; | ||
| return `${i + 1}. [${channel}] from ${sender}: ${text}`; |
There was a problem hiding this comment.
This code places item.prompt (which originates from user messages in queued followups) directly into a system event, and those events are later emitted as trusted System: lines on the next turn. After a restart, a crafted queued message can therefore be elevated from untrusted user input into system-priority instructions, bypassing the normal System (untrusted) protection path and potentially changing model behavior; replay text should be escaped/quoted as untrusted content instead of injected as trusted system context.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 8:24 AM ET / 12:24 UTC. Summary PR surface: Source +200, Tests +819. Total +1019 across 8 files. Reproducibility: yes. source-level: current main keeps accepted followup queues in a process-global map and has no durable followup replay path, so a full process restart can drop queued items; no destructive live channel proof was run in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land or sponsor a rebased SQLite/doctor-backed followup recovery path that restores via normal or explicitly untrusted replay semantics, proves exactly-once delivery in a real channel restart, and leaves the canonical issue open until merged. Do we have a high-confidence way to reproduce the issue? Yes, source-level: current main keeps accepted followup queues in a process-global map and has no durable followup replay path, so a full process restart can drop queued items; no destructive live channel proof was run in this read-only review. Is this the best way to solve the issue? No: persisting queued followups is the right bug boundary, but this branch uses a JSON sidecar, trusted system-event replay, an obsolete startup hook, duplicate-prone in-process restart handling, and a fixed TTL. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f786efddcd88. Label changesLabel justifications:
Evidence reviewedPR surface: Source +200, Tests +819. Total +1019 across 8 files. View PR surface stats
Security concerns:
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
Review history (5 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
removed
the
rating: 🧂 unranked krab
clawsweeper
Bot
added
status: 📣 needs proof
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
|
Thanks @tomsalphaclawbot. Closing this stale implementation branch, not the restart-recovery problem; #51620 and the newer #82572 remain open. This head depends on obsolete startup behavior and a JSON sidecar design that can replay untrusted or duplicate events, retarget sessions, and lose messages. A replacement needs the current queue/session model plus restart and duplicate-delivery proof. |
2 participants
Summary
Fixes #51620 — gateway restart drops queued and in-flight inbound messages.
Changes
New:
src/auto-reply/reply/queue/persist.tsSerializes all non-empty
FOLLOWUP_QUEUESentries tostate/pending-messages.jsonbefore shutdown. Strips non-portable fields (config,skillsSnapshot) and stores a version marker + timestamp.New:
src/gateway/server-pending-messages.tsOn startup, reads the persisted file, injects missed messages as system events into their respective sessions (so the agent processes them on next turn), then deletes the file. Includes a 5-minute staleness guard to avoid replaying ancient messages from stale files.
Modified:
src/gateway/server-close.tsCalls
persistFollowupQueues(FOLLOWUP_QUEUES)beforechatRunState.clear()so queue state is captured while still available. Best-effort — failure doesn't block shutdown.Modified:
src/gateway/server-startup.tsCalls
replayPersistedPendingMessages()1.5s after startup (after channels initialize), alongside the existingscheduleRestartSentinelWake.Tests
16 new tests covering:
persistFollowupQueues: empty queues, non-empty queues, config stripping, multiple queuesconsumePersistedQueues: no file, read+delete, staleness guard (>5min), corrupt file, wrong versionreplayPersistedPendingMessages: no entries, empty entries, system event injection, multiple queues, prompt truncation, empty items skipLimitations / Follow-up
This PR covers Phase 1: persisting in-memory followup queue items across restart.
Still not covered (follow-up work):
GatewayDrainingError) — these are rejected at the channel level before reaching the queueTesting
All 16 tests pass.
Upstream issue: #51620