fix(gateway): durably hand off restart continuations#70780
Conversation
Greptile SummaryThis PR introduces a durable file-based session-delivery queue for restart continuations, replacing the previous atomic Confidence Score: 5/5Safe to merge — all findings are P2 style and cleanup suggestions with no correctness impact. The core durability guarantee (enqueue before sentinel deletion, crash recovery on next startup) is implemented correctly. Idempotency, backoff, ack/fail bookkeeping, and the fallback session-wake path all behave as intended per the tests. The three flagged items are a minor orphaned-.tmp accumulation concern, an inconsistent .catch(() => {}) vs. narrow ENOENT catch for moveSessionDeliveryToFailed, and a repeated pure-function call — none of which affect correctness or the primary user path. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/session-delivery-queue-storage.ts
Line: 74-81
Comment:
**Orphaned `.tmp` files not cleaned up on crash**
`writeQueueEntry` writes to `${filePath}.${process.pid}.tmp` before renaming. If the process crashes between the `writeFile` and `rename` calls, the `.tmp` file is left on disk indefinitely — `loadPendingSessionDeliveries` only sweeps `.delivered` files, not `.tmp` files. Over repeated crashy restarts (the primary scenario for this queue), these orphaned temporaries accumulate in the queue directory without any cleanup path.
Consider adding a `.tmp` sweep to `loadPendingSessionDeliveries`, or using a suffix pattern that the existing cleanup loop already covers.
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/infra/session-delivery-queue-recovery.ts
Line: 226-229
Comment:
**Inconsistent error handling for `moveSessionDeliveryToFailed`**
`drainPendingSessionDeliveries` (line 157–163) catches only `ENOENT` from `moveSessionDeliveryToFailed` and rethrows anything else, making I/O errors visible. Here, `.catch(() => {})` silently swallows all errors. If a persistent I/O failure (e.g., a permissions issue on the `failed/` subdirectory) prevents the move, the entry stays in queue at max retries, `skippedMaxRetries` is incremented without any warning, and the silent failure repeats on every subsequent recovery pass. Narrowing the catch to `ENOENT` (consistent with `drainPendingSessionDeliveries`) would surface real filesystem problems without breaking the happy path.
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/gateway/server-restart-sentinel.ts
Line: 217-244
Comment:
**`resolveQueuedSessionDeliveryContext` called twice per branch**
In both the `systemEvent` branch (lines 220–226) and the routeless `agentTurn` fallback (lines 235–241), `resolveQueuedSessionDeliveryContext(params.entry)` is invoked twice — once in the truthiness check and once to spread the result. The function is pure and cheap, so this is not a correctness concern, but capturing the result in a local variable once per branch would avoid the redundant call and make the conditional clearer.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(gateway): durably hand off restart c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: aa201ee772
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99376bc05c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 233e1eaddb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e665930 to
f7550b1
Compare
obviyus
left a comment
obviyus
left a comment
There was a problem hiding this comment.
Verified the restart-sentinel loss window: continuations are durably queued before sentinel deletion, and stale queued work recovers after crashy restarts.
Maintainer follow-up bounded startup recovery to pre-boot queue entries, moved the changelog note to the active Unreleased fixes block, and resolved the current-head review thread.
Local gate: targeted restart/queue tests, pnpm check, pnpm build, and manual linked-CLI sentinel/queue smoke.
|
Landed on main. Thanks @fuller-stack-dev. |
2 participants
Summary
flowchart TD A[gateway.restart writes restart sentinel] --> B[gateway boots] B --> C[startup reads sentinel] C --> D[enqueue continuation in durable session queue] D --> E[delete restart sentinel] E --> F[send restart notice] F --> G[drain queued continuation] B --> H[startup recovery scans older queued entries only] H --> I[replay pre-existing stuck continuations]Testing
pnpm test src/gateway/server-restart-sentinel.test.ts src/gateway/server-runtime-services.test.ts src/infra/session-delivery-queue.storage.test.ts src/infra/session-delivery-queue.recovery.test.tspnpm buildgit merge main-> already up to dateNotes
upstream/main, fast-forwarded localmainto the same SHA, and opened the PR from that branch.pnpm check:changed --stagedpath still fails in clean upstream worktrees on unrelated infra tests (src/infra/git-commit.test.ts,src/infra/host-env-security.test.ts,src/infra/run-node.test.ts,src/infra/update-check.test.ts) that inspect git/worktree behavior. I used the repo'sFAST_COMMITescape hatch only after the targeted restart-continuation tests and full build passed.