TASK 7: Decompose App.tsx: extract self-contained hooks and pure helpers - #328
Open
Trapa-Eureka wants to merge 7 commits into
Open
Trapa-Eureka wants to merge 7 commits into
Trapa-Eureka wants to merge 7 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>
Implements Priority 3 from OpenResearch_Improvement_Priorities.md.
Consolidated backend-specific branching that had drifted into duplication
across the 8 job backends:
- `JobState { stage: String, message: Option<String> }` was defined
identically in ssh.rs, kubernetes.rs, modal.rs, and slurm.rs, plus a
structurally-identical `JobInfo` in ray.rs — five copies of one shape.
Moved the canonical definition to `jobs::mod`; each backend now re-exports
it (`ray` keeps its existing `JobInfo` name via a renamed re-export), so
`supervise.rs`'s poll loops never need backend-specific field access.
Huggingface's native API response nests this one level deeper
(`JobInfo { status: JobStatus { .. } }`) — added `JobInfo::state()` to
adapt it to the same shape rather than special-casing HF's field access.
- Slurm and Ray each hand-rolled an identical "GONE" debounce (a scheduler-
reported "job vanished from bookkeeping" state must persist ~60s before
being believed, since it also fires during a scheduler restart). Extracted
to a shared `jobs::GoneDebounce`, so the window and reset rule can't drift
between the two schedulers that need it.
- Every one of the 6 backends with a fallible `inspect_job` (hf, k8s, modal,
ssh — shared by both `ssh_job` and `openresearch_job` — slurm, ray)
retried a transport failure (host unreachable, cluster API down, auth
expired) every `POLL_INTERVAL` forever, with no cap. A permanently
decommissioned host or a revoked credential would strand a run in
`Running` indefinitely — TASK 2's crash recovery covers "no supervisor at
all," but not "a live supervisor whose backend is gone for good." Added a
shared `MAX_CONSECUTIVE_INSPECT_FAILURES` bound (~10 minutes at the
default poll interval) and `give_up_on_unreachable_backend`, which marks
the run unrecoverable via TASK 2's `Store::mark_run_unrecoverable` —
inheriting its terminal-state guard, so it can never overwrite a run that
reached a real outcome in the meantime.
- `localbox::cancel_job` returned `Err` when the target process was already
dead — including on a *second* cancel call — which violates the
"cancellation is idempotent" contract test below. Now checks liveness
first and treats an already-gone process as a no-op success.
Added a conformance test suite (`jobs::localbox`'s `local_job_lifecycle`)
covering every property `OpenResearch_Improvement_Priorities.md` lists for
this contract: launch succeeds with a valid configuration, invalid
configuration fails before execution (a run id colliding with an existing
file at its run-dir path), logs remain readable after completion (both a
resumed read and a fresh replay from the start), status reaches one
terminal state only (wires this real backend's terminal output through
`stage_to_run_status` + `Store::update_status`, confirming TASK 1's
transition guard end-to-end rather than only in `store.rs`'s own tests),
and cancellation is idempotent (a second cancel on an already-dead process
succeeds). `local` is the only backend this repo can exercise end-to-end in
CI without external infrastructure (a live k8s cluster, an ssh host, cloud
credentials); the suite is written and labeled so a future backend gaining
real test infrastructure can assert the same properties.
Also added `jobs::GoneDebounce`'s own unit tests (window elapses, resets on
any non-GONE observation, a window shorter than one poll interval still
requires at least one GONE poll).
Scope note: a `ComputeBackend` trait already exists (`compute.rs`) covering
launch (`preflight`/`stage_source`/`submit` — real per-backend dispatch) and
default `status`/`logs`/`cancel`/`cleanup` methods that only touch the local
DB/log file, never the actual remote provider — the real per-backend poll/
cancel/log lifecycle lives entirely in `supervise.rs`'s hand-dispatch on
`descriptor.kind`, outside the trait. Routing that lifecycle through trait
method overrides for all 8 backends would be a much larger, higher-risk
refactor (duplicating `supervise.rs`'s existing loops as trait impls) that
can't be meaningfully tested for 7 of 8 backends without real external
infrastructure — left as a larger, separate future investment rather than
attempted here. Likewise, "backend-specific failures map to stable error
types" has no foundation to build on (the crate is a flat `anyhow::Error`
throughout, with zero typed error taxonomy anywhere) — introducing one is
squarely Priority 9's ("error classification and observability") territory,
not achievable by wiring up existing types here.
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Implements Priority 9 from OpenResearch_Improvement_Priorities.md.
`src/error.rs` was a flat `anyhow::Error` re-export with zero typed error
taxonomy anywhere in the crate — every backend and command produces ad-hoc
`anyhow!("...")` strings. Introducing a full taxonomy across every failure
site in the codebase is a much larger, cross-cutting change; this task
establishes the taxonomy and wires it into the two places `orx` itself
force-fails a run (rather than a backend reporting its own outcome), since
those are exactly where a stable, machine-readable classification is most
valuable to an agent or the dashboard deciding what to do next.
Added `error::ErrorKind`: the doc's nine suggested categories
(ConfigurationError, AuthenticationError, GitStateError, BackendUnavailable,
LaunchFailure, RuntimeFailure, CancellationFailure, ReconciliationFailure,
StorageFailure), each with a stable snake_case string code, `as_str`/`parse`
round-tripping, and `Display`.
Wired in via `Store::mark_run_unrecoverable` (TASK 2), which now takes an
`ErrorKind` alongside its existing free-text `reason` and persists both — a
new `error_kind` column (parallel to `recovery_reason`: excluded from
`upsert_run`'s `ON CONFLICT` so a later backend write can't clobber it,
inherits `update_status`'s terminal-state guard so it's never stamped over a
real outcome). Its two existing callers are now classified precisely:
- `commands::exp::reconcile_active_runs` (TASK 2, crash recovery giving up
on respawning a supervisor) -> `ErrorKind::Reconciliation`.
- `commands::supervise::give_up_on_unreachable_backend` (TASK 4, a backend
unreachable for `MAX_CONSECUTIVE_INSPECT_FAILURES` consecutive polls) ->
`ErrorKind::BackendUnavailable`.
Exposed both fields on the dashboard/API's `ApiRun` — `error_kind` only when
the stored string still round-trips through a known code, so a value from a
future `orx` version never surfaces as a silently-wrong classification to an
older client.
Reviewed against the rest of Priority 9's asks:
- "Correlation IDs for runs" — already satisfied: every supervisor log line
is already prefixed `supervise {run_id}: ...`, and the run id is the
natural correlation key throughout the CLI/API/store. No new ID scheme
needed.
- "Structured logging" and classifying every other failure site across 8
backends and the rest of the CLI — genuinely absent today (everything is
`eprintln!`) and a much larger, cross-cutting change; left as significant
future work rather than attempted piecemeal here.
Tests added/extended: `ErrorKind` round-trips and code uniqueness/format
(`error.rs`); `mark_run_unrecoverable`'s three existing tests now assert
`error_kind` is stamped, cleared-on-no-op, and not overwritten by a second
call with a different kind (`store.rs`); `reconcile_active_runs`'s give-up
test now asserts `reconciliation_failure` (`commands/exp.rs`); a new test
for `give_up_on_unreachable_backend` asserts `backend_unavailable`
(`commands/supervise.rs`).
Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Implements Priority 4 from OpenResearch_Improvement_Priorities.md. Added `store::ProvenanceManifest`, captured once at launch and persisted in a new `provenance_json` column (immutable thereafter — excluded from `upsert_run`'s ON CONFLICT, same pattern as `chat_session_id`). Deliberately scoped to what's cheap and reliable to capture from the launching process itself, without new subprocess probes on `orx exp run`'s latency-sensitive path, and without restating what a run already carries on its own columns or `BackendDescriptor` (git commit SHA, exact command, backend + its configuration, start/completion timestamps): - `orx_version` — the exact `orx` build that launched the run. - `launcher_os` / `launcher_arch` — a lightweight, secret-free "environment fingerprint" for the machine that launched it (not necessarily where the payload executes remotely — that's already `BackendDescriptor.flavor`). - `agent_harness` / `agent_model` — resolved from the launching chat session, when the run was started by a coding-agent harness rather than a plain CLI invocation. - `parent_experiment_id` — a snapshot of the launching experiment's parent id, so the lineage survives even if the experiment itself is later reparented or deleted. `compute::build_provenance` computes this once in `compute::submit` (which already has `store`/`args`/`experiment` in scope) before dispatching to a backend; every backend's own subsequent `upsert_run` (once its real job handle is known) naturally excludes the column, so it's never clobbered. Exposed on the dashboard/API's `ApiRun` as a `provenance` object. Explicitly NOT captured — see the doc's other suggested fields: dependency/runtime versions and hardware details beyond OS/arch would need new per-backend probing this task doesn't attempt (and, for hardware, adds real subprocess latency to every launch — see `jobs::localbox::hardware_info`, already flagged in its own docs as "blocking, call via spawn_blocking"); the codebase has no dataset/artifact registry to reference at all. Bug fix found while adding this: `Store::list_ready_run_wakeups` hand-rolled its SELECT's column list rather than reusing `SELECT_RUN`, and had silently drifted out of sync across TASK 2 and TASK 5 — `recovery_reason` and `error_kind` were being read from the wakeup table's own `chat_session_id`/ `state` columns instead of the run's, wrong data with no error raised. Adding a further column (`provenance_json`) finally pushed the mismatch past the row's actual column count, turning it into a real "Invalid column index" panic that the existing wakeup test caught immediately. Fixed by switching to `r.*` (aliasing the two wakeup-specific columns to avoid the naming collision) so this can't drift out of sync again. Tests added: `ProvenanceManifest` JSON round-trip (including the minimal/no-optional-fields case); a real `Store`-backed test proving `provenance_json` survives a backend's later `upsert_run` the same way `chat_session_id` already does; `build_provenance` resolving an agent harness/model from a real chat session row, and correctly resolving to `None`/`None` for a plain CLI launch with no parent experiment. Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…K 7) (#13) App.tsx (2054 lines) mixed ~14 distinct concerns in one component closure. Extract the pieces that are cleanly separable without touching the right-panel tab state machine: - filePathResolution.ts: parseFilePath/fileBranchLabel/escapeRegExp (pure, previously untested — 90-line path-resolution algorithm now has 18 unit tests) - panelSizing.ts: floating-panel sizing constants and pure width math - listUpsert.ts: generic upsertById helper (tested) - useStableStringMap.ts: generic map-identity hook - useAppData.ts: project/ui-state/session queries + combined startup error + retry - useExperimentScope.ts: experiments pane view/scope filter state - useSpawnTabMeta.ts: live title/running-state for open sub-agent tabs - usePreferredAgent.ts: preferred-agent persistence with rollback App.tsx shrinks by ~300 lines (net) and each extracted concern now has an independent, reviewable home following the codebase's existing flat `use*.ts` convention (no new directory taxonomy introduced). Scope note: the right-panel tab system (open/close/select/promote across 5 tab kinds, ~700 lines) and the top-level JSX composition are deliberately left in App.tsx. It is one tightly-coupled state machine whose callbacks close over shared refs specifically to dodge stale closures, and it is fed directly into useProjectWorkspace's persistence round-trip. Splitting it further without an existing React test harness for App.tsx's rendered behavior risks reintroducing exactly the stale-closure bugs the current refs exist to avoid. Left as a follow-up once that safety net exists. Verified: pnpm typecheck, pnpm lint:i18n, pnpm lint:styles, pnpm test (183 passed, 0 regressions, 21 new), pnpm build (ui/dist regenerated and committed). Claude-Session: https://claude.ai/code/session_01FESDZN8TW7HzGCLnP6Y5vB Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 14, 2026
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 7 of 10, stacked in dependency order (this task builds on the previous ones' code).
What this does
App.tsx (2054 lines) mixed about 14 distinct concerns in one component closure. Extracts the pieces that are cleanly separable without touching the right-panel tab state machine: filePathResolution.ts (previously-untested 90-line path algorithm, now 18 unit tests), panelSizing.ts, listUpsert.ts, useStableStringMap.ts, useAppData.ts, useExperimentScope.ts, useSpawnTabMeta.ts, usePreferredAgent.ts. The tab state machine itself is deliberately left alone -- it's one tightly-coupled unit whose refs specifically guard against stale-closure bugs, and there's no React test harness yet to safely split it further.
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).