feat(webapp,run-engine,run-store,redis): wire the execution-snapshot store behind an off-by-default dial - #4783
Open
d-cs wants to merge 138 commits into
Open
feat(webapp,run-engine,run-store,redis): wire the execution-snapshot store behind an off-by-default dial#4783d-cs wants to merge 138 commits into
d-cs wants to merge 138 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.
…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.
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
… 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.
… 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.
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.
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.
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.
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.
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.
…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
…h preserves the latch
…is fault does not stall sweeps The sweep lock release is a write, so the same Redis brownout that fails a sweep pass also fails the release, and a single best-effort attempt left the lock held for its full TTL (sweep budget plus two hours). One transient blip could pause orphan sweeping across the fleet for hours. The release now retries over a short bounded window, so a Redis that recovers within seconds frees the lock promptly; the TTL remains only as the last-resort backstop.
…s Postgres writes, throw on miss)
…gres is never served
…figured With no snapshot-store Redis host set, the feature must add zero standing cost. Two paths did not honour that: - The org census started its periodic organization.findMany poll at import, gated only on NODE_ENV, so a production process with no host still polled the replica every reload interval. Gate the poll on a host check that is independent of NODE_ENV. - The run store wired the redis-only Postgres-suppression predicate into every PostgresRunStore unconditionally, so every snapshot write ran a mode resolution (and a background per-org replica read on a cold cache). Thread the predicate through buildRunStore and wire it only when configured; unset leaves the store a plain passthrough that writes every snapshot row. Both now route through a single isSnapshotStoreConfigured gate.
![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.
…ss gaps Found by an adversarial review of the off-by-default snapshot store. None affect the merged-inert state; all bite only once a Redis host is configured and orgs are ramped. - The v1 programmatic org flag route set the per-org dial but never stamped the one-way residency latch (only the v2 route did), so a run born after a v1 enable was resident while the census kept classifying the org never-enabled and skipped its transitions, freezing its Redis head. Stamp the latch and refresh the census, matching v2. - At redis-only a non-throwing append outcome (forked or skippedNoKeyspace) reached the outcome handler and enqueued a Postgres repair or was dropped, but Postgres holds no snapshot there. Treat both as fatal at redis-only, matching the thrown-error path, so the caller retries rather than losing the transition. - A failed post-save primary read of an org dial left it wedged on the deployment-wide position until the next save or a restart. Retry the primary briefly, then clear the pending flag so refreshes resume. - Correct the redis-only boot warning: this build suppresses Postgres snapshot writes at redis-only, so the old "always writes Postgres" note was misleading.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add the snapshotStoreOrgDials key and catalog schema, a global system-maintained map of orgId to current dial (presence is a one-way enrollment latch, value is the current dial, ramp-to-off stores off). Mark it global-only and org-forbidden by mirroring snapshotStoreGlobalModeEverEnabled across the locked-flag lists and the org-forbidden strip set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both admin org-flag routes now maintain the org's entry in the global cohort map with a single atomic jsonb_set inside the existing FOR UPDATE transaction, so concurrent org saves never clobber each other and off is stored rather than deleted. A zero-rows guard fails loudly if the flag row is missing. After commit the routes reload the global registry instead of invalidating the per-org cache and refreshing the census, so the writing pod converges at once. Add a backfill migration that seeds the flag row from the current enrolled cohort (idempotent via ON CONFLICT), guaranteeing the row always exists on a configured deploy. Cover the map maintenance with a real-Postgres route test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an interleaved-transaction test for the cohort map maintenance. Two independent clients overlap on the single map row: one holds its transaction open after writing org A, the other blocks on the row lock writing org B, then merges onto the committed value once the first commits. Both entries must survive. Verified to fail against a read-modify-write implementation, so it guards the single-atomic-jsonb_set invariant the sequential test could not see. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… map Rewire the snapshot-store dial resolver to read the polled snapshotStoreOrgDials map (pure in-memory) instead of the per-org dial cache, and delete both that cache (createOrgModeSource, its LRU, primary retry machinery, and invalidateSnapshotStoreOrgMode) and the org census. The four aggregate and census accessors now derive from the map: anyOrgReadEnabled, anyOrgRedisOnly, isCohortMember, and orgDefinitelyNeverEnabled. Aggregates and cohort membership read values so a retained "off" entry does not re-enable reads or count as a member, while the definite-negative reads absence. Cold-registry defaults are preserved exactly: reads-enabled true, redis-only false, cohort-member false, definitely-never-enabled false. The metrics cohort label now sources isCohortMember from the map. The decorator, snapshotRunOrg, and runStore are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ual-write DB read Make the hot snapshot read path do no per-request run to org DB read during pure dual-write. resolve(runId) is now a pure cache get: the fire-and-forget replica populate is gone. The decorator primes the run to org cache for free on every mirrored write and every Redis read hit, so a resident run's mapping is known with zero DB. resolveAuthoritative stays as the fail-closed backstop for the redis-only guard, and the fallback order is unchanged. Also corrects the stale writesRedisForTransition doc comment, which still claimed the predicate was org-blind after it became per-org presence-as-latch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The map-maintenance jsonb_set ran on every org flag save, so an unrelated admin save on a never-enrolled org wrote its entry as off. The resolver reads a present off as a per-org opt-out that beats the global dial, which would silently pin that org off the fleet rollout once the global dial was raised. Gate the map write (and its zero-rows guard) on the one-way enrollment latch in the blob being written, in all three write paths (v1, v2 save, v2 clear-all), so only a genuinely enrolled org is recorded. Cover the never-enrolled skip and the enrolled-maintain cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| [FEATURE_FLAG.additionalApiKeysEnabled]: z.boolean(), | ||
| [FEATURE_FLAG.additionalApiKeyIssuanceEnabled]: z.boolean(), | ||
| [FEATURE_FLAG.additionalApiKeyLookupEnabled]: z.boolean(), | ||
| [FEATURE_FLAG.snapshotStoreMode]: z.enum(["off", "dual-write", "redis-read", "redis-only"]), |
Contributor
There was a problem hiding this comment.
🔍 Organization dial exceeds described scope
snapshotStoreOrgMode accepts both read positions, beyond the rollout contract described for organization overrides. Review the operational controls for this expanded cutover path.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: e6272ecd-13b8-4ba4-99c4-31004f113f9f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
apps/webapp/app/env.server.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/runStore.server.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/snapshotStoreFlags.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/test/snapshotSweepRunner.test.tsdocker/docker-compose.extras.ymlinternal-packages/database/prisma/migrations/20260902120000_seed_snapshot_store_org_dials/migration.sqlinternal-packages/run-engine/src/engine/index.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/docker-compose.extras.yml
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
🧰 Additional context used
📓 Path-based instructions (16)
Use Remix flat-file route conventions with dot-separated segments; for example, `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v1.feature-flags.ts
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/runStore.server.ts
Never use `request.signal` to detect client disconnects.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/runStore.server.ts
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/env.server.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/runStore.server.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsapps/webapp/app/v3/snapshotStoreMode.server.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsapps/webapp/app/v3/runStore.server.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsinternal-packages/database/prisma/migrations/20260902120000_seed_snapshot_store_org_dials/migration.sqlapps/webapp/app/v3/featureFlags.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsapps/webapp/app/v3/snapshotStoreMode.server.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsapps/webapp/app/v3/runStore.server.ts
When adding indexes to existing tables, use `CREATE INDEX CONCURRENTLY IF NOT EXISTS` to avoid production table locks.
📄 CodeRabbit inference engine (internal-packages/database/CLAUDE.md)
Files:
internal-packages/database/prisma/migrations/20260902120000_seed_snapshot_store_org_dials/migration.sql
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/runStore.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsapps/webapp/app/v3/featureFlags.server.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsapps/webapp/app/v3/snapshotStoreMode.server.tsapps/webapp/app/v3/runStore.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsapps/webapp/app/v3/snapshotStoreMode.server.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsapps/webapp/app/v3/runStore.server.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/app/routes/admin.feature-flags.tsxinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsapps/webapp/app/v3/snapshotStoreMode.server.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsapps/webapp/app/v3/runStore.server.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 metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/app/v3/snapshotStoreConfigured.server.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/app/v3/snapshotStoreInstance.server.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsapps/webapp/test/snapshotStoreFlagGuard.test.tsapps/webapp/test/cohortMetricLabel.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthWarm.test.tsapps/webapp/app/v3/cohortMetricLabel.server.tsapps/webapp/test/featureFlags.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsapps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.tsapps/webapp/app/env.server.tsapps/webapp/test/snapshotStoreMode.test.tsapps/webapp/test/adminOrgFeatureFlagsDials.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.hotPath.test.tsapps/webapp/app/v3/featureFlags.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsinternal-packages/run-engine/src/engine/snapshotSweepSchedule.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.checkpoint.test.tsinternal-packages/run-engine/src/engine/workerCatalog.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.priming.test.tsapps/webapp/test/snapshotSweepRunner.test.tsapps/webapp/test/snapshotStoreGlobalModeEverEnabled.test.tsapps/webapp/test/snapshotStoreFlags.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.latch.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/v3/snapshotStoreBoot.server.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/snapshotStoreFlagGuard.server.tsapps/webapp/app/v3/snapshotSweepRunner.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/snapshotStoreMetrics.server.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.tsapps/webapp/app/v3/snapshotStoreMode.server.tsinternal-packages/run-engine/src/engine/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsapps/webapp/app/v3/runStore.server.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: internal-packages/run-engine/src/engine/snapshotSweepSchedule.test.ts:1-42
Timestamp: 2026-08-26T12:51:12.946Z
Learning: In the Trigger.dev repository, `apps/webapp` Vitest tests conventionally live in `apps/webapp/test/`, which is included by the webapp Vitest configuration. Tests in `internal-packages/run-engine` are colocated with their source modules, such as `internal-packages/run-engine/src/engine/snapshotSweepSchedule.test.ts`.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: apps/webapp/app/v3/snapshotStoreBoot.server.ts:217-219
Timestamp: 2026-08-28T13:46:10.361Z
Learning: In PR `#4783`, `RUN_ENGINE_SNAPSHOT_STORE_HALT` is a retired compatibility environment variable in `apps/webapp/app/env.server.ts`. `apps/webapp/app/v3/snapshotStoreBoot.server.ts` reads it through the `env` adapter and refuses boot only when its value is `"1"`. A value of `"0"` is ignored. The active hard stop is the `snapshotStoreHalt` feature flag.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: internal-packages/run-store/src/redisSnapshotStore.ts:897-900
Timestamp: 2026-08-28T13:48:49.469Z
Learning: In `internal-packages/run-store/src/redisSnapshotStore.ts`, if a transition finds the Redis `idxKey` missing while the keyspace remains live, it must continue the append and mark the `seqKey` gap marker (`g`) rather than reject the transition. Rejecting would freeze the Redis head and cause later compare-and-set appends to fork. Both `getSince` and `getSinceCreatedAt` must treat the gap marker as a miss so the caller falls back to Postgres and does not serve a shortened Redis history.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: internal-packages/run-store/src/redisSnapshotStore.ts:1016-1025
Timestamp: 2026-08-28T13:48:56.706Z
Learning: In `internal-packages/run-store/src/redisSnapshotStore.ts`, `SnapshotOrphanSweeper` discovers snapshot keyspaces through the entry hash. Orphaned `snap:{runId}:wp:<n>` cycle keys without the entry hash are not sweep-visible, so `dropSnapshotRun` must delete cycle keys even when `seqKey` is absent or stale.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts:957-964
Timestamp: 2026-08-28T13:45:01.831Z
Learning: In `internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts`, `findLatestExecutionSnapshot` receives `completedWaitpointIds` from the Redis head read, so `#hydrate` does not call `RedisSnapshotStore.getSnapshotWaitpointIds` for that path. For `findManyExecutionSnapshots`, only the window head row contains decoded waitpoint IDs. A non-head cycle-bearing entry causes `#hydrate` to call `getSnapshotWaitpointIds`, so Redis-error fallback tests must place a later transition after the waitpoint-bearing snapshot and assert that hydration made at least one follow-up Redis call.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: internal-packages/run-engine/src/engine/index.ts:2979-2993
Timestamp: 2026-08-26T13:57:21.260Z
Learning: In `internal-packages/run-engine/src/engine/index.ts`, `RunEngine.#handleSweepSnapshotOrphans()` intentionally logs and swallows failures from `options.snapshotStore.runSweep`. In `packages/redis-worker/src/worker.ts`, both the acknowledgement path and the dead-letter path reschedule cron jobs, so a thrown sweep error is not required to preserve the next sweep. Rethrowing would add a dead-letter entry for each transient sweep failure.
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4783
File: apps/webapp/app/v3/featureFlags.ts:47-54
Timestamp: 2026-08-26T12:50:18.046Z
Learning: In `apps/webapp/app/v3/featureFlags.ts`, `snapshotStoreOrgMode` must remain in `FeatureFlagCatalog` so organization-level feature-flag routes can validate it. Global feature-flag save paths must reject `snapshotStoreOrgMode` because no runtime code reads it from the global row.
📚 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:
apps/webapp/test/snapshotStoreConfigured.server.test.tsapps/webapp/test/snapshotStoreMetricsEmit.test.tsapps/webapp/test/cohortMetricLabel.test.tsapps/webapp/test/snapshotStoreOrgEverEnabled.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.orgReads.test.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/test/snapshotSweepRunner.test.tsinternal-packages/run-store/src/redisSnapshotStore.gaps.test.ts
📚 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-engine/src/engine/snapshotSweepSchedule.tsapps/webapp/test/snapshotRunOrg.server.test.tsapps/webapp/app/v3/snapshotRunOrg.server.tsinternal-packages/run-store/src/redisSnapshotStore.tsapps/webapp/app/v3/snapshotStoreMode.server.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/routes/admin.feature-flags.tsx
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/adminOrgFeatureFlagsDials.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.orgReads.test.tsapps/webapp/test/adminOrgFeatureFlagsV1Route.test.ts
🔇 Additional comments (12)
internal-packages/run-engine/src/engine/snapshotSweepSchedule.test.ts (1)
75-75: 🎯 Functional CorrectnessNo duplicate declaration exists. The block contains one
addeddeclaration.apps/webapp/app/v3/snapshotStoreBoot.server.ts (1)
98-98: LGTM!apps/webapp/app/v3/snapshotStoreInstance.server.ts (1)
16-21: LGTM!Also applies to: 27-28, 92-94
apps/webapp/test/snapshotStoreFlagGuard.test.ts (1)
6-11: LGTM!Also applies to: 78-121, 157-212
apps/webapp/test/snapshotStoreFlags.test.ts (1)
19-23: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.birthFatality.test.ts (1)
1-89: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.redisOnlyFallback.test.ts (1)
1-443: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.residency.test.ts (1)
36-37: LGTM!Also applies to: 52-67
internal-packages/run-store/src/taskRunExecutionSnapshotStore.transitionFatality.test.ts (1)
1-162: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
961-970: 🗄️ Data Integrity & IntegrationNo change needed.
cycleMismatchis a boolean property of the"written"result, not an append outcome.RedisSnapshotStore.appendreturns only the four outcomes handled by the switch.internal-packages/run-engine/src/engine/snapshotSweepSchedule.ts (1)
41-41: 🎯 Functional Correctness
SeedableCounterappears only once ininternal-packages/run-engine/src/engine/snapshotSweepSchedule.ts; the duplicate-declaration claim is refuted.apps/webapp/test/snapshotStoreMode.test.ts (1)
130-130: 🎯 Functional CorrectnessKeep the
orgModeproperty. The test contains oneorgModeproperty in this object literal; the other occurrences are in separate object literals.
| ? await applyGlobalGracedFlips(prisma, requestedFlags, env.RUN_OPS_MINT_FLIP_GRACE_MS) | ||
| : await makeSetMultipleFlags(prisma)(requestedFlags); | ||
| // Stamp the one-way global mode latch before the write, so neither merge branch bypasses it. | ||
| const stampedFlags = await stampGlobalModeLatchForMerge(prisma, requestedFlags); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve snapshotStoreEverEnabled once it is true on both global save paths.
Both paths can write snapshotStoreEverEnabled: false, and replacement saves can delete it because only snapshotStoreGlobalModeEverEnabled is protected. This makes later attempts to enable snapshotStoreMode fail the save guard. Preserve a stored true, ignore false payloads, and exclude the key from replacement deletion. snapshotStoreGlobalModeEverEnabled, not this latch, controls transition mirroring.
| controlPlaneResolver.invalidateOrganization(organizationId); | ||
| // Reload the global registry in THIS process at once so the writing pod converges on the new | ||
| // cohort dial immediately. Other pods lag at most the reload interval. | ||
| void globalFlagsRegistry.reload(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Finish the cohort registry reload before reporting success.
void globalFlagsRegistry.reload() does not wait for the new snapshotStoreOrgDials map. A run created on the writing pod during that interval can resolve its organization against the previous map and receive the wrong residency mode.
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts#L183-L183: Await or synchronously invalidate the registry reload before returning success.apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts#L163-L163: Apply the same completion guarantee after clear-all saves.apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts#L259-L259: Apply the same completion guarantee after normal saves.
📍 Affects 2 files
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.ts#L183-L183(this comment)apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts#L163-L163apps/webapp/app/routes/admin.api.v2.orgs.$organizationId.feature-flags.ts#L259-L259
| // Clear all flags, but preserve the one-way per-org residency latch so an ever-enabled org can | ||
| // never drop out of the census. Locked read-then-write so a concurrent enabling save (which also | ||
| // takes FOR UPDATE) can't slip a latch in between the read and the wipe. | ||
| const updated = await prisma.$transaction(async (tx) => { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use the database transaction helper.
Replace both direct prisma.$transaction calls in this action with $transaction(prisma, ...). The wrapper applies the control-plane maxWait and configured transaction-start retry settings. Direct calls bypass them, so flag updates can fail during pool contention without retry handling.
| organizationId: string | undefined, | ||
| isCohortMember: (organizationId: string) => boolean | ||
| ): string { | ||
| return organizationId !== undefined && isCohortMember(organizationId) ? organizationId : "other"; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not export organization IDs as metric labels.
Line 7 returns organizationId for cohort members. Each enrolled organization creates a separate OTEL series. Return a fixed bounded label such as "cohort" instead.
As per coding guidelines, “Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs … organizationId”.
Source: Coding guidelines
Comment on lines
+57
to
+59
| const cache = new LRUCache<string, string>({ | ||
| max: env.RUN_ENGINE_SNAPSHOT_STORE_RUN_ORG_CACHE_MAX ?? DEFAULT_CACHE_MAX, | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove import-time environment and Prisma dependencies from the testable factory. Importing snapshotRunOrg.server.ts evaluates EnvironmentSchema.parse(process.env). Its prisma import can also invoke singleton(...), create a Prisma client, and call $connect before the test supplies its fake client. Add a cacheMax option, pass it from the test, and keep the environment and Prisma dependencies in a lazy production-only provider. The webapp test convention prohibits direct or indirect env.server.ts imports.
| // global dial. A cold registry yields an empty map, which reads as NO_OVERRIDE for every org. | ||
| get: (organizationId) => orgDials()[organizationId] ?? NO_OVERRIDE, | ||
| refresh: () => {}, | ||
| warm: async () => {}, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Wait for the organization dial before birth routing.
createRun awaits warm, but the production warm implementation resolves immediately. While globalFlagsRegistry.current() is undefined, get() returns NO_OVERRIDE, so resolve() uses the global mode. The asynchronous boot check does not gate createRun. An enrolled organization can therefore be born with the global mode instead of its organization mode.
Make warm await the registry's first successful load, or resolve the organization dial authoritatively before allowing the birth to continue.
Comment on lines
+104
to
+109
| it("resolveAuthoritative throws when the read exceeds the deadline", async () => { | ||
| const primary = fakeClient({ mapping: { run_a: "org_a" }, delayMs: 2000 }); | ||
| const source = createSnapshotRunOrgSource({ primary }); | ||
|
|
||
| await expect(source.resolveAuthoritative("run_a")).rejects.toThrow(/deadline|exceed/i); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the deadline test to a shorter delay.
resolveAuthoritative clears only its 500 ms deadline timer. The fake client's real 2000 ms setTimeout remains referenced after the rejection, so it can keep the Vitest worker alive for the remaining 1500 ms. Use a delay slightly above the deadline, such as 600 ms.
Comment on lines
+132
to
+148
| function flakyEvalClient(real: RedisClient, failEvalTimes: number): RedisClient { | ||
| let fails = 0; | ||
| return new Proxy(real, { | ||
| get(target, prop, receiver) { | ||
| if (prop === "eval") { | ||
| return (...args: unknown[]) => { | ||
| if (fails < failEvalTimes) { | ||
| fails += 1; | ||
| return Promise.reject(new Error("simulated release eval failure")); | ||
| } | ||
| return (target.eval as (...a: unknown[]) => unknown)(...args); | ||
| }; | ||
| } | ||
| const value = Reflect.get(target, prop, receiver); | ||
| return typeof value === "function" ? value.bind(target) : value; | ||
| }, | ||
| }) as unknown as RedisClient; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the test doubles from these tests.
The repository convention prohibits mocks in test files and requires testcontainers. Replace the Redis and RunStore Proxy objects with real fixtures. Rewrite the snapshotStoreMode.test.ts assertion without vi.fn; it does not need a container.
| // The join rows link to the snapshot row above. With snapshot writes off there is no such row, | ||
| // so inserting them would leave dangling links for a snapshot that only the Redis store holds. | ||
| if (this.snapshotWrites) { | ||
| if (this.#writesSnapshot(data.snapshot.organizationId)) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve snapshotWrites once for the whole write operation.
#nestedSnapshot() evaluates the predicate before taskRun.update() awaits. Line 1327 evaluates it again after the update. If an organization dial changes in that gap, a snapshot can commit while its completed-waitpoint links are skipped. A resume read can then lose waitpoints.
Compute the boolean before the update. Use that same value for both the nested snapshot and the join-row write.
Comment on lines
+1060
to
+1061
| for probe = cycles + 1, cycles + 512 do | ||
| redis.call('DEL', wpKey(probe)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Delete cycle keys outside the fixed probe range.
If seqKey is missing after a run created wp:513, cycles is 0. This loop deletes only wp:1 through wp:512. wp:513 remains after the entry hash is deleted, and the orphan sweep cannot discover it.
Use a cleanup strategy that covers every possible cycle key. Alternatively, enforce a storage limit that makes 512 a valid maximum. Add a regression test with wp:513 and no seqKey.
Based on learnings, SnapshotOrphanSweeper discovers snapshot keyspaces through the entry hash.
Source: Learnings
At redis-only the TaskRunExecutionSnapshot row is not written to Postgres, but the checkpoint hydration read it back through that row, so a suspended run resumed with a null checkpoint and restarted with no state to restore from. The TaskRunCheckpoint row itself is never suppressed and the Redis entry carries the checkpointId, so read the checkpoint directly by id instead. Adds a residency-aware findTaskRunCheckpointById to the run store (routed to the run's co-located store) and points the decorator's hydration at it. Covered by a new redis-only checkpoint test that fails on the old path and passes now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the Redis-backed execution-snapshot store reachable from production, off by default.
With no
RUN_ENGINE_SNAPSHOT_STORE_REDIS_HOSTset, nothing is constructed, no connection is opened, no metric series is registered and no job is scheduled, so the store chain behaves exactly as it does today. A fresh self-host is in that state: none of the new variables appear in.env.example, the docker files, or the Helm chart. The dial cannot activate anything on its own either, because construction is gated on the connection rather than on the dial.Once the connection is configured, expect three Redis connections and the orphan sweep running on its cron schedule, at any dial position. The sweep runs at every position on purpose: an operator has to observe a full pass before dual-write starts. Until the dial moves, the keyspace is empty and a pass finds nothing, but it is not nothing.
Stacked on #4765, which builds the store and the decorator. Review that one first.
The dial is a feature flag, not an environment variable
A sustained append failure burns a task attempt on every state transition, so runs can exhaust their retry budget on infrastructure failure rather than task failure. Dialling down is therefore a correctness control, and it cannot wait for a deploy.
Two catalog keys, because they must accept different values:
snapshotStoreMode(global) holds all five positions.snapshotStoreOrgMode(per organisation) holdsoff,dual-writeandcompareonly.Snapshot reads are global, so an organisation at a read position would read state its own writes never created. The narrower enum makes that unrepresentable rather than documented.
ORG_LOCKED_FLAGSturns out to enforce nothing (it is a client-side predicate and no save path consults it), so the line is held the way the mint grace stamps hold it: both organisation routes strip the global key from an incoming payload.The environment keeps what cannot be hot-swapped, plus
RUN_ENGINE_SNAPSHOT_STORE_MODEas the floor used when no flag row exists, so a self-host install still works with no rows at all. No variable falls back to the genericREDIS_*: this is a distinct durable endpoint, and a fallback would silently put execution state on the general-purpose cache.The resolver never queries
The dial is read on every snapshot write, while the per-run lock is held. Seven decorator methods accept a caller-supplied transaction, so this code cannot see a caller's transaction boundary, and an awaited read could land inside someone else's open interactive transaction, on the same connection pool for single-DB and self-host.
So the resolver is synchronous. The global value comes from the existing
globalFlagsRegistry, already an in-memory snapshot read synchronously on the trigger hot path. The per-organisation value comes from a bounded LRU. A miss returns the global answer and warms the cache off-path, so a cold organisation costs no round trip and a control-plane blip cannot fail a state transition.Gating, and the sweep
Construction is gated on the connection, not the dial. The store's options require a constructed Redis store, that store opens its socket in its constructor, and the client factory sets no
lazyConnect, so building it unconditionally would open a doomed localhost connection in every self-host install, developer machine and CI run.The orphan sweep runs as a cron job on the engine's existing worker.
enqueueOncegives no overlap protection (its dedup record is the queue item and the ack deletes it, and nothing extends the visibility timeout), so each pass takes a fenced lock released with a compare-and-delete. A bareDELwould let a pass that overran its own lock delete its successor's.The append-failure hook binds to the existing repair job, sharing the stall watchdog's job id and its
availableAt, so the two compensators can never enqueue two repairs for one run and neither can win a race that changes the delay.Notes for review
Three existing assertions changed, deliberately.
runInTransactionnow always installs the staging facade andforWaitpointCompletionalways wraps its handle: both were conditional on the dial, which cannot work once the dial moves at runtime and a per-organisation value lives in an organisation row. The replacements assert the property that matters, that nothing is appended atoff.@internal/redisgains a cluster-capable client. Cluster mode currently reaches the sweep connection but not the hot path, because the Redis snapshot store still builds its own single-node client; that becomes a one-line change once its options accept a pre-built client.Six guards are verified by reintroducing the defect rather than by observing a pass: the organisation key strip, the module-scope instrument check, the construction gate, the graded boot check, the lock fence and the repair job id each fail when their guard is removed.
Three items are threaded and not yet honoured, because they need #4765's store API: the sweep budget, the abort signal, and
CONFIRM_ORPHAN_AFTER_MS. Cluster mode reaches the sweep connection but not the hot path for the same reason. Each is commented at its site.No changeset and no server-changes entry: the dial defaults off,
@internal/redisis not consumed independently, and nothing user-visible changes yet.