fix(omp): keep parent non-idle while internal task subagents run - #2245
fix(omp): keep parent non-idle while internal task subagents run#2245jasonhnd wants to merge 1 commit into
Conversation
![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: 638e4a9ff2
ℹ️ 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".
| } | ||
|
|
||
| async getSubagents(): Promise<OmpSubagentSnapshot[]> { | ||
| const data = OmpSubagentsResultSchema.parse(await this.request({ type: "get_subagents" })); |
There was a problem hiding this comment.
When running against OMP builds that predate get_subagents, this call does not fall back promptly: the OMP RPC docs note that unknown-command responses omit the request id, while JsonlRpcProcess only resolves pending requests by id and otherwise waits for its default 30s timeout. In that environment, every otherwise-idle turn now waits about 30 seconds before turn_completed is emitted, making normal prompts appear stuck; use a short timeout/probe and cache unsupported runtimes before gating completion on this RPC. Source checked: https://github.com/can1357/oh-my-pi/blob/main/docs/rpc.md#requestresponse-correlation
Useful? React with 👍 / 👎.
| state.description = snapshot.description ?? state.description; | ||
| state.toolCallId = snapshot.parentToolCallId ?? state.toolCallId; | ||
| state.status = mapSnapshotStatus(snapshot.status); | ||
| events.push(this.upsert(snapshot.id, state.status, state)); |
There was a problem hiding this comment.
When an OMP task child keeps running after agent_end, the idle gate polls roughly every 10ms and reconcileSnapshots returns an upsert for every unchanged snapshot on every poll. The provider-subagent store applies and forwards each upsert with a fresh updatedAt, so a minutes-long fan-out can produce thousands of duplicate WebSocket/store updates per child even though nothing changed; only emit when the descriptor actually changes or debounce this polling path.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| packages/server/src/server/agent/providers/omp/agent.ts | Core fix: adds hasRunningOmpSubagents() guard to completeTurnAfterProviderIdle; logic is correct and well-scoped. |
| packages/server/src/server/agent/providers/omp/subagent-index.ts | Adds hasRunning and reconcileSnapshots; reconcileSnapshots unconditionally overwrites status on every poll (previously flagged in thread). |
| packages/server/src/server/agent/providers/omp/history.ts | Adds orphan child transcript discovery; all discovered orphans are hardcoded as "completed" regardless of actual terminal state. |
| packages/server/src/server/agent/providers/omp/rpc-types.ts | Adds OmpSubagentSnapshotSchema/OmpSubagentsResultSchema and get_subagents command; replaces hand-written interface with schema-inferred type, making index optional and adding detached. |
| packages/server/src/server/agent/providers/omp/agent.test.ts | Two new regression tests for the idle-gate fix; behavior coverage is strong but both tests use a bare setImmediate hack in the test body. |
| packages/server/src/server/agent/providers/omp/subagent-index.test.ts | New unit tests for hasRunning and reconcileSnapshots; clear setup/act/assert and cross the module interface. |
| packages/server/src/server/agent/providers/omp/history-mapper.test.ts | New test for orphan transcript discovery; only covers the success case — a failed/aborted child transcript would show as "completed" due to the hardcoded status. |
| packages/server/src/server/agent/providers/omp/cli-runtime.ts | Adds getSubagents() RPC call with schema validation; straightforward and correct. |
| packages/server/src/server/agent/providers/omp/runtime.ts | Adds getSubagents() to the OmpRuntimeSession interface; clean addition. |
| packages/server/src/server/agent/providers/omp/test-utils/fake-omp.ts | Simplifies FakeOmpSubagentSnapshot to a type alias for OmpSubagentSnapshot; aligns fake with real type. |
Sequence Diagram
sequenceDiagram
participant Paseo
participant OmpAgentSession
participant SubagentIndex
participant OmpRuntimeSession
Note over Paseo,OmpRuntimeSession: Parent model turn ends (agent_end + isStreaming=false)
loop completeTurnAfterProviderIdle
OmpAgentSession->>OmpRuntimeSession: getState()
OmpRuntimeSession-->>OmpAgentSession: "{isStreaming: false, isCompacting: false}"
OmpAgentSession->>OmpRuntimeSession: getSubagents()
OmpRuntimeSession-->>OmpAgentSession: "[{id: child-1, status: running}]"
OmpAgentSession->>SubagentIndex: reconcileSnapshots(runtimeSession, snapshots)
SubagentIndex-->>OmpAgentSession: [upsert events]
OmpAgentSession->>SubagentIndex: hasRunning(runtimeSession)
SubagentIndex-->>OmpAgentSession: true - keep waiting
end
Note over OmpRuntimeSession: Child task completes
OmpRuntimeSession-->>OmpAgentSession: "subagent_lifecycle {status: completed}"
OmpAgentSession->>SubagentIndex: handleLifecycle(...)
loop Next idle poll
OmpAgentSession->>OmpRuntimeSession: getSubagents()
OmpRuntimeSession-->>OmpAgentSession: "[{id: child-1, status: completed}]"
OmpAgentSession->>SubagentIndex: reconcileSnapshots(...)
OmpAgentSession->>SubagentIndex: hasRunning(runtimeSession)
SubagentIndex-->>OmpAgentSession: false - complete turn
OmpAgentSession->>Paseo: turn_completed
end
Reviews (6): Last reviewed commit: "fix(omp): keep parent non-idle while int..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| reconcileSnapshots( | ||
| parent: object, | ||
| snapshots: ReadonlyArray<{ | ||
| id: string; | ||
| agent: string; | ||
| description?: string; | ||
| status: "pending" | "running" | "completed" | "failed" | "aborted"; | ||
| parentToolCallId?: string; | ||
| }>, | ||
| ): AgentStreamEvent[] { | ||
| const events: AgentStreamEvent[] = []; | ||
| for (const snapshot of snapshots) { | ||
| const state = this.stateFor(parent, snapshot.id, snapshot.agent); | ||
| state.title = snapshot.agent || state.title; | ||
| state.description = snapshot.description ?? state.description; | ||
| state.toolCallId = snapshot.parentToolCallId ?? state.toolCallId; | ||
| state.status = mapSnapshotStatus(snapshot.status); | ||
| events.push(this.upsert(snapshot.id, state.status, state)); | ||
| } | ||
| return events; | ||
| } |
There was a problem hiding this comment.
terminalizeRunning sets in-flight children to "canceled" and emits those events. If completeTurnAfterProviderIdle is still polling on the same tick, the subsequent reconcileSnapshots call can receive "running" from OMP (OMP hasn't processed the abort yet) and unconditionally overwrite the now-"canceled" index entry back to "running". The loop exits correctly via the activeTurnId guard, but the contradiction produces a spurious "running" upsert event after the "canceled" upsert — visible to any downstream consumer ordering those events. A forward-progress guard (only allow status changes in the terminal direction) would prevent the reversal.
| ): AgentStreamEvent[] { | ||
| const events: AgentStreamEvent[] = []; | ||
| for (const snapshot of snapshots) { | ||
| const state = this.stateFor(parent, snapshot.id, snapshot.agent); | ||
| state.title = snapshot.agent || state.title; | ||
| state.description = snapshot.description ?? state.description; | ||
| state.toolCallId = snapshot.parentToolCallId ?? state.toolCallId; | ||
| state.status = mapSnapshotStatus(snapshot.status); | ||
| events.push(this.upsert(snapshot.id, state.status, state)); | ||
| } | ||
| return events; |
There was a problem hiding this comment.
reconcileSnapshots unconditionally pushes an upsert event for every snapshot on every call, regardless of whether status or metadata changed. Because hasRunningOmpSubagents calls this on every completeTurnAfterProviderIdle iteration, a 60-second fan-out with 5 children at a 1-second poll rate generates ~300 extra upsert events, all carrying identical "running" status. Adding a guard that only emits when the recorded state actually changes would limit events to genuine transitions.
06eae4e to
f39f809
Compare
OMP can end the parent model loop (agent_end + isStreaming=false) while task children still write under the session stem directory. Gate turn completion on running subagents, reconcile via get_subagents, and recover orphan child history when the parent .jsonl is missing. Fixes getpaseo#2232
f39f809 to
1ecc693
Compare
|
Reviewed this against a live OMP v17 session while chasing the same issue, and found two gaps. The idle-gate half of #2232 works — parent stays non-idle. The subagent-card half does not, and I think the test in this PR hides it. 1. The card still terminalizes on dispatch
OMP's
At subscription level So the deferral only engages in the ordering that never occurs. The non-deferred branch runs, the card flips to Parent idle is unaffected, since The regression test encodes the wrong order. 2.
|
diff --git a/packages/server/src/server/agent/providers/omp/agent.ts b/packages/server/src/server/agent/providers/omp/agent.ts
index df838cfdb..a396df940 100644
--- a/packages/server/src/server/agent/providers/omp/agent.ts
+++ b/packages/server/src/server/agent/providers/omp/agent.ts
@@ -1664,11 +1664,11 @@ export class OmpAgentSession implements AgentSession {
this.subagentCardTracker.handleLifecycle(payload, (toolCallId) =>
this.emitActiveToolCall(toolCallId),
);
- this.settleDeferredTaskCall(payload.parentToolCallId);
}
for (const mapped of this.subagentIndex.handleLifecycle(this.runtimeSession, payload)) {
this.emit(mapped);
}
+ this.settleDeferredTaskCalls();
return true;
}
if (event.type === "subagent_progress") {
@@ -1677,11 +1677,11 @@ export class OmpAgentSession implements AgentSession {
this.subagentCardTracker.handleProgress(payload, (toolCallId) =>
this.emitActiveToolCall(toolCallId),
);
- this.settleDeferredTaskCall(payload.parentToolCallId);
}
for (const mapped of this.subagentIndex.handleProgress(this.runtimeSession, payload)) {
this.emit(mapped);
}
+ this.settleDeferredTaskCalls();
return true;
}
if (event.type === "subagent_event") {
@@ -1940,21 +1940,25 @@ export class OmpAgentSession implements AgentSession {
const result = parseToolResult(event.result);
const error = event.isError ? event.result : null;
const status = event.isError ? "failed" : "completed";
+ if (event.toolName === "task" && !event.isError) {
+ // OMP's `task` defaults to `async.enabled=true` (RPC mode resets it to the
+ // built-in default), so this result means "the children were dispatched",
+ // not "the children finished" — and the first child frame lands *after* it
+ // (measured ~2.3s at subscription level "events"). Gating the deferral on
+ // already-observed children therefore never fires for the case it exists
+ // for. Defer unconditionally and let the subagent index — which receives
+ // late frames outside the `activeToolCalls` guard — decide when the call
+ // is really done. Re-adding the id to `activeToolCalls` is what re-opens
+ // the card channel so those frames still reach the tracker.
+ this.deferredTaskCompletions.set(event.toolCallId, { toolCall, result, error, status });
+ this.activeToolCalls.set(event.toolCallId, toolCall);
+ this.emitToolCallEvent(event.toolCallId, toolCall, "running", result, null);
+ this.settleDeferredTaskCall(event.toolCallId);
+ return;
+ }
+ this.emitToolCallEvent(event.toolCallId, toolCall, status, result, error);
if (event.toolName === "task") {
- // OMP's batched `task` resolves once the children are dispatched, so this
- // result is not their completion. Hold the Paseo tool call open — and keep
- // it in `activeToolCalls` so later child frames still route to its card —
- // until every child reports a terminal status.
- if (this.subagentCardTracker.hasRunningItems(event.toolCallId)) {
- this.deferredTaskCompletions.set(event.toolCallId, { toolCall, result, error, status });
- this.activeToolCalls.set(event.toolCallId, toolCall);
- this.emitToolCallEvent(event.toolCallId, toolCall, "running", result, null);
- return;
- }
- this.emitToolCallEvent(event.toolCallId, toolCall, status, result, error);
this.subagentCardTracker.delete(event.toolCallId);
- } else {
- this.emitToolCallEvent(event.toolCallId, toolCall, status, result, error);
}
if (event.toolName === "todo") {
const item = mapOmpTodoToolResult(result);
@@ -1971,9 +1975,20 @@ export class OmpAgentSession implements AgentSession {
* finished. Emits before dropping the card so the final tool call still
* carries the accumulated child log.
*/
- private settleDeferredTaskCall(toolCallId: string): void {
+ private settleDeferredTaskCall(toolCallId: string, force = false): void {
const deferred = this.deferredTaskCompletions.get(toolCallId);
- if (!deferred || this.subagentCardTracker.hasRunningItems(toolCallId)) {
+ if (!deferred) {
+ return;
+ }
+ // Before the first child frame arrives the index knows nothing about this
+ // call, which must not read as "done" — require a linked child *and* no
+ // running ones. `force` is the turn-end backstop for a `task` that never
+ // produced a child at all.
+ if (
+ !force &&
+ (!this.subagentIndex.knowsToolCall(this.runtimeSession, toolCallId) ||
+ this.subagentIndex.hasRunningForToolCall(this.runtimeSession, toolCallId))
+ ) {
return;
}
this.deferredTaskCompletions.delete(toolCallId);
@@ -1988,6 +2003,12 @@ export class OmpAgentSession implements AgentSession {
this.subagentCardTracker.delete(toolCallId);
}
+ private settleDeferredTaskCalls(force = false): void {
+ for (const toolCallId of this.deferredTaskCompletions.keys()) {
+ this.settleDeferredTaskCall(toolCallId, force);
+ }
+ }
+
/**
* OMP can resume model work after Paseo already completed a turn — a parked
* hub wait releasing, or autonomous follow-up work. Without a `turn_start` for
@@ -2214,6 +2235,10 @@ export class OmpAgentSession implements AgentSession {
}
private completeTurn(turnId: string | undefined, messages: OmpAgentMessage[]): void {
+ // The idle gate already waited for every OMP child to leave `running`, so a
+ // still-deferred `task` here produced no child at all (an instant no-op or a
+ // spawn OMP never reported). Settle it rather than leaking an open card.
+ this.settleDeferredTaskCalls(true);
this.activeTurnId = null;
this.activeClientMessageId = null;
this.activeAssistantMessageId = null;
diff --git a/packages/server/src/server/agent/providers/omp/subagent-card-tracker.ts b/packages/server/src/server/agent/providers/omp/subagent-card-tracker.ts
index d742c1067..7034c4c50 100644
--- a/packages/server/src/server/agent/providers/omp/subagent-card-tracker.ts
+++ b/packages/server/src/server/agent/providers/omp/subagent-card-tracker.ts
@@ -159,25 +159,6 @@ export class OmpSubagentCardTracker {
return baseDetail.actions ? { ...detail, actions: baseDetail.actions } : detail;
}
- /**
- * True while any tracked child of this `task` call is still in-flight. OMP's
- * batched `task` returns once children are dispatched, so the tool result is
- * not a completion signal — the card has to stay open until this goes false.
- */
- hasRunningItems(toolCallId: string): boolean {
- const state = this.states.get(toolCallId);
- if (!state) {
- return false;
- }
- for (const item of state.items.values()) {
- const status = item.status ?? "running";
- if (status === "pending" || status === "running") {
- return true;
- }
- }
- return false;
- }
-
flush(toolCallId: string): void {
const state = this.states.get(toolCallId);
if (!state || !state.dirty) {
diff --git a/packages/server/src/server/agent/providers/omp/subagent-index.ts b/packages/server/src/server/agent/providers/omp/subagent-index.ts
index 5642e5451..7f9dbd386 100644
--- a/packages/server/src/server/agent/providers/omp/subagent-index.ts
+++ b/packages/server/src/server/agent/providers/omp/subagent-index.ts
@@ -17,6 +17,8 @@ interface OmpSubagentState {
status: "running" | "completed" | "failed" | "canceled";
/** Serialized last upsert forwarded from a `get_subagents` poll, for dedupe. */
lastReconciled: string | null;
+ /** Whether a successful `get_subagents` reply has ever listed this child. */
+ seenInSnapshot: boolean;
mapper: OmpHistoryMapper;
}
@@ -99,6 +101,43 @@ export class OmpSubagentIndex {
return false;
}
+ /**
+ * True while a child of this specific `task` tool call is still in-flight.
+ * A child whose parent link has not arrived yet (`toolCallId` still null —
+ * `handleEvent` can learn a child before any frame names its parent) counts
+ * as in-flight for every open call, so a partially populated index can never
+ * settle a card early.
+ */
+ hasRunningForToolCall(parent: object, toolCallId: string): boolean {
+ const states = this.statesByParent.get(parent);
+ if (!states) {
+ return false;
+ }
+ for (const state of states.values()) {
+ if (state.status !== "running") {
+ continue;
+ }
+ if (state.toolCallId === null || state.toolCallId === toolCallId) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /** True once any child has been linked to this `task` tool call. */
+ knowsToolCall(parent: object, toolCallId: string): boolean {
+ const states = this.statesByParent.get(parent);
+ if (!states) {
+ return false;
+ }
+ for (const state of states.values()) {
+ if (state.toolCallId === toolCallId) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/**
* Merge a `get_subagents` snapshot into local state so missed event frames
* do not leave idle detection blind.
@@ -116,6 +155,7 @@ export class OmpSubagentIndex {
const events: AgentStreamEvent[] = [];
for (const snapshot of snapshots) {
const state = this.stateFor(parent, snapshot.id, snapshot.agent);
+ state.seenInSnapshot = true;
state.title = snapshot.agent || state.title;
state.description = snapshot.description ?? state.description;
state.toolCallId = snapshot.parentToolCallId ?? state.toolCallId;
@@ -136,6 +176,28 @@ export class OmpSubagentIndex {
state.lastReconciled = descriptor;
events.push(event);
}
+ // OMP's `get_subagents` reply lists only still-running children: a finished
+ // child disappears rather than reporting a terminal status, so absence is
+ // the only terminal signal this channel gives. Without this sweep the merge
+ // is one-directional — it can confirm "still running" but never clear it,
+ // and a dropped terminal frame wedges the idle gate open forever (the
+ // #2260/#2281 stuck-running class). Only sweep ids a snapshot previously
+ // listed, so a child whose lifecycle frame beat its first snapshot is not
+ // terminalized on arrival.
+ const states = this.statesByParent.get(parent);
+ if (!states) {
+ return events;
+ }
+ const present = new Set(snapshots.map((snapshot) => snapshot.id));
+ for (const [id, state] of states) {
+ if (state.status !== "running" || !state.seenInSnapshot || present.has(id)) {
+ continue;
+ }
+ state.status = "completed";
+ const event = this.upsert(id, state.status, state);
+ state.lastReconciled = JSON.stringify(event.event);
+ events.push(event);
+ }
return events;
}
@@ -154,6 +216,7 @@ export class OmpSubagentIndex {
toolCallId: null,
status: "running",
lastReconciled: null,
+ seenInSnapshot: false,
mapper: new OmpHistoryMapper("omp", [], OMP_HISTORY_MAPPER_HOOKS),
};
states.set(id, state);The two behavioural changes are the |
|
Replaced by #3371. This PR had the right idle-gate idea, but review found two defects that made it unsafe to land: the card tests used the reverse of the OMP v17 wire order, and snapshot reconcile never treated a disappeared #3371 rewrites both halves on current |
2 participants
Summary
Fixes #2232: an OMP parent agent can flip to idle / UI “stopped” while OMP-internal
tasksubagents are still actively writing child session logs.Background / root cause
Paseo marks an OMP turn complete after:
agent_end(with an assistant message), andget_state()reports!isStreaming && !isCompactingThat only means the parent model loop is idle.
OMP’s
isStreamingis approximately:It does not include background
task/ hub-wait children. So a long fan-out audit can:*.jsonlunder the session artifacts directoryThis matches the reporter’s observation:
paseo inspect→Status: idlenativeHandle→ missing....jsonl..../directory with live child jsonlsOMP’s on-disk layout (confirmed against OMP v17.0.5) is:
Paseo already assumed that layout for history replay. The bug was treating parent turn idle as whole job finished.
What this PR changes
Turn-complete gate (main fix)
In
completeTurnAfterProviderIdle, also require no running OMP-internal subagents before emittingturn_completed/ flipping the agent idle.get_subagentsreconciliationEach idle poll calls OMP’s
get_subagentsRPC and merges into the local subagent index.Orphan child history recovery
If the parent
.jsonlis missing (ENOENT) but the stem artifacts directory exists, history still surfaces child transcripts instead of returning empty.Non-goals (intentionally not in this PR)
waiting/delegated)nativeHandlefrom file path to directory (OMP--sessionstill expects the logical.jsonlpath)Evidence
Issue reproduction shape (from #2232)
Reporter case:
0.1.110, OMP17.0.5idlewhile child files under session stem continued updating.jsonlhandles; the broken case had missing parent file + live stem directoryCode-path evidence (this repo)
Before this PR, turn completion was solely:
OMP subagent events were already mapped (
subagent_lifecycle/subagent_progress→provider_subagent), but never consulted when deciding whether the parent turn was done.After this PR:
hasRunningOmpSubagents()reconciles viaget_subagents, then checks the index.Upstream OMP evidence
OMP v17.0.5:
get_stateexposesisStreaming/sessionFilebut not running childrenget_subagents→{ subagents: [...] }with per-child status.jsonlstrippedSo Paseo was missing a signal that OMP already exposes.
Test plan
Automated (ran locally)
38 tests passed, including new coverage:
stays active while OMP internal task subagents are still runningagent_end+isStreaming=false+ running lifecycle child ⇒ noturn_completeduntil child completesstays active when get_subagents reports running children without prior eventsget_subagentsalone can block idle and later release ithasRunning/reconcileSnapshotsunit testsdiscovers child transcripts when parent session file is missingAlso:
npm run typecheck(pre-commit / workspace)npm run linton touched filesManual verification checklist
tasksubagents (audit/research style)runningin list/inspectidle.jsonlis absent but stem dir has child jsonlsProblem solved
.jsonl⇒ empty historyCloses #2232