feat(run-store): add an execution-snapshot store decorator behind an off-by-default dial - #4765
feat(run-store): add an execution-snapshot store decorator behind an off-by-default dial#4765d-cs wants to merge 13 commits into
Conversation
The decorator that dual-writes snapshots to Redis has to own the snapshot id, or the same snapshot carries a different id in each store and the comparator chases a difference that is not real. Four of the six snapshot input types had no id field, so four write sites could not carry one. Add it to CompletionSnapshotInput, ExpireSnapshotInput, RescheduleSnapshotInput and CreateExecutionSnapshotInput, and thread it through every nested create. createCancelledRun built its create inline and dropped the id its input already carried; it now passes it too. The field is optional everywhere, so an absent id still falls through to Prisma's @default(cuid()) and no existing caller changes.
…ators RunStore has 71 members. A decorator that intercepts a dozen of them should not restate the other 59 forwarders alongside its real logic, and hand-writing them invites a typo no test would catch. Generate the base from the interface instead. The generator also emits the member-name lists, so the suite can assert that the class and the interface hold exactly the same members: a method added to RunStore and not to the base fails a test rather than becoming a silent hole in the decorator. The one data property on the interface becomes a getter over the delegate, read live rather than captured, so a delegate whose client changes is not cached.
…parity tests No nested write site returns the snapshot it created: createRun returns the run, expireParkedRun returns a count, and the rest return a selected TaskRun. So the Redis entry is built from each site's own input plus the caller-minted id. That means every value Postgres derives rather than receives has to be reproduced: the DEQUEUED-to-PENDING rewrite, the four values lockRunToWorker hard-codes, the three rescheduleRun defaults, and the engine column default a completion leaves unset. The parity suite covers all ten physical write sites, comparing the built entry against the row Postgres actually wrote. It caught the dropped id in createCancelledRun.
…write The last dial position makes the Redis store the sole snapshot writer, so Postgres has to stop writing snapshot rows without changing anything else it does. One constructor flag does that across all ten write sites. With it off, the nine nested creates are omitted and the run mutation still lands; createExecutionSnapshot echoes its input in the shape callers expect rather than inserting; and the completed-waitpoint join inserts are skipped, since they would otherwise link to a row that no longer exists. Defaults to true, so every existing caller and test is unaffected.
A decorator over any RunStore that also writes execution snapshots to Redis. It overrides only the methods that touch a snapshot and inherits the rest. Write order is the correctness property, and the two orders differ on purpose. A transition writes Postgres first: a crash in the gap leaves a stale latest snapshot, which the heartbeat stall watchdog already heals. A birth writes Redis first: a crash there leaves an unreachable key for a run that does not exist, where Postgres-first would leave a run with no snapshot at all and no way to read one. Each order is chosen so the crash state is the harmless one. A failed transition append retries three times, then hands the run to the repair job. It never rethrows, because Postgres has already committed and a throw would turn a healable gap into a caller-visible error. A failed birth append is survivable before redis-only, where Postgres still holds the snapshot, and refuses at redis-only, where it would otherwise create a run with no snapshot anywhere; refusing works only because the birth append comes first. None of the four non-failure append outcomes enqueues a repair: an absent keyspace is every pre-cutover run's transitions, a fork means another writer advanced the head, a duplicate is a retry that landed, and a cycle mismatch is the store refusing an untrustworthy pointer on purpose. At mode off the decorator makes no Redis call and builds no entry.
…ore handles Proves the deferral from inside the transaction callback rather than assuming it: a staged append is absent from Redis while the transaction is open and present once it commits, and a rollback leaves both stores agreeing the transition never happened.
The engine resolves its since-cursor to a createdAt before it asks for the window, so the snapshot id is gone by then and the id-addressed read cannot serve it. Adding a cursor-addressed read is the alternative to changing the engine's read path, which stays untouched. The cursor is exclusive and keeps the same-millisecond blind spot the Postgres read has. Matching it is the requirement, not an oversight: a Redis read that is more correct than the Postgres read shows up as divergence during compare mode, which exists to surface real defects. Closing the blind spot needs seq ordering on both sides and belongs after the cutover. The walk goes newest-first and stops at the first entry at or before the cursor, so its length is the length of the answer rather than the run's history. This adds a read operation. It does not touch the append script, the keyspace, or the write-ordering protocol.
…back Two of the five snapshot reads take arbitrary Prisma arguments, and a key-value store cannot answer an arbitrary query. Only three production call sites exist, all in the engine's executionSnapshotSystem, and both generic ones send a single fixed shape, so the decorator recognises exactly those shapes and delegates everything else. Each matcher rejects an argument object carrying a key it does not know, because a query that has drifted must be answered correctly by Postgres rather than approximately from Redis. A miss is the coexistence path, not an error: a pre-cutover run or expired history falls back to Postgres. The entry supplies every scalar column, and the checkpoint and waitpoint rows are read back through the delegate only when the entry says they exist, so the common read of a running run makes no Postgres call at all. Which runs read from Redis is a hash of the run id, so a run does not change store between two reads of one poll, two instances of the same dial agree, and raising the dial only ever adds runs to the cohort.
Two rules, because neither can see what the other leaves behind. A terminal run whose keyspace never received the completion expiry gets one applied, so it reaps on the schedule a healthy terminal append would have set. A keyspace with no run row at all, past an age threshold, is deleted outright — that is a crashed birth, which is non-terminal so it carries no expiry and has no run row, so the first rule can never match it. It never reaps on an unknown answer: a live run is left alone however old its keyspace, a young orphan is left for the birth that may still be in flight, and a batch whose run lookup failed is skipped rather than treated as absent. Run rows are resolved through the run store rather than a raw client, because under the run-ops split a run can live on either database and a raw lookup would report a live run as an orphan. Nothing schedules this. The engine's worker has to run it, and run-store cannot reach the engine. Also moves the decorator suites onto the worker-scoped container fixture. The per-test one boots a Postgres and a Redis container for every test, which is what the replication tests need and these do not; the sweeper suite alone went from repeated two-minute timeouts to ten seconds.
…eads on The engine's own flows, driven against the decorator with every snapshot read served from Redis, injected through the store seam that runStoreInjectability already proves. Same flows, same expectations, different store underneath — the point is that nothing in the engine has to know, so no existing suite changes. Covers a run driven to completion, the execution data at each step, a since-window wider than the fifty cap, and a pre-cutover run with no keyspace falling back to Postgres. The environment-boundary test asserts parity rather than a fixed shape: whatever Postgres answers for a foreign environment, Redis has to answer the same, or the tenant boundary behaves differently once reads move over.
…oth stores Three defects, all of which passed the existing suites because no test drove a snapshot that actually carried waitpoints, and because the parity suite compared createdAt against a value it had just read back from the row. The decorator never passed a cycle to the append, so no wp:<cycleSeq> key was written for any snapshot and the completed-waitpoint side of Redis was permanently empty. It now mints a cycle when the id set differs from the current head and carries the previous cycleSeq forward when it does not, so a resume writes the record set once and the copy-forwards that follow write no key at all. The since-window hydration returned an empty completedWaitpointOrder. That column is not the join: the engine reads it off the head row as the oracle that gives each completed waitpoint its position in a batch, so an empty order resumed every batched triggerAndWait with an undefined index. Seven of the eight write sites stamped the entry from the app clock while Postgres stamped its own column default, so the two stores held different instants for one snapshot. The decorator now supplies createdAt, and an equal updatedAt, at every site, and the standalone path supplies it too rather than reading the row back. Beyond making the field comparable, this aligns the since-window: the cursor is resolved from one store and applied in the other, and two different instants misfilter that window. The parity suite gains an independent clock-provenance guard, and a case proving an absent instant still takes the database default, which is what keeps the store's behaviour unchanged while the decorator is off.
…oint The generator that emits the pass-through store base is a runnable script, not dead code, and the same glob covers any script added there later.
|
WalkthroughThe change adds a Redis-backed execution snapshot decorator with rollout modes, sampled reads, transaction staging, retries, repair handling, and waitpoint-cycle support. PostgreSQL snapshot writes can be disabled while preserving run mutations. Snapshot IDs and timestamps can be shared across stores. Redis gains timestamp-window reads and orphan sweeping. Generated delegation infrastructure keeps Merge Risk: 🟡 Moderate · up to The snapshot orphan sweeper does not honor custom key prefixes, so configured stores can retain orphaned Redis data, and the test suite still uses mocks where the repository requires Testcontainers; merge should wait for these bounded correctness and validation issues to be fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
internal-packages/run-store/src/snapshotEntry.ts (1)
37-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd development crumbs to the new snapshot routing logic.
internal-packages/run-store/src/snapshotEntry.ts#L37-L158: mark entry derivation paths with//@crumbs`` or an@crumbsregion.internal-packages/run-store/src/snapshotReadShapes.ts#L36-L95: mark accepted and rejected query-shape branches with//@crumbs`` or an@crumbsregion.As per coding guidelines, “Add crumbs as you write code — not just when debugging.”
Source: Coding guidelines
internal-packages/run-store/src/redisSnapshotStore.ts (1)
502-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared window-decoding block.
Lines 502-529 duplicate lines 443-471 of
getSinceverbatim, including theheadSurvivedinvariant and the cycle-mismatch check. The two paths must stay in agreement. Extract a private helper that takes the reply array and the options, and call it from both readers.♻️ Sketch of the extraction
+ `#decodeWindow`( + reply: string[], + runId: string, + environmentId: string | undefined + ): { entries: SnapshotRead[]; headWaitpointIds: WaitpointIds } { + const headOrder = reply[1] ?? ""; + const rows: SnapshotRead[] = []; + let headSurvived = false; + for (let i = 2; i + 3 < reply.length; i += 4) { + const decoded = this.#decode( + [reply[i], reply[i + 1], reply[i + 2], reply[i + 3], ""], + environmentId, + runId, + false + ); + if (decoded) { + rows.push(decoded); + if (i === 2) headSurvived = true; + } + } + rows.reverse(); + const head = headSurvived ? rows[rows.length - 1] : undefined; + const headWaitpointIds = decodeWaitpointIds(head !== undefined, head ? headOrder : ""); + if (head) { + head.completedWaitpointIds = headWaitpointIds; + if (head.cycle) { + this.#checkCycleMismatch(runId, head.cycle.count, headWaitpointIds.order.length); + } + } + return { entries: rows, headWaitpointIds }; + }internal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.ts (1)
204-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the completed-waitpoint join gate with a non-empty id list.
This test passes
completedWaitpointIds: [], so#connectCompletedWaitpointsis a no-op even without the newsnapshotWritesgate atPostgresRunStore.tsLine 1312. The test therefore does not prove the gate works. Add a case that seeds a waitpoint and passes its id incompletedWaitpointIds, then assert that no_completedWaitpointsjoin row exists whensnapshotWritesis false.internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
705-715: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe window hydration issues one round trip per returned entry.
#hydrateruns once per entry in the window. Each call can add aredis.getSnapshotWaitpointIdsround trip whenread.completedWaitpointIdsis absent, and a delegatefindExecutionSnapshotPostgres query whenentry.checkpointIdis set. Withtake: 50that is up to 50 extra Redis calls and 50 extra Postgres queries for one read that is meant to avoid Postgres.Consider hydrating the waitpoint ids and the checkpoint rows for the head entry only, or batching the checkpoint lookup into a single delegate query keyed by the snapshot ids in the window.
internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)
250-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe born-terminal test does not assert the completion expiry.
The test name states that the birth applies the completion TTL. The assertions only check that both reads are non-null. Those assertions pass whether or not the birth set an expiry, so the test cannot detect the regression it describes.
Assert the TTL directly on the terminal keyspace and assert its absence on the non-terminal one.
💚 Proposed assertion using a probe client
const terminal = await redis.getLatest(runId); const alive = await redis.getLatest(nonTerminal); expect(terminal).not.toBeNull(); expect(alive).not.toBeNull(); + + // The birth is the only write a born-terminal run gets, so it must carry the TTL itself. + const terminalTtl = await probe.pttl(`snap:{${runId}}:e`); + const aliveTtl = await probe.pttl(`snap:{${nonTerminal}}:e`); + expect(terminalTtl).toBeGreaterThan(0); + expect(terminalTtl).toBeLessThanOrEqual(COMPLETED_TTL_MS); + expect(aliveTtl).toBe(-1);Create
probewithcreateRedisClient(redisOptions, { onError: () => {} }), as the waitpoint-cycle suite does, and quit it in thefinallyblock.internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts (1)
69-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
seedRunappends the birth twice.
build()uses moderedis-readordual-write, sodecorated.createRunalready appends the birth to Redis before it writes Postgres. The directredis.appendabove it repeats that write with the same snapshot id and an earliercreatedAt.Two consequences follow. The keyspace exists before the decorator runs, so every test in this suite would still pass if the decorator stopped writing births. And the Redis entry keeps the fixture's timestamp while Postgres records the decorator's, so the birth loses timestamp parity inside this suite.
♻️ Proposed fix: let the decorator write the birth
- await redis.append({ - entry: entryFromCreateRun({ id: snapshot.id, runId, createdAt: new Date() }, snapshot), - kind: "birth", - isTerminal: false, - }); await decorated.createRun({ data: buildCreateRunData(runId, env), snapshot }); return runId;The
redisparameter and theentryFromCreateRunimport then become unused in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84c3b0f1-469a-4170-98b4-5c0a5092a064
📒 Files selected for processing (32)
internal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/scripts/generateDelegatingRunStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/types.tsknip.json
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (31)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: runops-guard / runops-guard
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
internal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/scripts/generateDelegatingRunStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/types.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
internal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/scripts/generateDelegatingRunStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/types.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/scripts/generateDelegatingRunStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/types.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
internal-packages/run-store/src/index.tsknip.jsoninternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/scripts/generateDelegatingRunStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/types.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: We use vitest exclusively. Never mock anything - use testcontainers instead.
Test files go next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
internal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts
internal-packages/run-engine/src/engine/tests/**/*.test.ts
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Implement tests for RunEngine in
src/engine/tests/using testcontainers for Redis and PostgreSQL containerization
Files:
internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
🧠 Learnings (3)
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
internal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/types.ts
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/snapshotEntry.parity.test.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
internal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.ts
🪛 OpenGrep (1.26.0)
internal-packages/run-store/scripts/generateDelegatingRunStore.ts
[ERROR] 105-105: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 111-111: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (20)
internal-packages/run-store/src/redisSnapshotStore.ts (2)
743-801: LGTM!
901-909: LGTM!internal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.ts (1)
1-193: LGTM!internal-packages/run-store/src/snapshotOrphanSweeper.ts (1)
42-51: LGTM!Also applies to: 104-128
internal-packages/run-store/src/snapshotOrphanSweeper.test.ts (1)
1-345: LGTM!internal-packages/run-store/src/PostgresRunStore.ts (1)
121-127: LGTM!Also applies to: 648-671, 747-748, 766-766, 778-778, 795-795, 834-853, 947-961, 1154-1166, 1286-1326, 1393-1405, 1466-1480, 2000-2001, 2017-2068, 2088-2088
internal-packages/run-store/src/PostgresRunStore.snapshotId.test.ts (1)
1-150: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.ts (1)
10-94: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.ts (1)
13-95: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.ts (1)
21-397: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)
172-174: 🩺 Stability & AvailabilityDo not add the
quit()guard.RedisSnapshotStore.quit()already swallows connection and double-quit errors before returning.> Likely an incorrect or invalid review comment.internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
639-659: 🩺 Stability & AvailabilityNo validity-filter change is needed
> Likely an incorrect or invalid review comment.internal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.ts (1)
20-27: LGTM!Also applies to: 29-53, 67-231
internal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.ts (1)
33-63: LGTM!Also applies to: 72-97, 136-454
internal-packages/run-store/src/testFixtures/snapshotIdFixture.ts (1)
20-77: LGTM!Also applies to: 86-167
internal-packages/run-engine/src/engine/tests/helpers/decoratedStore.ts (1)
19-75: LGTM!internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts (1)
14-53: LGTM!Also applies to: 55-304
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (2)
32-58: LGTM!Also applies to: 106-163, 279-399
232-237: 🩺 Stability & AvailabilityNo teardown guard is needed
RedisSnapshotStore.quit()already memoizes the shutdown promise and swallows rejectedredis.quit()calls, including failed connections.> Likely an incorrect or invalid review comment.knip.json (1)
40-42: LGTM!
| function recordingDelegate(): { store: RunStore; calls: { name: string; args: unknown[] }[] } { | ||
| const calls: { name: string; args: unknown[] }[] = []; | ||
| const store: Record<string, unknown> = {}; | ||
|
|
||
| for (const name of RUN_STORE_METHOD_NAMES) { | ||
| store[name] = (...args: unknown[]) => { | ||
| calls.push({ name, args }); | ||
| return `result:${name}`; | ||
| }; | ||
| } | ||
| for (const name of RUN_STORE_PROPERTY_NAMES) { | ||
| store[name] = `property:${name}`; | ||
| } | ||
|
|
||
| return { store: store as unknown as RunStore, calls }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace the manual RunStore fake.
recordingDelegate() mocks every RunStore method and property. Use a concrete, Testcontainers-backed RunStore and valid operations instead.
As per coding guidelines, “We use vitest exclusively. Never mock anything - use testcontainers instead.”
Source: Coding guidelines
…arity real The sweep discovered keyspaces by their cur key, which the append script writes only when an entry is valid. A keyspace whose entries all carry an error has no cur and no index, so neither sweep rule could ever see it and it leaked with no expiry, which is the same unbounded leak the second rule exists to close. It now scans on the entry hash, which every append writes, and the age probe falls back to the newest instant in that hash when the index is empty. Enumerating a run's cycle keys used KEYS. That command iterates the whole database and blocks while it does, and a hash tag routes a key without scoping the scan, so a sweep pass would have issued one full keyspace scan per run. It now reads the dense cycle high-water counter the append script maintains, which is the same source the store's own terminal-expiry loop uses, and pipelines the existence checks into one round trip. The timestamp parity assertion was still tautological. The previous commit added a note saying the builders receive an independent instant and did not change the builder calls, which kept reading the value off the row under test. Every case now mints one instant, passes it to the store, and gives the builder the same value, so a write site that stops forwarding the caller's instant fails here. Also documents what an injected fault actually does at each write path, since only the birth path rethrows, and scopes a run count in the chaos suite to the environment under test.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e5fbb6a-d5a7-4274-a849-fb630afffe9c
📒 Files selected for processing (5)
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- internal-packages/run-store/src/snapshotFaultInjection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: All PR Checks
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
internal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
internal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: We use vitest exclusively. Never mock anything - use testcontainers instead.
Test files go next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
internal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
internal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
internal-packages/run-engine/src/engine/tests/**/*.test.ts
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Implement tests for RunEngine in
src/engine/tests/using testcontainers for Redis and PostgreSQL containerization
Files:
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts
🔇 Additional comments (1)
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (1)
189-191: LGTM!
| /** | ||
| * Every key for one run: the four core keys plus each wait-cycle key. | ||
| * | ||
| * The cycle keys are enumerated from the `c` high-water field on the seq hash, which the append | ||
| * script mints densely with HINCRBY, so 1..high covers every wp key that was ever written. This | ||
| * is the same source the store's own terminal-expiry loop uses. | ||
| * | ||
| * It deliberately does NOT use `KEYS`. That command iterates the whole database and blocks while | ||
| * it does, and a hash tag routes a key without scoping the scan, so one sweep pass over a batch | ||
| * would issue a full keyspace scan per run. | ||
| * | ||
| * The trade-off: if the seq hash is evicted while a wp key survives, `high` reads 0 and that | ||
| * orphaned cycle key is left behind. That is the right way to be wrong here. Leaving one small | ||
| * key costs bytes, where scanning the keyspace to find it costs every hot-path client latency on | ||
| * every pass. | ||
| */ | ||
| async #allKeys(runId: string): Promise<string[]> { | ||
| const core = snapshotKeys(runId); | ||
|
|
||
| const high = Number((await this.#redis.hget(core.seq, "c")) ?? "0"); | ||
| const cycles: string[] = []; | ||
| for (let n = 1; n <= high; n++) { | ||
| cycles.push(`${this.#prefix}{${runId}}:wp:${n}`); | ||
| } | ||
|
|
||
| const candidates = [core.e, core.idx, core.cur, core.seq, ...cycles]; | ||
|
|
||
| // One round trip for the whole set, rather than one per candidate. | ||
| const pipeline = this.#redis.pipeline(); | ||
| for (const key of candidates) { | ||
| pipeline.exists(key); | ||
| } | ||
| const replies = await pipeline.exec(); | ||
|
|
||
| return candidates.filter((_key, index) => replies?.[index]?.[1] === 1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add crumb markers to the changed blocks. The selected new code has no // @Crumbs marker or `#region `@crumbs block.
internal-packages/run-store/src/snapshotOrphanSweeper.ts#L219-L253: mark the wait-cycle discovery and key-existence logic.internal-packages/run-store/src/snapshotEntry.parity.test.ts#L62-L77: mark the timestamp fixture and snapshot helper.internal-packages/run-store/src/snapshotOrphanSweeper.test.ts#L313-L352: mark the all-invalid-keyspace integration test.
As per coding guidelines, “Add crumbs as you write code — not just when debugging. Mark lines with // @Crumbs or wrap blocks in `// `#region` `@crumbs.”
📍 Affects 3 files
internal-packages/run-store/src/snapshotOrphanSweeper.ts#L219-L253(this comment)internal-packages/run-store/src/snapshotEntry.parity.test.ts#L62-L77internal-packages/run-store/src/snapshotOrphanSweeper.test.ts#L313-L352
Source: Coding guidelines
| async #allKeys(runId: string): Promise<string[]> { | ||
| const core = snapshotKeys(runId); | ||
|
|
||
| const high = Number((await this.#redis.hget(core.seq, "c")) ?? "0"); | ||
| const cycles: string[] = []; | ||
| for (let n = 1; n <= high; n++) { | ||
| cycles.push(`${this.#prefix}{${runId}}:wp:${n}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor keyPrefix when deriving core snapshot keys.
When callers set keyPrefix, Line 236 calls snapshotKeys(runId), which always uses snap:. The scan at Line 118 finds the custom-prefixed entry hash, but #allKeys() cannot find its core keys or sequence counter. Both cleanup rules then skip that keyspace. Build core keys from this.#prefix in both #allKeys() and #newestEntryAgeMs(). Add a non-default-prefix test.
1 participant
Summary
Adds a
RunStoredecorator that mirrors execution snapshots into Redis alongside Postgres, plus the orphan-key sweep and the fault-injection suite that prove the write protocol converges after a crash. Nothing constructs it, so merging this changes no behaviour: the configuration, the production wiring and the Redis client all arrive in later work.The execution-state log is the hottest table in the run graph, and moving it out of Postgres has to happen without a big-bang cutover. This is the attachment point for that: a decorator that wraps the existing storage interface and intercepts only the methods that touch snapshots, so none of the many callers change.
Design
Write order is the correctness property, and the two orders differ on purpose.
A transition writes Postgres first and Redis second. A crash in the gap leaves a run whose latest snapshot is stale, which is the state the heartbeat stall watchdog already heals in production today.
A birth writes Redis first and Postgres second. A crash there leaves an unreachable key for a run that does not exist. Postgres first would instead leave a run with no snapshot at all, which the engine treats as a hard error, so the run would be stuck.
Each order is chosen so the state a crash leaves behind 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. A failed append retries, then hands the run to that job, and never rethrows, because Postgres has already committed and a throw would turn a healable gap into a caller-visible error.
Inside a transaction the Redis half is staged and flushed only after the commit, so a rollback cannot leave Redis holding a transition that never happened.
Reads are shape matched. Two of the snapshot reads take arbitrary Prisma arguments, and a key-value store cannot answer an arbitrary query, so the decorator recognises exactly the shapes the engine sends and delegates everything else. A miss falls back to Postgres, which is also how runs created before any cutover keep working.
The sweep reaps under two rules, because neither can see what the other leaves behind. A finished run whose keyspace never received its completion expiry gets one applied. A keyspace with no run row at all, past an age threshold, is deleted; that is a crashed birth, which is non-terminal so it carries no expiry and has no run row, so the first rule can never match it.
Inertness
Three independent reasons this is a no-op if merged alone:
Notes for review
The snapshot id and the creation instant are both minted by the decorator and written into both stores, so one snapshot has one identity and one timestamp wherever it is read. Without that, the two stores disagree on values that later tooling has to compare, and the cursor for a snapshot window resolved from one store misfilters the window walked in the other.
Three defects in this work passed the full existing test suites before being found by review rather than by a test: the decorator wrote no wait cycle at all, the snapshot window dropped the ordering used to give each completed waitpoint its position in a batch, and the two stores stamped different creation times. The common cause was that no test drove a snapshot that actually carried waitpoints, and that the parity suite compared a timestamp against a value it had just read back from the row it was checking. Both gaps now have tests.