fix(omp): bound the post-agent_end completion gate (stacked on #3371) - #3667
fix(omp): bound the post-agent_end completion gate (stacked on #3371)#3667joeshull wants to merge 5 commits into
Conversation
OMP can end the parent model loop while `task` children are still writing. Do not complete the Paseo turn until the subagent index reports no runners. Reconcile successful get_subagents replies so listed ids stay running, and treat absence of a previously listed id as completed. Never-listed lifecycle children stay running so an empty first snapshot cannot fake-idle the parent. Closes getpaseo#2232
OMP emits tool_execution_end for `task` as a dispatch ack, then starts children. Keep the parent call running and settle it from the subagent index once a linked child exists and none remain running. completeTurn force-settles a task that never produced a child. Wire-order tests emit the result before subagent_lifecycle started.
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
|
Closing this — on reflection it shouldn't carry #3371's commits. Merging it would have pulled that PR in without its own review, and the gate changes it builds on are @jasonhnd's to land. The three commits on top are still available at ZGEnergy:omp-idle-budget-upstream if they're useful to anyone: they bound the post-agent_end idle gate (#3654) so a stale or unreachable get_state can't hold a finished turn in running forever, and they cover the subagent wait #3371 adds. I'll reopen against main once #3371 lands, or happy for those commits to be taken into #3371 directly instead. |
|
Reopening as a draft rather than sitting on this until #3371 lands — @jasonhnd got the notification already, and the findings are more useful to you now than after your PR merges. Two of them touch #3371 directly:
Also worth knowing: every Take any of it into #3371 if it's useful — I'd rather that than this stack. |
|
Closing in favour of jasonhnd#1, which puts the same three commits into @jasonhnd's The findings stand on their own if that PR isn't wanted: the post-agent_end gate has had no deadline since #2067, |
2 participants
Fixes #3654. This branch is #3371's head (
fa9fc5e62) plus three commits. GitHub shows all five until #3371 merges; review only the last three.I cannot push to this repository, so I cannot open this against a base branch holding #3371's commits. Once #3371 lands on
main, this narrows to its own three commits automatically. Merges cleanly into currentmain(8f0c35e92); no omp drift since #3371's base.I cannot push to this repository, so I cannot open this against a base branch holding #3371's commits. If a maintainer creates that branch here, or once #3371 lands on
main, this PR narrows to its own three commits automatically. Merges cleanly into currentmain(8f0c35e92); no omp drift since #3371's base.Problem
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, so nothing surfaces.The loop has not changed since it shipped in #2067, and the two tests that pin the unbounded wait came in that same commit. #2261 and #2282 both fixed reaching the gate and explicitly kept it as-is ("the change does not synthesize idle or add a timeout"). #3371 adds a third reason to wait inside it — hence the stack.
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
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 routinely outlasts 60 s.The subagent wait from #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 #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, and it logs atwarn.turn_failedalready carriescodeanddiagnostic, 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. Every
agent_endused to open another concurrent loop, and two loops that both observed idle each calledcompleteTurn— twoturn_completedfor one turn, reproduced in the harness. Gates are keyed on the turn ID, with a shared key for autonomous cycles, which poll like any other gate. 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, cancel the new turn's tool calls, and clear its turn ID — after which its events go out with no turn ID and the manager back-fills whatever turn is active.Terminalize on stall. A stall leaves OMP's state unknown, so tool calls and subagents still marked running are cancelled; otherwise they spin with no turn left to finish them.
The safety property holds: no ordinary
turn_completedwhile OMP reports streaming, compacting, or running children, and a healthy state response before the budget still completes exactly once. #3371's tests and the two pre-existing gate tests are unchanged and pass.Tests
Sixteen new tests in
packages/server/src/server/agent/providers/omp/agent.test.ts, 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 answeringQA
npx vitest run packages/server/src/server/agent/providers/omp— 19 files, 143 tests passnpm run linton the omp provider — 0 warnings, 0 errorsnpm run format:check— cleannpm run typecheckin my checkout reports missing-module errors for@replit/codemirror-lang-csharpand@paseo/pluginat this base; those come from resolving mynode_modulesagainst a different lockfile, not from these commits. CI is the authority.Not exercised against a live OMP process; the harness reproduces every failure mode deterministically. This surfaces a 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.