refactor(run-store): name the records-read gate and unit test it

The gate was an inline predicate inside a private method, so the cost decision it drives had
only integration coverage. Extracts it as mayHoldRecords beside deriveOrder and
deriveDistinctIds, its sibling pure helpers, and covers it with unit tests: empty, all-legacy,
one store id, a store id among legacy ones, index-less, all four waitpoint types, and a
foreign prefix.

The mixed case is the one a per-organisation rollout produces, where a run holds waitpoints
minted either side of the flip. One store-format id is enough to require the read.

Author: d-cs

internal-packages/run-store/src/redisSnapshotStore.test.ts

@@ -4,7 +4,9 @@ import { expect, describe, vi } from "vitest";
 import { redisTest, slotOf } from "@internal/testcontainers";
 import { createRedisClient } from "@internal/redis";
 import { Logger } from "@trigger.dev/core/logger";
+import { generateWaitpointId } from "@trigger.dev/core/v3/isomorphic";
 import {
+  mayHoldRecords,
   snapshotKeys,
   deriveOrder,
   isValidFor,
@@ -24,6 +26,46 @@ describe("snapshotKeys", () => {
   });
 });
 
+describe("mayHoldRecords", () => {
+  it("is false for an empty set", () => {
+    expect(mayHoldRecords([])).toBe(false);
+  });
+
+  it("is false when every id is legacy", () => {
+    expect(mayHoldRecords([{ id: "w_cuid_a", index: 0 }, { id: "w_cuid_b" }])).toBe(false);
+  });
+
+  it("is true when one id is store-format", () => {
+    expect(mayHoldRecords([{ id: generateWaitpointId("MANUAL"), index: 0 }])).toBe(true);
+  });
+
+  // The mixed case is the one a per-organisation rollout produces: a run holding waitpoints
+  // minted either side of the flip. One store-format id is enough to require the read.
+  it("is true when a store-format id sits among legacy ones", () => {
+    expect(
+      mayHoldRecords([
+        { id: "w_cuid_a", index: 0 },
+        { id: generateWaitpointId("MANUAL"), index: 1 },
+        { id: "w_cuid_c" },
+      ])
+    ).toBe(true);
+  });
+
+  it("reads the id, not the index, so an index-less store id still counts", () => {
+    expect(mayHoldRecords([{ id: generateWaitpointId("DATETIME") }])).toBe(true);
+  });
+
+  it("counts every store waitpoint type", () => {
+    for (const t of ["RUN", "BATCH", "DATETIME", "MANUAL"] as const) {
+      expect(mayHoldRecords([{ id: generateWaitpointId(t) }])).toBe(true);
+    }
+  });
+
+  it("is false for a foreign prefix that is not a waitpoint id", () => {
+    expect(mayHoldRecords([{ id: "run_0123456789abcdefghijklm" }])).toBe(false);
+  });
+});
+
 describe("deriveOrder", () => {
   it("drops entries with no index, sorts by index, and maps to id", () => {
     expect(

internal-packages/run-store/src/redisSnapshotStore.ts

@@ -6,6 +6,7 @@ import {
   type Result,
 } from "@internal/redis";
 import { Logger } from "@trigger.dev/core/logger";
+import { parseWaitpointId } from "@trigger.dev/core/v3/isomorphic";
 import type { CompletedWaitpoint } from "@trigger.dev/core/v3/schemas";
 
 export type SnapshotKeys = { e: string; idx: string; cur: string; seq: string };
@@ -48,6 +49,22 @@ export function deriveDistinctIds(completedWaitpoints: CompletedWaitpointRef[]):
   return [...new Set(completedWaitpoints.map((w) => w.id))];
 }
 
+/**
+ * Whether a cycle carrying these ids could hold a record set.
+ *
+ * A record set is written for store-format waitpoint ids and nothing else, so a cycle whose ids
+ * are all legacy has none, and a read for one could only ever return nothing. Callers use this to
+ * skip that read, which keeps a deployment holding no store-format waitpoint at zero extra round
+ * trips on the resume path.
+ *
+ * This is the one place this module looks INSIDE a waitpoint id rather than treating the record
+ * set as opaque. It is named and exported rather than inlined so the cost decision it drives is
+ * testable on its own.
+ */
+export function mayHoldRecords(completedWaitpoints: CompletedWaitpointRef[]): boolean {
+  return completedWaitpoints.some((w) => parseWaitpointId(w.id).format === "b32hexW");
+}
+
 // isValid is derived, never stored, so the entry JSON stays byte-identical to the caller's document.
 export function isValidFor(entry: { error?: unknown }): boolean {
   return !entry.error;

internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts

@@ -12,7 +12,7 @@
 // Each order is chosen so the crash state is the harmless one. A lost cross-store write is never
 // recovered by a transaction or an outbox: recovery is always the existing stall-and-repair job.
 import { Logger } from "@trigger.dev/core/logger";
-import { generateInternalId, parseWaitpointId } from "@trigger.dev/core/v3/isomorphic";
+import { generateInternalId } from "@trigger.dev/core/v3/isomorphic";
 import { DelegatingRunStore } from "./delegatingRunStore.js";
 import type {
   CompletedWaitpointRecord,
@@ -21,7 +21,7 @@ import type {
   SnapshotEntryInput,
   SnapshotRead,
 } from "./redisSnapshotStore.js";
-import { deriveDistinctIds, deriveOrder } from "./redisSnapshotStore.js";
+import { deriveDistinctIds, deriveOrder, mayHoldRecords } from "./redisSnapshotStore.js";
 import {
   entryFromCompletion,
   entryFromCreateExecutionSnapshot,
@@ -630,22 +630,13 @@ export class TaskRunExecutionSnapshotStore extends DelegatingRunStore {
         // cycle already minted. But the script may refuse the pointer and mint a replacement
         // from these refs, and a replacement minted with no records holds ids that nothing
         // resolves. So carry the surviving cycle's records for that branch.
-        // Read only when a record could exist. A record set is written for store-format waitpoint
-        // ids and nothing else, so a cycle whose ids are all legacy has none, and the read could
-        // only ever return nothing. Gating on the id keeps a deployment with no store-format
-        // waitpoint at zero extra round trips, and confines the cost to the organisations that
-        // actually hold them.
-        //
-        // This is the one place the snapshot store looks INSIDE a waitpoint id rather than
-        // treating the record set as opaque. It buys a per-append round trip on the resume path,
-        // which is worth the narrower layering.
-        const mayHaveRecords = completedWaitpoints.some(
-          (w) => parseWaitpointId(w.id).format === "b32hexW"
-        );
-
+        // Read only when a record could exist: see mayHoldRecords. A legacy-only cycle has none,
+        // so the read would return nothing and the round trip is pure cost.
         const carried =
           records ??
-          (mayHaveRecords ? await this.#recordsForCycle(runId, head.cycle.cycleSeq) : undefined);
+          (mayHoldRecords(completedWaitpoints)
+            ? await this.#recordsForCycle(runId, head.cycle.cycleSeq)
+            : undefined);
 
         return {
           kind: "carryForward",