harden: supervise the task tree and add worker liveness - #639
Closed
RembrandtK wants to merge 8 commits into
Closed
Conversation
…cking The worker loop called panic! when the LISTEN/NOTIFY listener returned an error. The panic killed the worker task; main's JoinSet join loop only logged the dead task while every other service kept running, so the process looked healthy but processed zero jobs — a silent total stall. queue.pop() is independent of LISTEN/NOTIFY (the notification only wakes the loop earlier than the poll interval), so a listener fault is recoverable by construction: drop the listener, continue in poll-only mode, and re-subscribe on a later poll. Job processing never stops and the worker is never left in an uncertain state. Extract the wait-for-trigger logic into await_next_tick over a JobNotifications trait so the degrade-to-polling path is unit tested without a live Postgres.
…d task death The main join loop merely logged a completed/failed task and kept blocking on the rest. A critical task dying (panic, or a loop returning Err) left the process up and looking healthy while doing no useful work — the same silent- stall class the worker panic produced. Add a supervisor: every long-running service runs until the shutdown sequence stops it, so a task finishing before shutdown was requested is by definition an unexpected critical-task exit. supervise() treats the first such exit as fatal: it requests shutdown (a shared Shutdown handle the signal-handler task also waits on, so the existing reverse-order stop sequence runs), keeps draining, then returns an error so the process exits non-zero for the orchestrator to restart cleanly. Shutdown::requested_signal uses register-then-check ordering so a request racing the wait can't be lost. Tests cover both the fatal path and the deliberate-shutdown path, bounded by a timeout so a regression fails fast instead of hanging.
The worker holds the pgmq transaction (and the row's Running lock + a pooled DB connection) open across the entire process_job call, which makes external calls. Although each external call is individually bounded, there was no overall timeout: a dependency that accepts the connection but never responds would pin those resources and wedge the single sequential worker indefinitely. Wrap process_job in a 300s backstop timeout — comfortably above the legitimate worst case (summed per-call timeouts, ~couple of minutes) so it only fires on a genuine hang. On elapse the job becomes a retryable error: it reschedules and the JobGuard drops, rolling back the transaction and releasing the lock and connection. Recovery is idempotent (chain-as-source-of-truth), so re-running is safe. Extract run_job_with_timeout so the timeout-to-retryable mapping and result pass-through are unit tested without a live job.
The supervisor catches a worker that exits, but not one that is wedged — alive yet making no progress. Nothing external could observe that state: there was no health or readiness endpoint at all. Add a Liveness watermark the worker ticks every loop iteration (including idle polls), and a minimal HTTP health server (axum, reusing the hyper stack already linked by the RPC servers) that returns 503 once the watermark is older than a threshold, else 200. An orchestrator liveness probe can then restart a wedged process. The threshold defaults to 600s — above the worst-case gap between ticks (poll period + the 300s process_job backstop) so a legitimately slow job never trips it. Gated behind optional [health] config (listen_addr + threshold); no endpoint is started when unset. The server is spawned into the supervised task tree and stopped first in the graceful shutdown sequence. The staleness predicate and the 200/503 responses are covered by unit and TCP integration tests.
…ing it The domain refresh was the one task started with a bare tokio::spawn: no stop handle, outside the task tree, unaffected by graceful shutdown, and invisible to the new supervisor. A panic in it would silently stop domain refreshes, letting dipper sign offers under a stale EIP-712 domain after a contract upgrade — an incorrect state with no signal. Extract the loop into chain_client::run_domain_refresh (generic over the refresh action, with a stop channel) and spawn it into the supervised task tree with the others. It now stops in the graceful shutdown sequence and an unexpected exit is caught by the supervisor. A test covers the stop wiring without a live chain.
Shutdown::request used Notify::notify_one, which wakes a single parked waiter. requested_signal is a general-purpose, cloneable signal that multiple tasks may await concurrently, so a shutdown could leave all but one waiter hanging forever. Production wires a single awaiter today, so this was latent, but the abstraction is wrong as written. Switch to notify_waiters, which wakes every parked waiter. It stores no permit for future waiters, but none is needed: the register-then-check in requested_signal returns via the bool flag for any waiter that arrives after the request. Adds a deterministic regression test that parks two waiters and asserts a single request wakes both.
Member
|
Closing this PR as superseded. See the following PR's that implement this PR's work:
|
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.
Observations from reviewing dipper-service for use with AMS (Agreement Management Service), with hardening suggestions for the failure modes identified. Consider this PR more like a issue and suggestion report; I have not carefully reviewed them. They are likely not all equally valid, and they should be independently reviewed.
Main concern is about silent stalls for thread hygeine (and considering use case of adding a new thread): states where the process looks healthy but isn't making progress. Five observations/suggestions:
Each failure mode includes a test.