oracle: fix shutdown race in signing orchestrator - #442
Open
JohnnyLawDGB wants to merge 1 commit into
Open
Conversation
OracleSigningOrchestrator registers itself as a validation interface subscriber, and Shutdown() destroys it immediately after Stop(). Stop() only calls UnregisterValidationInterface(), which is non-blocking and can return while a notification is still in flight, so a queued BlockConnected callback can run on the destroyed object: OnBlockConnected() -> CleanupOldSessions() locks m_sessions_mutex on freed memory. Locking a destroyed std::mutex is undefined behaviour, and the two runtimes express it differently: on glibc the scheduler thread parks in a futex and never wakes, so Shutdown() blocks behind it forever in CScheduler::stop(); on libc++ it aborts, which surfaces as exit code -6. -stopatheight makes the collision near-certain, because StartShutdown() is called from KernelNotifications::blockTip() during block connection, so a BlockConnected notification for that same block is already queued when shutdown begins. That is why rpc_blockchain.py fails intermittently in CI, on either platform depending on load. The window is not specific to -stopatheight: any shutdown that begins while a block is connecting can hit it. Drain the queue with SyncWithValidationInterfaceQueue() before destroying the orchestrator, per the contract documented in validationinterface.h. The use-after-free is a microsecond-wide race, so the added test pins the contract that prevents it instead: Shutdown() must not return while a queued notification is still pending. Without the drain that check fails and the suite additionally takes a SIGSEGV as the freed orchestrator is touched.
ycagel
requested review from
DigiSwarm,
JaredTate,
SmartArray,
digicontributer,
gto90 and
ycagel
and removed request for
DigiSwarm
August 12, 2026 02:01
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.
Fixes #441.
Problem
OracleSigningOrchestratorregisters itself as a validation-interface subscriber (signing_orchestrator.cpp:195), andShutdown()destroys it immediately afterStop():validationinterface.h:27marks that unregister pathDEPRECATED. This is not safe to use when the RPC server or main message handler thread is running, and:32-35spells out why: "unregistration is nonblocking and can return before the last notification is processed." Both conditions hold at the callsite —init.cpp:280runs beforeStopRPC()(:285), beforenode.scheduler->stop()(:303), and beforeFlushBackgroundCallbacks()(:334).So a queued
BlockConnectedruns on the destroyed object:OnBlockConnected()→CleanupOldSessions()→std::lock_guard<std::mutex> lock(m_sessions_mutex)on freed memory.Locking a destroyed
std::mutexis UB, and the runtimes differ, which is why one bug produced two CI symptoms:Shutdown()blocks behind it forever inCScheduler::stop()→ the node hangs.-6.-stopatheightmakes the collision near-certain becauseStartShutdown()fires fromKernelNotifications::blockTip()during block connection (kernel_notifications.cpp:63-66), so the emission for that block is in flight exactly as shutdown proceeds.How far this reaches beyond that option is narrower than the mechanism first suggests, and #441 now carries the measurement: a notification dispatched after
Stop()no longer reaches the unregistered orchestrator, so only an already-in-flight emission is dangerous. 300 ordinarydigibyte-cli stoptrials against a node continuously connecting blocks, unpatched, produced no hangs, no aborts, and no callback-after-teardown. Treat this as a latent use-after-free on an API the header marks unsafe, not a demonstrated operator-facing hazard.Fix
One call, honouring the documented contract — let the in-flight callback finish before destroying what it touches:
#441lists two alternatives I'd be glad to switch to if preferred:RegisterSharedValidationInterface()(the header's own suggestion for race-free cleanup), or moving the destruction afternode.scheduler->stop()— the patternnode.peermanalready uses, unregistered atinit.cpp:296but destroyed at:309.Regression test
The use-after-free is a microsecond-wide race and can't be asserted on directly, so
src/test/oracle_shutdown_tests.cpppins the contract that prevents it:Shutdown()must not return while a queued notification is still pending. It occupies the queue with a callback that outlives a non-drainingShutdown(), then checks the callback finished first.Reverting just the
SyncWithValidationInterfaceQueue()line makes it fail, and the failure is not only the assertion — the freed orchestrator gets touched:With the fix restored:
*** No errors detected.Testing
Unit: full suite passes on this branch — 3408 cases with the new test, no errors.
Functional: the race needs load to lose — solo,
rpc_blockchain.pyalmost always passes. Harness is 6 concurrent copies on an 8-core box, each with a distinct--portseedso instances can't collide on bind ports:_test_stopatheightfailuresdevelop(16159311b3)One of the unpatched reproductions was caught mid-hang, with the scheduler and shutoff threads deadlocked against each other — thread states are in #441.
Investigation and patch assisted by AI tooling; the root cause was reproduced and the fix verified locally against
develop(16159311b3).