Skip to content

fix(consensus): wait for SIGTERM shutdown task - #380

Open
orkunsahin wants to merge 1 commit into
circlefin:mainfrom
orkunsahin:fix/sigterm-graceful-shutdown-race
Open

fix(consensus): wait for SIGTERM shutdown task#380
orkunsahin wants to merge 1 commit into
circlefin:mainfrom
orkunsahin:fix/sigterm-graceful-shutdown-race

Conversation

@orkunsahin

Copy link
Copy Markdown

Summary

Coordinate Node::run() with the SIGTERM shutdown task so the Tokio runtime cannot be dropped while graceful shutdown is still draining and persisting the savepoint.

Problem

The SIGTERM handler runs in a spawned Tokio task. Stopping the Node actor can make the application task return cleanly, allowing Node::run() to return before the SIGTERM task reaches drain_before_exit() and exit(143).

Fix

  • Track whether SIGTERM has been received before node teardown begins.
  • Keep the SIGTERM task's JoinHandle.
  • If SIGTERM initiated shutdown, have Node::run() wait for that task instead of allowing the runtime to return early.
  • Add Unix regression coverage verifying the shutdown task is awaited after SIGTERM is marked received.

Testing

  • cargo fmt --all — passed.
  • git diff --check — no whitespace errors; Git emitted only the Windows LF/CRLF conversion warning.
  • cargo test -p arc-node-consensus — could not complete on native Windows because the reth-provider dependency imports ChangesetOffsetReader, which is gated behind cfg(all(feature = "std", unix)) in reth_static_file_types.
  • The added regression test is Unix-only and should be exercised by Linux CI.

Fixes #360

@osr21

osr21 commented Sep 11, 2026

Copy link
Copy Markdown

Reviewed dd7fb33 against the issue and the shutdown ordering. The fix mechanism is correct.

Why this fixes the reported race

The important ordering in stop_node_and_teardown is:

  1. graceful_shutdown.cancel() can let app::run return through the graceful consensus-channel-closed path.
  2. node.stop_and_wait(...) can still be running for up to its 10-second bound.
  3. cancel_token.cancel() follows the node stop.
  4. the handler then writes the savepoint, waits 500 ms, and calls exit(143).

On the PR branch, received.store(true, Release) happens before stop_node_and_teardown, and Node::run checks the matching Acquire flag immediately after handles.app.await. If the app returns while the SIGTERM task is still stopping the node, wait_if_received() awaits the retained JoinHandle instead of dropping the runtime. That keeps the runtime alive through the node stop, savepoint, and drain. This is the right handoff, and using the JoinHandle as the lifetime fence is a valid alternative to the issue's proposed Notify.

It also preserves the existing HaltAndWait behavior: that path parks Node::run indefinitely, so the SIGTERM task remains the owner of exit(143).

Test coverage caveat

sigterm_handler_waits_for_shutdown_task_after_signal is useful, but it is narrower than the regression described in the PR. It starts with received = true and supplies an already-created synthetic task; it does not send SIGTERM, invoke install_sigterm_handler, run the app future, or assert that a savepoint/drain survives runtime shutdown. In other words, it proves that wait_if_received() awaits a task when the flag is already set, but not the production ordering that makes the flag set before the app can return.

That is not a reason to reject the implementation — the ordering is visible and correct in the source — but it means the new test would still pass if the signal wiring or the placement of received.store regressed. A subprocess or test seam around the actual signal path would be the stronger regression test, especially because std::process::exit makes the production path awkward to exercise in-process.

Verification status

  • The source-level ordering and the issue's standalone reproduction agree: the original handler could be dropped while inside stop_and_wait, before the drain completed; this branch retains the task before handles.app.await can unwind the runtime.
  • The PR has one focused Unix test and no formal review yet.
  • Public CI is still action_required for the external fork, so there is no hosted Linux result for this head. I could not complete the workspace test locally because the resolved Reth dependencies require rustc 1.93 while the available local compiler is older; that is an environment limitation, not a reported PR failure.

One small implementation note: wait_if_received() normally never returns after a real signal because the handler ends in std::process::exit(143); its JoinHandle await is intentionally a runtime-lifetime fence, while the unit test uses a returning synthetic task to exercise that fence.

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory review only, not an approval.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGTERM handler race abruptly kills graceful shutdown

2 participants