fix(omp): bound the post-agent_end completion gate - #1
Open
joeshull wants to merge 9 commits into
Open
Conversation
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
Author
|
Superseded by getpaseo#3667, which stacks the same three commits on getpaseo#3371 in the upstream repository so review happens there rather than in your fork. Closing this to avoid two paths for the same change — happy to reopen if you would rather carry the commits on your branch directly. |
Author
|
Reopening — you should have the option to take these directly rather than reviewing them under someone else's pull request. Three commits on top of I've closed my upstream PR (getpaseo#3667) in favour of this, so getpaseo#3371 stays the one place this gets reviewed. Merge, cherry-pick, rewrite, or close it — all fine by me. |
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
1 participant
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.
Targets your
fix/2232-omp-idle-gatebranch (getpaseo#3371) rather than upstreammain, because it builds directly on the subagent wait your PR adds. Nine commits on top offa9fc5e62; applies cleanly, no conflicts.Addresses getpaseo#3654.
Updated 2026-08-21. This offer previously carried three commits and described a different budget model. Five review rounds since then changed the design in ways that matter to your branch — read the subagent section below even if you read the earlier version, because the earlier one said the opposite.
bound the post-agent_end completion gate— budgets and a structuredturn_failedinstead of an unbounded 10 ms poll; also stops a secondagent_endopening a concurrent gate.keep the idle gate from failing a turn it no longer owns— re-check ownership after the awaits, give compaction its own budget, use a monotonic clock.let the idle budget cover the subagent wait— the only one that touches your change.budget each idle wait from when that wait beganmake a poll after gate abandonment fail the testspend the idle budget on silence, not on elapsed timecount idle silence per budget and per real progressput a ceiling on the idle gate that no progress can liftrun the idle gate against a real OMP and measure lastUpdateWhy it belongs on top of getpaseo#3371
completeTurnAfterProviderIdle()pollsget_stateevery 10 ms after an assistant-bearingagent_end, swallows every error, and has no deadline. If OMP reports a stale state or the state RPC keeps failing, the turn shows as running until the user cancels — andstartTurnthrows while a turn is active, so the session is wedged for new prompts. The only signal is alogger.debugline, and the daemon runs at info level.The loop has not changed since it shipped in getpaseo#2067, and 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"). Your PR adds a third reason to wait inside it, which is why this is stacked here rather than sent separately.
can1357/oh-my-pi#6916is a field occurrence of the symptom — a session heldrunningfor 25m44s after the final answer, cleared only by a forced cancel. It is still open, waiting on RPC evidence the host never emitted.Change
The budget spends silence, not elapsed time. This is the part that changed most since the first version of this offer. Each poll, a progress signal — an inbound frame that means the turn advanced, or a
get_subagentsreply that differs from the last one — zeroes the accrued silence. Otherwise the delta accrues. Silence is kept in three separate buckets, one per class (compaction, work, other), so time spent under the 10-minute compaction budget is not inherited by the 60-second stall budget the moment compaction ends.Five rounds of review each found another signal that could be stuck on: first the state flags, then a
get_subagentsreply merely listing a child, then any inbound frame. Enumerating signals was the wrong shape of fix, so there is now a wall-clock ceiling of one hour measured from the gate opening that nothing resets. An unforeseen sixth signal costs an hour rather than the turn.The numbers: back off 10 ms → 1 s; give up after 3 consecutive
get_staterejections, after 60 s of silence, after 10 minutes of silence while compacting or while work is outstanding, or at the 1-hour ceiling whatever the class.Your subagent wait. The earlier version of this offer said the wait budget does not apply while
get_subagentsreports running children. That is no longer true, and the reversal is deliberate: a child merely being listed again is the same stuck flag the state field would be. Only a reply that changed since the last one counts as progress. A confirmed running child buys the longer 10-minute budget, not an unbounded wait.Measured, not assumed. That design rests on OMP moving
lastUpdatewhile a child works, so commit 9 captures realget_subagentsframes instead of reasoning about it. Against OMP 17.4.0:lastUpdatemoves per child activity, not per reply — so it is a progress signal, not a clock. 62 samples, 9 distinct values.sleep 25held one value for 25.1 s.agent_endthere is no other signal either. A detached child runningsleep 40held onelastUpdatefor 39.6 s across 80 polls with zero inbound frames. The 500 mstool_execution_updatetimer that covers the waiting case belongs to the parent's owntasktool call, which has already ended once the parent detaches.The bounded consequence, stated plainly: a child that is healthy but silent for longer than 10 minutes fails its parent turn, because a snapshot that never changes is indistinguishable from a wedged one. The 1-hour ceiling is the backstop. If that tradeoff is wrong for the workloads you have in mind, the subagent budget is one constant and I have no attachment to its value.
Structured failure. On abandon the turn fails with
omp_provider_idle_timeout,omp_provider_state_unavailable, oromp_provider_subagent_stall; 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, so there is nopackages/protocolchange.One gate per turn. Every
agent_endused to open another concurrent loop, and two loops that both observed idle each calledcompleteTurn— twoturn_completedfor one turn, which I reproduced in the harness. Gates are now keyed on the turn ID, with a shared key for autonomous cycles. The gate holds the terminal payload and the newestagent_endoverwrites it, unless that would discard an error the gate already holds.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.
The safety property 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. Your tests and the two pre-existing gate tests are unchanged and pass.Verification
npx vitest run packages/server/src/server/agent/providers/omp— 19 files, 161 unit tests pass. The same suite at your head (fa9fc5e62) is 127.npm run test:integration:real --workspace=@getpaseo/server— the 2 new real-provider tests pass against a local OMP 17.4.0. A plain turn and a fan-out turn each end in exactly one terminal turn event.npm run linton the omp provider — 0 warnings, 0 errors.npm run format:check— clean.The earlier version of this offer said "not exercised against a live OMP process". That gap is now closed, which is how the
lastUpdatenumbers above were obtained.The fan-out real test asserts one terminal turn event rather than a completion. The small test model (
openrouter/google/gemini-2.5-flash-lite) callstaskwithout its requiredcontextfield often enough to error the stream; what the gate owes either way is one terminal event, never two and never none. The plain-turn test has passed every run; the fan-out test has passed 4 consecutive runs since it was reformulated this way.I have not run
npm run typecheckon this base — my checkout resolvesnode_modulesagainst a different lockfile, which produces missing-module noise unrelated to these commits. Instead I checked the dependency closure: the omp provider directory here is byte-identical to the branch I typecheck, lint, and test on. Of its 20 relative imports outside that directory, 19 are byte-identical to that branch; the twentieth,agent-sdk-types.ts, differs only by the steer types (AgentSteerOptions,SteerResult,SteerActiveTurnOptions,steerActiveTurn,acceptsPromptDuringAutonomousTurn), none of which the omp code references. Its one cross-package import,@getpaseo/protocol/provider-manifest, is byte-identical. Worth a CI run on your side regardless.Happy to fold these into your branch directly instead if you'd rather carry them yourself, and equally happy for you to take only the commits you want — 1, 2, and 4–8 stand without 3 if you'd rather handle the subagent interaction yourself.