harden: supervise the task tree and add worker liveness#639
Open
RembrandtK wants to merge 7 commits into
Open
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.
There was a problem hiding this comment.
Pull request overview
This PR hardens dipper-service against “alive-but-stalled” failure modes by adding task-tree supervision, bounded job execution, and an optional HTTP health endpoint suitable for Kubernetes liveness probing.
Changes:
- Add supervised task-tree shutdown coordination (
supervisor) and move RCA domain refresh into the supervised tree with a stop handle. - Harden worker behavior: degrade LISTEN/NOTIFY failures to poll-only mode and add a per-job backstop timeout with retryable rescheduling.
- Add worker liveness watermark + optional
/healthendpoint and switch the k8s liveness probe to HTTP.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| k8s/deployment.yaml | Switch liveness probe from TCP to HTTP /health and expose a health port. |
| k8s/configmap-example.yaml | Document example health endpoint configuration (listen address + threshold). |
| Cargo.lock | Pull in new deps for the health endpoint (axum/hyper-related). |
| bin/dipper-service/Cargo.toml | Add axum dependency (minimal features) for health serving. |
| bin/dipper-service/src/worker/service.rs | Add LISTEN/NOTIFY degrade-to-poll logic, liveness ticking, and job-level timeout wrapper + tests. |
| bin/dipper-service/src/worker/queue.rs | Introduce JobNotifications trait to unit test worker notification behavior. |
| bin/dipper-service/src/worker/context.rs | Add shared Liveness watermark to worker context. |
| bin/dipper-service/src/supervisor.rs | New shutdown coordination + supervision logic for “unexpected task exit” handling. |
| bin/dipper-service/src/main.rs | Wire up supervision, health server startup/shutdown, liveness sharing, and supervised domain refresh. |
| bin/dipper-service/src/health.rs | New liveness watermark + minimal axum /health endpoint with tests. |
| bin/dipper-service/src/config.rs | Add health config section and defaults for threshold parsing. |
| bin/dipper-service/src/chain_client.rs | Add stoppable run_domain_refresh loop + test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
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.