Drop teardown-time futures with intercepts disabled - #77
Merged
Merged
Conversation
Jobs enqueued on the blocking pool but never run are dropped when PoolShared is dropped, from Executor::drop on the main thread after the reactor context is gone. Such a job's closure can own resources (e.g. a RocksDB handle) whose Drop makes intercepted syscalls like close(); the interceptor would then reach context::current() with no reactor and panic, aborting the process. Drain the queue in PoolShared::drop with intercepts disabled so those late syscalls reach the real libc. The interceptors are otherwise left strict - a context-less intercepted syscall elsewhere is still a bug worth surfacing.
`pause()` parks a node's woken runnables in `Node::paused`. The node map is shared with `runtime::Handle`, so it outlives the executor and is torn down on the main thread after `block_on` has returned and the context is gone. Dropping a parked runnable drops its future, whose Drop impls can make intercepted syscalls - a socket or file `close` routes through `plugin::simulator()`/`plugin::node()`, which panic with "there is no reactor running". The panic is inside an `extern "C"` fn, so it aborts the process rather than failing the test. Drain the map in `Executor::drop` with intercepts disabled, the same treatment `PoolShared::drop` gives un-run blocking jobs. The interceptors stay strict everywhere else. Add teardown regression tests covering both sites: each parks a future owning an fd and drops the runtime, and each aborts with its respective fix reverted. Two further cases are covered as guards rather than reproductions - a task woken as the main future completes is still drained by `run_all_ready` (the main task runs inside that loop), and a waiter woken by blocking-pool shutdown unwinds on the pool thread. Futures parked on the timer wheel are not covered: they are leaked rather than dropped, so they run no Drop code at all. The file is gated on `cfg(msim)` - `cargo test` without it builds the std path, where the simulator API does not exist.
mystenmark
force-pushed
the
mlogan-drop-unrun-blocking-jobs
branch
from
July 30, 2026 22:00
78f5ba5 to
912090f
Compare
lxfind
approved these changes
Jul 30, 2026
8 tasks
mystenmark
added a commit
to MystenLabs/sui
that referenced
this pull request
Jul 31, 2026
## Description Picks up two mysten-sim changes: - [#76](MystenLabs/mysten-sim#76) — deterministic blocking-task pool (limited multi-threading), so `spawn_blocking` work is scheduled deterministically instead of escaping the simulation. - [#77](MystenLabs/mysten-sim#77) — drop teardown-time futures with syscall intercepts disabled. Without it, a future still holding a resource when the runtime is torn down closes its fd after the reactor context is gone, and the interceptor panics inside an `extern "C"` fn, aborting the process. That is the failure mode that made ~38 transactional tests SIGABRT under #76. All four pin sites move together — the `msim`/`msim-macros` git deps in `Cargo.toml` and the `tokio`/`futures-timer` patch revs in `scripts/simtest/cargo-simtest`. They must stay in lockstep: the patched tokio is built from the same mysten-sim revision as msim itself, and a mismatch produces confusing link/behavior errors rather than a clean failure. ## Test plan `cargo simtest --test simtest` (sui-benchmark, 34 tests) against the new pin, resolved from git rather than a local checkout. Also exercised extensively against this revision during mysten-sim#77 development. Worth a careful look at the simtest and simtest-mainnet CI jobs: #76 changes how blocking tasks are scheduled, so this bump can shift timing in any simtest, not just ones that call `spawn_blocking` directly. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] Indexing Framework: Co-authored-by: Mark Logan <mark@marklgn.com>
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.
What
Two teardown sites drop user futures on the main thread after
block_onhas returned and the reactor context is gone: the blocking pool's un-run jobs, and runnables parked inNode::pausedbypause(). Drain both with syscall intercepts disabled, so resources captured by those futures close through the real libc.Why
A dropped future runs the
Dropimpls of everything it holds. Those can make an intercepted syscall —close()on a file or socket routes throughplugin::simulator()/plugin::node(), which reachcontext::current()with no reactor and panic ("there is no reactor running"). The panic is inside anextern "C"fn, so it cannot unwind and aborts the process.Blocking pool. A job enqueued but never run is dropped when
PoolSharedis dropped, fromExecutor::drop. This surfaced as ~38sui-adapter-transactional-testsaborting with SIGABRT once a sui workspace pinned this repo's blocking-pool revision; the stack wasPoolShareddrop → un-run job closure → suiIndexStore→Arc<RocksDB>→rocksdb_close→close()→ interceptor → panic.Paused runnables.
pause()parks a node's woken runnables inNode::paused. The node map is shared withruntime::Handle, so it outlives the executor and is torn down after the context is gone. Same abort, reached from a different direction. No current sui caller usespause(), so this one is latent rather than observed — but it is a headache to debug when it does fire, and the fix is the same one line.The interceptors are left strict otherwise (a scoped disable that restores the prior setting) rather than made globally tolerant of a missing reactor — a context-less intercepted syscall anywhere else is still a real bug worth surfacing.
Test plan
msim/tests/teardown_drop.rsparks a future owning aFileat each teardown site and drops the runtime;File::dropcalls the interceptedclose(). Run under nextest so each test gets its own process — an abort cannot be caught.unrun_blocking_job_dropped_at_teardownandpaused_runnable_dropped_at_teardownare the regression tests, one per site. Each was verified by reverting its own fix and re-running: both SIGABRT without it, pass with it.task_woken_as_main_future_completesandwaiter_woken_by_blocking_pool_shutdownpass both before and after. They are guards, not reproductions: the ready queue is not a hazard because the main task runs insiderun_all_ready's drain loop, so anything its completion schedules is still run with a context; and a waiter woken by pool shutdown unwinds on the pool thread, which has its own guard.Arc::strong_countis still 2 afterdrop(rt), with zeroDropcalls), so they run no user code and are not a hazard. That leak looks worth a separate look, but it is not this PR.The file is gated on
cfg(msim);cargo testwithout it builds the std path, where the simulator API does not exist. Full suite: 40/40 pass under--cfg msim, std mode builds clean. Suicargo simtest --test simtest(sui-benchmark) against this branch: 34/34 pass, and the transactional tests that originally aborted now pass instead of SIGABRT.