🧷 fix: Wait for Long Dispatch Turns Before Giving Up Background Code Results - #16345
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: d08397739c
ℹ️ 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".
| ...(input.user != null && { user: input.user }), | ||
| status: { $in: ['pending', 'capability_pending', 'leased', 'capability_leased'] }, | ||
| }, | ||
| [ |
There was a problem hiding this comment.
On supported Amazon DocumentDB 5.0 deployments, this array-valued updateMany argument is an aggregation-pipeline update and also uses $$REMOVE, both of which DocumentDB rejects. As a result, every result- or settlement-driven expedite falls into the warning path and leaves completion delivery waiting for its periodic backoff (and the repository's DocumentDB compatibility test rejects this construct); use classic update operators or a guarded CAS instead.
Useful? React with 👍 / 👎.
…d announce store-won approval expiry
…nto danny-avila/bg-code-receipt
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6055cfe433
ℹ️ 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".
| onFilesPersisted: (attachments) => | ||
| backgroundTaskRegistry.finishHarvest( | ||
| backgroundUserId, | ||
| backgroundConversationId, | ||
| task.id, | ||
| attachments, |
There was a problem hiding this comment.
When retained payload crosses the per-user or global memory cap during a long dispatch turn, this early finishHarvest clears harvestPending before persistBackgroundCodeResult finishes. That makes the task eligible for settledCandidates eviction while the row patch may still be waiting for hours; subsequent polling then loses the task and its files, and resolveBackgroundTask can no longer observe claim state for the final patch. Store the attachments without clearing the pending protection, or hold completionPersistencePending until row persistence completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3372bae: completionPersistencePending (which excludes a task from retention eviction) is now held for the whole settled-result persist on the code path too, not only for ordinary tools. So releasing the files early no longer exposes the task to eviction while the row patch waits. The long-turn handler test asserts the flag is still set during the wait.
There was a problem hiding this comment.
Fixed at a4c69b8. The existing completionPersistencePending capacity protection is now also respected by both the task TTL and idle-bucket TTL. Finishing persistence releases protection on all outcomes, including policy-blocked artifacts. Registry regressions verify results and manual claim state survive a seven-hour wait and expire after protection is released.
|
|
||
| const ACTIVE_STATUSES: ReadonlySet<GenerationJobStatus> = new Set(['running', 'requires_action']); | ||
| const DEFAULT_RECHECK_MS = 60_000; | ||
| const DEFAULT_MAX_WAIT_MS = 24 * 60 * 60 * 1_000; |
There was a problem hiding this comment.
The production wrapper supplies no maxWaitMs, so a dispatch generation that remains active for more than 24 hours makes this waiter return false; the harvest then performs only its short settled retries and retires automatic delivery, recreating the lost-result behavior this change is intended to prevent. This newly introduced timeout should be wired through configSchema with the current value as its default rather than fixed in the module.
AGENTS.md reference: AGENTS.md:L77-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3372bae by removing the cutoff rather than adding a lever. The waiter now has no default maximum: it waits only while the generation is active, and the settle event or the status re-read (every 60 s, which also catches a crashed or reaped job) ends it. A turn longer than 24 hours can no longer turn into a lost result. maxWaitMs stays as an optional test hook.
| unsubscribeGenerationSettled ??= deps.subscribeGenerationSettled?.(({ userId }) => | ||
| expediteCompletions({ user: userId }), |
There was a problem hiding this comment.
When one generation settles, this discards its conversationId and expedites every background and subagent completion delivery belonging to the user. With many concurrent tasks, each child settlement pulls all still-running siblings back to now; their resolvers perform conversation/message reads, discover they are not ready, and defer again, so a fan-out of N tasks creates roughly quadratic claim and database traffic despite the new backoff. Carry the settled conversation or another precise readiness key into the store selection instead of waking the entire principal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #16339 (d9f1ab5, merged here at 06d27d0): a settled generation expedites only deliveries that resume its own conversation (envelope.target.conversationId), and a settled subagent child expedites only its parent conversation's. Sibling fan-out in other conversations is no longer pulled forward.
… remaining signal gaps
…nto danny-avila/bg-code-receipt
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0331ec7700
ℹ️ 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".
| const status = await source.getJobStatus(conversationId); | ||
| if (status == null || !ACTIVE_STATUSES.has(status)) { | ||
| finish(!initial); |
There was a problem hiding this comment.
When the retry schedule expires after claimTerminalJob has changed the job status to complete/aborted but before the terminal response save finishes, this check treats the generation as already settled and unsubscribes even though terminalPersistencePending is still true. The caller then performs only the 1 s and 5 s settled retries, so a terminal save taking longer than that can still cause the background result to be retired just before its message row becomes patchable. Read the pending-persistence state, or wait for the actual settled event, rather than using terminal status alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the post-settle retries now run 1 s, 5 s, 15 s and 30 s, outlasting the job manager's 45-second terminal-persistence bound (waitForGenerationPersistence). A status re-read that sees a terminal job whose final save is still running no longer gives the result up. The settle event itself fires only after persistence is final. Covered by a test where the row stays unfinished for 40 s after settlement and is still anchored.
There was a problem hiding this comment.
Fixed in a4c69b8. The new read-only generation-state reader includes terminalPersistencePending. The waiter retains its subscription and retries while the matching epoch owns a terminal save; local events request a fresh state read rather than bypassing the flag. Final patch delays no longer claim to bound persistence. harvest.lifecycle.spec.ts composes the real waiter/harvester with GenerationJobManagerClass and holds beforeErrorPublication open beyond two minutes, proving the result is not retired before the save finishes. The composed and focused tests pass.
| subagentType: request.subagentType, | ||
| createdAt: task.createdAt, | ||
| }); | ||
| this.wakeupTaskIds.add(task.taskId); |
There was a problem hiding this comment.
When prepareThread returns an already-terminal replay, this ID is added here but execution returns through the prepared.replay branch without calling touchAfterMessage, which is the only place that deletes it. The same leak occurs when failure or cancellation persistence returns early because the child conversation/input disappeared. A long-running server therefore retains one string forever for every such replayed or deleted completion task; clear the registration from the execution cleanup path, or avoid adding IDs for terminal replays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| const released = this.releaseJobOwnership(streamId, job.createdAt); | ||
| if (released) { | ||
| /** The store won the expiry CAS, so no local claim announced it. */ | ||
| this.notifyGenerationSettled({ |
There was a problem hiding this comment.
When this manager wins an approval-expiry CAS, expireApproval already emits a settlement notification but leaves the generation in ownedJobs. On the next expiry sweep the same aborted job reaches this branch, releaseJobOwnership returns true, and the notification is emitted a second time. Paused generations commonly have no active controller left to release ownership in between, so listeners such as completion expediting run twice despite the new notification contract requiring one event per generation; release ownership when the local expiry is announced or record that it was already announced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
…nto danny-avila/bg-code-receipt
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb96b3b2f
ℹ️ 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".
| if (initial) { | ||
| finish(false); |
There was a problem hiding this comment.
If the initial status lookup fails transiently (for example, during a brief Redis outage while the dispatch generation is still running), this immediately unsubscribes from settlement events and returns false. The harvest then performs only its roughly 51 seconds of post-settlement retries and can retire an unanchored result even though the multi-hour dispatch turn is still active; keep the waiter subscribed and let the periodic status checks recover from the initial read failure just as they do from later failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at a4c69b8. Initial and later read errors now keep waiting and retry. Single-flight state reads and a composed harvest regression verify that an initial outage cannot consume the long-turn protection.
| const status = await source.getJobStatus(conversationId); | ||
| if (status == null || !ACTIVE_STATUSES.has(status)) { | ||
| finish(!initial); |
There was a problem hiding this comment.
In a multi-replica deployment, if the original generation settles remotely and a new turn starts in the same conversation before this process's 60-second recheck, this status-only read sees the successor as active and keeps waiting for it. The original message row is already patchable, but the harvest performs no further row updates while waiting, so a long or approval-paused successor can delay receipt persistence for hours; retain the initially observed generation epoch and finish when that epoch disappears or changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at a4c69b8. Initial and later read errors now keep waiting and retry. Single-flight state reads and a composed harvest regression verify that an initial outage cannot consume the long-turn protection.
| backgroundTaskRegistry.markCompletionPersistencePending( | ||
| backgroundUserId, | ||
| backgroundConversationId, |
There was a problem hiding this comment.
For a code result whose dispatch turn remains active for more than one hour, this flag prevents cap-based settledCandidates eviction but does not prevent sweepBucketTasks from unconditionally deleting every non-running task older than COMPLETED_TASK_TTL_MS. Any later poll or registry activity can therefore remove the result, files, claim state, and retirement handle while the persistence promise is still waiting, causing same-turn polling to report the task missing and potentially forgetting an earlier manual claim. Fresh evidence beyond the earlier cap-eviction thread is the unconditional TTL predicate in background.ts lines 835-839; that sweep must also honor completionPersistencePending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a4c69b8. Both sweepBucketTasks and whole idle-bucket eviction preserve completionPersistencePending. markCompletionPersistenceFinished clears protection and refreshes expiry even for policy-blocked tasks, without clearing their artifact guard. Registry regressions advance seven hours and then verify normal expiry resumes after success, failure, cancellation, and policy rejection, including retained result/claim state.
…backoff # Conflicts: # packages/api/src/agents/triggers/README.md # packages/api/src/agents/triggers/engine.spec.ts # packages/api/src/agents/triggers/service.delivery.spec.ts # packages/api/src/agents/triggers/service.ts # packages/data-schemas/src/methods/triggerDelivery.spec.ts # packages/data-schemas/src/schema/triggerDelivery.ts
…nto danny-avila/bg-code-receipt
…ceipt # Conflicts: # api/server/services/Endpoints/agents/subagentThreadStore.js # packages/api/src/agents/subagentThreads.spec.ts # packages/api/src/agents/subagentThreads.ts # packages/api/src/agents/triggers/README.md # packages/api/src/agents/triggers/engine.spec.ts # packages/api/src/agents/triggers/service.delivery.spec.ts # packages/api/src/agents/triggers/service.ts # packages/data-schemas/src/methods/triggerDelivery.spec.ts # packages/data-schemas/src/methods/triggerDelivery.ts
…-harvest-lifecycle
|
Review handoff for exact remote head The in-scope lifecycle findings are repaired:
Self-review followed producers and readers, same-turn claims, TTL and capacity eviction, failure/abort cleanup, save ownership, epoch replacement, scope isolation, and store-error/event interleavings. No persistence schema or migration is required. Verified: 345 focused API tests; 79 CJS wiring tests; API no-emit typecheck; scoped static checks; production frontend/backend builds. Lighthouse reached server readiness but Chromium could not launch due to missing libatk-1.0.so.0. Redis cross-replica integration, live DocumentDB, full suites, and model-driven Code API/load reproductions were not rerun. A maintainer needs to trigger Codex for this exact SHA. Lia GitHub App comments cannot trigger that review. CI and independent review of the new head are still pending. |
2 participants
Summary
A backgrounded code-execution result is given up for automatic delivery whenever the turn that dispatched it runs longer than about 16 minutes. The result is attached to the dispatch turn's tool call on its message row, and that row stays unfinished while the turn streams. The harvest retries the patch on a fixed backoff that ends after roughly 16 minutes. When it runs out, the code path treats the row as never persisted and retires the completion wake-up with
background code result was not persisted. Only a manual poll can then surface the result, so an agent that doesn't poll never sees it. Long coding turns are exactly where agents dispatch background code: on the demo deployment this produced 372 "Could not anchor code result… the dispatch turn never persisted" warnings in about 30 hours. Every dispatch message checked from those warnings existed and finished normally, after running 35 minutes to over 3 hours.The retry schedule also held the task's generated files back. The registry keeps a harvest "pending" until the harvest returns, and while it's pending a poll in the same turn gets the result text but not the files. So for the whole ~16-minute retry window, a same-turn poll could not deliver the files the harvest had already stored.
This PR waits for the dispatch turn instead of a timer. When the schedule runs out with the row still absent or unfinished, the harvest waits until that conversation's generation settles and then patches it once more, so the result is anchored, its durable receipt is written, and the wake-up delivers it. If no generation is running, or the turn ends without saving the tool call, the result is still given up as before. The task's producer heartbeat keeps renewing throughout, as it already does for the whole persistence step, so the waiting delivery is never mistaken for a lost executor. Generated files are released to the task as soon as they are stored, before the row patch, so same-turn polls deliver them immediately. The warning now says what actually happened.
The harvest also listens for its dispatch turn to settle from the start. The fixed schedule used to leave a finished result waiting for its next step after a short turn ended: minutes, once the gaps reach 2 to 5 minutes. Now a settle observed mid-schedule attaches the result at once. Only a positive settle cuts the schedule short. An unreadable store remains retryable, including the first read. A confirmed missing generation keeps the ordinary schedule when no dispatch epoch is known. The listener is released as soon as the result is attached, cancelled, or timed out. On the demo, a 3.5-minute turn with three background bash tasks showed that lag before this change.
Builds on merged #16339. The branch includes the current merged wakeup implementation; this PR changes code-result harvesting and its dispatch-generation wait.
How it works
The owning stream and generation epoch are passed from initialization. Nested child conversations do not inherit the parent's epoch; without a supplied epoch, the first successful state read pins the generation. The read-only state reader neither attaches runtime state nor forces stale-owner recovery of a slow terminal save. Events are hints to re-read, not permission to skip persistence. Store errors remain retryable, reads cannot overlap, and replacement/disappearance of the observed generation ends the wait.
The waiter handles already-aborted signals without allocating listeners or timers. Cancellation, optional deadlines, and successful settlement clear timers and abort/event subscriptions. Its timers do not keep the process alive. The bounded final row-patch attempts are a fallback after generation completion, not a claimed deadline on Mongo persistence.
While persistence is pending, both the settled-task TTL and idle-bucket TTL preserve the result, attachments, manual-claim state, and retirement handle. Finishing persistence releases this protection and refreshes the expiry clock, including failure, cancellation, and policy-blocked results without clearing their artifact guard. Capacity limits remain enforced.
Type of change
Testing
Reproduced end to end on a local LibreChat build against a local stateful Code API, with the anchor retry schedule shortened to 250/500/1000 ms in both builds so a one-minute turn outlives it the way a real turn outlives ~16 minutes. The agent dispatches
time.sleep(3); print('CODE-DONE-PRC')in the background, then runs a 60-second foreground task in the same turn, and never polls:background code result was not persistedCODE-DONE-PRC13 s after the turn endedThe first version of this change waited only for an existing unfinished row, and the run still failed. The dispatch turn hadn't saved its row yet (a turn writes it partway through), so a missing row has to wait too. The waiter still returns at once when no generation is running, so a row that truly never appears is given up promptly.
Original author reproduction environment (not rerun for the lifecycle repairs below):
USE_REDIS=true)stateful_code_sessionseventDriven.completionWakeups,durableReceipts,coalescing,actorMailboxopenai/gpt-6-lunaAutomated tests:
harvest.spec.ts: waits past the schedule for a running dispatch turn whose row is unfinished, or not written yet, then anchors; gives the result up when no generation is running and the row never appears, or when the turn ends without saving the tool call; keeps the bounded schedule when no settlement signal is wired; files are released before the first row patchsettled.spec.ts: no generation or a settled one resolves at once; the settle event for this conversation (not others) resolves it; an approval pause keeps waiting; a status re-read observes settlement on another replica; the maximum wait and an unreadable status give up; listeners are always removedhandlers.background.spec.ts: a same-turn poll receives stored files while the row patch is still waitingnpx tsc --noEmitinpackages/apiLifecycle-repair verification at
a4c69b823fnpx tsc --noEmitinpackages/apipassed. Scoped ESLint, Prettier, import order, package-manifest validation, and circular-dependency checks passed.npm run lighthousereached server readiness, but Chromium could not launch because the sandbox lackslibatk-1.0.so.0; no Lighthouse score is claimed.Screenshots / recordings
No user-facing change.
Risk / compatibility
A harvest for a long turn now stays in memory until that turn ends, instead of for about 16 minutes, and the task keeps its eviction protection (
completionPersistencePending) throughout: one pending promise per settled code task holding its output and attachment references, plus one status read a minute from the job store (Redis or in-memory, not MongoDB). The wake-up delivery waits inRESULT_NOT_READYmeanwhile, which #16339 makes cheap. A same-turn poll still claims the result directly and retires the wake-up, as today. Ordinary (non-code) background tools are unchanged, since their receipts never depended on the row.Checklist