fix race condition in sigterm handler

[ikwuoz]

This PR address #360
It fixes the race condition bug with minimal changes

[GG5533]

The handover works on the path where the app task resolves normally, but the ? on the line above it means a panicking app task still takes the original path.

let result = handles.app.await?;   // JoinError returns here

if handles.sigterm_received.load(Ordering::SeqCst) {
    handles.sigterm_done.notified().await;
    return Err(eyre::eyre!("Received SIGTERM signal"));
}

app is JoinHandle<eyre::Result<()>>, so the await yields Result<eyre::Result<()>, JoinError> and the ? discharges the outer JoinError before the new check is reached. If the app task panics, run returns immediately, main::start drops the runtime, and the handler is aborted mid-cleanup — the behaviour #360 describes, with drain_before_exit(|| store.savepoint()) never running.

On how it's triggered: nothing in crates/malachite-app aborts handles.app — the .abort() calls I found are on app_req_task in app.rs, a different task, or in the test-integration harness which builds its own. And stop_node_and_teardown cancels cooperatively via CancellationToken, so a cancelled task still resolves Ok. That leaves a panic as the trigger here. Narrower than the case #360 fixes, but the same failure mode, and the savepoint is skipped.

Moving the check above the ? closes it without changing anything else — result is only inspected and returned, so binding it a line later is a no-op:

let joined = handles.app.await;

// SIGTERM handover runs even if the app task panicked, so the runtime
// outlives the handler's teardown + savepoint either way.
if handles.sigterm_received.load(Ordering::SeqCst) {
    handles.sigterm_done.notified().await;
    return Err(eyre::eyre!("Received SIGTERM signal"));
}

let result = joined?;

Two smaller notes:

The exit-code fallback is stringly-typed. Err(e) if e.to_string().contains("SIGTERM") in main::start makes the 143 depend on the wording of eyre!("Received SIGTERM signal"). Rewording that message later would silently reintroduce the exact symptom #360 reports, with nothing failing to signal it. A typed variant, or a bool/enum returned out of run, would decouple the exit code from prose.

That fallback is also rarely exercised, which is worth knowing when reasoning about it: the handler calls notify_one() and std::process::exit(SIGTERM_EXIT_CODE) back-to-back with no yield point between them, so the parked run task almost never gets polled before the process terminates. Both paths give 143, but it does mean a regression in the main.rs mapping would likely go unnoticed.

For a regression test: the existing tests cover stop_node_and_teardown and drain_before_exit individually, but nothing covers the sequencing in run that #360 is about, and this diff doesn't add one. A single test covers both the original bug and the case above — drive run with an app task that panics, and assert the savepoint callback still ran. That fails on the current diff and passes with the reorder. Happy to send it as a PR against this branch if useful.

[GG5533]

Three corrections to my own comment above, after a second pass.

The reorder I suggested swallows the JoinError. When the flag is set and the join failed, it returns the SIGTERM error and drops the join failure entirely — so a panic during shutdown would vanish from the logs, which is worse than the current behaviour in that one respect. It should keep it:

let joined = handles.app.await;

if handles.sigterm_received.load(Ordering::SeqCst) {
    if let Err(e) = &joined {
        warn!(%e, "app task failed while SIGTERM cleanup was in progress");
    }
    handles.sigterm_done.notified().await;
    return Err(eyre::eyre!("Received SIGTERM signal"));
}

let result = joined?;

I overstated the consequence. I wrote that the savepoint "never runs". Runtime shutdown cancels async tasks at their yield points; it does not interrupt an already-running synchronous call, so if store.savepoint() has begun it may well complete. The accurate claim is that returning early can allow the runtime to shut down before cleanup finishes — the savepoint is not guaranteed, rather than guaranteed absent.

And I overstated the trigger. "That leaves a panic as the trigger here" is stronger than what I checked. An unwinding app-task panic is one case that bypasses the handover; I verified there is no .abort() on this handle in crates/malachite-app, but that doesn't exclude every mechanism that can produce a JoinError. (An aborting panic wouldn't be repaired by the reorder either.)

Also, my regression-test sketch was too thin — a panicking task alone isn't sufficient. It needs the SIGTERM flag set first, cleanup held incomplete until the join is observed, and process::exit stubbed or driven in a subprocess, otherwise it won't discriminate between the two orderings.

The ? short-circuit itself is unchanged: the check sits below a ? that discharges the outer JoinError, so that path skips the handover.

[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. Everything below is measured against a standalone Rust model rather than reasoned about; the model's limits are stated at the end.

Assessment of 9521cdf (fix race condition in sigterm handler).

I wrote the analysis on #360 that this PR implements, so rather than re-reasoning about the diff I re-ran the same standalone model against it: tokio 1.52.3 (the version pinned in Cargo.lock), multi-threaded runtime built the way build_runtime builds it, mirroring the real orderings in App::run, app::run, install_sigterm_handler and main::start, with ractor and redb replaced by timed stubs. Every number below is an observed exit code or log ordering.

The core fix works

	before this PR	with this PR
exit code (node-stop 0 / 300 / 1000 ms)	0, 0, 0	143, 143, 143
stop_and_wait completes	no — killed mid-call	yes
500 ms drain window elapses	never	every run

Two details that are easy to get wrong and that this diff gets right:

• notify_one() before a waiter is safe. Notify stores one permit, so if run reaches notified() after the handler has already signalled, it returns immediately rather than hanging. No lost-wakeup race here.
• sigterm_received is stored before graceful_shutdown.cancel(), so run cannot observe the app future resolving before the flag is visible.

One correction to how the fix is likely to be read, restated from #360 so the change isn't credited with more than it does: the savepoint was never actually lost on this path. app::run takes its own at app.rs:112, and it ran in every pre-patch run in the table above. What the patch recovers is the exit code, the drain window, and completion of the teardown sequence — not durability.

1. The main.rs mapping is ~5% live, and it's the fragile half of the diff

The in-code comment says Node::run returns Err("SIGTERM") and "main::start maps it to 143". Measured over 40 runs of the patched ordering:

• handler's own std::process::exit(143) reached: 40 / 40
• main.rs:306 mapping reached: 2 / 40

notify_one() and std::process::exit(SIGTERM_EXIT_CODE) are back-to-back with no yield point between them, so the woken run task usually loses. In the 2 runs where both markers printed, which exit() actually terminated the process is a coin flip — both are 143, so there's no bug today.

That matters because the 5%-live path is the one that depends on prose. Err(e) if e.to_string().contains("SIGTERM") couples the exit code to the wording of eyre!("Received SIGTERM signal") twenty lines away in another crate module, and nothing fails if someone rewords it — it would silently reintroduce exactly the symptom #360 reports. This is the concern I raised in #360 about returning Err under oneline_eyre (a bare Err exits 1, not 143); the string match does address it, but re-introduces it as a wording dependency.

There's a cleaner option that removes the whole question: have run call std::process::exit(SIGTERM_EXIT_CODE) itself after the handover, the way the EL-IPC watchdog branch twenty lines below already does (stop_node_and_teardown → drain_before_exit → std::process::exit(EL_IPC_SHUTDOWN_EXIT_CODE), inline in run). That drops the pub on the const, the new import in main.rs, and the string match — and makes the two shutdown paths in run look the same.

2. The handover is unbounded, and a stalled shutdown is no longer killable by SIGTERM

handles.sigterm_done.notified().await has no timeout and no escape hatch. If the handler task fails to reach notify_one(), run waits forever. Measured, with the handler panicking after teardown and before the signal:

ordering	result
pre-patch	exits 0 (the #360 bug)
this patch	hangs indefinitely — still alive after 5 s, survived 3 further SIGTERMs, needed SIGKILL
handover via the handler's JoinHandle instead	exits 143

The "survived 3 further SIGTERMs" part is the sharp edge. Tokio's signal registration is process-global and stays installed after the handler task is gone, and install_sigterm_handler does sigterm.recv().await once — not in a loop — so there is no second-signal escape. In the hung state the process ignores SIGTERM entirely. Against the deployment docs in this repo (docs/running-an-arc-node.md, Restart=always + TimeoutStopSec=300) that's a 5-minute stall before systemd escalates to SIGKILL; on Kubernetes it's the grace period, then SIGKILL — the un-drained kill the PR is trying to avoid, just later.

To be fair about likelihood: this is not a probable crash. Store::savepoint swallows its errors (ensure_allocator_state_table().is_err() → warn!), and stop_and_wait is bounded at 10 s and its error is handled. So the window needs an unwinding panic from ractor or redb. The point is the shape, not the odds — the patch replaces a bounded wrong behaviour with an unbounded one, and the fix is free either way:

• Await the handler's JoinHandle (store it in Handle in place of sigterm_done, one field instead of two). JoinHandle resolves on panic as well as completion, so it cannot hang. Verified above: exits 143.
• Or wrap the existing wait: let _ = timeout(Duration::from_secs(15), handles.sigterm_done.notified()).await;

Note that crates/node/src/main.rs:645-700 — the EL binary's SIGTERM handler — already has both guards this one lacks: timeout(Duration::from_secs(30), done_rx) and a nested task that force-exits 143 on a second SIGTERM.

3. @GG5533's ? finding is real — and the exit code is worse than 1 might suggest

Confirmed independently: let result = handles.app.await?; discharges the outer JoinError before the new flag check, so a JoinError skips the handover. Modelled with the app task panicking after the flag is set:

[306ms] app::run -> PANIC (JoinError to Node::run)
[306ms] main: Err -> exit 1

Worth adding to that thread: the observed exit is 1, not 143 — the string-match fallback doesn't catch a JoinError either, so this path loses the exit code and the handover. Their revised snippet (bind the join result, check the flag, log the JoinError, then ?) is the right shape; awaiting the handler's JoinHandle per §2 composes with it.

4. SIGTERM during the startup-failure park still exits 1

run's startup-failure branch parks in wait_for_termination() and then returns Err(startup_error), where startup_error = e.wrap_err("Node failed to start"). eyre's to_string() renders only the outermost message, so:

wrapped to_string : "Node failed to start"
contains "SIGTERM": false
chain             : ["Node failed to start", "Received SIGTERM signal"]

That path exits 1. Non-zero is arguably right for a failed start, but by the PR's own rationale — "so the container orchestrator observes the correct termination reason" — a node that was parked waiting for SIGTERM and then received it is a SIGTERM termination. Either way it's a case the current mapping can't express, which is another argument for a typed signal over a string.

5. No regression test

The existing tests in node.rs cover stop_node_and_teardown and drain_before_exit in isolation; nothing covers the sequencing in run, which is the entire subject of #360, and this diff doesn't add one. As @GG5533 notes, a unit test needs the flag set, cleanup held incomplete until the join is observed, and process::exit stubbed or driven out-of-process. crates/test/framework/tests/errors.rs already spawns a process, so a subprocess-level assertion (send SIGTERM, assert exit 143 and that the drain log line appears) may be the lower-friction route than unit-testing run.

Scope

The diff correctly leaves the EL binary alone — as noted in §2 it already has the guards this handler is missing, so it isn't affected by #360.

---

Model caveats: this is a standalone binary reproducing the control flow, not arc-node itself. ractor's stop_and_wait is a timed stub that closes the consensus channel partway through (matching the real ordering, where stopping the Node actor closes the channel before the call returns), and redb is a no-op savepoint. It reproduces the pre-patch symptom and the post-patch fix, so I believe the orderings are faithful, but the exit-path ratio in §1 is scheduler-dependent and will differ on other hardware. Happy to share the model if useful. Nothing here is a maintainer decision — treat §2 as the one I'd not merge without, and the rest as calibration.

[GG5533]

@osr21 — your correction on the savepoint is right and mine was too strong; thanks for the measurement. app::run does take its own at app.rs:112, so on the ordinary SIGTERM path the savepoint was never the thing at risk. What the patch recovers is the exit code, the drain window and teardown completion. I'd already walked "never runs" back to "not guaranteed" upthread, but your version is the accurate one.

One refinement on where durability is implicated.

app.rs:112 sits after the match on the cancellation result. There's no catch_unwind and no scopeguard in run_app, and neither state.rs nor consensus-db/src/store.rs has a Drop impl, so nothing takes a savepoint on the way out of an unwind. An unwinding panic inside go(...) therefore never reaches line 112.

That panic then surfaces to Node::run as a JoinError from the JoinHandle, and handles.app.await? discharges it before the sigterm_received check — so the handler's drain_before_exit is skipped too:

path	app.rs:112	handler's drain_before_exit (node.rs:1154)
ordinary SIGTERM cancellation	runs	runs
app task returns Err	runs	reached (? only discharges the outer JoinError)
app task panics	skipped by the unwind	skipped — JoinError propagates via ?

So: wrong on the ordinary path, and the panic row is the only one where durability is actually at stake — both savepoints, not one. Narrower than what I first wrote, and it needs the panic rather than following from the ? alone.

Your §2 is a better finding than mine and I'd prioritise it over the ? reorder. Replacing a bounded wrong exit code with an unbounded hang is a worse trade, and "survived 3 further SIGTERMs" is what turns a papercut into an incident — a process that ignores SIGTERM is one only SIGKILL ends, which is the un-drained kill this PR exists to prevent.

The two fixes also converge. Awaiting the handler's JoinHandle instead of the Notify covers the panic case as well, since a JoinHandle resolves on panic rather than hanging, and it replaces two Handle fields with one. The flag check still needs to sit above the ?, but the thing being awaited stops being able to hang. That seems strictly better than my reorder.

On §1 — agreed, and your 2/40 measurement settles something I could only state as a guess. I wrote that the mapping was "rarely exercised" with no basis for a frequency; you have one. Exiting inline in run, matching the EL-IPC watchdog branch twenty lines below, removes the string match, the pub on the const and the extra import in one move. That's what I'd land.

[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.

@GG5533 — I checked your panic-path claim rather than taking it, and then measured it. It holds, and it's a better finding than either of the two we'd been arguing about. One refinement matters for what the fix actually buys you, though.

Verified first: the unwind assumption

Before anything else — if this repo built with panic = "abort", the whole JoinError argument would be moot (no unwind, no JoinError, process dies on SIGABRT). It doesn't: no panic key in any Cargo.toml, and [profile.release] sets only lto/opt-level/codegen-units/strip. Unwind is the strategy, so your reasoning applies.

The rest of your source claims check out too: app.rs:112 sits after the match closes at :109; the only Drop impls anywhere in malachite-app are EnvGuard (test helper) and MetricsGuard, neither on this path; the only catch_unwind in the crate is streaming.rs:1239; and handles.app.await? at node.rs:976 does precede the flag load at :981.

Measured

Standalone model of node.rs:976-983 + install_sigterm_handler (tokio 1.53.1, multi-threaded, rustc 1.82.0), real SIGTERM delivered to a real process, node-stop 300 ms, drain 500 ms:

mode	n	exit	app.rs:112 savepoint	handler savepoint	drain completed
PR as written, normal SIGTERM	10	143 ×10	✅	✅	✅
PR as written, app task panics	10	1 ×10	❌	❌	❌
Fix shape, app task panics	10	143 ×10	❌	✅	✅
Fix shape, normal SIGTERM	10	143 ×10	✅	✅	✅
PR as written, handler panics	6	hang ×6 — deaf to 3 further SIGTERMs, needed SIGKILL	✅	❌	❌
Fix shape, handler panics	6	143 ×6	✅	—	—

So the app-panic row is real and deterministic, not a narrow race: 10/10 exit 1 with the entire teardown lost. That is precisely the failure this PR exists to prevent, re-entering through a different door.

The refinement: the fix recovers one savepoint, not both

You wrote "both savepoints, not one". The measurement says otherwise, and I think this is the part worth carrying forward:

Fix shape, app task panics → app.rs:112 savepoint still never runs.

It can't. It's inside the task that's unwinding, so no change to how Node::run waits can bring it back — moving the flag check above the ? and awaiting a JoinHandle both happen in the observer, not the unwinding task. What the fix recovers on that path is the exit code, the drain window, and the handler's savepoint. Recovering app.rs:112 needs something categorically different: a Drop guard on the state, or catch_unwind around go(...).

Whether that's worth doing is a separate call — the handler's savepoint may well be sufficient — but it shouldn't be folded into this PR's scope on the belief that the reorder covers it.

The window is wider than it looks

I mapped when the panic has to land to do damage, by varying the panic delay against a 300 ms node-stop:

panic at	outcome
0 ms, 200 ms	exit 1, no savepoint at all
400 ms, 700 ms, 750 ms	exit 1, handler savepoint ran but drain never completed
900 ms (after handler exits)	exit 143, clean

The exposed window is the handler's whole runtime, not an instant — and it scales with the node stop. Re-running with a 2 s node-stop and a 1.5 s panic delay: 3/3 exit 1, no savepoint. Since stop_and_wait is bounded at Duration::from_secs(10), the worst-case window is ~10.5 s, and it's widest exactly when the node is being slow to stop, which is when you most want the drain.

Agreed on convergence, with one caveat on how §1 lands

Your point that the JoinHandle covers both is confirmed — the handler-panic rows go from hang×6 to 143×6 with no change to the normal path.

One thing to watch when combining with §1: in the handler-panic case, the 143 comes from the main.rs string match — it's the only thing producing the right code once the handler is dead. So "make run exit inline and delete the string match" is right, but the inline exit has to sit on that fallback path too; a plain return Err(...) there would drop the exit code that the string match is currently catching. Both fixes are still free, just not independent.

Minor: the swap keeps the field count the same rather than going two-to-one — you still need sigterm_received to know whether to await at all, so it's sigterm_done: Arc<Notify> → JoinHandle<()>, with the flag staying.

On likelihood, to keep this honest

I applied the same standard I used on the handler panic, and it cuts the same way: the production half of app.rs (lines 1-646; everything below 647 is #[cfg(test)]) contains zero unwrap(), expect(, panic! or unreachable!. go(...) calls out into engine, store and streaming code so I can't rule a panic out, but I can't point at a trigger either.

So this is a shape objection, not a live-bug report — same as §2. The argument for fixing it is that the fix is one line of ordering plus a field swap you're making anyway, and the failure mode is losing the drain that the PR was written to protect.

Happy to re-run any row against a revised patch.