refactor(observability): shared deadline helpers for external waits

BN
1h ago
+2873-216

What

Adds preloop_observability::deadline, the single shared implementation of every bounded wait on an external dependency, and migrates the first caller (credential_store.rs) onto it. This is the foundation branch for the six fix/bound-* slices: they rebase onto fix/bound-shared and replace their inline copies with these helpers.

The branch is current with main (a0409afdf merged; one formatting-only commit fixes an unformatted assertion main inherited from a [skip ci] docs merge, which cargo fmt --all --check rejects).

API

use preloop_observability::deadline::{
    run_command, run_command_capped, run_command_with_stdin, run_command_status,
    run_command_blocking, run_command_blocking_capped,
    run_blocking, DeadlineGuard, CommandError, BlockingError, GuardError, DeadlineExceeded,
};

// Async process runners (tokio::process::Command, pre-configured by the caller).
async fn run_command(cmd: Command, deadline: Duration, what: &str)
    -> Result<std::process::Output, CommandError>;
async fn run_command_capped(cmd: Command, deadline: Duration, what: &str, max_capture: usize)
    -> Result<(std::process::Output, bool), CommandError>; // bool = truncated
async fn run_command_with_stdin(cmd: Command, input: &[u8], deadline: Duration, what: &str)
    -> Result<std::process::Output, CommandError>;
async fn run_command_status(cmd: Command, deadline: Duration, what: &str)
    -> Result<std::process::ExitStatus, CommandError>;

// Synchronous twins for Drop impls / runtime-free contexts (std::process::Command).
fn run_command_blocking(cmd: std::process::Command, deadline: Duration, what: &str)
    -> Result<std::process::Output, CommandError>;
fn run_command_blocking_capped(cmd: std::process::Command, deadline: Duration, what: &str,
    max_capture: usize) -> Result<(std::process::Output, bool), CommandError>;

// Blocking closure off the async workers.
async fn run_blocking<T: Send + 'static>(deadline: Duration, what: &str,
    work: impl FnOnce() -> T + Send + 'static) -> Result<T, BlockingError>;

// Fail-fast guard for blocking OS services.
impl DeadlineGuard {
    fn new(timeout: Duration, thread_name: impl Into<String>,
           unresponsive_text: impl Into<String>) -> Self;
    fn run<T: Send + 'static, E: Send + 'static>(
        &self, op: impl FnOnce() -> Result<T, E> + Send + 'static)
        -> Result<T, GuardError<E>>;
    fn is_unresponsive(&self) -> bool;
    fn timeout(&self) -> Duration;
}

Behavior guaranteed by every runner:

  • kill_on_drop(true) and stdin from /dev/null; stdout/stderr are drained concurrently with the wait, so a child that fills a 64 KiB pipe buffer is never mistaken for a hang.
  • The deadline covers spawn, output capture, and wait; on expiry the child is killed and reaped within a short grace period.
  • On Unix the child runs in its own process group and expiry SIGKILLs the whole group, so a killed git fetch takes its index-pack, transport, and credential helpers with it instead of leaving them writing into the repository after the caller released its cache lock. The group signal is a raw kill(2) declaration: no libc dependency, Cargo.lock untouched; non-Unix platforms keep the direct-child kill.
  • CommandError names the caller's what, the program, and the deadline (for example "git ls-remote origin timed out after 60s: killed the hung git process"); timed_out(), what(), program(), deadline() accessors.
  • Capture is unbounded by default; the _capped variants keep at most max_capture bytes per stream and discard the excess while still reading to EOF, so a cap bounds memory without reintroducing the pipe deadlock. The returned bool reports truncation.
  • Every expiry also emits one structured warn!: what, program, deadline_ms, and whether the kill reached the
    child's process group, with the same text as the returned error ("git fetch timed out after 30m: killed the hung git process"). A caller that wraps or discards the error still leaves the reason in the engine's log.
  • No environment policy is baked in: callers apply GIT_TERMINAL_PROMPT, GIT_ASKPASS, etc. to the Command they pass.
  • These futures are large: awaiting one through several nested handler futures can overflow a 2 MiB test-thread stack in a debug build, so box the call where it sits several layers deep (Box::pin(run_command(...)).await).

run_blocking cannot cancel its closure — documented at the function. Put the deadline inside the blocking body (run_command_blocking, DeadlineGuard::run) for external calls; this helper only protects the caller.

DeadlineGuard generalises the old OsStoreGuard: dedicated thread, recv_timeout, sticky unresponsive flag with fail-fast after the first expiry, caller-supplied error text, and a panic in the operation does not trip the circuit. credential_store.rs now uses it and OsStoreGuard is deleted; the guard test keeps its semantics (an_unresponsive_os_store_times_out_once_then_fails_fast).

Deadline constants stay with their slices: only the kill/reap grace for these helpers is shared.

Migration notes for the fix/bound-* slices

  • Rebase onto fix/bound-shared and delete the inline copies; slice-specific *_TIMEOUT constants stay in your files.
  • Async git/container callers: build the tokio::process::Command (env, cwd, -c args) and call run_command / run_command_with_stdin; use run_command_capped when the caller has a public capture bound.
  • Synchronous CLI callers (Drop, install): run_command_blocking is the cancellable piece; wrapping a plain Command::output() in run_blocking alone is not cancellable.
  • status-only children whose stdio must be inherited: run_command_status.

Tests

  • deadline (17 unit + 1 integration): sleeping child killed at the deadline (async + sync + stdin + status variants), chatty child draining both pipes (async + sync), capped drains truncating without blocking (async + sync) and leaving small output untouched, expiry killing the child's process group — a child that spawns a background worker records both pids and both are gone afterwards (async + sync); a direct-child-only kill was separately confirmed to leave the worker running, run_blocking returns at its deadline, guard trips once then fails fast, guard stays closed after an operation panic; the integration binary
    (tests/deadline_logging.rs) captures the process-global subscriber and asserts the expiry warning names the operation,
    program, and deadline.
  • credential_store: full module suite (41 tests) green, including the migrated timeout test.

Gates

  • cargo fmt --all --check — clean (merge required the one formatting commit described above)
  • cargo clippy --locked -p preloop-observability --all-targets -- -D warnings — clean
  • cargo clippy --locked -p preloop-runner-server --all-targets -- -D warnings — clean
  • cargo test --locked -p preloop-observability — 47 unit passed, 1 integration passed, 1 doctest passed
  • cargo test --locked -p preloop-runner-server --lib credential_store::tests — 41 passed
  • cargo test --locked -p preloop-runner-server --lib — 1232 passed / 0 failed (pre-merge head; CI shards re-run the whole workspace on the merge head)

No new crates and no new external dependencies; Cargo.lock is untouched.

Summary by CodeRabbit

  • New Features
    • Added deadline-bounded execution for external commands and blocking operations, with timeout handling and capped output capture.
    • Added fail-fast handling for unresponsive blocking dependencies.
    • Added shared TLS certificate roots for runner and OTLP clients, including configured CA bundles.
    • Added configurable OTLP client builders.
  • Refactor
    • Moved configuration and secret operations to bounded blocking execution.
    • Applied explicit request and connection timeouts to runner operations.
  • Documentation
    • Updated the architecture overview with deadline-bounded wait coverage.