feat(run-ops): read presenters — de-join control-plane relations + read-through hydration#4122
feat(run-ops): read presenters — de-join control-plane relations + read-through hydration#4122d-cs wants to merge 10 commits into
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces optional "read-through" dependency injection across numerous v3 presenters (batch results, run results, run lists, waitpoints, waitpoint lists/tags, batch lists, batches, run/span/run-stream presenters, and test-task) to route database reads between a "new" run-ops Postgres client and a legacy read replica during a migration window. Several presenters replace raw SQL keyset scans with Prisma-based scan/merge/probe helpers supporting split-read routing. Control-plane concerns (project/organization membership, environment resolution, task schedules, locked-worker versions) are separated from run-ops reads via a Related PRsNone referenced. Suggested labelsNone specified. Suggested reviewersNone specified. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
d-cs
force-pushed
the
runops/pr07-replication
branch
from
5de29cb to
5c2d010
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
9b6eee8 to
94eaef6
Compare
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
apps/webapp/test/presenters/TaskDetailPresenter.getActivity.test.ts (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoosely-typed insert schema.
schema: z.any()bypasses row-shape validation for the ClickHouse insert, so a malformedTaskRunV2fixture (e.g. wrong field name/type) would silently pass through instead of failing fast at insert time.apps/webapp/app/presenters/v3/ApiWaitpointPresenter.server.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming inconsistency across sibling presenters.
ApiWaitpointPresenterReadThroughDepsusesnewClient/legacyReplica, whileApiWaitpointListPresenter/WaitpointListPresenterin this same PR userunOpsNew/runOpsLegacyReplicafor the equivalent concept. Consider aligning naming across the presenter family for consistency.apps/webapp/test/apiWaitpointPresenter.readthrough.test.ts (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a Proxy wrapper for consistency with the sibling test file.
This
recording()uses object-spread ({...client, waitpoint}), while the equivalent helper inwaitpointPresenter.readthrough.test.tsuses aProxywrapper, which is more robust against Prisma client internals not being plain own-enumerable properties. Not currently broken, but worth aligning the pattern across the two nearly-identical helpers.apps/webapp/app/presenters/v3/WaitpointTagListPresenter.server.ts (1)
30-41: 📐 Maintainability & Code Quality | 🔵 TrivialNaming inconsistency for the split-read DI seam across presenters.
This presenter names the constructor option
readRoutewith fieldsrunOpsNew/runOpsLegacyReplica, whileWaitpointPresenterusesreadThroughDepswithnewClient/legacyReplicafor the same conceptual seam. Consider aligning naming across presenters for consistency.apps/webapp/test/waitpointPresenter.controlPlane.test.ts (1)
104-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test never actually calls
WaitpointPresenter.The test validates the standalone building blocks (
prisma17.waitpoint.findFirstand a locally constructedControlPlaneResolver) but never exercisesWaitpointPresenter.call()itself, despite the describe block being about presenter cross-DB read-through. Since the seeded waitpoint has noconnectedRuns, callingWaitpointPresenter.call()directly here wouldn't require mockingclickhouseFactory, making it feasible to assert against the actual presenter output (e.g.result.url, matchingapiKey/organizationId-derived fields) rather than just its dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75888a07-9162-45ef-b719-038223ee3d18
📒 Files selected for processing (37)
apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.tsapps/webapp/app/presenters/v3/ApiRunListPresenter.server.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/app/presenters/v3/ApiWaitpointListPresenter.server.tsapps/webapp/app/presenters/v3/ApiWaitpointPresenter.server.tsapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/presenters/v3/BatchPresenter.server.tsapps/webapp/app/presenters/v3/NextRunListPresenter.server.tsapps/webapp/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/presenters/v3/RunStreamPresenter.server.tsapps/webapp/app/presenters/v3/SpanPresenter.server.tsapps/webapp/app/presenters/v3/TestTaskPresenter.server.tsapps/webapp/app/presenters/v3/WaitpointListPresenter.server.tsapps/webapp/app/presenters/v3/WaitpointPresenter.server.tsapps/webapp/app/presenters/v3/WaitpointTagListPresenter.server.tsapps/webapp/test/SpanPresenter.readthrough.test.tsapps/webapp/test/apiBatchResultsPresenter.readthrough.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/apiRunListPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/nextRunListPresenter.readthrough.test.tsapps/webapp/test/presenters/ApiBatchResultsPresenter.split.test.tsapps/webapp/test/presenters/ApiBatchResultsPresenter.test.tsapps/webapp/test/presenters/TaskDetailPresenter.getActivity.test.tsapps/webapp/test/presenters/TestTaskPresenter.readthrough.test.tsapps/webapp/test/realtime/clickHouseRunListResolver.test.tsapps/webapp/test/runPresenterReadRoute.test.tsapps/webapp/test/spanPresenterReadthroughDecompose.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.controlPlane.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.ts
💤 Files with no reviewable changes (1)
- apps/webapp/test/presenters/ApiBatchResultsPresenter.split.test.ts
d-cs
force-pushed
the
runops/pr07-replication
branch
from
8fb6e8a to
4e08dc7
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
6e4c0de to
48c395b
Compare
d-cs
force-pushed
the
runops/pr07-replication
branch
from
4e08dc7 to
11dc0b7
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
f8b64bd to
e145839
Compare
d-cs
force-pushed
the
runops/pr07-replication
branch
from
11dc0b7 to
a9bc9e6
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
e145839 to
6cd0085
Compare
d-cs
force-pushed
the
runops/pr07-replication
branch
from
a9bc9e6 to
277ecea
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
6cd0085 to
5ccf63f
Compare
…ad-through hydration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- readRedirectMarker fails open on an unprovisioned marker table (undefined_table): the known-migrated read optimization must never break the run-list read path when the marker table is absent. - expose runOps new/legacy handles through the db.server test seam so the routed-store reads resolve to the real containers. - hoist the runStore ref so the vi.mock factory no longer hits a TDZ. - correct the getActivity bucket expectation: a 6h window buckets into 72 five-minute buckets (chooseBucketSeconds targets ~72), not 6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… only Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r09 presenters
Comment/label hygiene pass over the PR09 presenter read-routing work. No
product logic, assertions, seeds, or test structure changed — only comment
text and test/it titles.
- Remove the SPLIT-NEUTRAL scaffolding comment on TaskDetailPresenter.getActivity.
- Drop `// --- ... ---` comment fencing across the touched test files, keeping
the behavioral text.
- Strip Task/Step enumeration prefixes from comments and it/test titles, keeping
the behavioral descriptions.
- Remove Definition-of-Done ("DoD") framing and RED/GREEN TDD phase commentary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tPresenter hydrate select GET /api/v1/waitpoints/tokens/:id returned HTTP 500 under split routing. The hydrate() select in ApiWaitpointPresenter included the connectedRuns RELATION, but it is never referenced in the returned object (all scalars) — dead code left over from the presenters de-join pass (48ae62b70), which stripped relations from WaitpointListPresenter but missed this one. Under split routing a standalone cuid token classifies LEGACY, so readThroughRun probes the scalar-only run-ops NEW client FIRST. The dedicated run-ops Waitpoint model is scalar-only (relations de-normalized), so Prisma threw PrismaClientValidationError: Unknown field connectedRuns before the legacy fallback could run. Removing the block makes the select all-scalar and valid against both the control-plane client and the scalar-only run-ops-new client. No return-shape, type, or SDK change. Regression test: extends apiWaitpointPresenter.readthrough.test.ts with a heteroRunOpsPostgresTest case that uses the REAL scalar-only run-ops client (prisma17) as the split-mode NEW client and seeds a cuid token on the legacy side. Fails before the fix (Unknown field connectedRuns), passes after (resolves via the legacy fallback). Not caught by the existing heteroPostgresTest cases because those run the full control-plane schema on both sides. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d-through presenter design
The pr04-era ApiBatchResultsPresenter.split.test.ts asserted a bare
`new ApiBatchResultsPresenter()` routes the batch-row lookup through the
runStore singleton (via vi.mock("~/v3/runStore.server")) so a NEW-resident
ksuid batch resolves. pr08's call() supersedes that contract: a bare
presenter has readThrough === undefined, so splitEnabled is false and it
takes #callPassthrough, which reads this._replica.batchTaskRun directly
(not through runStore). The test therefore fails against pr08 and encodes a
design that no longer exists.
Both cases it covered are already covered by pr08's own
apiBatchResultsPresenter.readthrough.test.ts against the current #callSplit
API: (a) a NEW-resident batch resolving under split (batch row + items on the
NEW DB, resolved via newClient) and (b) a genuinely-missing batch returning
undefined (split on). Remove the redundant, superseded test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…epair read-side test shims - WaitpointPresenter: default omitted read-through clients to the presenter's own replica instead of the global run-ops singletons, so single-DB/self-host reads hydrate the waitpoint and its connected runs from one DB. - SpanPresenter test: bind proxy-returned store methods to the target so private field access holds for all methods (e.g. findRunOnPrimary), not just findRun/findRuns. - ApiRetrieveRunPresenter read-route test: select scalar lockedToVersionId and fold the resolved lockedToVersion per node, matching the presenter's current shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… testcontainer The RunStore singleton is built once at import and its error-normalizing wrapper memoizes each Prisma model delegate on first access. The test's delegating proxy returned current.taskRun directly, so the store cached the first test's container-bound delegate and later tests (fresh containers) failed with "Database test_0 does not exist". Object-valued delegates now return a stable per-key sub-proxy the store can safely cache, re-delegating to the live current client on each access. Production code is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d-cs
force-pushed
the
runops/pr07-replication
branch
from
277ecea to
2bba3b8
Compare
d-cs
force-pushed
the
runops/pr08-presenters
branch
from
5ccf63f to
105ba1c
Compare
…c empty CH window Two webapp unit-test shards were red on this branch: - Shard 6 (apiRetrieveRunPresenter.readroute.test.ts): the organizationDataStoresRegistry singleton is constructed at import (pulled in transitively via the ClickHouse factory instance) and immediately fires a `forever` pRetry(loadFromDatabase) plus a setInterval reload against db.server's $replica. In CI (no Postgres on localhost) those retry forever and saturate the worker event loop, timing out an awaiting test's hook. Mock the instance module to a no-op in test/setup.ts, mirroring the other eager-singleton stubs. No unit test uses this singleton — the registry-behavior tests construct the class directly — so this is safe and should also help other shards/branches that import the presenter graph. - Shard 2 (nextRunListPresenter.readthrough.test.ts): the two empty-state tests seeded a run then assumed it had NOT yet replicated to ClickHouse within the page window, so result.runs would be empty. That held on a slow local stack but raced on CI, where replication had completed and the run surfaced (length 1, not 0). Scope those two calls to a `to` one hour in the past; the CH page filters created_at <= to, so a just-created run is deterministically excluded regardless of replication timing. The PG existence probe has no time filter, so it still finds the row and hasAnyRuns stays true — the exact behavior each test verifies. Reproduced both failures and verified the fix under a CI-faithful env (DATABASE_URL + REDIS at dead ports, GITHUB_ACTIONS=true, --no-file-parallelism): shard 2 now 18 files / 118 tests green, shard 6 now 16 files / 212 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Reworks the read-side presenters so they no longer depend on control-plane relations being co-located with the run subgraph. This is a read-path-only change — no writes, no schema, no runtime flag flips.
For every presenter that previously joined across what is becoming a database boundary, the join is removed from the primary query and the missing data is instead hydrated per-run through the run store. The pattern is a pair of internal helpers (
#callSplit/#callPassthrough): when the split is active, the presenter selects only scalar/id-shaped columns from the primary query and hydrates the run-subgraph fields via a read-through lookup; when it isn't, it falls back to the existing passthrough path. Presenters key their behaviour on id-shape only — they don't consult a per-record "known-migrated" flag.Presenters touched:
ApiBatchResultsPresenter,ApiRetrieveRunPresenter,ApiRunListPresenter,ApiRunResultPresenterApiWaitpointPresenter,ApiWaitpointListPresenter,WaitpointPresenter,WaitpointListPresenter,WaitpointTagListPresenterBatchPresenter,BatchListPresenter,NextRunListPresenterRunPresenter,RunStreamPresenter,SpanPresenter,TestTaskPresenterA dead
connectedRunsrelation is dropped from theApiWaitpointPresenterhydrate select.New read-through / read-route tests cover each presenter under both the split-active and passthrough paths. A superseded
ApiBatchResultsPresenter.split.test.tsis removed and the remainingApiBatchResultsPresenter.test.tstrimmed — its coverage is subsumed by the newapiBatchResultsPresenter.readthroughsuite.Why
Part of the run-ops database split. This is PR8 of the series (PR8/9/10) and covers the read path only: it makes presenters correct against a split store before any routes are switched to route reads (PR9) or the split is actually enabled (PR10). Landing the read-side rework on its own keeps the behaviour-changing activation isolated at the top of the stack.
Tests
pnpm run test --filter webappfor the affected presenter suites. New suites exercise both the split-active read-through path and the passthrough path for each reworked presenter.Notes
Draft, stacked on #4119 (
runops/pr07-replication). Review that first; this diff is against it.Server-change / changeset note to be added at stack-assembly time.
🤖 Generated with Claude Code