SIGTERM handler race abruptly kills graceful shutdown

[ikwuoz]

Problem

arc-node/crates/malachite-app/src/node.rs

Lines 1139 to 1156 in 97f8da0

	tokio::spawn(async move {
	sigterm.recv().await;
	warn!("Received SIGTERM, shutting down...");
	stop_node_and_teardown(
	node.stop_and_wait(
	Some("Received SIGTERM signal".to_string()),
	Some(Duration::from_secs(10)),
	),
	&cancel_token,
	&graceful_shutdown,
	)
	.await;
	drain_before_exit(|| store.savepoint()).await;
	std::process::exit(SIGTERM_EXIT_CODE);
	});

install_sigterm_handler as seen above, spawns a task that attempts to perform three ops in order:

1. stop_node_and_teardown which cancels a cancellation token
2. drain_before_exit which creates a savepoint in the db
3. finally exit with code 143

However, Node::run returns immediately after the app future is cancelled in step (1) above, causing main::start to drop the Tokio runtime, which aborts the sigterm handler clean-up mid task.

This result in the process exiting with code 0 as opposed the intended code 143 as documented

Evidence

A simple binary script was drafted here to verify this race bug

Fix

Add one-shot handover:

• in Handle struct, sigterm_received: Arc<AtomicBool> and sigterm_done: Arc<Notify>
• in the sigterm handler, call store(true) before teardown, and notify_one() after drain_before_exit (step 2 above)
• in Node::run(), if received then await notify and return "SIGTERM" error
• in main::start, map this "SIGTERM" error to exit(143); keep handler exit(143) for HaltAndWait path where run sleeps forever.

[ikwuoz]

@romac @ZhiyuCircle
Please have a look at this minor bug

[osr21]

Disclosure: I'm not affiliated with Circle — I'm an external community contributor, not a maintainer, and I have no write access to this repository. This is one contributor's technical assessment, advisory only; please defer to Circle maintainers. The repro below is a standalone Rust model of the shutdown path, not the arc-node tree itself, so CI and maintainers remain authoritative.

Confirmed — the race is real, and I reproduced it. But the mechanism is not the one in the description, and the difference matters both for severity and for the fix.

Reproduced

I built a standalone model on tokio 1.52.3 (the version pinned in Cargo.lock) on a multi-threaded runtime, matching build_runtime (crates/malachite-cli/src/runtime.rs), and mirroring the real ordering in install_sigterm_handler → stop_node_and_teardown → drain_before_exit:

[handler] SIGTERM received
[app]     savepoint (app.rs:112)
[run]     app task joined -> Ok(())
[run]     handler savepoint reached? false
[main]    run() returned Ok(()); dropping runtime
[main]    runtime dropped, returning Ok -> exit 0
PROCESS EXIT CODE = 0

Same result across node-stop durations of 0 ms, 50 ms, 300 ms and 1000 ms: exit code 0 every time, exit(143) never reached, the 500 ms drain never completes. So the headline claim holds.

The app doesn't return because of the cancel token

The issue says Node::run returns "after the app future is cancelled in step (1)". On the SIGTERM path it almost never gets that far. Look at the order inside stop_node_and_teardown (node.rs:1027-1038):

graceful_shutdown.cancel();      // 1
stop_node.await;                 // 2  <-- Node actor + children stop here
cancel_token.cancel();           // 3

Stopping the Node actor closes the consensus channel, and app::run treats a closed consensus channel during a graceful shutdown as a clean exit (app.rs:94-99, the ConsensusChannelClosed branch) — returning Ok(()) at step 2, not step 3. The cancel-token branch (run_until_cancelled_owned → None) is the other path, taken by the EL-IPC watchdog.

That makes the window much wider than described. The handler isn't losing a 500 ms sleep — it can be killed while still inside node.stop_and_wait(…), which is bounded at 10 seconds. In my run with a 300 ms node stop, main had already dropped the runtime 1 ms after SIGTERM, before the stop returned.

What is actually lost — and what isn't

This is where I'd push back slightly on the framing. The savepoint is not lost: app::run takes its own savepoint at app.rs:112 before returning, and that ran in every one of my runs. The handler's drain_before_exit(|| handles.store.savepoint()) is redundant with it on this path.

So the concrete damage is narrower than "graceful shutdown is abruptly killed":

• the exit code is 0 instead of the intended 143;
• the 500 ms SHUTDOWN_DRAIN_DELAY never elapses, so in-flight tasks (RPC server, store monitor, rpc-sync) are dropped at their await points instead of getting the grace window;
• the ractor child teardown inside stop_and_wait can be cut short mid-sequence — which is the part I'd actually worry about, since stop_node_and_teardown's doc comment says the ordered stop exists specifically so "in-flight effects never fail on a torn-down sibling".

Worth noting the same two helpers are used correctly one branch up: the EL-IPC path (node.rs:969-981) calls stop_node_and_teardown + drain_before_exit + exit(1) inline in run(), where nothing can return out from under it. Only the spawned-handler path races. That's a useful framing for the fix — the bug isn't in the helpers, it's that one caller runs in a task the runtime owns.

On the proposed fix

The handover is the right idea and one detail is safe that looks risky: Notify::notify_one() firing before Node::run reaches notified().await does not lose the wakeup — tokio stores one permit ("At most one permit may be stored by Notify", tokio 1.52.3 sync/notify.rs), so the later await returns immediately.

Two things I'd change:

1. Don't model SIGTERM as an error. Step 4 proposes returning a "SIGTERM" error and mapping it in main::start. main() returns Result<()> under oneline_eyre (main.rs:70-71), so an Err prints an error report and exits 1, not 143 — you'd need an explicit std::process::exit(143) anyway, and every clean operator-initiated stop would emit an error line into the journal. Since the handler already owns the exit, Node::run only needs to not return: await the notify and let exit(143) win. That drops the AtomicBool, the error variant, and the mapping in start.
2. Set the flag before graceful_shutdown.cancel(), not just before teardown generally. The app can unblock the instant the consensus channel closes, so anything ordered after that point is already racing.

Your point about keeping the handler's exit(143) for the HaltAndWait path is correct — run parks on sleep(Duration::MAX) there (node.rs:1000), so the handler must remain the one that exits.

Two adjacent things

The EL binary has the same shape. crates/node/src/main.rs:646-700 installs a plain tokio::spawn SIGTERM handler that ends in exit(143), while handle.node_exit_future — per the handler's own doc comment — completes when the engine shuts down. reth's CliRunner does insert a graceful phase before dropping the runtime, but reading reth-cli-runner / reth-tasks at the pinned v2.2.0 (88505c7), do_graceful_shutdown only spins while graceful_tasks > 0 — a counter incremented by task-manager registrations, not by plain tokio::spawn. So that phase returns immediately and the handler gets no extra window. I can't execute the EL to confirm, so treat that as source-review only, but if the CL fix lands alone the EL keeps the behaviour.

Nothing in the documented deployment depends on the exit code today. Both systemd units in docs/running-an-arc-node.md use Restart=always with TimeoutStopSec=300, so 0 vs 143 changes no restart behaviour there, and 143 appears nowhere in the docs — only in the SIGTERM_EXIT_CODE comment at node.rs:1012-1014. That's an argument for treating this as a correctness/consistency fix (the code should do what it says, and the drain should actually happen) rather than an operational incident. If maintainers do land it, docker stop and Kubernetes will start surfacing the container's last state as a non-zero exit, which is worth a CHANGELOG line under ### Fixes — v0.8.0 already carries a related entry for the SIGTERM path.

One note on the gist: it mirrors an older layout (node.rs:826-867) and puts the 500 ms sleep before the engine stop, on a current-thread runtime, and both branches end in exit(0) — so it demonstrates "handler killed" but can't show the exit code. The ordering it models is also the more forgiving one. Happy to share my version if useful.