🛬 fix: Settle Background Tool Results Before the Server Shuts Down - #16365
danny-avila wants to merge 1 commit into
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d5d330f240
ℹ️ 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".
| return false; | ||
| } | ||
| })(); | ||
| void durableReceiptWrite.then(resolveDurableReceipt); |
There was a problem hiding this comment.
When persistResult legitimately resolves false (for example, the projection-only fallback covered by handlers.background.spec.ts), this unconditionally resolves durableReceiptSettled, so trackShutdown removes the handle while backgroundToolCompletion.persist may still be writing the only durable copy. A shutdown can then see no tracked task and exit before that projection completes; the flush path similarly treats the same false result as success. Only resolve the durable marker when the write returns true, otherwise keep waiting for the full settlement/fallback path.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
| registerShutdownTask('background task admission', () => registry.closeAdmission(), { | ||
| phase: 'pre-drain', | ||
| priority: 100, | ||
| }); |
There was a problem hiding this comment.
Both pre-drain tasks use priority 100, but ties preserve registration order, and importing api/server/services/Agents/triggers.js constructs the trigger service and registers its stop task before startServer() registers this one. Consequently shutdown first sets deliveryReady = false and may await the delivery engine while admission remains open; an in-flight HTTP generation can dispatch a background tool in that interval, fail preregistration, and become poll-only, so the subsequent restart loses its result. Give admission closure a higher priority so it actually precedes delivery shutdown as intended.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
| const getClusterShutdownBudgetMs = () => { | ||
| const remaining = getRemainingShutdownMs(); | ||
| const elapsed = getShutdownElapsedMs(); | ||
| if (remaining == null || elapsed == null) { | ||
| return GenerationJobManager.destroy(); | ||
| return null; |
There was a problem hiding this comment.
This new helper implements shutdown policy in api/server/experimental.js: it branches on coordinator state, chooses between the primary deadline and an elapsed-time fallback, and combines budgets. That is backend behavior rather than entrypoint wiring, and it leaves the clustered path dependent on untyped CJS logic while the reusable shutdown implementation lives in packages/api; move this calculation behind a TypeScript API and keep the server file to supplying inputs and registering the call.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Summary
Restarting the API loses every background tool task that is still running or still persisting its result. Graceful shutdown drains generations, but nothing drains background tasks: their abort signal is not tied to anything shutdown aborts, and they die with the process. Their automatic completion deliveries then dead-letter as
BACKGROUND_TOOL_PRODUCER_LOSTonce the producer lease expires, which cannot be retried, so the agent never learns what happened. On the demo, one routine config restart lost five in-flight results this way.There is a second gap: once shutdown begins, even a task that finishes on its own cannot record its result.
renewProducerLeaseandpersistBackgroundToolResultgo throughrequireMethods(), which throws as soon as shutdown starts, so a completed result is reported as an ambiguous write failure and also dead-letters later.This PR drains background tasks within the shutdown budget:
shutting_downrejection that tells the model to run the call in the foreground or retry after the restart. Replays of an existing task and launches that already hold a capacity permit still resolve.retire,purgeUser).endpoints.agents.backgroundTasks.shutdownInterruptGraceMs(default 5000, 0 to 60000) sets that grace.Clustered workers measure the drain against the primary's force-exit deadline, the same budget the generation job manager already uses.
How it works
Each task registers a shutdown handle with the registry when it is dispatched. The durable receipt write is now a single flight per task, so a shutdown flush and the task's own late settlement share the first write: neither can store a contradicting receipt, and
retireFailedPersistencenever retires a delivery that already holds one.Code tasks write their receipt only after the file harvest, which can wait on the dispatch turn. The flush covers that case by writing the already settled result directly.
Type of change
Testing
Automated tests:
background.shutdown.spec.ts(new):handlers.shutdown.spec.ts(new), through the real tool execute handler:service.delivery.spec.ts: lease renewal and receipt writes still work after the delivery engine stops, whileenqueuestays refused.config.spec.ts: the new default, a configured value, and out-of-range values.index.spec.js,experimental.spec.js: the drain is registered at startup, and cluster workers share the primary's deadline with the generation job manager.handlers.background.spec.ts,background.spec.ts,backgroundCompletionWakeup.spec.ts,service.delivery.spec.ts(290 passing together).npx tsc --noEmitinpackages/api;node scripts/static-checks.mts --full --against origin/dev(TypeScript and config migration tests included) passes.Not yet run: a live restart of a server with a running background task. That is the next check.
Screenshots / recordings
No user-facing change beyond the delivered error text for an interrupted task.
Risk / compatibility
librechat.yamlfiles are unaffected.Checklist