feat(run-store,testcontainers): execution-snapshot read comparator and shared test utilities - #4772
Conversation
Reverts the cycleKey export, snapshotBackfill read/mapper/apply, and the CLI wrapper (Tasks 6-9). Forward self-heal (re-enable snapshot writes) plus Redis drain covers a redis-only rollback's correctness, and the ClickHouse audit tail covers history, so a reverse Redis->Postgres backfill reconstructs rows nothing reads before they age out. Removes the item from the plan's redis-only go/no-go gate pending plan-owner sign-off. Comparator, fixture, and shared test utils (Tasks 1-5) are unaffected.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (32)
🧰 Additional context used📓 Path-based instructions (7)We use vitest exclusively. **Never mock anything** - use testcontainers instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging. Mark lines with📄 CodeRabbit inference engine (AGENTS.md) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🔇 Additional comments (1)
WalkthroughThe run-store package adds a public 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
…shared test utilities Carry unrecognised source keys through snapshot normalization so an unknown field surfaces as a divergence instead of being silently dropped. Bound the comparator metric op to a fixed union. Hash cluster-slot keys as UTF-8 bytes, strip line comments in the import-isolation scan, and validate the fault harness times argument.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…n imports on raw source Use an own-property check and skip __proto__/constructor/prototype so an inherited-name field surfaces as a divergence and no key can pollute the prototype. Scan import statements on the raw source (line-anchored) so comment stripping cannot hide a real import.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal-packages/run-store/src/snapshotComparator.isolation.test.ts (1)
29-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle aliases in inline type-only imports.
For
import { type Foo as Bar } from "pkg", the replacement removes onlytype Fooand leavesas Bar. The subsequent check can classifyasas a value specifier, although TypeScript 7.0.2 accepts this type-only import.Parse each named specifier and ignore the complete
type Foo as Barform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d991186-9b6b-43a7-b7fd-afe9fa0e7a3f
📒 Files selected for processing (3)
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (28)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-store/src/snapshotComparator.isolation.test.tsinternal-packages/run-store/src/snapshotComparator.test.tsinternal-packages/run-store/src/snapshotComparator.ts
🪛 ast-grep (0.45.2)
internal-packages/run-store/src/snapshotComparator.ts
[error] 87-90: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const k of Object.keys(source)) {
if (DANGEROUS_KEYS.has(k)) continue;
if (!KNOWN_KEYS.has(k) && !hasOwn(target, k)) target[k] = source[k];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
🪛 GitHub Check: code-quality / code-quality
internal-packages/run-store/src/snapshotComparator.test.ts
[failure] 146-146: eslint(prefer-object-has-own)
internal-packages/run-store/src/snapshotComparator.test.ts:146:12: Disallow use of Object.prototype.hasOwnProperty.call() and prefer use of Object.hasOwn().
internal-packages/run-store/src/snapshotComparator.ts
[failure] 86-86: eslint(prefer-object-has-own)
internal-packages/run-store/src/snapshotComparator.ts:86:51: Disallow use of Object.prototype.hasOwnProperty.call() and prefer use of Object.hasOwn().
🪛 OpenGrep (1.26.0)
internal-packages/run-store/src/snapshotComparator.isolation.test.ts
[ERROR] 26-26: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 39-39: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (1)
internal-packages/run-store/src/snapshotComparator.ts (1)
85-90: 📐 Maintainability & Code QualityDo not replace these checks with
Object.hasOwn(). The run-store source and test configurations use theES2020library.Object.hasOwn()requiresES2022and would fail TypeScript checking unless the configuration changes.
…Keys The normalizer only ever sets known keys, so a non-known source key is never already present; the check was unnecessary and tripped the lint rule against hasOwnProperty. Skipping the prototype-pollution keys still holds.
![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.
…scanner Strip an `as` alias along with the `type Foo` specifier so an aliased type-only import isn't misread as a value import.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Derive the Postgres waitpointIdSet from completedWaitpointOrder (index-bearing) to match the Redis read surface, whose distinctIds is the dedupe of the same ordered set. Comparing it against the full completedWaitpoints relation flagged a spurious divergence for a non-indexed completed waitpoint (a single wait).
2 participants
Summary
Adds the read comparator for the in-progress migration of the run execution-snapshot log from Postgres to Redis. The comparator samples a single read against both stores, normalizes the two results to one shape, and reports any per-field difference with a tagged metric. It never serves a read itself: the diff layer imports only types, so it cannot hold a store client, and a test enforces that by failing if any value import appears.
Also adds a combined Postgres-and-Redis test fixture and two shared test utilities (a cluster-slot assertion and a generic fault-injection harness) that the parallel Redis-store work reuses.
Everything here is inert. Nothing constructs the comparator, so merging changes no runtime behavior. It becomes active only when a later change turns on compare mode.
Notes
The divergence classes separate real differences (scalar, ordering, waitpoint id set, validity, missing on one side) from two expected classes that must not be driven to zero: a rotated idempotency key, and a Redis-only surplus at a since-cursor tie. The since comparison is direction sensitive: a Postgres-only entry at the cursor is always a lost write, never an expected tie.