feat(run-store): add an execution-snapshot store decorator behind an off-by-default dial - #4765
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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Redis-backed execution snapshot decorator with rollout modes, sampled reads, transaction staging, retries, repair handling, and waitpoint-cycle hydration. Adds PostgreSQL controls for conditional snapshot writes and caller-supplied IDs and timestamps. Adds strict Redis read queries and orphan-keyspace cleanup. Adds delegation infrastructure, test fixtures, and integration coverage for parity, lifecycle reads, crash recovery, stale snapshots, and write failures. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, design rationale, inertness explanation, and review notes. It omits the template’s checklist, explicit testing section, changelog, screenshots, and issue reference, but the core change and validation context are sufficiently documented. ✨ Finishing Touches 💡 1📝 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Review asked why the pass-through base is tested against a hand-built delegate rather than a real store. Checking what the compiler already guarantees showed the test's own stated reason was wrong, and that one of its cases could not fail. implements RunStore already rejects a missing member with TS2420, so the claim that a method added later would become a silent hole was not true. The case comparing the class against the generated name list could not detect a parse miss either, because both the class and the list come from one parse of the interface, so a miss drops the member from both sides. The generator's comment asserting otherwise was false. Parity now lives where it can actually fail: assertions tying the name lists to keyof RunStore in both directions, and one rejecting a public member the class declares and the interface does not. They sit in src rather than in a test, because the build config excludes test files, so a type assertion written in a test is never checked. Each was verified by making it fail. What the compiler cannot see is inside the forwarder bodies, since every one is typed (...args: any[]): any. A forwarder wired to the wrong member, or dropping an argument, typechecks cleanly. The remaining probe covers exactly that, using a per-member sentinel so a misrouted body returns the wrong value rather than merely returning something. Verified by rewiring a forwarder: typecheck passes, the probe fails and names the member. Renames the double to forwardingProbe across both suites and says at the top why a container cannot replace it: no database is involved in whether a pass-through passes through.
A member was removed from the generated list while verifying that the new parity assertion fails when one goes missing, and the restore did not run, so the verification state was committed. Regenerated from the interface. The assertion did its job: typecheck rejects the list, naming the missing member.
…ecorator-tri-13449
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… client key prefix Two defects from review, both silent. An append staged inside a transaction dropped its expected-head argument, and the post-commit flush passed undefined in its place. That disabled the compare-and-set for every snapshot written inside a transaction, which is the path both engine transaction writers use, so a stale append that the store would have refused as forked was written instead and became the head. The expectation now travels with the staged entry. The sweep built its scan pattern without the client key prefix. ioredis prepends that prefix to keys for ordinary commands but not to a SCAN MATCH pattern, and returns matched keys with it still attached, so a prefixed client made the sweep match nothing and report a clean pass. The engine sets a prefix on every other Redis client it builds, so this would have surfaced at wiring time as a reaper that silently protected nothing. Also removes a keyPrefix option on the sweep that could never work: the keyspace prefix belongs to snapshotKeys in the store, which writes snap: keys unconditionally, so there was no other keyspace to point it at. Both fixes have a test verified by reintroducing the defect: the staged stale append is written without the guard, and the prefixed sweep scans nothing.
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… source The generator was scaffolding for a one-off job: writing 70 near-identical forwarders. Keeping it meant carrying a hand-rolled scanner over the interface body, because the TypeScript compiler API is not resolvable in this workspace, which is more machinery than a file that changes only when the interface does. The two files it produced are now maintained by hand, and their headers say so. Nothing is lost, because the generator was never what guaranteed they were right. That is the compiler: implements RunStore rejects a missing member, and the parity assertions tie both name lists to keyof RunStore in each direction and reject a public member the interface does not declare. Each was re-verified by making it fail after the generator was removed. Also drops the knip entry that existed only to treat that script as an entry point.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The sweeper needs to know which run statuses are terminal and cannot import the list, because run-engine depends on run-store rather than the other way round. The copy's comment claimed a parity test kept the two equal. No such test existed, so the claim was false and the copy could drift silently. Drift is not symmetric. A status added to the engine and not the copy makes the sweep treat a finished run as live and never apply its completion expiry. A status removed from the engine and not the copy makes it treat a live run as finished, and that reaps state a run is still using. Verified by removing a status and rebuilding: the test fails and reports seven members against eight.
The forwarders were (...args: any[]): any, so the compiler could not see inside them. A body that called the wrong delegate member, or reordered its arguments, typechecked cleanly. That is not a theoretical gap: it is why a runtime probe existed to catch it, and it is the same shape of hole that let three other defects on this branch pass a green suite. Every member now restates its interface signature and forwards its arguments by name, so both mistakes are compile errors. Verified by making them: a forward to the wrong member produces two type errors, and swapping two arguments produces one. Seven members are overloaded. TypeScript cannot express a single body that satisfies an overload set, so their overloads are declared for callers and their one implementation forwards through a cast. That cast is now the only place the compiler is not checking the forward. The probe shrinks to what is left: those seven casts, a dropped OPTIONAL argument (omitting a trailing tx compiles and silently stops forwarding the transaction), and whether the data property is read live or captured once. Its header states which of those the compiler already covers. Headers on both files now describe what they are rather than that they were once scaffolded.
Typing the forwarders closed the wrong-member and reordered-argument holes but not this one: omitting a trailing OPTIONAL argument still compiles. Two forwarders did exactly that, because the retyping pass read parameter names with a pattern that a preceding inline comment defeated, and both affected parameters happened to be optional and commented. The effects were silent and not small. findLatestExecutionSnapshot stopped applying its tenant scope, so a direct use of the base could read across the environment boundary. upsertWaitpointTag stopped applying its residency hint, so a tag write for a new-database environment would land on legacy. A source-level guard now asserts that every single-signature member forwards exactly the parameters it declares, in order. It reads the interface and the base and compares them, because that property is invisible to the compiler by definition. It carries a vacuity check, so a parse failure fails the suite instead of quietly matching nothing, and that check earned itself immediately by catching a parser that skipped every generic member. Verified: with a parameter dropped again, typecheck reports zero errors and the guard names the member and the missing argument.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A completed waitpoint with no batch index was invisible to every Redis read, so a run resumed from the store lost that wait's result while Postgres still returned it. That is every wait.for, every single triggerAndWait and every token: the engine passes index as batchIndex ?? undefined, so only waits inside a batch carry one. The cause was reading the id set out of the ordered list. That list is the index oracle and its positions ARE the indexes, so it can only ever hold indexed ids, and deduping it yields a set missing exactly the index-less ones. Postgres has no such restriction: its completed-waitpoint join records every id. The cycle key now carries the complete distinct set in its own field, written when the cycle is minted and read back beside the order. The order keeps its meaning and stays index-only. Two tests: one asserting an index-less wait survives a round trip with an empty order, and one asserting the set matches the Postgres join for a mix of indexed and index-less waits. Verified by deriving the set from the order again, which makes the waitpoint vanish. The suites missed this because every earlier case gave each waitpoint an index.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous fix stored the complete id set but left three places still deriving it from the ordered list, and the ordered list holds only batch-indexed ids. A carry-forward decided on the order alone. Two DIFFERENT single waits both present an empty order, so they compared equal, the second inherited the first's cycle, and a read returned the wrong waitpoint entirely. The comparison now requires the id set to match as well. The dequeue site built its Redis refs from the ordered list while the delegate connects the complete set in Postgres, so an index-less waitpoint reached Postgres and never reached Redis. Refs are now built from the complete set, with the index taken from the ordered list where the id appears in it. The entry decode derived the set from the order too, which meant getLatest and getById returned an incomplete set. That is the hot read: findLatestExecutionSnapshot hydrates the waitpoint rows from it, so a resume would have fetched no row at all for a single wait. The read scripts now return the stored set alongside the order. Four tests, each verified against its own defect: two consecutive single waits keep separate cycles, a repeated one still carries forward, the dequeue snapshot keeps an index-less id, and the hot read hydrates its row.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A sweep for values derived where they should be read found one more. The hydrated payload left out lastHeartbeatAt entirely, so a Redis-served read returned undefined for it where Postgres returns null. No code writes that column, so null is not a guess: it is the only value Postgres ever holds. The effect was small but constant, on every read served from Redis, and it is the kind of difference a comparator has to either explain or chase. Guarded by comparing the KEY SET of the two payloads rather than their values, so a column omitted by the hydrator fails as a missing key rather than passing as an absent value. Verified by removing the line again: the test names the column. Also covers the timestamp write on both schema variants. updatedAt is declared @updatedat, which Prisma manages, so whether an explicit value survives a create is a property of the client rather than of the schema, and the two variants are separately generated clients. Agreeing declarations were not evidence. Both honour the caller's instant.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
An independent pass hunting one shape, a value derived where it should be read, found these. None was reachable from a test that existed. The hot read paid a second Redis call in its most common case. An entry with no wait cycle has no waitpoints by construction, and the hydrator asked the store to confirm that rather than concluding it, on every read of a run that is not resuming from a wait. It now distinguishes the three cases and only asks when it genuinely does not know. decodeWaitpointIds still reconstructed the id set from the ordered list when the stored set was absent. That is the sixth instance of the bug fixed five times, surviving as a fallback. It is unreachable today, because both fields are written by one command, but the reconstruction is lossy by nature and the loss is silent. A missing set beside a non-empty order now reports the entry as not present, which sends the caller to Postgres. The window read checked one liveness anchor where the append script deliberately checks two and explains why. An index lost to eviction while the entry hash survived would have reported an empty hit rather than a miss, so the poll would have returned nothing new for the rest of the run's life while Postgres held the transitions. The wrapped store handle dropped the staging buffer, so a handle taken inside a transaction would have appended before the commit. No caller writes a snapshot through it today. Also restores excess-property checking on the nested snapshot writes. Routing them through a generic helper let a typo'd field name compile and fail at runtime; a concrete parameter type brings the check back at the five sites that pass a fresh literal. Verified: a bogus field is now TS2353.
Two paths reached the same silent hang, and neither had a test. When the store refuses a carried pointer it was still writing the entry, which then became the run's head with no pointer at all. A read of that answers present-with-nothing, and present-with-nothing is precisely the signal that tells the engine's read-repair it does not need to look, so the runner got a waitpoint-less continue and dropped it. Refusing the pointer stays right; the append now mints a fresh cycle from the refs the caller carried, in the same atomic call, so the entry always has a pointer that can be trusted. Refs are optional and only the fallback needs them, so callers that supply none keep the previous behaviour. The second path needs no refusal at all. An entry whose cycle key has gone still carries its pointer, and the read answered empty for it too. That is reachable by eviction and also by the completion expiry, which is applied to every key for a run at one moment but lets them expire independently. Reads now report such an entry as not present, which sends the caller to Postgres, where the join rows still are. The hot read and the window both fall back rather than serve it. Three tests. The refusal is driven at the store, because the decorator cannot reach it on purpose: its probe sees the id set no longer matches and mints a new cycle, so the refusal only happens when the key vanishes between probe and append. Each verified against its own defect.
At that position Postgres holds no snapshot rows, so a run routed away from Redis by the cohort percentage reads nothing at all. The percentage is only meaningful while both stores hold the data. Fixing it in the dial rather than documenting the constraint makes the combination unreachable, instead of leaving three settings that have to agree by convention.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal-packages/run-store/src/PostgresRunStore.ts (1)
661-676: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd required crumb markers.
The new snapshot-write feature has no
//@Crumbsmarker or `#region `@crumbsblock.
internal-packages/run-store/src/PostgresRunStore.ts#L661-L676: mark thesnapshotWritessetup and nested-write gate.internal-packages/run-store/src/PostgresRunStore.ts#L2029-L2066: mark the Redis-only snapshot return path.internal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.ts#L23-L81: mark the new timestamp test helper.As per coding guidelines, “Add crumbs as you write code” and mark lines with
//@Crumbsor `// `#region` `@crumbs.Source: Coding guidelines
internal-packages/run-store/src/redisSnapshotStore.ts (1)
619-645: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturn
danglingFor(pointer)from both window reads.
getSinceandgetSinceCreatedAtpass five-element rows to#decode, sodanglingCycleis never set. If the head cycle key is missing, Redis returns empty order data, whichdecodeWaitpointIdstreats as present.findManyExecutionSnapshotsthen skips its Postgres fallback and serves an authoritative empty waitpoint order.
🧹 Nitpick comments (1)
internal-packages/run-store/src/redisSnapshotStore.ts (1)
500-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale head-row index in these comments.
Both comments state the head row is at
i === 2. The loops now start ati = 3and the code checksi === 3. Align the comments with the new offsets, and consider deriving the head index from a named constant.Also applies to: 564-567
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69b4cc98-1d4d-449d-b898-8ea3fca6950e
📒 Files selected for processing (12)
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- internal-packages/run-store/src/runStoreMethodNames.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.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
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/delegatingRunStore.test.ts
🧠 Learnings (2)
📚 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/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/delegatingRunStore.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-store/src/taskRunExecutionSnapshotStore.reads.test.ts
🪛 ast-grep (0.45.2)
internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts
[warning] 168-168: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(return this\\.delegate\\.${name}\\(([^;]*)\\);)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.26.0)
internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts
[ERROR] 117-117: 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] 139-139: 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] 169-169: 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/delegatingRunStore.forwarding.test.ts (1)
19-47: LGTM!Also applies to: 49-61, 63-119, 121-150, 152-188
internal-packages/run-store/src/delegatingRunStore.test.ts (1)
1-42: LGTM!Also applies to: 83-107
internal-packages/run-store/src/delegatingRunStore.ts (1)
1-49: LGTM!Also applies to: 51-317, 319-321, 323-437, 439-609, 611-735
internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (6)
199-201: LGTM!
385-396: LGTM!Also applies to: 951-970
575-617: LGTM!Also applies to: 972-977
657-672: LGTM!
692-701: LGTM!
833-841: LGTM!Also applies to: 866-876
internal-packages/run-store/src/redisSnapshotStore.ts (7)
32-43: LGTM!Also applies to: 178-183
275-287: LGTM!Also applies to: 304-326, 346-347
454-464: LGTM!
663-680: LGTM!
733-774: LGTM!
848-848: LGTM!Also applies to: 861-865, 875-875, 910-912
1010-1010: LGTM!Also applies to: 1036-1044
internal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.ts (1)
49-56: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.ts (1)
128-149: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts (1)
245-306: LGTM!Also applies to: 308-359, 361-424, 426-480, 482-543
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.ts (1)
1-20: LGTM!
…ecorator-tri-13449 # Conflicts: # internal-packages/run-store/src/index.ts
…edis cluster SCAN carries no key, so a cluster cannot route it: one connection iterates one node's keyspace and then reports a completed cursor. The sweep now fans out over every master, resolved per pass so a failover cannot leave it scanning a stale node list, and reports how many it covered. Rule 2 deletes a whole keyspace when the run lookup returns no row. That lookup partitions ids by residency and reads each store's replica, so an absent row is not proof of absence. Deletion now needs the keyspace to be seen absent in two separate passes, and any run found to exist clears its mark. Both window reads returned the head's waitpoint order without its dangling flag, so a head whose cycle key had expired came back with an empty order rather than falling back to Postgres, losing every position on a batched resume. Also lets both classes take a caller-built client so they can reach a cluster at all, and gives the sweep a deadline and an abort signal so a pass can stop inside its budget instead of being killed mid-cursor.
…ecorator-tri-13449
…e live write Hoisting the value so the redis-only echo could reuse it also gave the live Prisma write a default it never had. Prisma omits an undefined key, and the column is nullable with no default, so the write stored an empty array where it used to store NULL. The echo needs a concrete array because its return type says so; the write does not, and now does not get one.
…ecorator-tri-13449
Manual testing performedThe automated suites cannot prove the main claim of this PR, which is that the live write path is unchanged. Every test in the branch asserts against expectations written alongside the change, so a test authored with a mistake inherits the mistake as its specification. Four manual checks were run to cover that gap. One of them found a real defect that the full suite was green against. 1. Differential SQL and row diff (the one that matters)A capture harness runs twice against a Postgres testcontainer. Between runs it swaps only Each pass captures two independent things:
Coverage is all ten physical write sites: Arguments deliberately mirror production: no This found a real defect. The first run produced 8 diff lines, all one cause: Root cause: the redis-only echo path needs a concrete array because its return type is not nullable. The expression was hoisted so the echo and the write could share it, and the default the echo needed came along into the write. Fixed by giving the write the possibly-undefined value and applying the default locally in the echo. After the fix both captures are 91 lines and 44,209 bytes, and the normalised diff is 0. The reason this caught what the unit suite did not: it contains no assertions. Its oracle is the previous implementation, so there is nothing to specify incorrectly. 2. Live workloadWebapp plus the hello-world reference project, five real runs (plain, batch trigger and wait, retry, a delayed run, parallel waits), then nine SQL assertions run against both databases. Checking only one would have reported clean while missing half the writes. Each assertion is written so a healthy database returns zero rows. All returned zero: no null Live traffic also confirmed the write-site boundary the diff predicted. Of seven QUEUED snapshots, six were One assertion surfaced 62 runs carrying two 3. Reachability against the built bundleSource greps can miss a re-export chain, so this checks the emitted JavaScript of a fresh webapp build rather than the source. All four new classes are absent from the bundle entirely, having been tree shaken. There are zero constructions of any of them, and Two corrections were needed getting here, both worth noting for anyone repeating it: an initial pass read a stale bundle, and a second matched a type annotation inside a 4. Hot read query plan
Automated results alongside these
Limits worth knowing
The differential harness is the only check that can defend this claim on an ongoing basis. It takes about four minutes and has no assertions to maintain, so it would work well as a CI job gated on changes to |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/run-store/src/snapshotOrphanSweeper.ts (1)
376-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo comments in this change describe behavior the code does not have.
Line 376-378 states the marker TTL is a multiple of the confirm window. The marker is a hash field with no TTL, as the very next sentence says. Remove the first claim.
Line 406-408 states
#clearOrphanMarkeris called "only for terminal runs" and that a live run never holds a marker.#sweepBatch(Lines 289-293) calls it for every run that exists, live ones included, and the comment there explains that a live run can hold a marker left by an incomplete lookup. Both statements contradict the guard they document, which is a destructive path.♻️ Proposed comment corrections
if (!dryRun) { - // The TTL is a multiple of the confirm window so a candidate gets several chances to be - // sighted again, while a marker left behind by a run that turned out to be alive cannot - // linger long enough to pre-authorise a later deletion. // The field carries no TTL of its own; it lives and dies with the seq hash, which the // keyspace's own completion expiry already governs. That removes the marker-lifetime knob // whose derivation was wrong in the first place.* Clears a rule 2 marker for a keyspace whose run turned out to exist after all, so a later * genuine absence still needs its own two sightings rather than inheriting a stale one. * - * Only called on a path that already found a run row, and only for terminal runs — a live run - * never reaches rule 2, so it can never hold a marker, and charging every live keyspace a round - * trip to prove that would cost more than the case is worth. + * Called for EVERY run row that came back, live ones included: a keyspace marked by an earlier + * incomplete lookup can belong to a live run, and leaving that marker in place would let a later + * genuine absence delete on what is really a first sighting. */Also applies to: 402-409
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f54771b8-5ed5-4d5d-b023-bb0daf0aa154
📒 Files selected for processing (10)
internal-packages/redis/src/index.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/redis/src/index.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/redis/src/index.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/redis/src/index.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/redis/src/index.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.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
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.tsinternal-packages/redis/src/index.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/PostgresRunStore.ts
🧠 Learnings (2)
📚 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/types.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/redisSnapshotStore.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-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts
🔇 Additional comments (11)
internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts (1)
605-646: LGTM!internal-packages/redis/src/index.ts (1)
1-21: LGTM!internal-packages/run-store/src/PostgresRunStore.ts (1)
121-127: LGTM!Also applies to: 661-677, 753-754, 772-801, 840-859, 953-967, 1160-1172, 1292-1332, 1399-1411, 1472-1486, 1863-1863, 2005-2074, 2101-2101
internal-packages/run-store/src/redisSnapshotStore.ts (2)
208-247: LGTM!Also applies to: 261-284, 335-357, 485-497, 529-535, 545-570, 600-641, 710-727, 780-821, 895-966, 996-1019, 1025-1045
679-691: 🗄️ Data Integrity & IntegrationKeep the current dangling-cycle guard.
findLatestExecutionSnapshotandfindManyExecutionSnapshotscheckdanglingCyclebefore#hydrateconsumescompletedWaitpointIds. No production resume path treats the dangling cycle as an authoritative empty set.internal-packages/run-store/src/snapshotOrphanSweeper.ts (1)
14-20: LGTM!Also applies to: 55-59, 61-127, 129-174, 188-263, 265-311, 313-400, 410-416, 434-455, 525-563
internal-packages/run-store/src/snapshotOrphanSweeper.cluster.test.ts (1)
1-160: LGTM!internal-packages/run-store/src/snapshotOrphanSweeper.confirm.test.ts (1)
1-360: LGTM!internal-packages/run-store/src/snapshotOrphanSweeper.test.ts (1)
47-50: LGTM!Also applies to: 94-97, 137-140, 164-164, 187-187, 220-223, 263-263, 284-288, 311-314, 350-353, 374-374, 404-407, 422-422, 443-443, 458-458
internal-packages/run-store/src/index.ts (1)
6-7: 📐 Maintainability & Code QualityRemove this comment. Both modules exist, and their explicit exports do not collide with the other barrel exports.
internal-packages/run-store/src/types.ts (1)
26-27: 🗄️ Data Integrity & IntegrationNo change required.
PostgresRunStore.findRunsByIdempotencyKeysselects bothidandcreatedAt.RoutingRunStoreandDelegatingRunStoreforward these rows without removing either field.
Both survived earlier revisions of the same change and now describe behaviour the code does not have. One claimed the marker has a TTL derived from the confirm window; it is a hash field with no TTL. The other claimed the marker is cleared only for terminal runs, when it is cleared for every run the lookup returned, which is what stops a live run's stale marker from pre-authorising a later deletion. Both sit on the path that deletes a keyspace, so a reader acting on either could reopen the hole the two-sighting rule closes.
|
Both stale comments corrected in Both survived earlier revisions of my own change rather than being wrong when written: The TTL claim. The marker started as a standalone key with a TTL derived from the confirm window. That derivation turned out to be the bug: at the default confirm window the TTL was shorter than the interval between passes, so a candidate lost its mark before its second sighting and rule 2 reaped nothing while every pass reported clean. The fix moved the marker onto the The "only for terminal runs" claim. That was true when written: the clear lived in the rule 1 path. It stopped being true when I moved it to cover every run the lookup returned, which is what closes the hole where a keyspace marked by an incomplete lookup is then seen alive, keeps its mark, and gets deleted on what is really a first sighting the next time the lookup misses. A That second one is the more dangerous of the two: it reads as an invitation to optimise the clear back to terminal-only, which would silently reopen the hole. The replacement says why it has to cover live runs and names the cost it accepts, so the tradeoff is on the page rather than implied. |
…redis-only `compare` was a name in the dial with no behaviour behind it: it wrote and read exactly as `dual-write` does, so turning it on would have looked like enabling divergence reporting and delivered plain dual-write. A dial value that silently does something other than its name is worse than a missing one. It returns with the ticket that implements the sampled dual-read and diff. `redis-only` was the thinnest tested position and the only one that cannot be rolled back, since the snapshots written while it is on exist nowhere else. It is also the only position that is a PAIR of settings, the decorator's mode and `snapshotWrites: false` on the store beneath it, and the previous single test used a store that still wrote snapshots. Every test in the new suite builds the pair, and covers the run mutation landing without its snapshot row, transitions, completions, the absent waitpoint join rows, and every read being Redis-served. One test characterises rather than endorses: a read shape the decorator does not recognise is delegated, and at this position Postgres holds nothing, so the caller gets an empty result rather than an error. Only the engine calls that method and it issues the recognised shape, so nothing is broken today. It is pinned so the terminal-cutover ticket decides deliberately whether a fall-through here should throw instead of answering empty.
Manual testing, part two: the dial exercised against a real webappThe earlier manual testing note covered the differential diff, a live workload, reachability and the query plan. All of it ran with the dial unset, so it proved the branch is inert. None of it proved the decorator works, because nothing in the repo constructs it. This round closes that. A temporary local patch, never committed, wired the decorator into the run-store singleton behind This matters because nearly every defect found in this branch was an integration fault rather than a logic fault: a guard whose precondition was never produced, a fallback that was dead inside transactions, a scan that does not fan out. Those are exactly the class the unit suites were green against. dual-writeOne full
Five distinct write sites, including the staging facade. That is the path that defers Redis appends until after the Postgres commit, and it only works if callers pass the transaction-bound store. It fired four times and every append landed. Reads were zero, which is correct at this position: dual-write writes Redis and reads Postgres. redis-read at 100 percentA batched
The wait-cycle key written by that run: The gap between Postgres, still written at this position, holds the same four ids in the same order and five join rows. The two stores agree on both halves. That is the parity claim the dual-write design rests on, now demonstrated rather than asserted. What this round did not cover
Correction to the earlier noteThe first manual testing comment said the live workload confirmed the write-site boundary. That was true, but it ran with the dial off, so it confirmed the boundary in the existing Postgres path only. It said nothing about the decorator. This round is the first time any decorator code has run inside a webapp. |
2 participants
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.