Skip to content

Commit f4131eb

Browse files
d-csclaude
andcommitted
fix(webapp): reads-layer Devin follow-ups
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e57bc5e commit f4131eb

4 files changed

Lines changed: 75 additions & 5 deletions

File tree

apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,32 @@ function bufferedStatusToTaskRunStatus(status: SyntheticRun["status"]): TaskRunS
534534
}
535535
}
536536

537+
// The PG path stores `TaskRun.payload` as `String?`, so in production
538+
// the buffered snapshot's `payload` is always a string. We defensively
539+
// coerce other types instead of silently dropping them: an object gets
540+
// JSON-stringified (matches how the trigger path would serialise it),
541+
// anything truly unrenderable falls back to an empty string. The log
542+
// line surfaces format drift to ops without crashing the read path.
543+
function synthesisePayload(buffered: SyntheticRun): string {
544+
const payload = buffered.payload;
545+
if (typeof payload === "string") return payload;
546+
if (payload === undefined || payload === null) return "";
547+
try {
548+
const serialised = JSON.stringify(payload);
549+
logger.warn("ApiRetrieveRunPresenter: buffered snapshot.payload non-string coerced", {
550+
runFriendlyId: buffered.friendlyId,
551+
payloadType: typeof payload,
552+
});
553+
return typeof serialised === "string" ? serialised : "";
554+
} catch {
555+
logger.error("ApiRetrieveRunPresenter: buffered snapshot.payload unserialisable", {
556+
runFriendlyId: buffered.friendlyId,
557+
payloadType: typeof payload,
558+
});
559+
return "";
560+
}
561+
}
562+
537563
function synthesiseFoundRunFromBuffer(buffered: SyntheticRun): FoundRun {
538564
const status: TaskRunStatus = bufferedStatusToTaskRunStatus(buffered.status);
539565

@@ -548,7 +574,13 @@ function synthesiseFoundRunFromBuffer(buffered: SyntheticRun): FoundRun {
548574
typeof buffered.metadata === "string" ? buffered.metadata : null;
549575

550576
return {
551-
id: buffered.friendlyId,
577+
// `id` is the internal cuid (Prisma TaskRun.id column), `friendlyId`
578+
// is the user-facing `run_xxx` token. Downstream logging keyed off
579+
// `taskRun.id` correlates with other systems via the cuid — using
580+
// the friendlyId here breaks log correlation. `SyntheticRun` carries
581+
// the cuid alongside the friendlyId for exactly this reason
582+
// (RunId.fromFriendlyId in readFallback.server.ts).
583+
id: buffered.id,
552584
friendlyId: buffered.friendlyId,
553585
status,
554586
taskIdentifier: buffered.taskIdentifier ?? "",
@@ -574,7 +606,7 @@ function synthesiseFoundRunFromBuffer(buffered: SyntheticRun): FoundRun {
574606
batch: null,
575607
runTags: buffered.tags,
576608
traceId: buffered.traceId ?? "",
577-
payload: typeof buffered.payload === "string" ? buffered.payload : "",
609+
payload: synthesisePayload(buffered),
578610
payloadType: buffered.payloadType ?? "application/json",
579611
output: null,
580612
outputType: "application/json",

apps/webapp/app/routes/api.v1.runs.$runId.trace.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ export const loader = createLoaderApiRoute(
9595
startTime: buffered.createdAt,
9696
duration: 0,
9797
isError: false,
98-
isPartial: true,
98+
// Cancelled is a terminal state — the span shouldn't
99+
// signal "still in progress" once it's been cancelled.
100+
// Mirrors the sibling api.v1.runs.$runId.spans.$spanId.ts
101+
// and syntheticTrace.server.ts logic.
102+
isPartial: buffered.status !== "CANCELED",
99103
isCancelled: buffered.status === "CANCELED",
100104
level: "TRACE",
101105
queueName: buffered.queue ?? undefined,

apps/webapp/app/routes/api.v1.runs.$runParam.attempts.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import type { ActionFunctionArgs, LoaderFunctionArgs } from "@remix-run/server-runtime";
22
import { json } from "@remix-run/server-runtime";
33
import { z } from "zod";
4+
import { $replica } from "~/db.server";
45
import { authenticateApiRequest } from "~/services/apiAuth.server";
56
import { logger } from "~/services/logger.server";
7+
import { findRunByIdWithMollifierFallback } from "~/v3/mollifier/readFallback.server";
68
import { ServiceValidationError } from "~/v3/services/baseService.server";
79
import { CreateTaskRunAttemptService } from "~/v3/services/createTaskRunAttempt.server";
810

@@ -32,6 +34,31 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
3234
return json({ error: "Invalid or missing run ID" }, { status: 400 });
3335
}
3436

37+
const { runParam } = parsed.data;
38+
const env = authenticationResult.environment;
39+
40+
// Verify the run belongs to the authenticated environment before
41+
// returning the parity-empty list. The response body is empty either
42+
// way, but other run-scoped endpoints (spans, trace, retrieve) all
43+
// 404 on cross-env access; matching that here means a third party
44+
// can't distinguish "run exists" from "doesn't exist" via this
45+
// endpoint either. PG-first then buffer fallback, consistent with
46+
// the other read paths.
47+
const pgRun = await $replica.taskRun.findFirst({
48+
where: { friendlyId: runParam, runtimeEnvironmentId: env.id },
49+
select: { id: true },
50+
});
51+
if (!pgRun) {
52+
const buffered = await findRunByIdWithMollifierFallback({
53+
runId: runParam,
54+
environmentId: env.id,
55+
organizationId: env.organizationId,
56+
});
57+
if (!buffered) {
58+
return json({ error: "Run not found" }, { status: 404 });
59+
}
60+
}
61+
3562
return json({ attempts: [] }, { status: 200 });
3663
}
3764

apps/webapp/app/v3/mollifier/syntheticRedirectInfo.server.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
1-
import { deserialiseSnapshot, type MollifierBuffer } from "@trigger.dev/redis-worker";
1+
import type { MollifierBuffer } from "@trigger.dev/redis-worker";
22
import type { PrismaClientOrTransaction } from "@trigger.dev/database";
33
import { z } from "zod";
44
import { prisma } from "~/db.server";
55
import { logger } from "~/services/logger.server";
66
import { getMollifierBuffer } from "./mollifierBuffer.server";
7+
// Use the webapp-side wrapper (not `deserialiseSnapshot` from
8+
// @trigger.dev/redis-worker directly) so this file shares a single
9+
// deserialisation path with readFallback.server.ts. The two are
10+
// behaviourally identical today (both wrap `JSON.parse`), but pinning
11+
// the shared helper keeps the two read-side modules from drifting if
12+
// snapshot encoding ever changes.
13+
import { deserialiseMollifierSnapshot } from "./mollifierSnapshot.server";
714

815
// Validated subset of a mollifier snapshot — just the fields needed to
916
// rebuild a canonical run-detail URL for a buffered run. Anything else
@@ -78,7 +85,7 @@ export async function findBufferedRunRedirectInfo(
7885

7986
let raw: unknown;
8087
try {
81-
raw = deserialiseSnapshot(entry.payload);
88+
raw = deserialiseMollifierSnapshot(entry.payload);
8289
} catch (err) {
8390
logger.warn("buffered redirect: snapshot deserialise failed", {
8491
runFriendlyId: args.runFriendlyId,

0 commit comments

Comments
 (0)