refactor(server): share attachment message intake#10580
4 unresolved threads ยท Jump to file
- apps/server/src/orchestration-v2/ThreadMessageIntake.ts:42 ยท ๐ **High** `orchestration-v2/ThreadMessageIntake.ts:42` Retrying an already accepted launch after
- apps/server/src/orchestration-v2/ThreadMessageIntake.ts:19 ยท ๐ **High** `orchestration-v2/ThreadMessageIntake.ts:19` Failed `dispatchCommand` and `launchThread
- apps/server/src/orchestration-v2/ThreadMessageIntake.ts:46โ49 ยท ๐ **High** `orchestration-v2/ThreadMessageIntake.ts:46` `launchThread` leaks the copied attachment
- apps/server/src/ws.ts:1484โ1495 ยท This catch-all `mapError` now inspects `cause._tag` and derives the wrapper's `message` from `cause.
apps/server/src/orchestration-v2/ThreadMessageIntake.ts ยท Earlier revision 43e6ee2a ยท right 42
Original diff
@@ -0,0 +1,50 @@
+import type { OrchestrationV2Command } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+
+import * as AttachmentClaims from "./AttachmentClaims.ts";
+import * as ThreadLaunch from "./ThreadLaunchService.ts";
+import * as ThreadManagement from "./ThreadManagementService.ts";
+
+// Claim failures clean up their partial batch. Once dispatch/launch starts, its
+// failure can follow a durable commit, so callers must not delete claimed files.
+export const dispatchCommand = Effect.fn("ThreadMessageIntake.dispatchCommand")(function* (
+ command: OrchestrationV2Command,
+) {
+ const threads = yield* ThreadManagement.ThreadManagementService;
+ if (command.type !== "message.dispatch") return yield* threads.dispatch(command);
+ const claimed = yield* AttachmentClaims.claimPendingAttachments({
+ threadId: command.threadId,
+ attachments: command.attachments,
+ });
+ return yield* threads.dispatch({ ...command, attachments: claimed.attachments });
+});
+
+export const sendToThread = Effect.fn("ThreadMessageIntake.sendToThread")(function* (
+ input: ThreadManagement.ThreadManagementSendInput,
+) {
+ const threads = yield* ThreadManagement.ThreadManagementService;
+ const claimed = yield* AttachmentClaims.claimPendingAttachments(input);
+ return yield* threads.sendToThread({ ...input, attachments: claimed.attachments });
+});
+
+export const launchThread = Effect.fn("ThreadMessageIntake.launchThread")(function* (
+ input: ThreadLaunch.ThreadLaunchInput,
+) {
+ const launches = yield* ThreadLaunch.ThreadLaunchService;
+ if (!input.initialMessage?.attachments.some(AttachmentClaims.attachmentIsPendingUpload)) {
+ return yield* launches.launch(input);
+ }
+ if (input.threadId === undefined) {
+ return yield* new AttachmentClaims.AttachmentClaimError({
+ message: "Uploaded attachments need a thread id at launch.",
+ });
+ }
+ const claimed = yield* AttachmentClaims.claimPendingAttachments({๐ High orchestration-v2/ThreadMessageIntake.ts:42
Retrying an already accepted launch after a lost response copies the pending attachment to a new random thread-scoped path, then launches.launch returns the existing receipt without referencing that copy. The unreferenced file is never released, so each retry permanently leaks an attachment; perform the idempotency check before claiming or release claimed.claimedPaths when the launch is already committed.
๐ Reply "fix it for me" or copy this AI Prompt for your agent:
| 1 | In file @apps/server/src/orchestration-v2/ThreadMessageIntake.ts around line 42: |
| 2 | |
| 3 | Retrying an already accepted launch after a lost response copies the pending attachment to a new random thread-scoped path, then `launches.launch` returns the existing receipt without referencing that copy. The unreferenced file is never released, so each retry permanently leaks an attachment; perform the idempotency check before claiming or release `claimed.claimedPaths` when the launch is already committed. |
Fixed in c80aae28752480aca3d0ec0d9a773dca4586e821. Shared intake now removes claims not referenced by the returned durable message/events, including accepted launch, command, and send replays. It also removes copies on typed receipt/planning rejections and launch project/receipt failures before message dispatch. The real V2 regression now proves replays leave only accepted files and missing-project/thread requests leave no orphaned copies. The same regression retains the lost-result-after-acceptance case and verifies the original bytes survive. Generic launch/dispatch errors remain ambiguous, so blanket tapError cleanup would reintroduce data loss; those copies are intentionally retained. No new receipt store, journal, lock, or read-after-failure query was added. Focused suites: 38 passed; server typecheck and targeted lint/format passed.
apps/server/src/orchestration-v2/ThreadMessageIntake.ts ยท Earlier revision 43e6ee2a ยท right 19
Original diff
@@ -0,0 +1,50 @@
+import type { OrchestrationV2Command } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+
+import * as AttachmentClaims from "./AttachmentClaims.ts";
+import * as ThreadLaunch from "./ThreadLaunchService.ts";
+import * as ThreadManagement from "./ThreadManagementService.ts";
+
+// Claim failures clean up their partial batch. Once dispatch/launch starts, its
+// failure can follow a durable commit, so callers must not delete claimed files.
+export const dispatchCommand = Effect.fn("ThreadMessageIntake.dispatchCommand")(function* (
+ command: OrchestrationV2Command,
+) {
+ const threads = yield* ThreadManagement.ThreadManagementService;
+ if (command.type !== "message.dispatch") return yield* threads.dispatch(command);
+ const claimed = yield* AttachmentClaims.claimPendingAttachments({
+ threadId: command.threadId,
+ attachments: command.attachments,
+ });
+ return yield* threads.dispatch({ ...command, attachments: claimed.attachments });๐ High orchestration-v2/ThreadMessageIntake.ts:19
Failed dispatchCommand and launchThread calls permanently retain the thread-scoped copies created by claimPendingAttachments, so repeated requests for nonexistent threads or projects leave unreferenced upload payloads on disk and can exhaust server storage. These failures occur before a message or thread is accepted, but no cleanup follows threads.dispatch(...) or launches.launch(...), and the pending-file sweep does not remove the copies. Release the claimed attachments whenever dispatch or launch fails before acceptance.
๐ Reply "fix it for me" or copy this AI Prompt for your agent:
| 1 | In file @apps/server/src/orchestration-v2/ThreadMessageIntake.ts around line 19: |
| 2 | |
| 3 | Failed `dispatchCommand` and `launchThread` calls permanently retain the thread-scoped copies created by `claimPendingAttachments`, so repeated requests for nonexistent threads or projects leave unreferenced upload payloads on disk and can exhaust server storage. These failures occur before a message or thread is accepted, but no cleanup follows `threads.dispatch(...)` or `launches.launch(...)`, and the pending-file sweep does not remove the copies. Release the claimed attachments whenever dispatch or launch fails before acceptance. |
Fixed in c80aae28752480aca3d0ec0d9a773dca4586e821. Shared intake now removes claims not referenced by the returned durable message/events, including accepted launch, command, and send replays. It also removes copies on typed receipt/planning rejections and launch project/receipt failures before message dispatch. The real V2 regression now proves replays leave only accepted files and missing-project/thread requests leave no orphaned copies. The same regression retains the lost-result-after-acceptance case and verifies the original bytes survive. Generic launch/dispatch errors remain ambiguous, so blanket tapError cleanup would reintroduce data loss; those copies are intentionally retained. No new receipt store, journal, lock, or read-after-failure query was added. Focused suites: 38 passed; server typecheck and targeted lint/format passed.
apps/server/src/orchestration-v2/ThreadMessageIntake.ts ยท Earlier revision 43e6ee2a ยท right 46โ49
Original diff
@@ -0,0 +1,50 @@
+import type { OrchestrationV2Command } from "@t3tools/contracts";
+import * as Effect from "effect/Effect";
+
+import * as AttachmentClaims from "./AttachmentClaims.ts";
+import * as ThreadLaunch from "./ThreadLaunchService.ts";
+import * as ThreadManagement from "./ThreadManagementService.ts";
+
+// Claim failures clean up their partial batch. Once dispatch/launch starts, its
+// failure can follow a durable commit, so callers must not delete claimed files.
+export const dispatchCommand = Effect.fn("ThreadMessageIntake.dispatchCommand")(function* (
+ command: OrchestrationV2Command,
+) {
+ const threads = yield* ThreadManagement.ThreadManagementService;
+ if (command.type !== "message.dispatch") return yield* threads.dispatch(command);
+ const claimed = yield* AttachmentClaims.claimPendingAttachments({
+ threadId: command.threadId,
+ attachments: command.attachments,
+ });
+ return yield* threads.dispatch({ ...command, attachments: claimed.attachments });
+});
+
+export const sendToThread = Effect.fn("ThreadMessageIntake.sendToThread")(function* (
+ input: ThreadManagement.ThreadManagementSendInput,
+) {
+ const threads = yield* ThreadManagement.ThreadManagementService;
+ const claimed = yield* AttachmentClaims.claimPendingAttachments(input);
+ return yield* threads.sendToThread({ ...input, attachments: claimed.attachments });
+});
+
+export const launchThread = Effect.fn("ThreadMessageIntake.launchThread")(function* (
+ input: ThreadLaunch.ThreadLaunchInput,
+) {
+ const launches = yield* ThreadLaunch.ThreadLaunchService;
+ if (!input.initialMessage?.attachments.some(AttachmentClaims.attachmentIsPendingUpload)) {
+ return yield* launches.launch(input);
+ }
+ if (input.threadId === undefined) {
+ return yield* new AttachmentClaims.AttachmentClaimError({
+ message: "Uploaded attachments need a thread id at launch.",
+ });
+ }
+ const claimed = yield* AttachmentClaims.claimPendingAttachments({
+ threadId: input.threadId,
+ attachments: input.initialMessage.attachments,
+ });
+ return yield* launches.launch({
+ ...input,
+ initialMessage: { ...input.initialMessage, attachments: claimed.attachments },
+ });apps/server/src/orchestration-v2/ThreadMessageIntake.ts ยท right 46โ49
๐ High orchestration-v2/ThreadMessageIntake.ts:46
launchThread leaks the copied attachment whenever launches.launch fails before accepting the initial message. claimPendingAttachments creates a non-pending final path, so the pending sweep never reclaims it and repeated failed launches accumulate orphaned files; release claimed.claimedPaths on launch failure.
| 1 | - return yield* launches.launch({ |
| 2 | + return yield* launches.launch({ |
| 3 | ...input, |
| 4 | initialMessage: { ...input.initialMessage, attachments: claimed.attachments }, |
| 5 | - }); |
| 6 | + }).pipe( |
| 7 | + Effect.tapError(() => |
| 8 | + AttachmentClaims.releaseClaimedAttachments(claimed.claimedPaths), |
| 9 | + ), |
| 10 | + ); |
Also found in 2 other location(s)
apps/server/src/ws.ts:1319
ThreadMessageIntake.dispatchCommandclaims and copies pending attachments before dispatch, but the new path no longer releasesclaimedPathsifthreads.dispatchfails before accepting the command. For example, dispatching a valid staged upload to a deleted/nonexistent thread copies it under that thread id, thenensureCommandTranscripts/the orchestrator can fail.sweepStalePendingAttachmentsonly deletes ids in the pending segment, so this final-path copy is retained indefinitely despite no message referencing it; repeated failed submissions accumulate orphaned attachment files.
apps/server/src/ws.ts:1437
ThreadMessageIntake.launchThreadcopies pending attachments beforelaunches.launch, but no longer removes the copy when launch fails before the initial message is committed. A launch using a staged attachment and a nonexistent project fails duringThreadLaunchService.launch's project lookup, leaving the copied final attachment unreferenced. These ids are not in the pending segment, sosweepStalePendingAttachmentswill never reclaim them; failed launch attempts leak attachment files.
๐ Reply "fix it for me" or copy this AI Prompt for your agent:
| 1 | In file @apps/server/src/orchestration-v2/ThreadMessageIntake.ts around lines 46-49: |
| 2 | |
| 3 | `launchThread` leaks the copied attachment whenever `launches.launch` fails before accepting the initial message. `claimPendingAttachments` creates a non-pending final path, so the pending sweep never reclaims it and repeated failed launches accumulate orphaned files; release `claimed.claimedPaths` on launch failure. |
| 4 | |
| 5 | Also found in 2 other location(s): |
| 6 | - apps/server/src/ws.ts:1319 -- `ThreadMessageIntake.dispatchCommand` claims and copies pending attachments before dispatch, but the new path no longer releases `claimedPaths` if `threads.dispatch` fails before accepting the command. For example, dispatching a valid staged upload to a deleted/nonexistent thread copies it under that thread id, then `ensureCommandTranscripts`/the orchestrator can fail. `sweepStalePendingAttachments` only deletes ids in the pending segment, so this final-path copy is retained indefinitely despite no message referencing it; repeated failed submissions accumulate orphaned attachment files. |
| 7 | - apps/server/src/ws.ts:1437 -- `ThreadMessageIntake.launchThread` copies pending attachments before `launches.launch`, but no longer removes the copy when launch fails before the initial message is committed. A launch using a staged attachment and a nonexistent project fails during `ThreadLaunchService.launch`'s project lookup, leaving the copied final attachment unreferenced. These ids are not in the pending segment, so `sweepStalePendingAttachments` will never reclaim them; failed launch attempts leak attachment files. |
Fixed in c80aae28752480aca3d0ec0d9a773dca4586e821. Shared intake now removes claims not referenced by the returned durable message/events, including accepted launch, command, and send replays. It also removes copies on typed receipt/planning rejections and launch project/receipt failures before message dispatch. The real V2 regression now proves replays leave only accepted files and missing-project/thread requests leave no orphaned copies. The same regression retains the lost-result-after-acceptance case and verifies the original bytes survive. Generic launch/dispatch errors remain ambiguous, so blanket tapError cleanup would reintroduce data loss; those copies are intentionally retained. No new receipt store, journal, lock, or read-after-failure query was added. Focused suites: 38 passed; server typecheck and targeted lint/format passed.
apps/server/src/ws.ts ยท Earlier revision c80aae28 ยท right 1484โ1495
Original diff
@@ -1454,104 +1432,68 @@ const makeWsRpcLayer = (
[ORCHESTRATION_V2_WS_METHODS.launchThread]: (input) =>
observeRpcEffect(
ORCHESTRATION_V2_WS_METHODS.launchThread,
- Effect.gen(function* () {
- const pendingUploads =
- input.initialMessage?.attachments.some(attachmentIsPendingUpload) ?? false;
- if (pendingUploads && input.threadId === undefined) {
- return yield* new OrchestrationV2ThreadLaunchError({
+ startup
+ .enqueueCommand(
+ ThreadMessageIntake.launchThread({
commandId: input.commandId,
+ ...(input.threadId === undefined ? {} : { threadId: input.threadId }),
+ ...(input.reuseExistingThread === undefined
+ ? {}
+ : { reuseExistingThread: input.reuseExistingThread }),
projectId: input.projectId,
- message: "Uploaded attachments need a thread id at launch.",
- });
- }
- const claimed =
- pendingUploads && input.threadId !== undefined && input.initialMessage !== undefined
- ? yield* claimPendingAttachments({
- threadId: input.threadId,
- attachments: input.initialMessage.attachments,
- }).pipe(
- Effect.mapError(
- (cause) =>
- new OrchestrationV2ThreadLaunchError({
- commandId: input.commandId,
- projectId: input.projectId,
- message: cause.message,
- cause,
- }),
- ),
- )
- : null;
- const initialMessage =
- input.initialMessage === undefined
- ? undefined
- : claimed === null
- ? input.initialMessage
- : { ...input.initialMessage, attachments: claimed.attachments };
- return yield* startup
- .enqueueCommand(
- threadLaunch.launch({
- commandId: input.commandId,
- ...(input.threadId === undefined ? {} : { threadId: input.threadId }),
- ...(input.reuseExistingThread === undefined
- ? {}
- : { reuseExistingThread: input.reuseExistingThread }),
- projectId: input.projectId,
- title: input.title,
- ...(input.generateTitle === undefined
- ? {}
- : { generateTitle: input.generateTitle }),
- modelSelection: input.modelSelection,
- runtimeMode: input.runtimeMode,
- interactionMode: input.interactionMode,
- workspaceStrategy: input.workspaceStrategy,
- ...(initialMessage === undefined
- ? {}
- : {
- initialMessage: {
- ...(initialMessage.messageId === undefined
- ? {}
- : { messageId: initialMessage.messageId }),
- text: initialMessage.text,
- attachments: initialMessage.attachments,
- },
- }),
- createdBy: "user",
- creationSource: input.creationSource ?? "web",
- }),
- )
- .pipe(
- Effect.tapError(() =>
- claimed === null
- ? Effect.void
- : releaseClaimedAttachments(claimed.claimedPaths),
- ),
- Effect.tap(() =>
- analytics
- .record("client.thread.started", originProps)
- .pipe(
- Effect.andThen(
- input.initialMessage === undefined
- ? Effect.void
- : analytics.record("client.turn.requested", originProps),
- ),
- Effect.ignore,
- ),
- ),
- Effect.map((result) => ({
- ...result,
- projection: projectThreadProjectionForWire(result.projection),
- })),
- Effect.mapError(
- (cause) =>
- new OrchestrationV2ThreadLaunchError({
- commandId: input.commandId,
- projectId: input.projectId,
- message: "Failed to launch thread",
- cause,
+ title: input.title,
+ ...(input.generateTitle === undefined
+ ? {}
+ : { generateTitle: input.generateTitle }),
+ modelSelection: input.modelSelection,
+ runtimeMode: input.runtimeMode,
+ interactionMode: input.interactionMode,
+ workspaceStrategy: input.workspaceStrategy,
+ ...(input.initialMessage === undefined
+ ? {}
+ : {
+ initialMessage: {
+ ...(input.initialMessage.messageId === undefined
+ ? {}
+ : { messageId: input.initialMessage.messageId }),
+ text: input.initialMessage.text,
+ attachments: input.initialMessage.attachments,
+ },
}),
- ),
- );
- }),
+ createdBy: "user",
+ creationSource: input.creationSource ?? "web",
+ }).pipe(Effect.provide(intakeContext)),
+ )
+ .pipe(
+ Effect.tap(() =>
+ analytics
+ .record("client.thread.started", originProps)
+ .pipe(
+ Effect.andThen(
+ input.initialMessage === undefined
+ ? Effect.void
+ : analytics.record("client.turn.requested", originProps),
+ ),
+ Effect.ignore,
+ ),
+ ),
+ Effect.map((result) => ({
+ ...result,
+ projection: projectThreadProjectionForWire(result.projection),
+ })),
+ Effect.mapError(
+ (cause) =>
+ new OrchestrationV2ThreadLaunchError({
+ commandId: input.commandId,
+ projectId: input.projectId,
+ message:
+ cause._tag === "AttachmentClaimError"
+ ? cause.message
+ : "Failed to launch thread",
+ cause,
+ }),
+ ),apps/server/src/ws.ts ยท right 1484โ1495
This catch-all mapError now inspects cause._tag and derives the wrapper's message from cause.message. Per the error conventions, a wrapper's message should come from its own attributes, and statically known tagged failures should be handled with Effect.catchTags where their context is known, instead of tag-sniffing a whole multi-step pipeline's error channel.
| 1 | Effect.catchTags({ |
| 2 | AttachmentClaimError: (error) => |
| 3 | new OrchestrationV2ThreadLaunchError({ |
| 4 | commandId: input.commandId, |
| 5 | projectId: input.projectId, |
| 6 | message: error.message, |
| 7 | cause: error, |
| 8 | }), |
| 9 | }), |
| 10 | Effect.mapError( |
| 11 | (cause) => |
| 12 | new OrchestrationV2ThreadLaunchError({ |
| 13 | commandId: input.commandId, |
| 14 | projectId: input.projectId, |
| 15 | message: "Failed to launch thread", |
| 16 | cause, |
| 17 | }), |
| 18 | ), |
Posted via Macroscope โ Effect Service Conventions
Fixed in 4cfe3581d4: the RPC launch boundary now handles AttachmentClaimError, ThreadLaunchError, and ServerRuntimeStartupError explicitly with Effect.catchTags. The attachment claim message remains unchanged because it is an existing, safe caller-facing validation message; launch/startup failures retain the existing fixed message. The suggested catchTags-then-mapError sequence would rewrap the translated claim error and erase that message, so the mapping is exhaustive in one catchTags instead. Canonical server typecheck, targeted type-aware lint, formatter and diff checks pass. No behavior or new test machinery was added.