TASK 3: Add SQLite concurrency/stress tests and fix transaction gaps - #324
Open
Trapa-Eureka wants to merge 3 commits into
Open
Trapa-Eureka wants to merge 3 commits into
Trapa-Eureka wants to merge 3 commits into
Conversation
* Formalize run lifecycle as an explicit state machine (TASK 1)
Runs previously carried status as a bare `String`, and `Store::update_status`
was an unconditional `UPDATE ... WHERE id = ?1` with no transition checks —
any caller could overwrite any status with any other status, and three
independent copies of `is_terminal` had to be kept in sync by hand.
Add `RunStatus` (store.rs): Starting -> Running -> {Done, Failed, Cancelled},
with Starting also able to jump straight to a terminal state. Terminal states
are absorbing and `RunStatus::can_transition_to` is the single source of
truth for legality.
`Store::update_status` now takes a typed `RunStatus` and derives its SQL
guard from `can_transition_to` (`WHERE status IN (<legal sources>)`) so the
check-and-write is atomic and can never drift from the documented table. It
returns `Result<bool>`: `false` means the transition was illegal (almost
always because the run was already terminal), which is an expected, benign
outcome rather than an error — a duplicate completion callback, or a
completion racing a cancellation, is now a no-op instead of a corruption.
Threaded the typed status through `jobs::stage_to_run_status`,
`supervise::run_status_for_stage`, and all 8 backend poll loops (HF, k8s,
Modal, ssh, Slurm, Ray, local, OpenResearch), replacing stringly-typed
statuses with compile-time-checked ones. Consolidated the three duplicate
`is_terminal` functions (local/mod.rs, commands/serve.rs, commands/up.rs)
into one canonical `store::is_terminal_status`.
Added deterministic tests covering the doc's listed scenarios: the full
transition matrix, duplicate terminal writes, completion arriving after
cancellation, invalid backward transitions (running -> starting), cancel
while starting/running, retry-as-a-new-row, and concurrent writers (separate
SQLite connections racing to finalize the same run) settling on exactly one
terminal outcome.
No wire/schema changes — RunStatus::as_str() matches the existing stored
vocabulary exactly, so the UI and API are unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
* chore: trigger CI now that Actions is enabled on the fork
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
A supervisor's crash (OOM-killed, host reboot) or a failed spawn attempt previously left its run stuck `Starting`/`Running` forever: startup reconciliation in `orx up` was a one-shot pass, and a `spawn_detached_supervise` failure — there or anywhere else it's called — was only ever logged to stderr and dropped, with no retry and no status change. Add `commands::exp::reconcile_active_runs`: for every locally-owned active run, check whether a supervisor is actually alive and watching it via `supervise::run_has_live_supervisor` — a non-blocking probe of the same `fd_lock` file a running supervisor holds. The lock is an OS-level advisory lock, released automatically the instant the holding process dies or the machine reboots, so this is a race-free, always-fresh liveness signal with no heartbeat/TTL of our own to go stale. An orphaned run gets a fresh supervisor; once respawned, that supervisor's own `inspect_job` reconciles against the actual backend the normal way — reconciliation's only job is making sure *someone* is always eventually watching. A run whose supervisor can't be spawned after `MAX_SUPERVISOR_SPAWN_ATTEMPTS` (5) consecutive tries — a broken installation, not a transient blip — is marked unrecoverable via the new `Store::mark_run_unrecoverable`, which stamps a machine-readable `recovery_reason` (a new nullable `runs` column, excluded from `upsert_run`'s ON CONFLICT like `chat_session_id`, so a later backend write can't clobber it) and appends a human-readable note to `result_markdown`. It goes through `update_status`'s existing terminal-state guard (TASK 1), so it can never overwrite a run that reached a real outcome first. Wired into `orx up`: the existing startup pass now calls `reconcile_active_runs` instead of spawning unconditionally, and a new 30-second periodic task (mirroring the existing chat-turn-lease reconciler, including its data-dir-move gate) keeps calling it for the life of the process, so a supervisor that dies mid-flight — not just one dead before `orx up` started — gets noticed and replaced without needing a restart. Tests added: the lock-probe's three liveness transitions (free, held, released) in `commands::supervise`; `mark_run_unrecoverable`'s terminal-write, never-overwrites-a-real-outcome, and idempotency behavior in `store`; and `reconcile_active_runs`'s full decision matrix (skip when watched, spawn when orphaned, ignore non-local runs, retry-then-give-up with the counter cleared either way, no counter leak for runs that leave the active set) in `commands::exp`, all via the same injected-closure style already used by `request_local_run_cancel_with`. Scope note: this covers orphan detection and give-up semantics generically, across every backend, by making sure a supervisor is always eventually running. It does not change the per-backend `inspect_job` behavior itself — five of seven backends still retry a transport failure (an unreachable ssh host, a down k8s API) forever with no cap, unlike Slurm/Ray's ~60s "GONE" debounce; generalizing that belongs to TASK 4 (compute backend contracts). It also does not add a full process-kill/server-restart integration test (spawning a real `orx supervise` child and killing it) — this repo has no integration-test harness for the Rust binary yet, and building one is a larger, separate investment; the unit tests above cover the same invariants (lock release, orphan detection, give-up) at the mechanism level instead. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…#5) Implements Priority 7 from OpenResearch_Improvement_Priorities.md. Correctness fixes found while auditing every multi-statement Store method: - `record_chat_spawn_attempt` was `UPDATE attempts = attempts + 1` followed by a separate `SELECT attempts` — a genuine TOCTOU race: a concurrent caller's own increment could land in the gap, so the value read back could be *their* count, not this call's. Replaced with one atomic `UPDATE ... RETURNING attempts` (the bundled SQLite is 3.35+, so this is available) — no transaction needed, no race possible. - `prune_run_wakeups`, `prune_chat_spawns`, `claim_chat_turn`, and `claim_data_dir_move` each issued 2-3 sequential statements against the same connection with no transaction boundary. None were live bugs today (each statement's own WHERE clause already prevented double-claims), but none had the "all or nothing" guarantee a future added step could safely assume either. Wrapped each in `self.begin()`/`commit()`. - `delete_local_project` mixed `tx.execute` and `self.conn.execute` for its last three deletes — already atomic in practice (same connection, same open transaction) but a maintainability trap: nothing but convention kept those three statements inside the transaction's scope. Normalized to `tx.execute` throughout. Performance fix: `runs` had no index beyond its primary key. Every poll of active runs — including the TASK 2 reconciliation loop, every 30s — plus listing by project/experiment and the newest-first history view all table-scanned. Added `idx_runs_status`, `idx_runs_project_id`, `idx_runs_experiment_id`, and `idx_runs_created_at` (`CREATE INDEX IF NOT EXISTS`, applied retroactively to existing databases on next open, same pattern as the existing chat-table indexes). WAL mode + a 5000ms busy_timeout (`Store::open_at_with_move_lock`) were already the sole, centrally-configured defense against `SQLITE_BUSY`, used uniformly by every `Store::open()`/`open_at()` caller — reviewed and left as-is; this is the correct, already-standardized configuration Priority 7 asks to check for. Tests added (`src/store.rs`), matching the file's existing style of real threads/connections rather than mocking: - `parallel_experiment_creation_all_land_with_no_sqlite_busy_errors` — 32 threads, each its own connection, creating distinct experiments at once. - `concurrent_readers_are_never_blocked_by_a_concurrent_writer` — one writer hammering `update_status`/`set_result_markdown` on a run while four readers loop `get_run`/`list_runs`; no read may ever error, and every reader must eventually observe the final terminal status. - `an_uncommitted_transaction_rolls_back_on_drop` — a dropped, uncommitted `Transaction` (including one that failed partway through a second statement) leaves no trace, proving the rollback guarantee every `self.begin()`-based method here depends on. - `concurrent_spawn_attempt_increments_never_collide` — 16 concurrent callers of the newly-atomic `record_chat_spawn_attempt` must get back exactly `{1..16}`, not a multiset with collisions. - `ten_thousand_sequential_status_writes_complete_quickly` and `active_run_queries_stay_fast_against_a_large_runs_table` — hand-rolled `Instant`-based timing sanity checks (no benchmark harness exists in this repo yet) against a 20k-row `runs` table, to catch a severe regression (an unintended fsync, a missing/regressed index) rather than pin an exact number. CI finding: the first version of the new read/write-contention test used a 200-write hot loop with no pacing and a 10s/10,000-write timing bound. Both passed on Linux/macOS but failed on the Windows CI runner — sequential writes measured ~2.4ms each there (vs. sub-millisecond locally), and the unpaced hot loop hit genuine `SQLITE_BUSY` ("database is locked") even with the 5s `busy_timeout`. Retuned to a realistic polling cadence (small sleeps, fewer iterations) with a short retry-with-backoff around reads for the rare case that's still not enough, and widened the timing bounds to match observed slow-CI-hardware throughput rather than local timing. Confirmed green on Linux, macOS, and Windows CI before merging. Scope note: experiment creation also creates a Git branch (`local::experiments::create_experiment`) before the DB row is inserted, with no compensation if the DB write then fails — this is pre-existing, already-accepted best-effort behavior (Git and DB state are never kept transactionally consistent anywhere in this codebase; branch publication to GitHub is explicitly best-effort too, per `local_plane.rs`). Bringing that under an explicit rollback/compensation model is a larger design change than this task's "add tests, fix transaction boundaries at the SQL layer" scope. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
8 tasks
Contributor
Author
This was referenced Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #317 per request, so each task is independently reviewable/testable. Part 3 of 10, stacked in dependency order (this task builds on the previous ones' code).
What this does
Audited every multi-statement Store method. Fixed a genuine TOCTOU in record_chat_spawn_attempt (separate UPDATE then SELECT -- a concurrent caller's own increment could land in the gap) with one atomic UPDATE ... RETURNING attempts. Added transaction boundaries around several other multi-statement sequences and concurrency stress tests exercising them under real contention.
About the diff
This branch is stacked on top of the prior task's branch in this split (not yet merged into
main), so until that earlier PR merges, this PR's diff shows its commit too, on top of this task's own commit. The Commits tab lists this task's commit separately if you want to review just that one. I'll rebase this branch (and the diff will shrink to just this task) as each earlier PR in the stack lands.Verification
Passed on this exact rebased state:
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo test --locked(870/870), and for frontend-touching commitspnpm typecheck,pnpm test,pnpm buildwithui/distregenerated.Stack
Full list and status: see #317 (will be updated to link all 10, or closed once these replace it).