Conversation
… dead-letters [#944] A recoverable provider error during a delivery-driven node-agent turn fires session.error before the message_delivery retry is scheduled. TaskAgentManager's registerCompletionCallback treated any session.error as terminal (marking the node execution blocked and tearing down listeners), so even when the delivery layer's automatic retry (PR #2471) succeeded, the workflow could no longer complete the node — one transient 5xx permanently blocked it. Now a recoverable session.error (details.recoverable === true, with delivery v2 enabled) is non-terminal: it arms a deliveryRetryPending flag, and the idle that follows it (the turn produced no result; a retry is pending) is suppressed rather than treated as completion. Only the dead-letter settlement (session.error with no details) or a genuinely non-recoverable error blocks the node. Gated on delivery v2 so the legacy inline path keeps its first-error-blocks behavior. Follow-up to PR #2471, which introduced the retry machinery this defers for.
![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: 59e9cf7cdd
ℹ️ 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".
…g-until-the-delivery
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (z.ai)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: z.ai
Verdict: REQUEST_CHANGES (1× P1, 4× P2, 6× P3). Posted as COMMENT because this is my own PR — the marker line below carries the verdict.
Recommendation: REQUEST_CHANGES
Premise & goal (verified against task #944 and #2471 thread r3771285317)
The ask: a recoverable error during the delivery-retry window should be invisible to Space; Space sees exactly one failure signal — the message_delivery dead-letter. The diff implements the decided direction (defer terminal handling until the dead-letter settlement), gated on delivery v2 so legacy keeps first-error-blocks, with a focused 7-test file. Scope is exactly the two files the task named — no scope creep. The core event contract checks out: exactly two session.error publishers exist (app.ts:1064 dead-letter with no details; error-manager.ts:515 with details = StructuredError, boolean recoverable set at error-manager.ts:115), the strict === true read degrades conservatively, and the error-before-idle ordering the flag relies on holds (the publishAsync microtask is queued during the awaited handleError before setIdle). The happy path — transient 5xx, retry succeeds, node completes on the retry's idle — is correct and pinned by tests.
The problem: the change treats "a recoverable session.error occurred" as equivalent to "the durable delivery layer will retry and will eventually idle again or dead-letter." That equivalence holds only for the narrow case this was written for. Every deviation either swallows a genuine completion (P1) or defeats the dead-letter's terminal signal (the first two P2s). The four inline threads carry the traces.
Findings
P1 — False-positive suppression eats the genuine completion idle; node hangs in_progress, invisible to every runtime sweep. A recoverable session.error can fire mid-turn from an unrelated subsystem while the turn still succeeds — the exact case the delivery layer itself guards against (agent-session.ts:2231-2239: "it came from an unrelated subsystem; this avoids a false-positive retry"; reachable via query-runner.ts:870-893, where a MESSAGE-category error is broadcast and the stream loop continues to a terminal result). The gate at task-agent-manager.ts:3724 arms deliveryRetryPending; the successful turn's completion idle is then suppressed at :3676; driveDeliveryTurn returns completed so no retry or dead-letter ever repays the flag. fired stays false and handleSubSessionComplete never runs. Recovery audit: handleAliveStuckExecutions (space-runtime.ts:7660) skips executions whose last SDK message classifies terminal (space-runtime.ts:7746-7749) — a successful turn's result row is exactly that — and the idle-status sweeps filter on status === 'idle' while this node sits in_progress. The run stalls until a daemon restart. Fix direction: before suppressing an idle, confirm a retry is actually pending for this session (an active delivery job for the session AND the turn produced no terminal result), or have the delivery layer publish an explicit retry-pending/turn-completed signal keyed to the messageUuid.
P2 — Error throttling breaks the deferral on the 4th+ identical recoverable error inside 10s. shouldThrottleError (error-manager.ts:357-393) drops the broadcast after 3 identical session:category:code errors per 10s. A throttled attempt does not arm the flag, but its terminal idle still publishes (query-runner.ts:1439-1441) — the completion path fires, marks the execution idle, and tears down both listeners. A later dead-letter's no-details session.error then hits if (fired) return and the node is never blocked; it sits idle with a failed kickoff row. Job backoff (1s/2s/4s) places fast-fail loops inside the window. Deriving retry-pendingness from job state rather than the broadcast (see the P1 fix) also fixes this.
P2 — Dead-letter repayment only exists for origin === 'space_inject' && role === 'turn'. The settlement publishes the terminal session.error solely for that shape (message-delivery-dead-letter.ts:51). A stranded kickoff re-enqueued by reconcileStrandedDeliveries carries origin: 'recovery' (message-delivery.ts:327); if that recovered job's retries exhaust after recoverable errors armed the flag, no terminal signal ever arrives — the flag is stranded and the node stalls until the ~15-min sweep. Pre-change, the first recoverable error would have blocked the node; this strictly widens that case.
P2 — The heuristic reuses a taxonomy the delivery layer has already publicly diverged from. details.recoverable === true is ErrorManager's classification, but delivery's own terminal classification (isTerminalTurnError + TERMINAL_TURN_ERROR_CATEGORIES, message-delivery.ts:169-184) treats auth as terminal despite recoverable === true. Space's correctness in the auth case is incidental — it works only because the immediate dead-letter fires a second, details-less session.error before any idle completes. Nothing in the code acknowledges that divergence, and the comment at :3705-3716 presents recoverable as a stable contract. The #944 design direction ("key off the dead-letter settlement") anticipated deriving this from delivery-layer state; consulting jobQueue.activeDeliveryMessageUuids(sessionId) (or an explicit event field) at suppression time would close the P1 and both preceding P2s with one mechanism.
P2 — Missing tests for the two failure modes above. (a) The P1 case: recoverable error mid-turn while the turn still succeeds — the sibling layer's own false-positive guard; nothing pins what Space does. (b) The sdkCount === 0 early return (:3661-3662) precedes the flag check (:3676) — startup-retry idles never consume the flag, so it survives to eat the first real idle of the eventual successful turn; the fixture hardcodes getSDKMessageCount: () => 5, so this ordering is never exercised.
P3 (×6):
- Import re-sort churn — ~106 of 169 changed lines are an ad-hoc alphabetical re-sort (partial: still two blocks with an interface interleaved), violating the surgical-change rule and polluting blame on a 5,800-line file.
- Stale
deliveryRetryPendingacross session re-activation — the closure is reused via thesessionListeners.hasearly-return (:3635) on the :1574 re-register path; an unconsumed flag suppresses the next activation's first genuine idle. Narrow window. - Kill-switch blast radius — the only rollback is
HYPERNEO_MESSAGE_DELIVERY_V2=0, which disables all of delivery v2, far broader than the node-blocking behavior changed here. - Docs —
docs/features/message-delivery-v2.mdhas zero mentions ofsession.error; the now-load-bearing convention (nodetails= terminal for Space nodes) lives only in code comments. The event map (internal-event-bus.ts:395) deserves the comment too. - Comment nits — "The QueryRunner broadcasts" (:167): the publisher is
ErrorManager.broadcastError; and the bare-filenamemessage-delivery-dead-letter.tsreference (:3645) sits two directories away. - Test hygiene — pin
HYPERNEO_MESSAGE_DELIVERY_V2inbeforeEachfor the deferral tests (ambient=0would silently flip them); assert listener/callback cleanup on the terminal path (it skipscompletionCallbacks.delete, unlike the idle path — pre-existing but now longer-lived); one composition case wiring the realsettleMessageDeliveryDeadLetterthrough the bus into the manager would pin the no-details contract end-to-end.
Verified sound (no action)
- Restart mid-retry-window self-heals:
rehydrateSubSessionre-registers the callback; the re-claimed job's idle completes the node. - Long-horizon agents and the Task Agent parent do not flow through
registerCompletionCallback; LHA deliveries useorigin: 'long_term_agent'— unaffected. - Recoverable errors still surface to users (web session-store toast) and
state-projection-service— only node-blocking is deferred. - Sequential recoverable errors re-arm correctly; the dead-letter-after-deferral and v2-off legacy paths are pinned by tests.
- Security: no new attack surface;
recoverableis set by a fixed table, never provider-controlled; the deferral window is bounded (retry budget 8, exponential backoff, consumption timeouts); nothing new logged. - Rebase is faithful: callback area and test file are byte-identical to the reviewed commit; PR-head deltas are dev-side features only.
— Reviewer (glm-5.1[1m] via HyperNeo), head inspected: f0c0331
…sion.error [#944] Review follow-up (PR #2488, REQUEST_CHANGES): the flag-based deferral armed `deliveryRetryPending` from the `session.error` broadcast, which is both throttled (ErrorManager drops the 4th identical error per 10s — a fast-fail retry loop could then leak the post-error idle as a completion) and ambiguous (an unrelated recoverable error mid-turn suppressed a successful turn's completion idle with no retry/dead-letter ever coming, hanging the node). The deferral now derives retry-pendingness from the durable job row: - An idle while `activeDeliveryMessageUuids(sessionId)` is non-empty is not a completion — covers both sides of the turn-end race (failed turn idles before the job's throw→backoff; successful turn idles just before the job row completes). - New processor `onComplete` lane hook publishes `session.delivery_settled` (real `processing`→`completed` only; parks excluded), which completes the node when nothing else is in flight. This repays the suppressed idle after a successful retry and completes a successful turn that carried an unrelated recoverable error. - New `session.delivery_failed` (published from `onDead` for EVERY dead delivery, unlike the settlement's `space_inject`-origin-gated session.error) blocks the node when `role === 'turn'` — recovery-origin re-enqueued kickoffs that dead-letter with no session.error no longer hang in_progress. Steers never block. - The recoverable `session.error` classification is now advisory: a misclassification (ErrorManager's taxonomy diverges from delivery's on auth) only delays the block until the immediate dead-letter; `delivery_failed` guarantees the terminal outcome. Also documents the contract in docs/features/message-delivery-v2.md §18 and adds tests for the throttled-error, unrelated-error, sdkCount===0, steer-vs- turn dead-letter, and real-settlement composition cases.
|
Review response — 89ff285 (rebased onto the dev merge head; all inline threads answered in place). The flag-based mechanism is removed entirely. Per your suggested single mechanism, retry-pendingness now derives from delivery-layer state: an idle completes only when Requested tests added: mid-turn error + successful turn, sdkCount===0 ordering on both completion paths, steer-vs-turn dead-letter, and a composition case wiring the real P3 dispositions:
Verified: 12 deferral tests + 146 related (delivery-v2, dead-letter, processor, runtime completion/terminal-error) pass; tsc/oxlint/biome/knip clean via pre-commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ff285a5f
ℹ️ 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".
… lost settles [#944] Codex follow-up on the job-row deferral (PR #2488 round 2): - P1: a steer settles at mid-turn CONSUMPTION while the agent is still working; completing the node on it tore down the error listeners mid-work, so a later turn failure could no longer block the node. `session.delivery_settled` now carries `role`, and only a TURN settle completes the node — the owning turn's settle always follows (its job completes at turn end). - P2: a dead-lettered STEER that was the last active job left a suppressed terminal idle unrepaid (the turn's settle was ignored while the steer was in flight). The delivery_failed handler now repays the suppressed completion for steers when nothing else is in flight, instead of stranding the node; steers still never block. - P2: a daemon crash between the job row's completion and the settle publication loses the signal permanently (the idle was already suppressed, the completed job is never re-claimed, and restart rehydration publishes no fresh idle). A one-shot unref'd reconciliation (default 30s, operator- tunable via HYPERNEO_DELIVERY_SETTLE_RECONCILE_MS) completes an idle session with history and no active delivery jobs — activation enqueues its job within milliseconds, so a still-jobless idle session this late is genuinely stranded. Docs updated (message-delivery-v2.md §18); tests added for all three cases (16 deferral tests, stable across repeated runs for timer races).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20886d412a
ℹ️ 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".
…turns [#944] Codex P2 (round 3): the processor's onComplete hook published session.delivery_settled for every completed message_delivery job, but the handler also completes jobs with non-success outcomes — a reclaimed ACP turn whose message is still `submitted` returns { outcome: 'skipped' }, plus aborted / no_content / archived / stale_attempt. Settling those let TaskAgentManager complete a workflow node whose prompt was never accepted or processed. New isCompletedTurnResult(result) predicate (outcome === 'completed', which also admits the completed-with-turn_terminated marker case — the turn genuinely ended on a prior attempt) gates the publication in app.ts. Parks never reach it (a requeued row makes the auto-complete a no-op, so onComplete does not fire). Unit-pinned across all outcome shapes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf5544fc3
ℹ️ 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".
…tion [#944] Codex P2 (round 3): the delayed reconciliation could not distinguish a lost settlement from a kickoff that was never enqueued. On a reused node-agent session the callback registers before the next kickoff's job is enqueued; a daemon exit in that window rehydrates an idle session with a historical transcript and no active job, and the timer would falsely complete the new execution whose turn never ran. The reconcile now requires SDK output postdating the execution's start (new SDKMessageRepository.hasMessagesSince) — a historical transcript is not proof the current activation ran. Consistent with the active-job check: an enqueued kickoff has a job row and is suppressed there, so "no job + output since start" is exactly the lost-settle shape. The timer body is also fully defensive now (best-effort by nature; a DB error must not surface as an unhandled timer exception). Doc §18 updated; tests: positive reconcile, reused-session negative, and timer-disarm hygiene between tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ae548e20
ℹ️ 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".
…ers [#944] Codex P1s (round 4): - The recoverable session.error deferral now requires an ACTIVE delivery job: errors from non-delivery work (rehydration's direct streaming start and tool-continuation replays have no job) cannot be retried and no delivery_failed will repay them — without the gate their idle completed the node despite failed work. - A SETTLED steer repays a suppressed terminal idle: in the ACP shape the owning turn settles while its steer is parked awaiting acceptance, so when the accepted steer later completes ('consumed'/'already_consumed') as the last active job, its settlement is the only repayment. New isSettledSteerResult publishes those outcomes (role-aware filter in the onComplete hook); the manager completes on a steer settle only when the session is live-idle and nothing else is in flight — never mid-turn. The dead-steer repayment now carries the same idle guard. Doc §18 updated; tests: settled-steer repayment (ACP), steer-settle while processing, recoverable error with no active job blocks.
93a04f5 to
5da7282
Compare
…944] Codex P2 (round 5): the delayed reconciliation used transcript evidence (any SDK message postdating the execution start), so a turn whose job row went dead before a crash — losing the fire-and-forget session.delivery_failed publication — would RECONCILE AS COMPLETE on restart (dead rows are absent from the active-job set and the failed turn's partial output satisfies the transcript check), marking a failed node idle instead of blocked. New JobQueueRepository.deliveryTurnOutcomeSince(sessionId, sinceMs) reads the durable terminal outcome of the session's turn jobs settled in the current activation: 'dead' (any dead-lettered turn → the reconcile BLOCKS, repaying the lost publication), 'completed' (a genuinely successful turn result → completes), else null (declines — covers the reused-session / kickoff-never-enqueued shape and non-success outcomes that isCompletedTurnResult already filters). Transcript-based evidence (hasMessagesSince) is removed — the job row is strictly stronger. Tests: lost-settle completes, lost-dead-letter blocks, never-enqueued declines, active/processing declines. Doc §18 updated.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (z.ai)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: z.ai
Re-reviewed at head 9195e2d33 (rounds 1–5: 89ff285 → 20886d4 → 7bf5544 → 59ae548 → 5da7282 → 9195e2d). The redesign landed exactly on the mechanism my first round asked for: retry-pendingness derives from the durable job row, and both terminal outcomes flow through the job lifecycle (delivery_settled completes / delivery_failed + settlement session.error blocks). Every P1/P2 from all five rounds — throttled 4th error, unrelated recoverable error during a successful turn, recovery-origin dead kickoffs, steer mid-work completion, settled-steer repayment, non-delivery-work errors, crash-lost publications — is fixed and pinned by tests. The crash reconciliation now derives from the durable job row (deliveryTurnOutcomeSince), which is strictly stronger than the transcript evidence it replaced. 22 deferral tests exercise the real internalEventBus, real repos, and the real settlement composition. No correctness, security, performance, or compatibility findings remain. Remaining items are P3 polish only.
Verified fixed (all prior rounds)
- Throttle immunity / job-row keying — suppression reads
activeDeliveryMessageUuids, not the error broadcast; ErrorManager's 4th-identical-error silence can no longer leak a completion or drop a block. Pinned by the throttled-4th test. - Steer lifecycle — steer settles are repayment-only (live-idle + nothing-in-flight), dead steers repay but never block, dead turns block regardless of origin, and the ACP awaiting-acceptance park budget exhausts into
DeadLetterImmediatelyError→delivery_failed, so the steer branches all terminate. - Non-delivery work — recoverable-error deferral now requires an active job; rehydration streaming / tool-continuation replays keep first-error-blocks.
- Crash windows —
deliveryTurnOutcomeSince(durable row, scoped to the activation viastartedAt) repays both a lost settle and a lost dead-letter; the 7-day job retention vs 30s reconcile window leaves no interference. - Processor contract —
onCompletefires only on a real claim-fencedprocessing→completedtransition; parks/requeues returnnulland cannot fire it; other lanes are untouched. - Security posture — role is DB-arbitrated (not caller-choosable), the subscription is session-scoped, parent resolution dies on unknown sessions, and the blocked path writes the constant
DEAD_LETTER_SESSION_ERRORinstead of provider-derived text (strictly better than the pre-PRevent.error).
P3 findings (non-blocking polish)
- Event payload types are looser than the source unions (
internal-event-bus.ts:411,427):role?: string; origin?: stringwhileMessageDeliveryRole = 'turn' | 'steer'andMessageDeliveryOriginare closed unions exported frommessage-delivery.ts:49-57. Typing them as the unions would give compile-time protection onevent.role !== 'turn'consumers and match the precision of the rest of theSessionEventsmap. - Stale comments after the listener expansion (
task-agent-manager.ts:3864"self-unsubscribes both listeners" — there are now four plus the reconcile timer;task-agent-manager.ts:31file-header completion description covers only the idle path). - Import re-sort churn still present (
app.ts:6-12region, ~145 of 204 changed lines in app.ts are the alphabetical re-sort; same in task-agent-manager.ts): no repo tooling enforces import order (no biomeassist/organizeImports, oxlint has three rules, CI runs formatter only). Not functional — but it inflates the diff on the two most-touched files. - Test origin value not from the real union (
task-agent-delivery-retry-deferral.test.ts:413): the recovery-origin case pinsorigin: 'system'; the real re-enqueue path writesorigin: 'recovery'(message-delivery.ts:343). Any non-space_injectvalue exercises the gate identically today, but the real value would keep the test honest if origin-specific logic ever lands.
P3 push-back accepted
- v2=0 kill-switch semantics: agreed — with the retry machinery absent, first-error-blocks is the correct legacy semantic, not a regression. The docs and code comments now scope the deferral correctly to v2-on with an active job.
Recommendation: APPROVE
- Event payload fields typed with the closed MessageDeliveryRole / MessageDeliveryOrigin unions instead of string (compile-time protection for the role-gated branches). - Stale "both listeners" comment and the file-header completion-detection description updated for the four-listener + reconcile-timer reality. - Test fixture origin 'system' → 'recovery' (matches reconcileStrandedDeliveries' re-enqueue origin). - Import re-sort churn reverted: app.ts and task-agent-manager.ts import blocks restored to dev's order (my local biome version re-sorted them differently from the version that formatted dev); diffs vs dev are now the functional changes only (app.ts 55+/1-).
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (z.ai)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: z.ai
Re-review of the P3 polish commit baf2c8b01 (head advanced past the approved 9195e2d33).
Delta verification (approved → new head, mechanically checked):
- app.ts — body byte-identical between the two commits; the change is a pure import-block reorder. Diff vs
devis now exactly the approved functional change (55+/1-: thesession.delivery_failedpublish on every dead-letter + the role-gatedonCompletesession.delivery_settledpublish). - task-agent-manager.ts — import reorder + the two comment updates (four-listener + reconcile-timer reality). The one non-reorder nuance (
NodeExecutionRepositorytype-only → value import) exactly restores dev's own form at dev:120. - internal-event-bus.ts — type-only:
role/originnarrowed fromstringto the closedMessageDeliveryRole/MessageDeliveryOriginunions. Both events' only publishers (app.ts:1055, app.ts:1118) pass already-union-typed payload fields, so no other call site is affected. - test fixture —
origin: 'system'→'recovery': not merely cosmetic, it was required by the new typing ('system'is not a member ofMessageDeliveryOrigin), and it correctly matchesreconcileStrandedDeliveries' re-enqueue origin (message-delivery.ts:343).
All four P3 items are addressed as claimed. All 24 review threads resolved. CI green on the completed shards of the new head (remaining shards pending; the delta cannot change behavior).
Recommendation: APPROVE
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf2c8b012
ℹ️ 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".
…off [#944] Codex P2 (round 6): createSubSession's fire-and-forget pending-message flush can deliver a PEER handoff as a role:'turn' job before the node's actual kickoff is enqueued. The crash reconciliation accepted any turn settled since execution.startedAt, so a daemon exit in that window treated the peer turn as the activation's successful kickoff and marked the unstarted node idle. spawnWorkflowNodeAgentForExecution now stamps the kickoff's message UUID onto the owning node execution (data.kickoffMessageUuid, persisted via the execution row — no schema change), and deliveryTurnOutcomeSince takes an optional messageUuid so the reconciliation only accepts THAT delivery's settlement. Without a stamped kickoff (older rows, or the crash-before-enqueue window itself) the reconciliation declines — conservative: a kickoff that lost the turn arbiter to a flushed peer settles as a steer and correctly does not qualify. Tests: flushed-peer-turn negative (no stamp → declines), plus the existing lost-settle / lost-dead-letter / never-enrolled cases now keyed on the uuid.
…urn [#944] Codex P2s (round 7): - The crash reconciliation no longer checks the CURRENT v2 flag: its decision reads durable rows written while v2 WAS enabled, so an operator restarting with the rollback switch after a crash still gets the repair — the settled job row is never re-claimed and rehydrating an idle session publishes no fresh idle, so the node would otherwise strand exactly when rolling back. - The recoverable-error deferral now requires a delivery job currently being DRIVEN (claimed `processing`, via new JobQueueRepository.hasProcessingDeliveryForSession), not any active job: a merely-queued job cannot retry a rehydration tool-continuation replay that errors while it waits, and deferring such an error let a later unrelated settlement complete the node instead of blocking it. Doc §18 updated; tests: queued-only continuation-replay error blocks, and the reconcile repairs a stranded settle with v2 switched off.
557d0b1 to
485be21
Compare
…concile [#944] Codex findings (round 11): - P2: the kickoff stamp was written AFTER the inject, but the inject awaits SDK consumption — a crash in that window stranded an unstamped execution with delivery underway (both the live settle and the reconcile would reject it forever). The uuid is now generated and stamped BEFORE the inject and passed as the injection's explicit message id. - P1: a kickoff that lost the arbiter to a flush settles as a consumed steer; the owning peer turn's settle carries a uuid ≠ the stamp and was rejected, and a turn longer than the 30s reconcile delay left the one-shot timer already spent — the node stranded. The settle's TURN branch now consults the durable correlated outcome on mismatch (kickoff-consumed-steer + this turn terminal → complete; dead → block), and the reconcile RE-ARMS while a delivery is in flight or the session is not idle. - P2: the recoverable-error deferral counted any processing row; a claimed STEER (mid-turn feed) must not enable it — the predicate is now role='turn'-filtered, so only the driving turn owns the retry. Tests: steer-only processing blocks; owning-turn settle completes and no-durable-settlement declines; reconcile re-arm across a long turn.
…e, re-arm, turn-only deferral [#944]
…r repayment [#944] Codex findings (round 12): - P2: an older stranded message re-enqueued by the startup reconciler as an origin:'recovery' turn job on a reused session dead-lettering must not fail the CURRENT activation — the delivery_failed turn branch now correlates event.messageUuid with the execution's stamped kickoff (an unstamped execution declines; the kickoff keeps its uuid across a recovery re-enqueue, so a genuine kickoff dead-letter still blocks). - P1: steer repayment (settle and dead-letter paths) now requires current-activation evidence — the kickoff's own durable settlement is 'completed'. A restored old steer settling 'already_consumed' in the pre-kickoff window (callback registered, kickoff not injected) on a reused session no longer completes the new execution off the historical transcript. Shared kickoffDeliveryOutcome helper carries the correlation (no-execution vs unstamped vs stamped+outcome).
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (Z.ai)
Model: glm-5.1[1m] | Client: HyperNeo | Provider: Z.ai
Re-review at head e4b117ebe (rounds 6–12 since the earlier APPROVE at 9195e2d33). The head moved three times while I read (c05004abc → 8caf83493 → d2b34196 → e4b117ebe); this verdict is against what I read at e4b117ebe.
Verified fixes
Every finding I independently confirmed on earlier heads is properly closed at this one:
- Pre-stamped kickoff — UUID generated → execution stamped → inject with it as the explicit id. The unstamped-crash window is gone. (d5MR)
- Re-armable reconcile — re-arms while the session is processing or delivery is in flight; the long-peer-turn strand is repaired after the turn ends. (d5MU)
- Owning-turn settle with durable correlation — a uuid-mismatched turn settle consults
deliveryTurnOutcomeSincecorrelated mode: kickoff-as-consumed-steer + owning turn terminal → complete,dead→ block, else decline. (d5MU) - Turn-only deferral predicate —
role='turn'filter inhasProcessingDeliveryForSession; a claimed steer can no longer suppress a continuation-replay error. (d5MX) - Dead-turn correlation —
delivery_failedturn events must match the stamped kickoff; unrelated recovery re-enqueues no longer false-block, unstamped executions decline, and the genuine kickoff keeps its uuid across a recovery re-enqueue. (d8oJ) - Activation-evidenced steer repayment — both steer repayment paths now require the kickoff's own durable
completedsettlement viakickoffDeliveryOutcome; the pre-kickoff restored-steer window can no longer complete off the historical transcript. (d8oM)
The new kickoffDeliveryOutcome helper centralizes the correlation the settle / dead-letter / repayment decisions share — good factoring, tested (35 manager tests + repo-level outcome tests), §18 doc in sync. All review threads resolved.
Nit (P3, non-blocking)
One stale doc comment: /** Returns a sub-session … */ at task-agent-manager.ts:3109 is orphaned — round 7's hasProcessingDeliveryJob insertion detached it from getSubSession (now at :3149, undocumented), and round 12's helper landed directly under it. Drop the dangling line and re-attach it to getSubSession (or fold into the helper's doc).
Verdict
Approve. Rounds 6–12 converge on a single coherent invariant — only the activation's own kickoff settlement (as turn, or consumed steer under a terminated owning turn) decides the node — and every failure mode raised across six review rounds has a landed fix with a regression test. CI is still running on this head at review time; please let it finish green before merge.
Recommendation: APPROVE
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b117ebe5
ℹ️ 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".
…lost kickoffs [#944] Codex P2s (rounds 13-14): - The dead-letter settlement's uuid-less fallback session.error (published for space_inject turns) bypassed the uuid correlation and blocked the node for an unrelated flushed peer turn's dead-letter. The error handler now attributes it via the durable rows (new hasDeadTurnExcept): it blocks only when the kickoff itself did not already settle and no dead non-kickoff turn exists. - The consumed-kickoff-steer recursion aggregated every turn since activation, so a later unrelated promoted-steer dead-letter blocked an activation whose kickoff was consumed successfully. The owner is now scoped: the earliest turn terminal at/after the steer's consumption. - A stamped kickoff whose delivery row never landed (crash between the execution update and the enqueue) left the reconcile declining with nothing left to re-arm. It now BLOCKS with an explicit lost-kickoff reason so the runtime's blocked-execution re-spawn machinery re-activates the node. Tests: fallback-error attribution (declines for non-kickoff dead, blocks kept for genuine kickoff failures), owner-scoped shapes, lost-kickoff re-spawn.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c7ec2503d
ℹ️ 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".
#944] Codex P1 (round 15): the owner-scoped query keyed on the steer row's completed_at, but an accepted ACP steer stays parked and completes ('already_consumed') on a later claim — after its owning turn already finished — so the owner's completed_at preceded the steer's and the match missed (or picked a later unrelated turn), leading the reconciliation to block and re-spawn a node whose kickoff was consumed successfully. The owner is now matched by claim window: the turn with started_at <= the steer job's CREATION <= completed_at (the turn active when the steer entered delivery), falling back to the earliest terminal turn at/after creation for retried owners whose last claim postdates the steer. Repo test pins the ACP shape.
…ow [#944] Round-16 review fixes (three P2s): - Stamp the kickoff uuid on the node execution BEFORE the awaited goal/memory construction, not between it and the inject. A daemon exit during those awaits previously rehydrated an unstamped in_progress execution that neither the live settle path nor the reconciliation could ever resolve; the recovery blocked forever. - A stamped kickoff whose delivery row is missing is no longer unconditionally "lost pre-enqueue": consult the kickoff's persisted sdk_messages row (it survives job-queue cleanup). send_status 'consumed'/'failed' → complete (the job row aged out of the 7-day retention window, or the activation used the legacy inline path with no job row at all — finished work must not be re-spawned); still 'enqueued' or a gone row → block for re-spawn as before. - Tests: lost pre-enqueue still blocks (cleared processing stand-ins so the reconcile path is exercised); expired settlement (consumed row, aged-out job row) completes; legacy v2-off activation with no job row completes via the message row. Doc §18 updated for the pre-await stamp and the send_status classification.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4103b0643
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Z.ai)
Model: glm-5.1 | Client: HyperNeo | Provider: Z.ai
Reviewing head f4103b064 (rounds 15–16 landed while I was reviewing 1c7ec2503; findings re-anchored and re-verified against the current head — the round-16 pre-stamp-before-awaits change and the persisted-message-row classification are incorporated below).
Verdict summary
The contract itself is sound and now well-hardened: stamp-before-await closes the unstamped-crash window, the claim-window steer-owner match (round 15) fixes the ACP already_consumed mis-attribution, and classifying a missing kickoff job row via the persisted sdk_messages row (round 16) correctly rescues aged-out and legacy-inline activations. The fired mutual exclusion, four-listener teardown, single-flight re-arm, and claim-fenced onComplete gating are all correct as written; all new SQL is parameterized; no new security surface. One P1 remains plus two P2s and two P3s.
P1 — lost-kickoff BLOCK false-positives on deferred kickoffs (rate-limit / parent-limited)
task-agent-manager.ts:3941-3963: the reconcile's terminal heuristic — stamped kickoff + deliveryTurnOutcomeSince → null + nothing active + idle — cannot distinguish "crash between stamp and enqueue" from "kickoff legitimately persisted as deferred". injectMessageIntoSession (~line 5048) defers even an immediate kickoff when inRateLimitCooldown || parentLimited: it persists the sdk_messages row carrying the stamp uuid with sendStatus: 'deferred' and enqueues no message_delivery job. Round 16's classification only completes on 'consumed' | 'failed' — a 'deferred' row still falls through to fireTerminalError('…lost before enqueue…').
Reachable two ways: (a) live: re-activation on a reused session whose parent task is rate/usage-limited — parentLimited keys off task status while the session itself sits idle, so nothing else holds the reconcile off; (b) after restart: the code's own comment (~line 5040) notes rehydration flips a persisted rate_limit_cooldown session to idle while the parent-task pause is restored only by a later sweep. Either way the 30s shot blocks a healthy-but-paused node, the re-spawn machinery issues a fresh kickoff, and when the deferred row later replays the node gets a duplicate kickoff.
Fix: a sendStatus === 'deferred' row should re-arm (or decline), not block — the durable evidence to disambiguate is the same lookup round 16 already added (getDeliveryContent(sessionId, stampUuid)). Inline comment attached.
P2 — re-activation window reads the previous activation's stamp
task-agent-manager.ts:1319 and :1617 both register listeners before spawnWorkflowNodeAgentForExecution overwrites data.kickoffMessageUuid (~line 1328). In that window (spanning the awaited goal/memory construction) the execution still carries activation N−1's stamp, whose durable row is already completed. A session.delivery_settled for a flush-driven peer turn then takes the mismatch branch (~line 3851), deliveryTurnOutcomeSince(…, oldStampUuid) returns 'completed' — correlated mode ignores sinceMs, so nothing bounds it to this activation — and completeFromDeliveryState fires with no active jobs and sdkCount > 0 on the reused session: the new activation is marked complete before its kickoff runs. Narrow (needs a peer turn settling inside a seconds-wide window) but it is exactly the race class this PR closes. Clearing data.kickoffMessageUuid at the start of re-activation closes it.
P2 — the reconcile's catch surrenders permanently
task-agent-manager.ts:3967-3972: a transient throw at the single shot (e.g. SQLITE_BUSY on the claim-contended job_queue table) logs at debug and never re-arms — yet the design premise is that no future event will re-publish the suppressed idle, so the crash-repair this timer exists to provide is lost forever. Every decline path re-arms; the error path should too, bounded the same way.
P3 — dead second check in the delivery_failed turn path
task-agent-manager.ts:4020-4023: when kickoff.kickoffMessageUuid === null, the preceding event.messageUuid !== kickoff.kickoffMessageUuid (string vs null) already returned, so the unstamped-decline check is unreachable-for-effect. Harmless; misleading to read.
P3 — session-scoped branch of deliveryTurnOutcomeSince is production-dead
job-queue-repository.ts:585-603: every production caller passes messageUuid (or declines before calling); the no-uuid mode runs only in its own test. Dead query paths on job_queue (unindexed json_extract scan) invite drift from the correlated mode that actually runs — drop it or wire a real caller.
Tests / coverage note (no severity)
The 37 manager tests + 10 repo-level deliveryTurnOutcomeSince shapes (including the round-15 ACP claim-window pin) are genuinely strong. Gap: hasDeadTurnExcept and hasProcessingDeliveryForSession have no repo-level tests — the manager tests exercise them only as stand-ins, so a SQL regression (e.g. wrong completed_at bound) would pass the whole suite. Worth two seeded rows each in job-queue-repository.test.ts.
Verified clean (spot-checks)
SQL fully parameterized (the conditional exceptUuid fragment is a constant-string idiom); kickoff uuid server-generated and confined to bound params; no new external surface (events stay on the in-process bus, sole subscriber TaskAgentManager); dead rows always carry completed_at (fail/markDead); restoreFromDatabase indeed broadcasts nothing (reconcile premise holds); recovery re-enqueue preserves the message uuid (correlation claim holds); complete() null-on-park correctly gates onComplete; 2 ** retryCount is semantics-preserving.
Recommendation: REQUEST_CHANGES
…ation [#944] Round-17 review fixes (P1 deferred kickoffs, 2 P2s, 2 P3s) plus the follow-up Codex P2 on the round-16 classification: - P1: a kickoff deferred by a rate-limit cooldown / parent-task limit is persisted 'deferred' with no delivery job. The rowless classification treated it as lost-before-enqueue and blocked a healthy-but-paused node (then re-spawned a duplicate when the cooldown re-enqueued). 'deferred' now declines and re-arms. - Codex P2: 'failed' send_status means the delivery terminalized as failed — it now BLOCKS for re-spawn instead of completing. 'consumed' alone proves only SDK acknowledgment (the flip precedes the turn), so it completes only when hasTerminalResultAfter finds a success terminal result after the kickoff's consumption. A turn that crashed mid-run (consumed, no result) blocks for re-spawn. - P2: clear data.kickoffMessageUuid synchronously before a re-activation's first await. The fresh completion callback registers inside createSubSession's session-reuse branch before the new stamp lands, so a peer-turn settle in that window could correlate the PREVIOUS activation's stamp and false-complete the new activation (and a kickoff:false re-activation never restamps at all). - P2: a thrown reconcile shot re-arms once (bounded: two consecutive throws give up; a successful shot resets the budget) so a transient SQLITE_BUSY cannot surrender the crash repair permanently. - P3: drop the unreachable unstamped-decline check in the delivery_failed turn branch (the uuid-mismatch return already covers the null case). - P3: drop the production-dead session-scoped mode of deliveryTurnOutcomeSince along with its now-unused sinceMs parameter — every caller correlates by the stamped kickoff uuid, which IS the activation scope now that re-activations clear it. Add repo-level tests for hasDeadTurnExcept and hasProcessingDeliveryForSession (manager tests used stand-ins only). Tests: deferred declines, failed blocks, consumed-without-result blocks, transient-throw re-arm recovers, persistent throw is bounded. Doc §18 updated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efcdfdd0cb
ℹ️ 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".
… retry-safe [#944] Round-18 review fixes (four Codex P2s): - Stamp the kickoff BEFORE the activation's first await: the clear+stamp is now one write issued before the createSubSession call (whose session-reuse branch registers the fresh completion callback and whose setup awaits), instead of after it and the attachment check. A daemon exit during session reuse, MCP reinjection, or the attachment check now rehydrates a stamped execution whose reconcile can classify it. - Guard the missing-row classification with an in-memory pre-inject marker (preInjectKickoffExecutions, released in the spawn finally): setup + goal/memory construction can outlast the reconcile delay (a cold embedder load), and the reconcile must not fire the lost-kickoff block while the inject is still pending — that ran work under a blocked execution. A daemon death empties the marker so the restart shot still blocks for re-spawn. - Reset the catch-side retry budget after EVERY non-throwing reconcile shot, declines included (the shot body moved into runReconcileShot so the reset covers the early returns): previously one transient failure permanently consumed the allowance even after many healthy checks. - Retain the terminal decision until it persists: fireTerminalError no longer tears down the listeners before handleSubSessionError resolves — a transient SQLite failure on the blocked-status update un-consumes the decision (fired=false), keeps the listeners, and re-arms the reconcile to retry from the durable dead row. Tests: alternating-throw chain keeps repairing (budget reset on declines), mid-construction activation declines then classifies after the guard releases, failed terminal persist retries the block. Doc §18 updated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abe5d0568
ℹ️ 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".
…e drive [#944] Round-19 review fixes (two P1s, one P2): - P1 (steer consumption attribution): a steer can sit unclaimed past its creation-time turn and be consumed by a LATER turn (feedDeliverySteer runs inside whatever turn is live at claim). deliveryTurnOutcomeSince matched the owner by the steer's creation timestamp, so an earlier successful turn masked the consuming turn's dead-letter (false complete) and vice versa. The owner is now the LATEST-STARTED turn whose claim window overlaps the steer's active interval [created_at, completed_at] — consumption happens inside the consuming turn's claim, so it always overlaps — with the earliest-terminal-after-creation fallback kept for retried owners whose consuming claim was overwritten. The ACP shape still matches its original owner (consumed during its claim; the later completion is administrative). - P1 (rollout): executions already in_progress at deploy time carry no kickoffMessageUuid (the parent revision never wrote one). The settle handler declined them as "pre-kickoff window" and the dead-letter turn branch ignored them, stranding the node in_progress regardless of outcome. An activation started before this daemon's boot (isPreUpgradeActivation) now keeps the pre-#944 unconditional behavior: the reclaimed delivery settles the node live, and its dead-letter blocks. - P2 (drive attribution): the recoverable-error deferral keyed on a session-wide claimed-turn row. During rehydration the restored session is published to the runtime caches before the direct streaming / continuation-replay awaits, and the processor can claim an unrelated restored turn in that window — its row satisfied the gate while being unable to retry the replay's error. AgentSession now exposes isDeliveryTurnDriving (an in-flight driveDeliveryTurn counter) and the deferral requires an actual drive, not merely a claimed row. Tests: late-consumed steer both directions (repo); pre-upgrade settle completes / dead blocks; claimed-but-not-driving recoverable error blocks. Doc §18 updated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07fcfd350
ℹ️ 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".
…inal-node-error-handling-until-the-delivery
…fter dev merge [#944] Merge origin/dev (brought queue-preview batch flush #2493, delivery observability #2492, orphan-process cleanup #2491, online shard tooling #2497/#2501/#2513). Git auto-merged driveDeliveryTurn's new batchUuids parameter into the wrapper signature without passing it through to driveDeliveryTurnCore — dev's batch-narrowing tests (agent-session "narrows the batch payload" / "refuses to feed a reduced batch") failed on the merge ref. Thread the parameter through; all shards green (1-core 4315, runtime-a 691, runtime-b 1075, 4-space-storage 2165).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c24e8f6e9
ℹ️ 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".
…e; batch-aware correlation [#944] Round-20 review fixes (one P1, three P2s): - P1 (steer consuming turn): the interval-overlap approximation could pick an unrelated later turn when an ACP park outlives its consumer — both turns overlap the steer's [created, completed] interval. The owner is now matched by the claim window CONTAINING the kickoff's persisted CONSUMPTION timestamp (getDeliveryConsumedAt; the consumed flip aligns the row timestamp atomically), with the overlap approximation kept only for legacy rows without one. Repo tests cover both ACP-park directions. - P2 (rowless evidence bound): hasTerminalResultAfter accepted ANY later success, so a reused session's later peer result could classify a kickoff whose own turn failed. The rowless classification now requires the FIRST terminal result after the kickoff's consumption to be a success (getFirstTerminalResultSubtypeAfter); an error subtype blocks with its reason, and no result keeps blocking. - P2 (drive marker): deliveryTurnDriveCount is now incremented only on the DRIVING path — after the session-lock critical section — not at driveDeliveryTurn entry, so a recoverable error from rehydration's direct replay during the lock wait is not suppressed by a job that cannot retry it. The wrapper/core split from round 19 is gone (single method again). - P2 (batch correlation): deliveryTurnOutcomeSince matches the head uuid OR any batchUuids member (queue-flush batches key their row on the head), and a batch-head dead-letter blocks live via the durable outcome consult on uuid mismatch. Tests: ACP-park consumption steering both directions, batch member completed/dead, first-terminal-error blocks, batch-head dead-letter blocks. Doc §18 updated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24bfc23c1b
ℹ️ 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".
| .get(sessionId, uuid) as { timestamp: number } | undefined; | ||
| return row?.timestamp ?? null; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier consuming-turn thread is that sdk_messages.timestamp is stored as ISO text, but this method casts and returns it as a number without conversion. durableKickoffOutcome() passes that string to the epoch-millisecond comparisons in deliveryTurnOutcomeSince(), so SQLite cannot match the turn whose claim window contains the consumption instant and falls back to creation-time inference. When a kickoff steer is created during turn A but consumed by turn B, this can report A's outcome—completing the execution when B dead-lettered, or blocking it when B succeeded. Convert the persisted value with new Date(...).getTime() before returning it.
Useful? React with 👍 / 👎.
| ORDER BY r.consumed_seq ASC | ||
| LIMIT 1` |
There was a problem hiding this comment.
Fresh evidence beyond the earlier rowless-success thread is that delivery retries reuse the same consumed kickoff UUID and can persist multiple terminal results: for example, error_during_execution on the first attempt followed by success on a retry. After the completed job row ages out, rowless reconciliation calls this method, whose ascending order returns the stale first error and blocks/re-spawns an activation that ultimately succeeded. Persist the delivery's final outcome outside the cleanup-managed job row, or otherwise bound the selected result to the final retry attempt without accepting results from later peer turns.
Useful? React with 👍 / 👎.
| const kickoffMessageUuid = ( | ||
| execution?.data as { kickoffMessageUuid?: unknown } | null | undefined | ||
| )?.kickoffMessageUuid; | ||
| if (typeof kickoffMessageUuid !== 'string') return; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier rollout-compatibility thread is that the compatibility fallback exists only in the live settlement handlers; this durable reconciliation still returns immediately for every unstamped execution. If a pre-upgrade activation's delivery becomes completed or dead and the upgraded daemon exits after committing that row but before its live event is handled, the next boot restores an idle unstamped execution, completed/dead jobs are never reclaimed, and this return prevents the only crash-window repair, leaving the execution in_progress indefinitely. Add a durable compatibility reconciliation path for activations identified by isPreUpgradeActivation() rather than relying exclusively on the one-time live publication.
Useful? React with 👍 / 👎.
|
Closing: owning task was cancelled and nothing is driving this PR. Branch kept for reference — reopen via a fresh task if this work is resumed. |
|
Reopening: the owning task was mass-cancelled by the 2026-08-16 space-stop incident, not deliberately abandoned — the earlier closure mistook incident quiescence for dead work. Task restoration pending the update_task status capability. |
|
Closing: sibling #2471 landed, this half did not, and the branch is 1,200+ commits behind. Re-cut from dev if recoverable node errors still block workflow nodes; no tracking issue exists yet. Branch retained. |
1 participant
A recoverable provider error during a delivery-driven node-agent turn no longer blocks the workflow node:
registerCompletionCallbacknow treats a recoverablesession.error(details.recoverable === true, delivery v2) as non-terminal, suppressing the post-error idle so the message_delivery retry can complete the node. Only the dead-letter settlement or a genuinely non-recoverable error blocks it. Follow-up to #2471 (Codex review r3771285317).