diff --git a/apps/server/src/orchestration-v2/Orchestrator.ts b/apps/server/src/orchestration-v2/Orchestrator.ts index 6abf3f18b593e..3c1005ba81eb8 100644 --- a/apps/server/src/orchestration-v2/Orchestrator.ts +++ b/apps/server/src/orchestration-v2/Orchestrator.ts @@ -1274,10 +1274,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ]); }); - const startNextQueuedRun = ( - threadId: ThreadId, - options?: { readonly failedRunId?: RunId; readonly endedRunId?: RunId }, - ) => + const startNextQueuedRun = (threadId: ThreadId, options?: { readonly resume?: boolean }) => Effect.gen(function* () { // Every terminal run checks the queue. Only a deliverable queued run // needs the transcript for provider handoff and legacy import context. @@ -1314,14 +1311,16 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio // and a message queued for another provider is how users recover. // A message queued behind a worktree that was cancelled or never created // would run in the project checkout instead, so it waits for the user. - const failedRun = latestExecutedRun(projection.runs); + // Commands can make the queue eligible before the terminal listener + // handles the last run. Only an explicit Resume bypasses its pause. + const latestRun = latestExecutedRun(projection.runs); const failureClass = - failedRun?.id === options?.failedRunId - ? latestRootProviderFailure(failedRun, projection.turnItems)?.class + options?.resume !== true + ? latestRootProviderFailure(latestRun, projection.turnItems)?.class : undefined; const worktreeMissing = - failedRun?.id === options?.endedRunId && - failedRun?.workspacePreparation?.type === "worktree" && + options?.resume !== true && + latestRun?.workspacePreparation?.type === "worktree" && projection.thread.worktreePath === null; // A project can turn a provider off while a message waits in the queue, // including between a run ending and this check, or while the queue is @@ -1348,7 +1347,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio instanceDisabledForProject || (failureClass !== undefined && failureClass !== "validation_error" && - failedRun?.providerInstanceId === queuedRun.providerInstanceId) + latestRun?.providerInstanceId === queuedRun.providerInstanceId) ) { const now = yield* DateTime.now; yield* writeSystemEvents( @@ -11074,10 +11073,13 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ); if ( command.type === "queue.resume" || + command.type === "queued-run.reorder" || command.type === "queued-run.edit.cancel" || (command.type === "queued-run.edit" && command.editId !== undefined) ) { - yield* mapDispatchError(command)(startNextQueuedRun(command.threadId)); + yield* mapDispatchError(command)( + startNextQueuedRun(command.threadId, { resume: command.type === "queue.resume" }), + ); } return { sequence: receipt.resultSequence, @@ -11216,10 +11218,13 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio } if ( command.type === "queue.resume" || + command.type === "queued-run.reorder" || command.type === "queued-run.edit.cancel" || (command.type === "queued-run.edit" && command.editId !== undefined) ) { - yield* mapDispatchError(command)(startNextQueuedRun(command.threadId)); + yield* mapDispatchError(command)( + startNextQueuedRun(command.threadId, { resume: command.type === "queue.resume" }), + ); } if (command.type === "notification.delivery.accept") { yield* mapDispatchError(command)(offerDelegatedCompletionDeliveries(command.threadId)); @@ -11272,20 +11277,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio ); } yield* threadDispatch - .withLock( - threadId, - startNextQueuedRun( - threadId, - stored.event.type === "run.updated" - ? { - endedRunId: stored.event.payload.id, - ...(stored.event.payload.status === "failed" - ? { failedRunId: stored.event.payload.id } - : {}), - } - : undefined, - ), - ) + .withLock(threadId, startNextQueuedRun(threadId)) .pipe( Effect.catchCause((cause) => Effect.logWarning("Failed to start the next queued V2 run", { threadId, cause }), diff --git a/apps/server/src/orchestration-v2/QueuedRunEditing.test.ts b/apps/server/src/orchestration-v2/QueuedRunEditing.test.ts index 283731586a803..3e022dd5ee9f2 100644 --- a/apps/server/src/orchestration-v2/QueuedRunEditing.test.ts +++ b/apps/server/src/orchestration-v2/QueuedRunEditing.test.ts @@ -1,6 +1,5 @@ import { assert, it } from "@effect/vitest"; -import { CommandId, EventId, RunId, TurnItemId } from "@t3tools/contracts"; -import * as DateTime from "effect/DateTime"; +import { CommandId, RunId } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; import * as Queue from "effect/Queue"; @@ -8,7 +7,7 @@ import * as Stream from "effect/Stream"; import * as EventSink from "./EventSink.ts"; -import { createQueue, layer } from "./QueuedRunEditing.testkit.ts"; +import { createQueue, layer, pauseTerminalListener } from "./QueuedRunEditing.testkit.ts"; it.layer(layer)("queued message editing", (it) => { it.effect.each([ @@ -21,10 +20,8 @@ it.layer(layer)("queued message editing", (it) => { ({ reason, action }) => Effect.gen(function* () { const name = `${reason}-${action}`; - const { orchestrator, threadId, active, queued, later, editId, begin } = yield* createQueue( - name, - null, - ); + const { orchestrator, threadId, queued, later, editId, begin, endActiveRun } = + yield* createQueue(name, null); const sink = yield* EventSink.EventSinkV2; yield* orchestrator.dispatch(begin); const heldRunIds = yield* Queue.unbounded(); @@ -38,63 +35,7 @@ it.layer(layer)("queued message editing", (it) => { Effect.forkScoped, ); yield* Effect.yieldNow; - const now = yield* DateTime.now; - yield* sink.write({ - events: [ - ...(reason === "provider_error" - ? [ - { - id: EventId.make(`${name}:error`), - type: "turn-item.updated" as const, - threadId, - runId: active.id, - nodeId: active.rootNodeId!, - providerInstanceId: active.providerInstanceId, - occurredAt: now, - payload: { - id: TurnItemId.make(`${name}:error`), - type: "error" as const, - threadId, - runId: active.id, - nodeId: active.rootNodeId!, - providerThreadId: active.providerThreadId, - providerTurnId: null, - nativeItemRef: null, - parentItemId: null, - ordinal: 2, - status: "failed" as const, - title: "Provider failure", - startedAt: now, - completedAt: now, - updatedAt: now, - failure: { - class: "provider_error" as const, - message: "Provider failed.", - code: "provider_failed", - retryable: null, - }, - }, - }, - ] - : []), - { - id: EventId.make(`${name}:terminal`), - type: "run.updated", - threadId, - runId: active.id, - providerInstanceId: active.providerInstanceId, - occurredAt: now, - payload: { - ...active, - status: reason === "provider_error" ? "failed" : "interrupted", - completedAt: now, - ...(reason === "missing_worktree" - ? { workspacePreparation: { type: "worktree" as const, baseRef: "main" } } - : {}), - }, - }, - ], - }); + yield* endActiveRun(reason); assert.equal(yield* Queue.take(heldRunIds), queued.id); yield* orchestrator.dispatch( action === "save" @@ -125,6 +66,165 @@ it.layer(layer)("queued message editing", (it) => { }), ); + it.effect.each([ + { reason: "provider_error", action: "save" }, + { reason: "provider_error", action: "cancel" }, + { reason: "provider_error", action: "reorder" }, + { reason: "missing_worktree", action: "save" }, + { reason: "missing_worktree", action: "cancel" }, + { reason: "missing_worktree", action: "reorder" }, + ] as const)( + "$action holds the queue after $reason before the terminal listener handles it", + ({ reason, action }) => + Effect.gen(function* () { + const name = `before-listener-${reason}-${action}`; + const { orchestrator, threadId, queued, later, editId, begin, endActiveRun } = + yield* createQueue(name, null); + yield* orchestrator.dispatch(begin); + const listener = yield* pauseTerminalListener(threadId); + yield* endActiveRun(reason); + yield* listener.entered; + const before = yield* orchestrator.getThreadProjection(threadId); + assert.isFalse(before.runs.some((run) => run.queueHeld === true)); + const finish = + action === "save" + ? { + type: "queued-run.edit" as const, + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: queued.id, + editId, + text: "Edited", + } + : action === "cancel" + ? { + type: "queued-run.edit.cancel" as const, + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: queued.id, + editId, + } + : { + type: "queued-run.reorder" as const, + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: later.id, + beforeRunId: queued.id, + }; + yield* orchestrator.dispatch(finish); + yield* orchestrator.dispatch(finish); + const after = yield* orchestrator.getThreadProjection(threadId); + for (const id of [queued.id, later.id]) { + const run = after.runs.find((candidate) => candidate.id === id); + assert.equal(run?.status, "queued"); + assert.isTrue(run?.queueHeld); + assert.isFalse(after.turnItems.some((item) => item.runId === id)); + } + assert.equal( + after.runs.find((run) => run.id === queued.id)?.queueEditId, + action === "reorder" ? editId : null, + ); + if (action === "reorder") { + assert.equal(after.runs.find((run) => run.id === later.id)?.queuePosition, 1); + } + yield* orchestrator.dispatch({ + type: "queue.resume", + commandId: CommandId.make(`${name}:resume`), + threadId, + }); + const resumed = yield* orchestrator.getThreadProjection(threadId); + assert.equal( + resumed.runs.find((run) => run.id === (action === "reorder" ? later.id : queued.id)) + ?.status, + "starting", + "An explicit resume still overrides the terminal pause", + ); + yield* listener.release; + }), + ); + + it.effect( + "reordering ahead of an edit-held head starts the eligible message after completion", + () => + Effect.gen(function* () { + const { orchestrator, threadId, queued, later, editId, begin, endActiveRun } = + yield* createQueue("reorder-completed"); + yield* orchestrator.dispatch(begin); + const listener = yield* pauseTerminalListener(threadId); + yield* endActiveRun("completed"); + yield* listener.entered; + yield* listener.release; + yield* listener.handled; + const before = yield* orchestrator.getThreadProjection(threadId); + assert.equal(before.runs.find((run) => run.id === queued.id)?.status, "queued"); + assert.equal(before.runs.find((run) => run.id === later.id)?.status, "queued"); + assert.isFalse(before.runs.some((run) => run.queueHeld === true)); + const reorder = { + type: "queued-run.reorder" as const, + commandId: CommandId.make("reorder-completed:reorder"), + threadId, + runId: later.id, + beforeRunId: queued.id, + }; + yield* orchestrator.dispatch(reorder); + yield* orchestrator.dispatch(reorder); + const after = yield* orchestrator.getThreadProjection(threadId); + assert.equal(after.runs.find((run) => run.id === later.id)?.status, "starting"); + assert.equal(after.runs.find((run) => run.id === queued.id)?.status, "queued"); + assert.equal(after.runs.find((run) => run.id === queued.id)?.queueEditId, editId); + assert.isFalse(after.turnItems.some((item) => item.runId === queued.id)); + assert.equal(after.turnItems.filter((item) => item.runId === later.id).length, 1); + }), + ); + + it.effect.each(["save", "cancel", "reorder"] as const)( + "%s does not pause the queue after a message-local validation failure", + (action) => + Effect.gen(function* () { + const name = `validation-${action}`; + const { orchestrator, threadId, queued, later, editId, begin, endActiveRun } = + yield* createQueue(name); + yield* orchestrator.dispatch(begin); + const listener = yield* pauseTerminalListener(threadId); + yield* endActiveRun("validation_error"); + yield* listener.entered; + yield* orchestrator.dispatch( + action === "save" + ? { + type: "queued-run.edit", + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: queued.id, + editId, + text: "Edited", + } + : action === "cancel" + ? { + type: "queued-run.edit.cancel", + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: queued.id, + editId, + } + : { + type: "queued-run.reorder", + commandId: CommandId.make(`${name}:finish`), + threadId, + runId: later.id, + beforeRunId: queued.id, + }, + ); + const after = yield* orchestrator.getThreadProjection(threadId); + assert.equal( + after.runs.find((run) => run.id === (action === "reorder" ? later.id : queued.id)) + ?.status, + "starting", + ); + assert.isFalse(after.runs.some((run) => run.queueHeld === true)); + yield* listener.release; + }), + ); + it.effect.each(["save", "cancel"] as const)( "%s releases an edit only after completion has left the original queued", (action) => diff --git a/apps/server/src/orchestration-v2/QueuedRunEditing.testkit.ts b/apps/server/src/orchestration-v2/QueuedRunEditing.testkit.ts index 4d5fb16e1d9c3..d3a7462a4bb14 100644 --- a/apps/server/src/orchestration-v2/QueuedRunEditing.testkit.ts +++ b/apps/server/src/orchestration-v2/QueuedRunEditing.testkit.ts @@ -7,15 +7,20 @@ import { ProviderDriverKind, ProviderInstanceId, ThreadId, + TurnItemId, } from "@t3tools/contracts"; import type { ProviderAdapterV2 } from "@t3tools/provider-core/server/ProviderAdapter"; import * as DateTime from "effect/DateTime"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import { vi } from "vite-plus/test"; import { CodexProviderCapabilitiesV2 } from "./Adapters/CodexAdapterV2.ts"; import * as EventSink from "./EventSink.ts"; import * as Orchestrator from "./Orchestrator.ts"; import * as ProviderAdapterRegistry from "./ProviderAdapterRegistry.ts"; +import * as ThreadCommandExecutor from "./ThreadCommandExecutor.ts"; import * as ProviderReplayHarness from "./testkit/ProviderReplayHarness.ts"; const modelSelection = { instanceId: ProviderInstanceId.make("codex"), model: "test" }; @@ -26,12 +31,56 @@ const adapter = { planSelectionTransition: () => Effect.succeed({ type: "apply_on_next_turn" as const }), openSession: () => Effect.die("provider execution is disabled in queue editing tests"), } satisfies ProviderAdapterV2["Service"]; -export const layer = ProviderReplayHarness.layerWithRegistry( - { name: "queued-run-editing" }, - ProviderAdapterRegistry.layerFromAdapters([adapter]), - { runEffectWorker: false }, +export const layer = Layer.merge( + ProviderReplayHarness.layerWithRegistry( + { name: "queued-run-editing" }, + ProviderAdapterRegistry.layerFromAdapters([adapter]), + { runEffectWorker: false }, + ), + ThreadCommandExecutor.layer, ); +// Park the terminal listener before it takes the thread lock, while allowing +// subsequent user commands through the real executor. +export const pauseTerminalListener = Effect.fnUntraced(function* (threadId: ThreadId) { + const executor = yield* ThreadCommandExecutor.ThreadCommandExecutor; + const entered = yield* Deferred.make(); + const released = yield* Deferred.make(); + const handled = yield* Deferred.make(); + const withLock = executor.withLock; + let paused = false; + let finalized = false; + const intercept: typeof withLock = (key, effect) => { + if (key !== threadId) return withLock(key, effect); + if (paused) { + return finalized + ? withLock(key, effect).pipe(Effect.tap(() => Deferred.succeed(handled, undefined))) + : withLock(key, effect); + } + paused = true; + return Deferred.succeed(entered, undefined).pipe( + Effect.andThen(Deferred.await(released)), + Effect.andThen(withLock(key, effect)), + Effect.tap(() => + Effect.sync(() => { + finalized = true; + }), + ), + ); + }; + const spy = vi.spyOn(executor, "withLock").mockImplementation(intercept); + yield* Effect.addFinalizer(() => + Effect.sync(() => spy.mockRestore()).pipe( + Effect.andThen(Deferred.succeed(released, undefined)), + ), + ); + return { + entered: Deferred.await(entered), + release: Deferred.succeed(released, undefined), + handled: Deferred.await(handled), + }; +}); + export const createQueue = Effect.fnUntraced(function* ( name: string, worktreePath: string | null = process.cwd(), @@ -80,22 +129,72 @@ export const createQueue = Effect.fnUntraced(function* ( runId: queued.id, previousEditId: null, }; - const complete = Effect.gen(function* () { + const endActiveRun = Effect.fnUntraced(function* ( + reason: "completed" | "provider_error" | "validation_error" | "missing_worktree", + ) { const now = yield* DateTime.now; yield* sink.write({ events: [ + ...(reason === "provider_error" || reason === "validation_error" + ? [ + { + id: EventId.make(`${name}:error`), + type: "turn-item.updated" as const, + threadId, + runId: active.id, + nodeId: active.rootNodeId!, + providerInstanceId: active.providerInstanceId, + occurredAt: now, + payload: { + id: TurnItemId.make(`${name}:error`), + type: "error" as const, + threadId, + runId: active.id, + nodeId: active.rootNodeId!, + providerThreadId: active.providerThreadId, + providerTurnId: null, + nativeItemRef: null, + parentItemId: null, + ordinal: 2, + status: "failed" as const, + title: "Provider failure", + startedAt: now, + completedAt: now, + updatedAt: now, + failure: { + class: reason, + message: "Provider failed.", + code: "provider_failed", + retryable: null, + }, + }, + }, + ] + : []), { - id: EventId.make(`${name}:complete`), + id: EventId.make(`${name}:terminal`), type: "run.updated", threadId, runId: active.id, providerInstanceId: active.providerInstanceId, occurredAt: now, - payload: { ...active, status: "completed", completedAt: now }, + payload: { + ...active, + status: + reason === "completed" + ? "completed" + : reason === "missing_worktree" + ? "interrupted" + : "failed", + completedAt: now, + ...(reason === "missing_worktree" + ? { workspacePreparation: { type: "worktree" as const, baseRef: "main" } } + : {}), + }, }, ], }); - yield* orchestrator.resumeQueuedRuns; }); - return { orchestrator, threadId, active, queued, later, editId, begin, complete }; + const complete = endActiveRun("completed").pipe(Effect.andThen(orchestrator.resumeQueuedRuns)); + return { orchestrator, threadId, active, queued, later, editId, begin, complete, endActiveRun }; });