Skip to content

fix(cp): supervise sweeper task and expose its liveness on /health - #1553

Open
Reese-max wants to merge 2 commits into
openabdev:mainfrom
Reese-max:devin/issue-1474
Open

Reese-max wants to merge 2 commits into
openabdev:mainfrom
Reese-max:devin/issue-1474

Conversation

@Reese-max

Copy link
Copy Markdown

Refs #1474

Summary

crates/openab-cp's lease/deadline sweeper was spawned fire-and-forget: its JoinHandle was only .abort()ed at shutdown, so a panic inside run_sweeper killed the task while the CP kept accepting registrations and delegations — leases never expired, deadlines never fired, and /health still answered ok. Same failure class as the session-pool reaper bug fixed in #1457.

  • run_sweeper now arms a SweeperExitGuard for the task's whole lifetime: its Drop flips the health signal to Dead on every exit — return, panic unwind, abort — so /health turns over during the task's own teardown, before its JoinHandle is even observed.
  • Each completed sweep pass stamps a heartbeat (Instant), so a sweeper wedged inside a pass — a case a bare JoinHandle watch can never observe — goes stale on /health after SWEEPER_STALL_SLACK (10s, i.e. 10 ticks of silence from a 1s loop).
  • GET /health returns 200 ok only while the sweeper is beating; otherwise 503 with the reason in the body (sweeper not started / sweeper dead / sweeper stalled). A CP serving app(state) without a sweeper reports down from the start rather than lying healthy.
  • main now polls the sweeper's JoinHandle in tokio::select! against the server future: any termination is fatal — the process exits non-zero so the orchestrator restarts a clean CP. (The issue accepts fatal or restart-with-backoff; fatal is chosen because a restarted loop would re-trip the same fault.)

The branch also carries a cherry-pick of 53ab5ff8 (test: guard cargo fmt cleanliness, resolve clippy --all-targets drift, also on devin/issue-1544 / PR #1552): the base tree is not cargo fmt --check-clean under the current toolchain, so a verified-clean tree requires that workspace-wide reformat + hygiene test. Identical mechanical output merges cleanly whichever PR lands first; the issue fix itself is e034393f only.

Review Contract

Goal

Make sweeper death impossible to miss: /health reflects sweeper liveness (never-started, dead, or stalled) and a terminated sweeper fails the process so a supervisor restarts it.

Non-goals

  • No in-process restart/backoff of the sweeper (fatal semantics chosen; restarting a loop that just panicked on shared state is a bigger risk than a clean restart).
  • No additional /health checks (registry depth, router health) — out of scope.
  • The fmt/clippy hygiene commit is environment-required (verifier runs cargo fmt --check workspace-wide); it is not part of the fix semantics and is identical to the commit already proposed in PR docs(adr): OpenAB Mac Agent — cloud brain (k8s) + thin macOS executor over Tailscale #1552.

Accepted Residual Risks

  • A one-shot 503 is possible if /health is hit between server start and the sweeper's first pass (~ms window; interval's first tick is immediate). Mitigation: readiness probes retry; the signal is correct throughout.
  • SWEEPER_STALL_SLACK is a compile-time constant (10s), not a config knob — deliberate, matches the hardcoded 1s tick.
  • Under a hypothetical panic = "abort" build the exit guard doesn't run, but the process aborts anyway — still fatal, still observable.

Acceptance Criteria

  • cargo test -p openab-cp — 143 unit + 12 integration tests pass, incl. new sweeper_health_lifecycle, sweeper_exit_guard_marks_dead_during_unwind, and tests/sweeper_health.rs (never-started → 503, alive → 200, dead → 503).
  • cargo test --workspace — full suite green.
  • cargo fmt --all -- --check — clean.
  • cargo clippy --workspace --all-targets -- -D warnings — clean.
  • TDD red proven: cargo test -p openab-cp --test sweeper_health fails on the base (exit 101; /health answered ok for never-started and dead sweeper).

Follow-ups

  • If a future review prefers recovery over fatal exit, the SweeperHealth signal + supervisor hook point already exist; main is where the policy lives.
  • Configurable stall slack if operators want a different liveness probe budget.

Test plan

  • cargo fmt --all -- --check
  • cargo test --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • Clean detached-replay of the candidate runs the same three commands green (issue-loop verifier)

Generated with Devin

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The sweeper that expires leases and fires delegation deadlines was spawned
fire-and-forget: its JoinHandle was only aborted at shutdown, so a panic
killed the task while the CP kept serving — leases never expired and
deadlines never fired, with /health still answering ok (openabdev#1474).

- run_sweeper arms an exit guard that flips the health signal to dead on
  ANY task teardown (return, panic unwind, abort) and stamps a heartbeat
  after each completed pass, so a wedged mid-sweep stall is as visible as
  a dead task.
- /health now reports 200 ok only while the sweeper is beating within a
  10s slack; otherwise 503 with the reason (not started / dead / stalled).
- main polls the sweeper's JoinHandle against the server future: any
  termination is fatal — the process exits non-zero so the orchestrator
  restarts a clean CP rather than recovering a poisoned loop.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@openab-app openab-app Bot added the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Sep 30, 2026
@openab-app

openab-app Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Caution

This PR is missing a Discord Discussion URL in the body.
This PR will be automatically closed in 24 hours if the link is not added.

All PRs must reference a prior Discord discussion to ensure community alignment before implementation.

Please edit the PR description to include a link like:

Discord Discussion URL: https://discord.com/channels/...

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant