fix(omni-runner): sequence route status acks — final reaction waits for the pending one - #2524
Conversation
…ver races the pending one The routed-run path fired the pending and final status reactions as two independent fire-and-forget HTTP calls; a run finishing before the pending call landed could reorder the final ack behind it at the API, leaving a finished run permanently showing the pending glyph (route reactions have no reconciliation pass). emitReaction now returns its always-fulfilled settlement promise and runOneShot chains the final emit on it — errors stay swallowed, nothing user-visible is delayed, and the caller still never awaits the acks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sg8vJv9r2yqmnbtVPqM2vG
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the pending status reaction (⏳) settles before the final success or failure reaction (✅/❌) is dispatched, preventing out-of-order reactions at the API for fast-running tasks. It updates the reaction functions to return their settlement promises and adds corresponding unit tests. The review feedback suggests optimizing this flow by only creating and chaining the pendingAck promise when a messageId is actually present, which avoids unnecessary promise allocations and microtask scheduling on the hot path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // settlement promise (always fulfilled — emit errors are swallowed inside) is | ||
| // kept so the final ✅/❌ can be chained AFTER the ⏳ HTTP call has landed: a run | ||
| // that finishes before the ⏳ reaches the API must never leave ✅→⏳ reordered. | ||
| const pendingAck = emitRouteReaction(route, messageId, STATUS_PENDING); |
There was a problem hiding this comment.
When messageId is not provided (which is common for messages that do not support reactions), calling emitRouteReaction and chaining on its promise is unnecessary. We can optimize this by only creating the promise chain when messageId is present, avoiding useless promise allocations and microtask scheduling on the hot path.
| const pendingAck = emitRouteReaction(route, messageId, STATUS_PENDING); | |
| const pendingAck = messageId ? emitRouteReaction(route, messageId, STATUS_PENDING) : undefined; |
| emitRouteReaction(route, messageId, ok ? STATUS_APPROVED : STATUS_DENIED); | ||
| // Chained on the ⏳ emit's settlement (fulfilled even when it failed) so the | ||
| // pair reaches the API in order; still fire-and-forget for this run. | ||
| pendingAck.finally(() => emitRouteReaction(route, messageId, ok ? STATUS_APPROVED : STATUS_DENIED)); |
There was a problem hiding this comment.
Only chain the final reaction if pendingAck was successfully created (i.e., when messageId is present).
| pendingAck.finally(() => emitRouteReaction(route, messageId, ok ? STATUS_APPROVED : STATUS_DENIED)); | |
| if (pendingAck) { | |
| pendingAck.finally(() => emitRouteReaction(route, messageId, ok ? STATUS_APPROVED : STATUS_DENIED)); | |
| } |
| publish(replySubject, buildRoutedReplyPayload(route.instance, route.chat, content, genId(), now())); | ||
| emitRouteReaction(route, messageId, STATUS_DENIED); // ❌ on timeout / crash | ||
| // ❌ on timeout / crash — same ordering chain as the success path. | ||
| pendingAck.finally(() => emitRouteReaction(route, messageId, STATUS_DENIED)); |
There was a problem hiding this comment.
Only chain the final reaction if pendingAck was successfully created (i.e., when messageId is present).
| pendingAck.finally(() => emitRouteReaction(route, messageId, STATUS_DENIED)); | |
| if (pendingAck) { | |
| pendingAck.finally(() => emitRouteReaction(route, messageId, STATUS_DENIED)); | |
| } |
1 participant
Closes the last confirmed finding from the #2516 independent-review record: the route-scoped ⏳ and final ✅/❌ reactions were two unordered fire-and-forget HTTP calls, so a fast-finishing run could land ✅ before ⏳ and end up permanently displaying ⏳ (route reactions have no reconciliation path).
runOneShotnow captures the ⏳ emit's settlement promise and chains the final emit on.finallyin both the success and catch paths — sequenced, but still never awaited by the run and still swallow-on-error.emitReaction/emitRouteReactionreturn an always-fulfilledPromise<void>. No retries, queues, or reconciliation changes.Independent review verdict: SHIP — all five exit paths traced (busy-notice and publish-throw behaviors byte-identical to dev), the always-fulfilled invariant verified,
whenIdledrain proven loss-free with zero flake across 20 suite runs, and the ordering test independently confirmed revert-sensitive (fails on dev's source exactly on the raced dispatch). Worst case after the fix is a final ack delayed by the ⏳ call's 10s timeout, never a lost or reordered one.Tests: 54 pass (52 pre-existing + 2 new), typecheck + biome clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Sg8vJv9r2yqmnbtVPqM2vG