fix(omp): bound the post-agent_end completion gate - #25
Merged
Conversation
joeshull
force-pushed
the
omp-idle-gate-3371
branch
from
August 21, 2026 21:59
750be93 to
fa9fc5e
Compare
joeshull
marked this pull request as draft
August 21, 2026 22:00
After an assistant-bearing agent_end, the OMP provider polled get_state every 10 ms waiting for a non-streaming, non-compacting state, swallowed every error, and had no deadline. A stale or unreachable state path left the turn showing as running until the user cancelled, and startTurn throws while a turn is active, so the session was wedged for new prompts. The only signal was a debug log, and the daemon runs at info. Give the gate budgets. waitForRetry now backs off 10 ms to 1 s and returns a decision: retry, or abandon because the 60 s wall-clock wait ran out or three consecutive get_state calls failed. Wall clock rather than a retry count, because each get_state carries the JSONL-RPC request timeout and counting attempts would stretch the real wait to tens of minutes. On abandon the turn fails with omp_provider_idle_timeout or omp_provider_state_unavailable, and the diagnostic carries both the last observed state and the last RPC error, so a stale provider is distinguishable from a lost state path. turn_failed already carries code and diagnostic, so no protocol change. The turn is cleared, so recovery is an ordinary prompt. Hold one gate per turn. Every agent_end opened another concurrent loop, and two loops that both observed idle completed the same turn twice. Autonomous cycles carry no turn ID and share a key of their own; they poll like any other gate. The gate holds the terminal payload and the newest agent_end overwrites it, so an error reported by a later cycle is not dropped in favour of the first snapshot. Terminalize in-flight work when the gate gives up. A stall leaves OMP's state unknown, so a tool call or subagent still marked running has no turn left to finish it. Refs getpaseo#3654
The gate re-checked ownership at the top of each poll, but the abandon path ran after two awaits without re-checking. A get_state carries the 30 s RPC timeout, so the window is wide: cancel a stuck turn, send another one, and the stale gate could fail the cancelled turn, cancel the new turn's tool calls, and null its turn ID. Events for the live turn then went out with no turn ID, which the manager back-fills with whatever turn is active. Check ownership before failing, and before completing for the same reason. Give compaction its own budget. The gate waits for isCompacting to clear, and compacting a large context is a model call that outlasts 60 s routinely, so a healthy provider could be failed mid-compaction. The observed compaction state now reaches the scheduler, which allows ten minutes while OMP reports it. Report an unavailable state path whenever no state was ever observed. A hanging get_state burns the 60 s wait budget before three rejections can land, so the failure budget only fired on fast rejections and the hanging case was labelled a timeout. Measure the budget on performance.now(). Date.now() steps with NTP and across suspend, either deferring the deadline or failing a healthy turn on wake. Keep an error the gate already holds when a later agent_end reports none: the fallback payload is a single assistant message, so newest-wins could drop the error the previous cycle reported. Refs getpaseo#3654
getpaseo#3371 adds a third reason the gate can wait: OMP-internal task children still running. That wait had the same shape as the two this branch bounded, so the budget now covers it, with one difference. A get_subagents reply that lists running children is positive evidence OMP is working, and a fan-out has no bounded length, so the wait budget does not apply while that evidence is current. It resumes the moment get_subagents stops answering, which is the stuck-parent risk that closed getpaseo#2245: an index left holding children it can no longer confirm no longer holds the turn open forever. The failure budget for get_state applies throughout, and the diagnostic records that subagents were still reported running. Refs getpaseo#3654, getpaseo#2232
Review found the gate failing healthy turns. elapsedMs measured total gate time while the budget was picked from the current observation, so time OMP spent demonstrably working was charged to the 60 s stall budget: a five-minute fan-out followed by the parent resuming to consume its children's results tripped wait_budget on the first poll after the children finished, cancelling in-flight tool calls and failing a turn that was mid-stream. Give each condition its own clock, restarted when the condition changes. Trust only what OMP confirms. The subagent exemption keyed off the index's hasRunning, which stays true for a lifecycle child no snapshot has ever listed, and off get_subagents merely not throwing. Both left an unbounded path: an index holding a child it can no longer confirm kept the turn open forever, which is the hang this branch exists to remove. The budget now trusts a reply that actually names a running child, and every such reply restarts the clock, so a fan-out runs as long as OMP keeps reporting it and a silent one is bounded. Report the subagent blocker. A gate that gave up while holding running children said "never reported an idle state" next to a diagnostic showing an idle state; it now fails with omp_provider_subagent_stall and says whether OMP still listed those children or only held them unconfirmed. A failed get_state also stops counting as a current subagent report. Take the monotonic clock as an injected dependency so the budget is testable without waiting out real minutes. Refs getpaseo#3654
The manual idle scheduler threw when the gate polled after being told to stop, but that throw lands in the gate promise, which the agent_end call site consumes with a .catch(). A second gate loop would have been absorbed silently and the test would still have passed. Record the poll and assert the record in afterEach instead, count it before waitCount moves so a violating poll cannot satisfy a waitForWaits() a test is blocked on, and deny again rather than throwing so the loop still stops.
Review found the previous accounting still failing live turns. Streaming had no budget of its own, so a second model cycle for the same prompt was charged to the 60 s stall budget and failed a turn mid-stream sixty seconds in. Restarting one clock per wait-class change also meant a state flag flipping between streaming and compacting reset the budget on every poll, and a confirmed subagent report reset it so reliably that its own ten-minute budget could never be reached. The flags were never the evidence. Any event on the session is proof OMP is working, and so is a get_subagents reply naming a running child; both restart the clock. A flag changing is not, and a flag stuck on is the stall being bounded, so neither restarts anything. The state and subagent flags now only choose how long silence may last: ten minutes while compacting or while children are outstanding, sixty seconds otherwise. That also gives OMP builds without get_subagents the subagent budget rather than the bare stall budget. Read the subagent index when the turn fails instead of a flag carried across polls that never queried it, so a finished child is no longer reported as the blocker. Refs getpaseo#3654
Review found two holes in the previous accounting. One clock shared by every budget meant silence accrued while compacting was instantly delinquent when compaction ended and the sixty-second stall budget took over, so a three-minute auto-compaction failed the turn the moment the model resumed. And a get_subagents reply naming a child restarted the clock whether or not anything about that child had changed, so a wedged or merely queued child pinned the budget at zero forever — the hang this branch exists to remove, on the one path it claimed to bound. Count silence per budget class, so time under a long budget is never inherited by a shorter one, and take progress from change rather than presence: a snapshot restarts the budget only when the running children, their statuses, or their lastUpdate differ from the previous reply. Stamp liveness at the runtime handler rather than the session handler. Subagent narration, auto-compaction and host-tool traffic return before handleSessionEvent, so a fan-out OMP was actively reporting counted as silence on builds without get_subagents. Read the subagent index each poll instead of carrying a flag across polls that never queried it, and treat a started tool call as outstanding work, so a tool that runs quietly gets the longer budget rather than the stall budget. Refs getpaseo#3654
Every round of this branch has bounded the gate by deciding what counts as progress, and every round found another signal that could be stuck on: first the state flags, then a get_subagents reply merely listing a child, now any inbound frame — notices and command-list updates arrive on the host's cadence and refilled all three budgets, which put an hour-long wedged turn back to never giving up. Enumerating signals is the wrong shape of fix. Add a wall-clock ceiling measured from the gate opening that nothing resets, so an unforeseen signal costs an hour rather than the turn. Narrow the liveness stamp to frames that mean the turn advanced. Host chatter — notices, command lists, goal timers, todo reminders — is not the model working. Scope outstanding tool calls to the turn that started them. OMP dropping a tool_execution_end used to leave that call in the map for the rest of the session, which quietly gave every later stall the ten-minute budget instead of sixty seconds. Align the budget's class precedence with the accrual's, so silence is compared against the budget it accrued under, and record that the subagent fingerprint assumes OMP moves lastUpdate while a child works. Refs getpaseo#3654
Every test of the completion gate so far drove a fake. Add a real-provider suite that proves a bounded gate did not cost ordinary completion: a plain turn and a fan-out turn each end in exactly one terminal turn event, against OMP 17.4.0. The fan-out case asserts one terminal event rather than a completion. The test model is small enough to call `task` without its required `context` field and error the stream; what the gate owes either way is one terminal event, never two and never none. That is the property, and it survives a model that gets its own tool call wrong. Settle the assumption ompSubagentFingerprint rested on by capturing real get_subagents frames rather than reasoning about them. lastUpdate moves per child activity, not per reply, so it is a progress signal and not a clock. It does not move while a child is busy and quiet: a child running `sleep 40` past its parent's agent_end held one value for 39.6 s across 80 polls, with no inbound frame either. The 500 ms tool_execution_update timer that covers the waiting case belongs to the parent's own `task` tool call, which has already ended once the parent detaches, so the gate's window is real silence. Keying the fingerprint on the running-id set instead would be strictly worse - that set is frozen for the child's whole life. Record the measured behaviour and its bounded consequence in the comment; no logic changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Downstream feature: true
Downstream rationale: Bounds the OMP post-agent_end completion gate so a stalled or unreachable get_state cannot hold a finished turn in
runningforever; authored in this fork, and also proposed upstream into the getpaseo#3371 feature branch.Merge order: this cannot target
internal/mainuntil #29 lands, because it builds on the subagent wait that #29 imports. CI only runs on pull requests targetingmainorinternal/main, so while this is stacked onomp-idle-gate-3371no CI runs on it. Once #29 merges, retarget this tointernal/main; its three commits then apply unchanged, and the downstream-feature exception above needs one non-author human approval of the current head.Refs getpaseo#3654. The first two commits shown in the branch are @jasonhnd's, imported by #29; this pull request's own diff is the three commits after them.
Problem
After an assistant-bearing
agent_end,OmpAgentSession.completeTurnAfterProviderIdle()polledruntimeSession.getState()every 10 ms waiting forisStreaming === false && isCompacting === false, swallowed every error, and had no deadline. If OMP reported a stale state or the state RPC kept failing, the turn showed as running until the user cancelled — andstartTurnthrows while a turn is active, so the session was wedged for new prompts. The only signal was alogger.debugline, and the daemon runs at info level, so nothing surfaced at all.The loop has never been modified since it shipped in getpaseo#2067; the two tests that pin the unbounded wait came in that same commit. getpaseo#2261 and getpaseo#2282 both fixed reaching the gate and explicitly kept it as-is ("the change does not synthesize idle or add a timeout"). getpaseo#3371 adds a third reason to wait inside it. Upstream
can1357/oh-my-pi#6916is a field occurrence of the symptom — a session heldrunningfor 25m44s after the final answer and only cleared on a forced cancel — still open, waiting on RPC evidence the host never emitted.While reproducing this in the provider harness I also found the gate could complete a turn twice: every
agent_endopened another concurrent loop, and two loops that both observed idle each calledcompleteTurn.Change
Budgets.
OmpProviderIdleScheduler.waitForRetry()takes{ attempt, consecutiveFailures, elapsedMs, isCompacting, isWaitingOnSubagents }and returns{retry: true} | {retry: false, reason}. The default scheduler backs off 10 ms → 1 s and gives up after 3 consecutiveget_staterejections, after 60 s of monotonic wall clock, or after 10 minutes if OMP reports compaction. Wall clock rather than a retry count is deliberate: eachget_statecarries the JSONL-RPC request timeout, so counting attempts would let a slow-but-answering state path stretch the wait to tens of minutes. Compaction gets its own budget because the gate waits forisCompactingto clear and compacting a large context is a model call that routinely outlasts 60 s.The subagent wait (getpaseo#3371). A
get_subagentsreply listing running children is positive evidence OMP is working, and a fan-out has no bounded length, so the wait budget does not apply while that evidence is current. It resumes the momentget_subagentsstops answering — which is the stuck-parent risk that closed getpaseo#2245: an index still holding children it can no longer confirm no longer holds the turn open forever. Theget_statefailure budget applies throughout.Structured failure. On abandon the turn fails with
omp_provider_idle_timeoutoromp_provider_state_unavailable. A gate that never observed any state reportsstate_unavailablewhichever budget ran out, because a hangingget_stateburns the wait budget before three rejections can land. The diagnostic carries the last observedisStreaming/isCompacting, the last RPC error, and whether subagents were still reported running.turn_failedalready carriescodeanddiagnostic(agent-sdk-types.ts), so there is nopackages/protocolchange and nothing to gate onserver_info.features. The turn state is cleared, so recovery is an ordinary prompt.One gate per turn. Gates are keyed on the turn ID, with a shared key for autonomous cycles (which carry no turn ID and poll like any other gate). The gate holds the terminal payload and the newest
agent_endoverwrites it — unless that would discard an error the gate already holds, since the fallback payload is a single assistant message.Ownership. Ownership is re-checked before completing and before failing, not only at the top of each poll. Both sites sit after two awaits, and a
get_statecarries the 30 s RPC timeout, so a cancel-then-reprompt could otherwise let a stale gate fail the cancelled turn and clear the new one's turn ID.Terminalize on stall. A stall leaves OMP's state unknown, so tool calls and subagents still marked running are cancelled — otherwise they spin in the UI with no turn left to finish them.
The safety property from the issue is preserved: no ordinary
turn_completedwhile OMP reports streaming, compacting, or running children, and a healthy state response before the budget still completes exactly once.Tests
Sixteen new tests in
packages/server/src/server/agent/providers/omp/agent.test.ts(on top of getpaseo#3371's), each written before its production change and watched fail — except the two that drivecreateOmpProviderIdleScheduler()directly, whose pre-change failure was an import error rather than a behavioral one:agent_end, foreground and autonomousget_statefailures, with the right code each timestate_unavailableagent_enderror wins over the first snapshot, and an earlier error survives a later error-free cycleget_subagentsstops answeringgetpaseo#3371's tests and the two pre-existing tests that assert the gate stays active while OMP is busy or unreachable are unchanged and still pass.
QA
npx vitest run packages/server/src/server/agent/providers/omp— 19 files, 143 tests passnpm run typecheck— cleannpm run linton the changed files — 0 warnings, 0 errorsnpm run format:check— cleanNot exercised against a live OMP process; the harness reproduces every failure mode deterministically. This change surfaces the stall with the last state and RPC error but does not persist raw child RPC frames, which is the evidence
oh-my-pi#6916is still waiting on.