Repository navigation
perf(orchestration): bound mutation ledger and run pages - #11432
AmethystLiang merged 5 commits into
Conversation
Co-authored-by: Orca <help@stably.ai>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOrchestration run listing now supports validated 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35e1d253-e22f-4ea0-8a16-480d42c22eaf
📒 Files selected for processing (10)
src/cli/handlers/orchestration-run-cli.test.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/orchestration-db-retention-pagination.test.tssrc/main/runtime/orchestration/orchestration-legacy-storage-db.test.tssrc/main/runtime/orchestration/orchestration-reset-db.test.tssrc/main/runtime/rpc/errors.tssrc/main/runtime/rpc/methods/orchestration-runs.tssrc/main/runtime/rpc/methods/orchestration.test.ts
Greptile SummaryBounds the durable mutation receipt ledger to 10,000 rows (age-evicting completed rows ≥ 30 days old, then oldest-completed eviction, then fail-closed when only unresolved rows remain) and adds stable keyset-cursor pagination to
Confidence Score: 5/5Safe to merge. All three mutation-receipt write paths are guarded within transactions, and the fail-closed behavior is tested end-to-end including the previously missing remote-attachment path. The capacity gate is correctly positioned inside existing BEGIN IMMEDIATE / catch-ROLLBACK blocks in every write path, so a full pending ledger fails atomically without partial state. The keyset pagination tie-breaking (created_at DESC, id DESC) is consistent between the WHERE clause and the ORDER BY. Backward compatibility for old clients is explicit and regression-tested in both directions. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/main/runtime/orchestration/db.ts | Core change: adds ensureMutationReceiptCapacity() called within existing BEGIN IMMEDIATE transactions in all three write paths, and keyset-paginated listRuns() with backward-compat empty-params path. Logic is correct and transaction-safe. |
| src/main/runtime/rpc/errors.ts | Adds mutation_ledger_full to STRUCTURED_RUNTIME_PASSTHROUGH_CODES; cursor_invalid was already present at line 74 pre-existing. |
| src/main/runtime/rpc/methods/orchestration-runs.ts | Expands RunListParams with optional bounded limit/cursor fields; handler passes params directly to listRuns(), which handles the legacy empty-object path internally. |
| src/main/runtime/orchestration/orchestration-db-retention-pagination.test.ts | New comprehensive test file covering age pruning, count-capped eviction, fail-closed pending-full ledger, remote-attachment capacity, keyset pagination with same-timestamp tie-breaking, and v22 migration idempotency. |
| tests/e2e/orchestration-run-pages.ts | Page-draining utility; correctly sends explicit limit: 100 and treats absent nextCursor from an older server as end-of-list, preventing infinite loops on version-skewed connections. |
| src/main/runtime/orchestration/orchestration-run-list-compatibility.test.ts | Tests malformed cursor rejection and the old-client to new-server case (empty-params RPC returns all rows including the legacy run). |
| tests/e2e/orchestration-run-pagination.unit.test.ts | Tests new-client to old-server compat: verifies listAllOrchestrationRuns sends { limit: 100 }, terminates after one response when nextCursor is absent. |
| src/cli/handlers/orchestration.ts | Updated run-list handler to forward limit/cursor flags and default to ORCHESTRATION_RUN_PAGE_LIMIT; output appends cursor hint when more pages exist. |
Sequence Diagram
sequenceDiagram
participant C as Client
participant RPC as orchestration.runList
participant DB as OrchestrationDb.listRuns()
C->>RPC: "{} old client no pagination"
RPC->>DB: "listRuns({})"
Note over DB: limit===undefined and cursor===undefined
DB-->>RPC: runs all, nextCursor null
RPC-->>C: runs all, nextCursor null
C->>RPC: limit 100 new client page 1
RPC->>DB: listRuns limit 100
Note over DB: LIMIT 101, hasMore=true, encode cursor from row 99
DB-->>RPC: runs r1-r100, nextCursor base64url
RPC-->>C: runs r1-r100, nextCursor base64url
C->>RPC: limit 100 cursor base64url
RPC->>DB: listRuns limit 100 cursor base64url
Note over DB: WHERE created_at less than or equal cursor
DB-->>RPC: runs r101 plus, nextCursor null
RPC-->>C: runs r101 plus, nextCursor null
Reviews (4): Last reviewed commit: "fix(orchestration): reject malformed run..." | Re-trigger Greptile
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)
src/main/runtime/orchestration/db.ts (1)
2307-2330: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMissing supporting index for
listRunskeyset pagination.The
runstable (Lines 291-301) has no index oncreated_at/idbeyond theidprimary key. The newlistRunsquery orders bycreated_at DESC, id DESCand filters via(created_at, id)keyset comparison — without a matching composite index, SQLite must do a full table scan + temp b-tree sort for every page request (including the very first page), which defeats the scalability goal of adding cursor pagination as therunstable grows (it has no retention/eviction cap unlikemutation_receipts).🚀 Proposed fix: add a covering index for the pagination sort/filter
CREATE TABLE IF NOT EXISTS runs ( id TEXT PRIMARY KEY, objective TEXT NOT NULL, home_database TEXT NOT NULL DEFAULT 'this_database', coordinator_handle TEXT, coordinator_pane_key TEXT, consumer_generation INTEGER NOT NULL DEFAULT 0, legacy INTEGER NOT NULL DEFAULT 0, created_at TEXT NOT NULL DEFAULT (datetime('now')), updated_at TEXT NOT NULL DEFAULT (datetime('now')) ); + + CREATE INDEX IF NOT EXISTS idx_runs_created_id ON runs(created_at, id);And in the
current < 22migration block:if (current < 22) { this.db.exec(` CREATE INDEX IF NOT EXISTS idx_dispatch_assignee_handle ON dispatch_contexts(assignee_handle); + CREATE INDEX IF NOT EXISTS idx_runs_created_id + ON runs(created_at, id); `) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6091c065-35ef-4c12-be2e-418929a370dc
📒 Files selected for processing (3)
src/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/orchestration-db-retention-pagination.test.tstests/e2e/orchestration-legacy-worker-restart-recovery.spec.ts
Co-authored-by: Orca <help@stably.ai>
…ion-ledger-and-index-dispatch-lookup
) * perf(orchestration): bound mutation ledger and run pages Co-authored-by: Orca <help@stably.ai> * fix(orchestration): close retention pagination gaps * fix(orchestration): preserve unpaginated run listing Co-authored-by: Orca <help@stably.ai> * fix(orchestration): reject malformed run cursors --------- Co-authored-by: Orca <help@stably.ai> Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Description
Bounds durable orchestration mutation receipts without weakening replay protection: completed receipts expire after 30 days or are evicted oldest-first above 10,000 rows, while unresolved receipts are never evicted and new mutations fail closed if they alone fill the ledger. Adds stable cursor pagination to
listRuns()and the run-list RPC/CLI with a 100-row maximum page.Bumps the orchestration schema from v21 to v22 and adds an idempotent
dispatch_contexts(assignee_handle)index. The migration runs transactionally on populated databases and preserves existing dispatch rows.ELI5
Orca keeps a receipt for each important orchestration action so retrying the same action cannot accidentally do it twice. Finished receipts now age out or make room for newer ones, but unfinished receipts are always kept; if there are too many unfinished actions, Orca stops accepting new ones rather than forgetting safety information. Run history is delivered a page at a time, and the database now has a lookup shortcut for finding work assigned to a terminal.
Review remediation
What changed
createRemoteDispatchAttachment()transaction before its receipt insert. Completed rows are pruned to make room; a ledger containing 10,000 pending rows now rejects the attachment without creating either record.resetAll()receipt-preservation behavior.orchestration.runListcursor page before locating the recovered Run. A repository-wide search found no otherorchestration.runListcaller that searched only the default page.listRuns()instead of failing increateRun().ELI5
Remote worker starts write safety receipts, and that receipt drawer now follows the same size limit as every other write path: old finished receipts can make room, but unfinished ones are never discarded. Run history can arrive in several boxes, so recovery now opens every box before deciding the Run is missing.
Proof of fix
The remote-attachment red phase reconstructed the PR tip before this remediation, copied in the new test, and failed for both missing capacity behaviors:
Result: exit 1; 1 file failed, 2 tests failed, 6 skipped. The completed-row case retained 10,001 rows instead of 10,000, and the all-pending case did not throw
mutation_ledger_full.The pagination red phase reconstructed production before pagination, copied in the new test, and reached the
listRuns()result assertion:Result: exit 1; 1 file failed, 1 test failed, 7 skipped. The intended failure was
TypeError: Cannot read properties of undefined (reading 'length')atpage.runs.length, provinglistRuns()was called and returned the pre-pagination array shape.Green at pushed SHA
6c8c9a7335364b2c40dbc1c341cf04903fbbcec9:pnpm exec vitest run --config config/vitest.config.ts \ src/main/runtime/orchestration/orchestration-db-retention-pagination.test.ts \ src/main/runtime/orchestration/orchestration-reset-db.test.ts \ src/main/runtime/orchestration/orchestration-worker-dispatch-db.test.ts \ src/main/runtime/rpc/methods/orchestration.test.ts \ src/main/runtime/rpc/methods/orchestration-federation-control-mail.test.ts \ src/main/runtime/rpc/methods/orchestration-federated-message-targeting.test.ts \ src/main/runtime/rpc/methods/orchestration-federation-setup.test.ts \ src/cli/handlers/orchestration-run-cli.test.ts \ src/shared/orchestration-rpc-contract.test.tsResult: 9 files passed, 232 tests passed.
Proof of no regression
Result: passed all three TypeScript projects.
pnpm exec oxlint \ src/main/runtime/orchestration/db.ts \ src/main/runtime/orchestration/orchestration-db-retention-pagination.test.ts \ tests/e2e/orchestration-legacy-worker-restart-recovery.spec.tsResult: passed with no findings. The green suite above includes adjacent receipt/reset, worker attachment, federation RPC, Run RPC/CLI, and shared-contract coverage. The v22 idempotent assignee-index migration, receipt replay semantics, and deliberate
resetAll()receipt preservation remain unchanged. No screenshots apply because this remediation has no UI changes.Backward-compatible Run listing
What changed
orchestration.runList/OrchestrationDb.listRuns({})now returns the complete legacy result when bothlimitandcursorare absent. Requests that provide either pagination field remain bounded to the shared 100-Run maximum.limit: 100explicitly. The drainer treats a missingnextCursorfrom an older server as completion, so version-skewed SSH/relay calls cannot loop.dispatch_contexts(assignee_handle)index were not changed.ELI5
Older clients ask for Run history without saying how many they want, so the server now gives those clients the whole list just as it did before. New clients explicitly ask for boxes of 100 Runs, and if an older server does not provide a “next box” marker, they stop instead of waiting forever.
Proof of fix
Both regressions were run against PR head
6c8c9a7335364b2c40dbc1c341cf04903fbbcec9before the production fix.pnpm --dir /Users/nwparker/orca/workspaces/orca/prod-scan-07-29 exec vitest run --config /Users/nwparker/orca/workspaces/orca/prod-scan-07-29/config/vitest.config.ts /Users/nwparker/orca/workspaces/orca/prod-scan-07-29/src/main/runtime/orchestration/orchestration-run-list-compatibility.test.tsOld client → new server red result: exit 1; 1 file failed, 1 test failed. An RPC request with
{}saw 100 Runs instead of all 102 (expected length 102, received 100).pnpm --dir /Users/nwparker/orca/workspaces/orca/prod-scan-07-29 exec vitest run --config /Users/nwparker/orca/workspaces/orca/prod-scan-07-29/config/vitest.config.ts /Users/nwparker/orca/workspaces/orca/prod-scan-07-29/tests/e2e/orchestration-run-pagination.unit.test.tsNew client → old server red result: exit 1; 1 file failed, 1 test failed. The old-server response omitted
nextCursor; the caller terminated after one request, but the assertion proved it had sent{}instead of explicitly opting into{ limit: 100 }.At pushed SHA
25b83c8c05e521ff0f1d53354db1d625668e5f49:pnpm --dir /Users/nwparker/orca/workspaces/orca/prod-scan-07-29 exec vitest run --config /Users/nwparker/orca/workspaces/orca/prod-scan-07-29/config/vitest.config.ts src/main/runtime/orchestration/orchestration-db-retention-pagination.test.ts src/main/runtime/orchestration/orchestration-run-list-compatibility.test.ts src/main/runtime/orchestration/orchestration-reset-db.test.ts src/main/runtime/orchestration/orchestration-worker-dispatch-db.test.ts src/main/runtime/rpc/methods/orchestration.test.ts src/main/runtime/rpc/methods/orchestration-federation-control-mail.test.ts src/main/runtime/rpc/methods/orchestration-federated-message-targeting.test.ts src/main/runtime/rpc/methods/orchestration-federation-setup.test.ts src/cli/handlers/orchestration-run-cli.test.ts src/shared/orchestration-rpc-contract.test.ts tests/e2e/orchestration-run-pagination.unit.test.tsResult: 11 files passed, 235 tests passed, including both skew directions.
Proof of no regression
Result: passed all three TypeScript projects.
pnpm --dir /Users/nwparker/orca/workspaces/orca/prod-scan-07-29 exec oxlint src/cli/handlers/orchestration-run-cli.test.ts src/cli/handlers/orchestration.ts src/main/runtime/orchestration/db.ts src/main/runtime/orchestration/orchestration-run-list-compatibility.test.ts src/main/runtime/rpc/methods/orchestration-runs.ts src/shared/orchestration-run-pagination.ts tests/e2e/orchestration-legacy-worker-restart-recovery.spec.ts tests/e2e/orchestration-run-pages.ts tests/e2e/orchestration-run-pagination.unit.test.tsResult: 9 changed files passed with no findings.
Result: passed; 354 grandfathered suppressions, no new bypasses. The focused orchestration suite still covers receipt retention/replay and the v22 dispatch-assignee index migration; neither implementation was modified by this compatibility fix.
Scan provenance
P2 — “Unbounded mutation_receipts + unpaginated listRuns(), and dispatch_contexts full-table scan.” Introduced by #9925 (
cd05f2ff93); it re-entered this release through the non-linear span because revertbf27227437exists only on the v1.4.161 side. Span-side touches: #11107 and #11150.Made with Orca 🐋