feat(webapp,docker): run-ops boot interlocks and migrations at N databases - #4780
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ith an injected shard resolver Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Includes the cross-field boot refinement requiring RUN_OPS_DATABASE_URL when the shard list is non-empty, since gen-1 v1 ids resolve to the new store permanently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…actory Dedupes buildRunOpsWriterClient/buildRunOpsReplicaClient into a single buildRunOpsClient parameterized by role and the resolved pool knobs. The control-plane builders (buildWriterClient/buildReplicaClient) are a separate path and stay untouched. Every resolved value matches the former builders, so split-on deployments are byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…get per pool Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
selectRunOpsTopology gains a shard loop and returns a keyed shard map. An aliasOf:"new" descriptor reuses the new store's clients by reference and opens no pool. Each real shard gets its own resilience budget and the new-role pool knobs merged with its per-shard overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the shard table at boot buildRunStore now produces one dedicated store per shard descriptor and the N-way router via RoutingRunStore.fromShards, keeping the two-store compat router when no shards are configured. The topology singleton logs the resolved shard table (key, address fingerprint, role) only when RUN_OPS_SHARDS is non-empty, so the unset case adds no output. The fingerprint is an address, never an identity claim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… descriptor keys computeMintShard now intersects the active shard set with routableKeys (the RUN_OPS_SHARDS descriptor keys), so a stored key with no descriptor is never minted into and falls back to gen-1. The empty-set check runs first, so an unconfigured deployment is unchanged. Inert until the gen-2 write path wires in resolveMintShard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at/lint Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ri-13429 # Conflicts: # packages/core/src/v3/isomorphic/friendlyId.test.ts
- Make probeOrder a true reverse of precedence so the merge and probe paths agree on a duplicate id, matching the RoutingRunStore invariant. - Split resolveRunOpsPoolKnobs into a pure applyPoolKnobOverrides (tested with literal defaults, no env import) plus an env-reading defaults function. - Move the pure boot-table helpers to runOpsShardTable.ts so their test does not construct the db.server Prisma topology. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ectRunOpsTopology Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The run-ops client factory eagerly $connects for warm-up, but only caught the rejection under NODE_ENV=test — outside test an unreachable shard/run-ops DB at boot surfaced as an unhandled promise rejection. Always catch and log instead; Prisma reconnects lazily on first query, so one unreachable shard must not take down startup. Scoped to the run-ops factory only; the control-plane/legacy builders are unchanged, so the RUN_OPS_SHARDS-unset path stays byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every boot check that must not treat two handles over one database as two databases needs the same list. The alias exemption keys on the declared aliasOf field, never on client object identity: two store objects can sit over one database, which identity comparison cannot see.
… store probeDistinctStores groups every target by its system identifier and database name, so a duplicate between any two stores blocks the boot, not only a duplicate between the gen-1 pair. Fail-closed is unchanged: a probe that cannot answer returns not-distinct. probeDistinctDatabases stays exported as a delegate over a 2-element list. Its four existing container tests are the proof that set uniqueness over one pair is the pairwise compare of today.
computeSplitEnabled builds one probe target per store and passes them to the set-uniqueness probe. An aliased shard is already absent from the list, so it needs no exception. The flag-off short circuit is unchanged, so a single-database boot still opens no second connection.
Every store that owns its own database is probed against the control plane. The legacy emission keeps its exact call shape and its untagged metric series, so a deployment with no shard configured reports what it reports today. A shard emission carries its shard key. Every store emits before any enforcement throw, so one offending store never costs another store its metric. A probe that throws degrades that one store to unknown. Two tests read RUN_OPS_LEGACY_DATABASE_URL from the developer's .env, because ?? only guards nullish and they passed undefined. They now pin the value, so they no longer depend on the local environment.
…stance The gate takes the shard replica handles and warns for any non-aliased shard whose client is not distinct from the control-plane or gen-1 new client. The returned verdict stays the gen-1 verdict: the distinctness sentinel already fail-closes the boot on the same condition, and a gen-2 fault must not disable the proven gen-1 read fan-out on top of that. An aliased shard shares its target's client on purpose, so identity equality is its correct state and never a fault.
…ust the first When a gen-2 shard is configured, RoutingRunStore builds three or more stores (legacy + new + shard) and a waitpoint that is not on its home/run store must be found by probing the others. Three call sites took only the first "other" store (`#shardsExcept(key)[0]`), which was correct with the two-store compat router but silently skips the remaining stores once a shard exists. The effect, observed with a single shard configured: waitpoint lookups return "Waitpoint not found", pending-token counts undercount (which prematurely unblocks a still-waiting run), and many-waitpoint reads miss rows. Fix `#resolveWaitpointStore`, `countPendingWaitpoints` and `#collectManyWaitpoints` to fan out over every other store and merge. Adds a routing unit test that reproduces all three at the production probe order, with the target placed on the store the first-other truncation skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…covered shard buildReplicationSources appends a source per shard that owns its own database, each with the slot, publication and origin generation its descriptor declares. An aliased shard takes no source: its target's slot already carries its WAL. assertReplicationCoversSplit now also requires a source per non-aliased shard. ShardReplicationMisconfiguredError subclasses the split error, so the boot catch site reaches the same process.exit(1) it reaches today. A shard whose runs never arrive in ClickHouse must not serve traffic. RunsReplicationService is untouched. Its own check already rejects a duplicate source id, slot name or origin generation.
… database The runner image has no jq, so the entrypoint cannot parse RUN_OPS_SHARDS on its own. This script prints one DSN per line and takes a unit test, which an inline node -e string could not. An unset or blank variable prints nothing, so a single-database install is unaffected. Invalid JSON exits 1, so a malformed descriptor stops the container before the migrations run rather than after.
The two hardcoded run-ops invocations gain a loop over the shard DSNs. Each shard runs the identical schema, so this is the existing migrations against a new DSN, and @internal/run-ops-database needs no change. A for loop and not a while-read pipeline: a pipeline subshell would swallow a failed migration on any iteration but the last, so a broken shard would boot. Tracing stays off across the capture and the loop, because set -x prints an assignment and the DSN carries credentials. Installs that never set RUN_OPS_SHARDS skip the block entirely.
…n builder the descriptors The read gate receives one handle per shard, with the declared aliasOf, so a shard whose client is not distinct warns. The verdict it returns is unchanged. The replication instance passes the shard descriptors to the source builder and to the coverage check. The distinctness sentinel and the co-residency advisory read the descriptors themselves, so the boot order is unchanged: the probe still runs where it ran, with a wider target list.
…t gate The descriptor parser checks uniqueness among shards only, so a shard could take the slot name, publication name or origin generation of the legacy or the new source. The service has its own check, but it throws from the constructor, which the caller reaches only after it has shut the bootstrap instance down. That left the process up with NO replication at all, legacy included, behind one console.error. It is the exact silent ClickHouse under-count this family of errors exists to prevent. The check now runs in assertReplicationCoversSplit, before anything is torn down, and raises a subclass the existing catch site already recognizes. A correct deployment satisfies this today, because two consumers on one slot is a data race that cannot work.
A shard with no replicaUrl takes its own writer as its replica handle, so its reads go to its primary. That is the per-shard analogue of the existing legacy warning, and it is reachable today. The control-plane identity check stays as a regression guard, now marked as unreachable by construction: a non-aliased shard always gets a freshly built client. It exists so a future control-plane fallback for shards cannot silently route a shard's reads to another database.
…oop's shell options One DSN per line is the protocol between the script and the entrypoint, and the URL parser strips ASCII line breaks, so a DSN holding one would split into two bogus DSNs with nothing upstream to reject it. The loop now runs in a subshell, so its IFS and noglob changes need no restore and cannot leak into the rest of the entrypoint.
|
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (28)
🧰 Additional context used📓 Path-based instructions (13)Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
We use vitest exclusively. **Never mock anything** - use testcontainers instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
Test files must not import `app/env.server.ts`; pass configuration as options instead.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
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:
**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 zod for validation in packages/core and apps/webapp📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
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:
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) 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:
🧠 Learnings (1)📚 Learning: 2026-06-16T09:19:47.637ZApplied to files:
🔇 Additional comments (2)
WalkthroughThe change adds support for independently addressed run-ops shards. Shard descriptors now provide migration DSNs and split-mode database targets. Split-read diagnostics and control-plane co-residency checks inspect non-aliased shards. Replication initialization creates and validates shard sources, including direct URL and identity checks. Tests cover parsing, migrations, database distinctness, runtime diagnostics, and replication coverage. Merge Risk: ⚪ Minimal · up to The PR generalizes run-ops boot checks and migrations for multiple databases while preserving existing two-database behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the implementation, design, behavior, compatibility, testing, and issue context. It omits the template checklist and screenshots sections, but these omissions are non-critical because the required change and validation details are clearly documented.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolve conflicts in runOpsStore.ts and runStore.server.ts against main's
RoutingRunStore refactor (constructor-based shards, #distinctStores,
#partitionAbsentIds/#gen1PairExcept, duplicate-id/probe-fallback metrics).
- runOpsStore.ts: take main. Main's #partitionAbsentIds/#gen1PairExcept
supersede this branch's #shardsExcept waitpoint fan-out fix and are more
precise (cuid relocation confined to the gen-1 pair; gen-2 id to its one
shard), so the earlier fix is dropped.
- runStore.server.ts: re-wire buildRunStore's N-way arm onto main's
RoutingRunStore constructor (shards: [{ key, store, aliasOf }],
resolveShard, metrics), replacing the removed fromShards factory.
- db.server.ts: carry each shard's declared aliasOf through
runOpsShardHandles so the router dedups aliased shards from fan-out sums.
- Rewrite the waitpoint fan-out guard test against the constructor API and
add a gen-2-shard collect case.
Verified: webapp typecheck, run-store typecheck, and the run-store routing
suite (shardMap, threeDbTopology, waitpoints, runKeyedRouting, guard) pass.
…dMatrix covers it Main's runOpsStore.nShardMatrix.test.ts is a four-store testcontainer matrix that already guards the N-way waitpoint fan-out on real databases — the gen-2-shard union with no double count, the mirrored-cuid case, alias dedup, and cross-tree completion. The removed test used fakeStore() stubs, which both duplicates that coverage and violates the repo's "never mock, use testcontainers" rule (CodeRabbit). Removing it also clears the code-quality oxfmt --check failure the unformatted file caused.
runOpsStore.fromShards.test.ts imported UnknownShardKey and called RoutingRunStore.fromShards — both removed in main's RoutingRunStore refactor (constructor-based shards). The file is unique to this branch and now references APIs that no longer exist, so it fails the run-store suite. Its routing coverage lives in main's shardMap/runKeyedRouting/nShardMatrix tests.
![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: |
…ot schema The script was laxer than the schema that validates the same variable. It treated any aliasOf value as an alias, so a shard with a typo in that field was skipped and its database never migrated. It also accepted a shard that owns its database but declares no replication, which the application rejects at boot: the entrypoint migrated the database first and the boot failed afterwards. The script now rejects an unsupported aliasOf value, a descriptor that sets both url and aliasOf or neither, and a non-aliased descriptor with no replication. So an invalid descriptor stops the container before any migration runs, which is what the block exists to guarantee.
# Conflicts: # apps/webapp/app/v3/runOpsShards.server.ts # apps/webapp/test/runOpsShards.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docker/scripts/runOpsShardDsns.mjs (1)
41-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate descriptor values before emitting migration DSNs.
Line 41 accepts
url: "not a URL"because it only checks for a non-empty string. Line 53 also accepts an invalid non-emptydirectUrl. The parser then emits that value.If a valid descriptor precedes an invalid descriptor, the entrypoint can migrate the valid database before the invalid configuration fails. Validate
url,directUrl, and the remaining descriptor fields against the boot-schema contract before collecting any DSN. Add regression cases for invalid non-empty URL values and malformed replication objects.
🧹 Nitpick comments (2)
apps/webapp/test/runOpsShardDsns.test.ts (1)
47-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate this test with its source.
Move this file to
docker/scripts/runOpsShardDsns.test.ts. Update its import path after the move. The source isdocker/scripts/runOpsShardDsns.mjs.Source: Coding guidelines
docker/scripts/runOpsShardDsns.mjs (1)
34-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required crumb instrumentation.
Add a temporary
//@crumbs`` marker for this changed validation path. Strip it withagentcrumbs stripbefore merge.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01161a3a-1e4c-484f-936a-5d47cc827020
📒 Files selected for processing (3)
apps/webapp/app/db.server.tsapps/webapp/test/runOpsShardDsns.test.tsdocker/scripts/runOpsShardDsns.mjs
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (21)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (13)
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/db.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsShardDsns.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/runOpsShardDsns.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/db.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsShardDsns.test.tsdocker/scripts/runOpsShardDsns.mjsapps/webapp/app/db.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.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/runOpsShardDsns.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.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
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/runOpsShardDsns.test.tsapps/webapp/app/db.server.ts
🔇 Additional comments (1)
apps/webapp/app/db.server.ts (1)
584-588: 🎯 Functional CorrectnessNo change is needed for
aliasOf.The consumer receives
aliasOffromenv.RUN_OPS_SHARDS, whose schema restricts it to"new". It does not receive the widenedrunOpsShardHandles.aliasOffield.
Manual verificationAutomated coverage for this change is unit-level, so the topologies and the container packaging were exercised by hand against real infrastructure. Boot topologies. Booted the webapp against one, two, three and four databases. With no shard configured the boot opens no shard client, prints no shard table, adds no metric series and runs replication with the legacy source alone. With one shard configured it opens Fail-closed. Two shards pointed at one database refuse the boot: the sentinel names both shard ids, the assertion throws, and the process exits. The colliding pair is shard against shard while the gen-1 pair is clean, which a pairwise probe cannot detect. Aliased shard. A descriptor declaring Replication end to end. A shard on its own cluster, with its own slot, publication and origin generation 2, streamed a row into ClickHouse. The Read gate. Warns when a shard declares no Container packaging. The image is not built on pull requests, so Not covered. No run has been triggered onto a shard. |
… emitting DSNs The script checked that url was a non-empty string, while the boot schema requires a parseable URL with no empty schema param. So a descriptor the application rejects could pass the script, and a valid descriptor ahead of an invalid one got its database migrated before the configuration failed. Every descriptor is now validated before any DSN is collected: the shard key shape, the region, the three URLs, the alias value, and the replication slot, publication and origin generation bound. Unknown fields are deliberately still accepted, because rejecting them would fail the entrypoint on a descriptor a newer application accepts.
|
Thanks, the first one was a real bug and is fixed in 7fa0609. Validate descriptor values (Major), fixed. You were right, and right about the same file for the same reason as last time: the script was checking The script now validates every descriptor before collecting any DSN: shard-key shape, region, all three URLs (mirroring One deliberate limit: it does not reject unknown fields, even though the schema is Co-locate the test (nit), declining, now with evidence. I asserted this last time without checking; I have now. Crumb instrumentation (nit), declining. Same as the previous round, where this finding was withdrawn against the repository learning that crumbs are temporary instrumentation stripped before merge and should not be flagged. This branch is being prepared to merge. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker/scripts/runOpsShardDsns.mjs (1)
14-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required crumb instrumentation to the changed blocks.
Add
//@Crumbsmarkers or `// `#region` `@crumbsblocks before merge.
docker/scripts/runOpsShardDsns.mjs#L14-L114: mark the descriptor-validation and DSN-output path.apps/webapp/test/runOpsShardDsns.test.ts#L116-L191: mark the new descriptor-validation test block.As per coding guidelines, “Add crumbs as you write code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5bd16d0-10d9-4a8d-a482-59cf0b8a135e
📒 Files selected for processing (2)
apps/webapp/test/runOpsShardDsns.test.tsdocker/scripts/runOpsShardDsns.mjs
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (34)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: typecheck / typecheck
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: audit
- GitHub Check: audit
🧰 Additional context used
📓 Path-based instructions (11)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsShardDsns.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/runOpsShardDsns.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docker/scripts/runOpsShardDsns.mjsapps/webapp/test/runOpsShardDsns.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.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/runOpsShardDsns.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/runOpsShardDsns.test.ts
🧠 Learnings (1)
📚 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/runOpsShardDsns.test.ts
![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.
… not the pooled one A shard source took the shard's writer dsn. Logical replication needs a session-mode connection, and a transaction pooler cannot serve one, so a pooled writer dsn makes the replication client throw inside start(). That throw is not a SplitReplicationMisconfiguredError, so the process stayed up with every source down, legacy included. Gen-1 already keeps this separation through its own RUN_REPLICATION_* variables, and the migration loop already prefers directUrl. A shard source now takes directUrl, and a shard that declares replication without a directUrl refuses the boot rather than falling back to a dsn that may be pooled.
|
On the crumb instrumentation: declining, as in the two previous rounds where this finding was withdrawn. The repository describes crumbs as temporary debug instrumentation stripped before merge, and asks reviewers not to flag their presence or absence. This branch is being prepared to merge, so adding instrumentation that the merge process removes would be backwards. |
3 participants
Summary
The run-ops boot interlocks and the migration entrypoint each assume exactly two run-ops
databases. This generalizes them to any number, so a deployment that configures
RUN_OPS_SHARDSgets the same safety guarantees it gets today with two stores: no two storesmay point at one database, every store that owns its own database must replicate to
ClickHouse, and every store must have its schema migrated.
With
RUN_OPS_SHARDSunset, nothing changes. The distinctness check over a two-element set isthe pairwise compare it replaces, replication coverage is the check it was, and the entrypoint
runs the same two migration invocations.
A shard may declare
aliasOf: "new", which shares an existing store's client by reference. Analiased shard is not its own database, so it is exempt from the distinctness check and needs no
replication slot of its own. Every check keys that exemption on the declared field, never on
client object identity: two client objects can sit over one database, which identity comparison
cannot see.
Design
Distinctness.
probeDistinctDatabasescompared two URLs. It now delegates toprobeDistinctStores, which reads every fingerprint in parallel and groups them by systemidentifier and database name. Any two stores under one key refuse the boot. The old pairwise
entry point stays, so its existing container tests are the proof that set uniqueness over one
pair gives the verdict it gave before. Fail-closed is unchanged: a probe that cannot answer
returns not-distinct, because "distinct" is a positive claim a failed probe cannot support.
Co-residency. The advisory runs once per store against the control plane. The legacy
emission keeps its exact call shape and its untagged metric series, so an existing dashboard
does not change. Each shard emits its own point carrying its shard key. Every store emits
before any enforcement throw, so one offending store never costs another store its metric.
Replication.
buildReplicationSourcesappends one source per shard that owns its owndatabase, taking the slot, publication and origin generation its descriptor declares.
assertReplicationCoversSplitthen requires a source per such shard.That check also closes a hole it inherited. The descriptor parser validates uniqueness among
shards only, so a shard could take the slot name, publication name or origin generation of the
legacy or the new source. The replication service does validate this, but it throws from its
constructor, and the caller reaches that constructor only after shutting the bootstrap instance
down:
The throw was not a
SplitReplicationMisconfiguredError, so the process stayed up with noreplication at all, legacy included, behind one logged line. That is the silent ClickHouse
under-count the error exists to prevent. The check now runs at the boot gate, before anything is
torn down, and raises a subclass the existing exit path already recognizes. A correct deployment
already satisfies it, because two consumers on one WAL slot is a data race that cannot work.
Migrations. Every shard runs the identical schema, so a new shard is the existing migrations
against a new DSN. The runner image has no
jq, so a small node script prints one DSN per lineand the entrypoint loops over them. The loop is a
forand not awhile readpipeline: apipeline subshell swallows a failed migration on any iteration but the last, which would let a
broken shard boot. Tracing stays off across the capture and the loop, because
set -xprints anassignment and a DSN carries credentials.
Verified end to end against real Postgres containers for the fingerprint probes, and against the
real shell block with a stubbed migration command: an aliased shard is skipped,
directUrlwinsover
url, a failing shard stops the container on the first failure, and a malformed descriptorstops it before it migrates anything.
Stacked on #4764.