Repository navigation
Conversation
`cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets -- -D warnings` are both red on this tree with the current stable toolchain (rust 1.99): 37 files are not rustfmt-clean, and 12 lint sites fail on newer rustc/clippy lints (`double_must_use` from `#[async_trait]` expansion, a deprecated `AtomicUsize::fetch_update`, `unnecessary_literal_unwrap`, `bool_assert_comparison`, `manual_str_repeat`, `map_get`, `dead_code`, and an `await_holding_lock` in a test that holds the env lock deliberately). This is a prerequisite for the openabdev#1474 fix that follows: it does not change behaviour, it only makes the tree pass the quality gates AGENTS.md requires (`cargo fmt`, `cargo clippy -- -D warnings`, `cargo test`) before pushing. - whole-tree `cargo fmt --all`; the only non-mechanical formatting change is dropping a trailing-whitespace-only line in acp_mcp.rs that rustfmt refuses to rewrite on its own. - `#[allow(clippy::double_must_use)]` on the four `#[async_trait]` traits: the bare `#[must_use]` the macro stamps on each expanded method is generated and cannot carry a message, so the lint is not actionable there. - `#[allow(clippy::await_holding_lock)]` on the one test that must hold the process env lock across an await to serialise `remove_var`. - `fetch_update` -> `try_update` (same signature and semantics, renamed in the newer toolchain); test-only assertions rewritten to the idiomatic form; an unused `#[cfg(test)]` helper removed.
`main` spawned `run_sweeper` fire-and-forget and only used the handle for a final `abort()`. A panic inside it therefore ended lease expiry and deadline sweeping while the CP kept accepting registrations and delegations: leases of disconnected agents never expired, overdue delegations never resolved, and nothing surfaced it — `/health` still answered `ok`. Same failure class as the session-pool reaper bug in openabdev#1457: a background loop whose silent death converts a maintenance function into unbounded state growth. The sweeper is now supervised, and its liveness is a first-class signal: - `run_sweeper` records a pass after every completed tick. That counter is the sweeper's only self-report, and the only signal available for a task wedged *inside* a pass — a wedged task never returns, so its JoinHandle never resolves. - `supervise_sweeper_task` treats a panic, an unexpected return, and a stall (no pass within 30 s) alike as death. It records the death BEFORE waiting out the restart backoff, restarts the task, and caps the retry rate with a doubling backoff (500 ms -> 30 s) whose budget resets only for a sweeper that both lasted a full stall window and kept sweeping through it — so a crash loop is a bounded retry loop, a sweeper that fails once after hours of service recovers immediately, and a task that completes a single pass then wedges does not get mistaken for a healthy run. - Shutdown interrupts the backoff wait rather than sitting out its ceiling, and aborts *and awaits* the sweeper: a dropped JoinHandle detaches, so the abort would not have been carried out before the supervisor returned. - `/health` answers 503 `sweeper degraded` from a death until the replacement completes a pass, so no sweeper ever comes up while the CP still claims to be healthy. Before this, only logs recorded the failure; now the orchestrator can. Documented as a behavior change: no probe in this repository targets the CP's `/health` (the Helm/Docker probes are for `openab-gateway`). - `main` signals the supervisor on shutdown instead of aborting the handle, so teardown goes through the same supervised path, and warns if the supervisor fails to stop. Tests drive the real supervisor on a paused clock with a compressed policy (`crates/openab-cp/tests/sweeper_supervision.rs`): panic restart, stall restart, the 200/503/200 sequence on the real `/health` route, shutdown while a sweeper runs, shutdown during a ceiling-length backoff, backoff growth and reset, no task spawned after shutdown. Every guard is mutation-checked: removing the stall watchdog, the death record, the 503 response, the backoff, the interruptible wait, the shutdown abort, the shutdown await, or the "kept sweeping" requirement turns the suite red. No new dependency: the dev-only `tokio/test-util` feature (already unified onto tokio in the lockfile) provides the paused clock. Refs openabdev#1474
openab-app
Bot
added
the
closing-soon
Contributor
|
Caution This PR is missing a Discord Discussion URL in the body. All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: |
|
🔒 Auto-closing: this PR has had the If you'd like to continue working on this, please submit a new PR and link to this one if necessary. |
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
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.
Review Contract
Goal
openab-cpspawned its background sweeper fire-and-forget: theJoinHandlewas used only for a final
abort()inmain. A panic insiderun_sweepertherefore ended lease expiry and delegation-deadline sweeping while the CP kept
accepting registrations and delegations — leases of disconnected agents never
expired, overdue delegations never resolved — and nothing surfaced it, because
/healthansweredokunconditionally. Same failure class as thesession-pool reaper bug in #1457: a background loop whose silent death converts
a maintenance function into unbounded state growth.
Refs #1474
Non-goals
exit; recovery is chosen because a crashed sweep is transient far more often
than a poisoned code path, and a fatal exit would turn a recoverable blip into
a pod restart.
backoff) is a constant, not new config keys. There is no operator-facing
reason to retune it per deployment, and a bad value here silently disables
either recovery or the liveness signal.
survive the process.
(
d13a2355), not out of scope creep: the base tree is red under the toolchainthe project's own gates run with, so no candidate can pass
cargo fmt --all -- --checkorcargo clippy --workspace --all-targets -- -D warningswithout it. Itcontains no behaviour change — 41 files, 30 of them byte-identical to
rustfmton the base content, the other 11 carrying exactly the 12 lintfixes enumerated in its message.
Accepted Residual Risks
pass only every >15 s would be aborted mid-pass every 30 s. Not reachable with
the real
run_sweeper(in-memory sweeps on a 1 s tick); the worst case is aslow restart loop, not a wedge.
sweep_leasescanderegister an instance before its shutdown frame is queued. That hazard
already existed for a panicking sweeper (the task was simply gone), so the
change does not add it — it makes it visible via
/health./healthis now capable of answering 503. Existing external deploymentsthat wired the CP's
/healthto a liveness probe will see the podrestarted during a sweeper restart instead of during a genuine process
failure. Documented in
docs/control-plane.mdas a behavior change; no probein this repository targets the CP (the Helm and Docker probes are for
openab-gateway).counter, because the alternative (fatal) was rejected above. The rate is
bounded at one attempt per 30 s, and each attempt is logged at
error.supervise_sweeper_task,SweeperSupervision,record_sweeper_pass/record_sweeper_death) is exposedon the crate so an embedder can supervise a task of its own, and so the
supervisor is testable against a paused clock.
Acceptance Criteria
/healthreports503 sweeper degradedfor the whole window in which sweeping is stopped — no replacementever comes up while the CP still claims to be healthy.
(
a_panicking_sweeper_is_restarted_and_the_replacement_comes_up_degraded,health_is_ok_only_while_the_sweeper_is_alive)one, is restarted the same way.
(
a_sweeper_that_stops_making_progress_is_restarted)loop, and a sweeper that failed once after a long healthy run recovers
promptly.
(
a_crash_looping_sweeper_is_backed_off_not_hot_restarted,a_long_lived_sweeper_resets_the_backoff_budget,a_long_healthy_run_resets_the_delay_seen_by_the_next_death,a_sweeper_that_makes_one_pass_then_wedges_keeps_its_backoff_growing)before the supervisor returns.
(
shutdown_while_the_sweeper_runs_terminates_it_and_waits,shutdown_interrupts_the_backoff_wait,shutdown_stops_the_supervisor_before_it_starts_a_task)record, the 503 response, the backoff, the interruptible wait, the shutdown
abort, the shutdown await, or the "kept sweeping" reset condition turns the
suite red. Verified by hand-mutation of each, three consecutive runs for the
shutdown-await guard.
cargo fmt --all -- --check,cargo test --workspace(1481 tests) andcargo clippy --workspace --all-targets -- -D warningsall exit 0, on thecandidate and in a clean detached replay of it.
docs/control-plane.mdand the control-plane ADR describe the behaviour thecode actually has, including the reset condition and the
/healthchange.Follow-ups
work transactional (so an abort mid-pass cannot deregister without announcing)
and give the supervisor a give-up-and-exit threshold.
Evidence
cargo fmt --all -- --checkcargo test --workspacecargo clippy --workspace --all-targets -- -D warningscargo test --workspace(TDD red, at base + this PR's test paths)Review: two read-only rounds, every finding fixed with a mutation-checked
regression test; no findings remain.