feat(runner): move structured workflows onto a leased, durable run rail - #291
Conversation
|
Important Review skippedToo many files! This PR contains 103 files, which is 3 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (103)
You can disable this status message by setting the 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 |
26b2744 to
06cf58f
Compare
Stack 2 of 3, base `feat/foundation-config-migration`. Structured `workflow-run` jobs previously rode the shared-daemon protocol: the payload carried a `workflowRun` ref, a daemon claimed it, and `src/daemon/workflow-executor.ts` ran it in-process. This replaces that rail with a leased attempt model in the database, plus a dispatch outbox so an enqueue and its `workflow_runs` row commit together. - migration `017_workflow_run_leases`: attempt/lease/deadline columns on `workflow_runs`, `offer_id` + result columns on `executions`, the `workflow_attempt_commands` receipt table, `repo_memory.content_sha256`. - `src/workflows/runs-store.ts`: the `mark*` helpers give way to a lease-based API. `dispatch-outbox.ts` and `completion-reconciler.ts` close the enqueue/commit and result/projection gaps. - `src/shared/ws-messages.ts`: `workflowRun` leaves the shared job payload, and the scoped message names take their namespaced form. - `src/daemon/workflow-executor.ts` is deleted along with it. - The webhook event handlers, connection handler, job dispatcher and daemon entrypoint follow the runs-store API change. Migration 017 lands here, not in stack 1: it creates `workflow_attempt_commands`, and DB tests reset their schema from inline DROP lists, so the migration is only safe with the test updates that know about that table. `workflow-run` jobs have no executor between this and stack 3. The two are written to merge together; this one alone leaves structured workflows enqueued but unclaimed. Verified: typecheck, lint, format, all check gates, 191/191 test files against live Postgres 17 + Valkey. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUPpJPtxAaHWrBsjytRGyM
06cf58f to
e4d0f95
Compare
![chrisleekr-bot[bot]](https://avatars.githubusercontent.com/in/2890262?s=60&v=4){kind=small}
Durability bugs: - markWorkflowFailureNotified is now a real compare-and-set. The COALESCE kept the first timestamp but the row still matched the WHERE and still RETURNed, so two reconcilers both saw success and both commented on the same PR. - An unresolvable parent chain writes its receipt instead of skipping. findTopAncestor returns null only for a row that is not there, and findPendingWorkflowFailureNotifications selects on failure_notified_at IS NULL, so the row was re-selected every pass forever with no path to clearing. - saveRepoLearnings regains its per-item guard. The targeted ON CONFLICT covers idx_repo_memory_learning_unique only, so it cannot absorb a violation of idx_repo_memory_env_unique; category is a bare string on the wire, and the caller only logs, so one re-sent env_var learning discarded the whole batch including deletions. - reconcilePendingWorkflowCascades mints an installation Octokit per row. It ran with emitGitHub: false while markAttemptCascadeCompleted wrote the receipt, so a crash between attempt completion and cascade permanently swallowed the outcome comment. Availability: - A protocol-incompatible close now calls onFatal, and the daemon exits non-zero. Reconnect is disabled for the process lifetime on that path, so the daemon was parked live, idle and healthy-looking, with only an info log, even after the orchestrator was upgraded. - Daemon re-registration no longer waits on GitHub. The disconnect cleanup awaited notifyDisconnectedDaemonWorkflows, which walks parent chains and posts per stranded run, so a flapping daemon held itself out of the pool for as long as GitHub was slow. The notification is detached but still drained on shutdown, and its receipt makes it retryable. Observability and contract: - Each failure-notice kind carries its own token-mint `via`; all five reported notifyExpiredWorkflowAttempts, so a dispatch expiry and a daemon disconnect were indistinguishable in the audit trail. - publishWorkflowRunById logs when a row is not eligible, so a stranded run is distinguishable from a benign no-op. - isSelfPush warns when self-login resolution fails, since the module docstring claims an invariant that a null login relaxes. - The auto-review claimDelivery moves ahead of the paginated listFiles, per the idempotency contract in CLAUDE.md. Docs and dormancy: - observability.md and architecture.md carry the second DispatchTarget and fifth DispatchReason; the TOKEN_MINT_VIA list in observability.md named a mint site that no longer exists and missed four that do. - The five sweeps and the two shutdown helpers say in their docstrings that nothing schedules them on this branch and name where the isolated-runner slice wires each one, so the gap reads as staged rather than as an oversight. - hasActiveShipIntent gains the four-branch test the other guards have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUPpJPtxAaHWrBsjytRGyM
087cf3f to
da77eb3
Compare
|
bot workflow 🔍 Code review complete, 103 files, +10924/-2499. SummarySecond-pass review of the leased, durable run rail (103 files, +10924/-2499). The prior round's 15 findings are all addressed in The core design holds up. Every mutating write in 2 findings: 1 major, 1 minor. Nothing blocking. What was checked
Findings
ReasoningI treated this as a second pass, so the first task was separating genuinely new risk from ground already covered. Several things that would read as bugs in isolation are settled decisions here: the dormant sweeps, the deleted workflow executor, and the full-table backfill in migration 017 all carry comments explaining that the paired half lands in #292. Re-raising them would be noise, so I verified the docstrings exist and moved on. I also persisted the dormancy convention as a repo review-policy directive so future passes don't relitigate it. Both findings come from the same technique: comparing a code path against its own siblings. The rail establishes two clear conventions — a fence exception propagates rather than being converted into a result, and a I discarded two candidates after tracing them. The cascade reconciler mints an Octokit before applying the DB cascade and only logs on failure, which initially looked like a permanently stuck parent — but the failed row stays in the selection set and is retried by the next sweep, so it is deferred, not lost. And cost: $4.4408 · turns: 66 · duration: 1121s |
…emember The three handlers converted every error into `status: "failed"`, including `StaleWorkflowAttemptError`. A lost lease is a transient fence loss, not a workflow failure, so the run was reported terminally failed to the user while another attempt held the lease. The other five handlers already re-throw. Also document that `commitAttemptHandOffChild` writes only the `workflow_runs` half of the durable pair: the dispatch outbox joins `executions` on `execution_delivery_id`, so a caller that skips `recordWorkflowExecution` in the same transaction strands the target behind the in-flight index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUPpJPtxAaHWrBsjytRGyM
1 participant
Stack 2 of 3 · base #290 · followed by the isolated runner
What this does
Structured
workflow-runjobs rode the shared-daemon protocol: the job payload carried aworkflowRunref, any daemon in the fleet claimed it, andsrc/daemon/workflow-executor.tsran it in-process. Nothing in that path was durable. A daemon that died mid-run left aworkflow_runsrow stuck inrunningwith no owner and no deadline, and an enqueue that succeeded while its row insert failed produced a job with no run behind it.This replaces that rail with leased attempts in Postgres, plus a dispatch outbox so the enqueue and the row commit together.
flowchart TD Hook["webhook event handler<br/>issue-comment, issues, pull-request"]:::edge Outbox["dispatch-outbox.ts<br/>row + queue job commit together"]:::store Runs["runs-store.ts<br/>attempt id, lease, deadline"]:::store Mig["migration 017<br/>lease columns + workflow_attempt_commands"]:::store Recon["completion-reconciler.ts<br/>result then projection, replay safe"]:::store Expiry["workflow-expiry-notifier.ts<br/>tells the author when an attempt expires"]:::store Gone["src/daemon/workflow-executor.ts<br/>DELETED with the workflowRun payload field"]:::stop Next["stack 3: isolated runner Pod<br/>claims the lease and executes"]:::next Hook --> Outbox --> Runs Mig --> Runs Runs --> Recon --> Expiry Runs -.->|"leased attempt,<br/>no executor yet"| Next Gone -.->|"old rail removed here"| Next classDef edge fill:#ecf0f1,color:#2c3e50 classDef store fill:#2c3e50,color:#ffffff classDef stop fill:#c0392b,color:#ffffff classDef next fill:#8e44ad,color:#ffffffRead this before merging
workflow-runjobs have no executor between this PR and stack 3. This PR deletes the daemon workflow rail; stack 3 adds the runner that replaces it. Jobs enqueue and lease but nothing claims them in between. The two are written to merge together.That is not an accident of the split, it is the shape of the change:
runs-store.tsreplaces the fivemark*functions the daemon executor called. Keeping both rails alive across the boundary would mean shipping a hybrid store that exists in no final state and that no reviewer could check against anything.Why migration 017 is here and not in stack 1
It creates
workflow_attempt_commands. Every DB-backed test resets its schema from an inlineDROP TABLElist, and those lists predate the table, so on any test that resets, the migration replays intorelation "workflow_attempt_commands" already exists. The migration is only safe alongside the test updates that know about it. I found this by running the suite against a real Postgres, not by reading the diff.What is in here
017_workflow_run_leases: attempt/lease/deadline columns onworkflow_runs,offer_id+ result columns onexecutions, theworkflow_attempt_commandsreceipt table,repo_memory.content_sha256runs-store.tsswaps themark*API for leased attempts;dispatch-outbox.tscommits the row and the queue job together;completion-reconciler.tsstores the terminal result before projections so a crash between them replays instead of losing itworkflowRunleaves the shared job payload; scoped messages take their namespaced form (scoped-job:offer,scoped-job:completion);PROTOCOL_VERSION2.0.0connection-handler,job-dispatcher,ws-connectionsplit,workflow-expiry-notifier,installation-token,repo-knowledgecontent-hash de-duplicationsrc/daemon/workflow-executor.tsand its testVerification
typecheck·lint·formatcheck:*gatestest🤖 Generated with Claude Code
https://claude.ai/code/session_01KUPpJPtxAaHWrBsjytRGyM