Skip to content

Commit 56ecb81

Browse files
author
PR Batch Tester
committed
fix(server): serialize forks with source thread deletion
1 parent 284c677 commit 56ecb81

3 files changed

Lines changed: 187 additions & 5 deletions

File tree

‎apps/server/src/orchestration-v2/Orchestrator.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4246,7 +4246,14 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
42464246
}),
42474247
),
42484248
);
4249-
// The source is unlocked; validate the modes copied here, not an earlier read.
4249+
if (sourceProjection.thread.deletedAt !== null) {
4250+
return yield* new OrchestratorDispatchError({
4251+
commandId: command.commandId,
4252+
commandType: command.type,
4253+
cause: `Thread ${command.sourceThreadId} is deleted.`,
4254+
});
4255+
}
4256+
// Source settings can change outside the deletion lock; validate the modes copied here.
42504257
yield* refuseAboveDispatchModeLimit(command, command.sourceThreadId, sourceProjection.thread);
42514258

42524259
const sourceRun = runForSourcePoint(sourceProjection, command.sourcePoint);
@@ -13293,7 +13300,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
1329313300
const dispatchWithReceipt = Effect.fnUntraced(function* (command: OrchestrationV2ServerCommand) {
1329413301
const dispatch = threadDispatch.withLock(
1329513302
commandThreadId(command),
13296-
command.type === "thread.delete"
13303+
command.type === "thread.delete" || command.type === "thread.fork"
1329713304
? threadDispatch.withDeletionLock(dispatchWithReceiptEffect(command))
1329813305
: dispatchWithReceiptEffect(command),
1329913306
);

‎apps/server/src/orchestration-v2/ThreadCommandExecutor.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,8 @@ export class ThreadCommandExecutor extends Context.Service<
99
ThreadCommandExecutor,
1010
KeyedLock.KeyedLock<ThreadId> & {
1111
/**
12-
* Serializes thread deletion planning through its commit, since deleting one thread
13-
* reads whether other threads still show its pages. Take it inside the thread lock and
14-
* never take a thread lock while holding it.
12+
* Serializes fork and deletion planning through commit because forks retain source pages.
13+
* Take it inside the thread lock; never take a thread lock while holding it.
1514
*/
1615
readonly withDeletionLock: <A, E, R>(effect: Effect.Effect<A, E, R>) => Effect.Effect<A, E, R>;
1716
}

‎apps/server/src/orchestration-v2/runtimeLayer.test.ts‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import * as Stream from "effect/Stream";
4444
import * as TestClock from "effect/testing/TestClock";
4545
import * as SqlClient from "effect/sql/SqlClient";
4646

47+
import { createAttachmentId } from "../attachmentStore.ts";
4748
import * as CheckpointStore from "../checkpointing/CheckpointStore.ts";
4849
import * as GitWorkflow from "../git/GitWorkflowService.ts";
4950
import * as ServerConfig from "../config.ts";
@@ -719,6 +720,181 @@ it.layer(layerProjectDeletionTest)("project deletion during thread commands", (i
719720
);
720721
}),
721722
);
723+
724+
it.effect.each(["fork", "delete"] as const)(
725+
"preserves page ownership when %s commits before a concurrent fork or deletion",
726+
(first) =>
727+
Effect.gen(function* () {
728+
const orchestrator = yield* Orchestrator.OrchestratorV2;
729+
const eventSink = yield* EventSink.EventSinkV2;
730+
const executor = yield* ThreadCommandExecutor.ThreadCommandExecutor;
731+
const now = yield* DateTime.now;
732+
const sourceThreadId = ThreadId.make(`fork-delete-source:${first}`);
733+
const targetThreadId = ThreadId.make(`fork-delete-target:${first}`);
734+
const runId = RunId.make(`fork-delete-run:${first}`);
735+
const forkCommandId = CommandId.make(`fork-delete-fork:${first}`);
736+
const deleteCommandId = CommandId.make(`fork-delete-delete:${first}`);
737+
const attachmentId = createAttachmentId(sourceThreadId, "html")!;
738+
yield* orchestrator.dispatch({
739+
type: "thread.create",
740+
commandId: CommandId.make(`fork-delete-create:${first}`),
741+
threadId: sourceThreadId,
742+
projectId: ProjectId.make(`fork-delete-project:${first}`),
743+
title: "Page source",
744+
modelSelection,
745+
runtimeMode: "full-access",
746+
interactionMode: "default",
747+
branch: null,
748+
worktreePath: null,
749+
createdBy: "user",
750+
creationSource: "web",
751+
});
752+
yield* eventSink.write({
753+
events: [
754+
{
755+
id: EventId.make(`fork-delete-run:${first}`),
756+
type: "run.created",
757+
threadId: sourceThreadId,
758+
runId,
759+
occurredAt: now,
760+
payload: {
761+
id: runId,
762+
threadId: sourceThreadId,
763+
ordinal: 1,
764+
providerInstanceId: modelSelection.instanceId,
765+
modelSelection,
766+
providerThreadId: null,
767+
userMessageId: MessageId.make(`fork-delete-message:${first}`),
768+
rootNodeId: null,
769+
activeAttemptId: null,
770+
status: "completed",
771+
requestedAt: now,
772+
startedAt: now,
773+
completedAt: now,
774+
checkpointId: null,
775+
contextHandoffId: null,
776+
},
777+
},
778+
{
779+
id: EventId.make(`fork-delete-page:${first}`),
780+
type: "turn-item.updated",
781+
threadId: sourceThreadId,
782+
runId,
783+
occurredAt: now,
784+
payload: {
785+
id: TurnItemId.make(`fork-delete-page:${first}`),
786+
threadId: sourceThreadId,
787+
runId,
788+
nodeId: null,
789+
providerThreadId: null,
790+
providerTurnId: null,
791+
nativeItemRef: null,
792+
parentItemId: null,
793+
ordinal: 1,
794+
status: "completed",
795+
title: "Shared page",
796+
startedAt: now,
797+
completedAt: now,
798+
updatedAt: now,
799+
type: "dynamic_tool",
800+
toolName: "mcp__t3-code__html_render",
801+
input: {},
802+
output: { htmlRender: { attachmentId, title: "Shared page", height: 300 } },
803+
},
804+
},
805+
],
806+
});
807+
const firstReady = yield* Deferred.make<void>();
808+
const releaseFirst = yield* Deferred.make<void>();
809+
const secondQueued = yield* Deferred.make<void>();
810+
const cleanupIds: Array<string> = [];
811+
const commitCommand = eventSink.commitCommand;
812+
const commitSpy = vi.spyOn(eventSink, "commitCommand").mockImplementation((input) => {
813+
if (input.commandId === deleteCommandId) {
814+
for (const effect of input.effects ?? []) {
815+
if (effect.request.type === "attachment.cleanup")
816+
cleanupIds.push(...effect.request.attachmentIds);
817+
}
818+
}
819+
return input.commandId === (first === "fork" ? forkCommandId : deleteCommandId)
820+
? Deferred.succeed(firstReady, undefined).pipe(
821+
Effect.andThen(Deferred.await(releaseFirst)),
822+
Effect.andThen(commitCommand(input)),
823+
)
824+
: commitCommand(input);
825+
});
826+
const withDeletionLock = executor.withDeletionLock;
827+
let lockRequests = 0;
828+
const observeLock: ThreadCommandExecutor.ThreadCommandExecutor["Service"]["withDeletionLock"] =
829+
(effect) =>
830+
++lockRequests === 2
831+
? Deferred.succeed(secondQueued, undefined).pipe(
832+
Effect.andThen(withDeletionLock(effect)),
833+
)
834+
: withDeletionLock(effect);
835+
const lockSpy = vi.spyOn(executor, "withDeletionLock").mockImplementation(observeLock);
836+
yield* Effect.gen(function* () {
837+
const fork = orchestrator
838+
.dispatch({
839+
type: "thread.fork",
840+
commandId: forkCommandId,
841+
sourceThreadId,
842+
targetThreadId,
843+
sourcePoint: { type: "run", runId },
844+
createdBy: "user",
845+
creationSource: "web",
846+
})
847+
.pipe(Effect.result);
848+
const deletion = orchestrator
849+
.dispatch({
850+
type: "thread.delete",
851+
commandId: deleteCommandId,
852+
threadId: sourceThreadId,
853+
})
854+
.pipe(Effect.result);
855+
const firstFiber = yield* (first === "fork" ? fork : deletion).pipe(
856+
Effect.forkChild({ startImmediately: true }),
857+
);
858+
yield* Effect.raceFirst(
859+
Deferred.await(firstReady),
860+
Fiber.join(firstFiber).pipe(Effect.andThen(Effect.die("Missing commit barrier"))),
861+
);
862+
const secondFiber = yield* (first === "fork" ? deletion : fork).pipe(
863+
Effect.forkChild({ startImmediately: true }),
864+
);
865+
yield* Effect.raceFirst(Deferred.await(secondQueued), Fiber.await(secondFiber));
866+
yield* Deferred.succeed(releaseFirst, undefined);
867+
const firstResult = yield* Fiber.join(firstFiber);
868+
const secondResult = yield* Fiber.join(secondFiber);
869+
assert.equal(firstResult._tag, "Success");
870+
if (first === "fork") {
871+
assert.equal(secondResult._tag, "Success");
872+
assert.deepEqual(cleanupIds, []);
873+
const forkProjection = yield* orchestrator.getThreadProjection(targetThreadId);
874+
assert.isNull(forkProjection.thread.deletedAt);
875+
assert.isTrue(
876+
forkProjection.visibleTurnItems.some(
877+
(row) => row.item.id === TurnItemId.make(`fork-delete-page:${first}`),
878+
),
879+
);
880+
} else {
881+
assert.equal(secondResult._tag, "Failure");
882+
assert.deepEqual(cleanupIds, [attachmentId]);
883+
const target = yield* orchestrator
884+
.getThreadProjection(targetThreadId)
885+
.pipe(Effect.result);
886+
assert.equal(target._tag, "Failure");
887+
}
888+
}).pipe(
889+
Effect.ensuring(
890+
Effect.sync(() => {
891+
commitSpy.mockRestore();
892+
lockSpy.mockRestore();
893+
}),
894+
),
895+
);
896+
}),
897+
);
722898
});
723899

724900
const layerSharedApplicationDataPlaneTest = Layer.mergeAll(

0 commit comments

Comments
 (0)