Skip to content

Make run admission durable - #727

Closed
swerner wants to merge 5 commits into
mainfrom
codex/durable-admission
Closed

Make run admission durable#727
swerner wants to merge 5 commits into
mainfrom
codex/durable-admission

Conversation

@swerner

@swerner swerner commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • atomically claim runnable runs by persisting the transition to Starting before provisioning or worker launch
  • reconcile claim losers from durable state so concurrent schedulers cannot launch the same run
  • add bounded, process-local backoff for transient storage failures while giving up on permanent failures
  • use one cancellation token across admission and execution, including the subprocess launch race
  • let workflow execution consume an existing Starting transition without emitting a duplicate event
  • preserve structured launch failure causes and keep in-memory status aligned with durable state

Observable behavior

The run.starting event is now emitted at scheduler admission, so it can appear earlier than before. The event schema and API remain unchanged.

Testing

  • cargo +nightly-2026-04-14 fmt --check --all
  • cargo +nightly-2026-04-14 clippy -p fabro-server -p fabro-workflow --all-targets -- -D warnings
  • cargo build --workspace
  • cargo nextest run -p fabro-server --no-fail-fast (813 passed)
  • clean-environment cargo nextest run -p fabro-workflow --no-fail-fast (1,341 passed; 31 skipped)
  • focused admission, cancellation, and launch-failure tests (12 passed)

swerner and others added 2 commits August 4, 2026 14:24
- Reuse fabro_redact::redacted_url_for_log in safe_error_chain instead of
  re-rolling the DisplaySafeUrl parse/redact fallback
- Reuse fabro_util::backoff::BackoffPolicy for admission retry delays
- Reuse projection_failure_message in worker-exit reconciliation
- Flatten AdmissionErrorClass into a { transient, kind } struct so the
  class is matched once instead of twice
- Return the resulting RunStatus from persist_cancelled_run_status_to_store,
  removing a redundant store read and the Result/Option reconciliation in
  finish_cancelled_run_before_execution
- Capture spec, pending_control, and execution_mode at the admission claim
  instead of re-reading the run projection up to two more times per
  admission; drop the unreachable "managed run disappeared" prologues
- Dedup test fixtures (daemon record helper, RunStarting event counting)
- Cross-reference the server claim from execute_persisted_run's twin
  bootstrap transition

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@swerner

swerner commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Ran a four-angle cleanup review (reuse, simplification, efficiency, altitude) over this PR and applied the fixes in 95d3216 and ea9ecde (net −50 lines). No behavior changes intended; all fabro-server, fabro-store, and fabro-workflow tests pass, clippy and rustfmt clean.

Applied

Reuse

  • safe_error_chain re-rolled the exact body of fabro_redact::redacted_url_for_log — the helper whose doc comment exists to prevent that. Now calls the helper.
  • The hand-rolled exponential backoff (checked_shl + cap) in AdmissionRetryState now uses fabro_util::backoff::BackoffPolicy via a const ADMISSION_RETRY_BACKOFF; same 1/2/4/8s schedule, existing backoff test unchanged.
  • Worker-exit reconciliation had an inline copy of the failure-message derivation this PR factored into projection_failure_message; it now calls the helper.
  • Tests: write_worker_test_server_record duplicated an inline daemon-record block (same magic port) in worker_command_test_state_inner — now shared. Two copy-pasted RunStarting-count blocks in start.rs tests became a starting_event_count helper.

Simplification

  • AdmissionErrorClass was an enum whose two variants carried identical payloads, forcing a second match just to peel the label back out. Now a struct { transient, kind } — one branch site.
  • persist_cancelled_run_status_to_store now returns the resulting RunStatus, so finish_cancelled_run_before_execution no longer re-reads the store after every persist; the Result × Option reconciliation (including the persist_result.is_ok() fallback branch) collapses. The fallback read survives only in the Err arm, where it suppresses log noise on concurrent terminal transitions.
  • Both executor prologues had an identical, effectively unreachable let-else (execution_mode_for_run → "Managed run disappeared") right after launch_is_allowed already covered that case. execution_mode is now captured in AdmittedRun at admission; execution_mode_for_run and both blocks are deleted.

Efficiency — the biggest win: per admission, the run projection was materialized up to three times (each a SlateDB range scan plus a deep clone of the whole projection, spec included) — once in the append_event_if predicate, once right after just to read pending_control, and once more in the subprocess path just for the spec. The claim predicate now captures spec and pending_control alongside the already-captured observed_status, both re-reads are gone, and AdmittedRun carries the spec. A mid-prologue cancel_token.is_cancelled() check that only guarded the removed await went with it; the later pre-launch gates are unchanged.

Altitude

  • The server's admission-error classifier exhaustively matched all 17 fabro_store::Error variants to decide transient-vs-permanent plus a snake_case label — store-internals knowledge that would drift on every variant change. That now lives next to the enum as fabro_store::Error::is_transient() plus a strum IntoStaticStr derive for the kind labels, with the structural test moved to fabro-store. The server keeps only its own policy (the non-store-error fallback and the retry schedule), and the slatedb dev-dependency this PR added solely to construct a store-internal error in a server test is gone. Two log-only kind labels change with the derive: serializationserde, internalother (both introduced on this branch). (ea9ecde)
  • Added a comment in execute_persisted_run cross-referencing the server's admit_run claim, since the two spell the same Runnable→Starting policy and must stay in sync.

Flagged but not applied

  • Permanent admission failures are memory-only: when classify_admission_error says permanent, the run stays durably Runnable forever with only an in-memory GivenUp flag and a log line — no managed_run.error, and the give-up evaporates on restart, after which admission silently resumes retrying. Routing permanent classifications through fail_run_before_execution (reserving in-memory give-up for stores that can't accept appends, e.g. read-only) would make the outcome durable and user-visible. Left out because it changes intended behavior — worth deciding before merge.
  • Pre-launch cancellation as a polling checklist: the interleaved is_cancelled()/launch_is_allowed checks in the two launch paths could be one tokio::select! racing prep against the token, as the in-process execution phase already does. Legitimate but a moderate control-flow refactor, so left as-is.
  • Unifying the three "adopt durable status into ManagedRun" sites into one helper: they intentionally differ in what they adopt (status vs. error), so a shared helper would need flags and lose the value.

🤖 Generated with Claude Code

swerner and others added 3 commits August 4, 2026 15:02
The server's admission-error classifier hand-mapped all 17
fabro_store::Error variants to transient/permanent plus a snake_case
label — store-internals knowledge that would drift on every variant
change. Replace it with Error::is_transient() next to the enum and a
strum IntoStaticStr derive for the kind labels, and move the structural
test to fabro-store where the wrapped error types are natural deps.

The server keeps only its own policy: the non-store-error fallback and
the retry schedule. This also drops the slatedb dev-dependency that
existed solely so the server test could construct a store-internal
error. Two log-only kind labels introduced on this branch change with
the derive: "serialization" is now "serde" and "internal" is now
"other".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@swerner

swerner commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by a smaller lifecycle correction. The underlying defect is that a worker launch can fail while the durable run is still Runnable, but that state currently permits failure only for cancellation. This implementation expanded into durable scheduler ownership and claim retry behavior that is unnecessary for the current single-server model. We will replace it with a focused change that makes the pre-launch failure transition valid while retaining the existing process-local double-start guard.

@swerner swerner closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant