Skip to content

Drop teardown-time futures with intercepts disabled - #77

Merged
mystenmark merged 2 commits into
mainfrom
mlogan-drop-unrun-blocking-jobs
Jul 30, 2026
Merged

mystenmark merged 2 commits into
mainfrom
mlogan-drop-unrun-blocking-jobs

Conversation

@mystenmark

@mystenmark mystenmark commented Jul 30, 2026 •

Copy link
Copy Markdown
Collaborator

What

Two teardown sites drop user futures on the main thread after block_on has returned and the reactor context is gone: the blocking pool's un-run jobs, and runnables parked in Node::paused by pause(). Drain both with syscall intercepts disabled, so resources captured by those futures close through the real libc.

Why

A dropped future runs the Drop impls of everything it holds. Those can make an intercepted syscall — close() on a file or socket routes through plugin::simulator()/plugin::node(), which reach context::current() with no reactor and panic ("there is no reactor running"). The panic is inside an extern "C" fn, so it cannot unwind and aborts the process.

Blocking pool. A job enqueued but never run is dropped when PoolShared is dropped, from Executor::drop. This surfaced as ~38 sui-adapter-transactional-tests aborting with SIGABRT once a sui workspace pinned this repo's blocking-pool revision; the stack was PoolShared drop → un-run job closure → sui IndexStore → Arc<RocksDB> → rocksdb_close → close() → interceptor → panic.

Paused runnables. 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 after the context is gone. Same abort, reached from a different direction. No current sui caller uses pause(), 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.rs parks a future owning a File at each teardown site and drops the runtime; File::drop calls the intercepted close(). Run under nextest so each test gets its own process — an abort cannot be caught.

  • unrun_blocking_job_dropped_at_teardown and paused_runnable_dropped_at_teardown are 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_completes and waiter_woken_by_blocking_pool_shutdown pass both before and after. They are guards, not reproductions: the ready queue is not a hazard because the main task runs inside run_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.
  • Futures parked on the timer wheel are deliberately not covered. They are leaked rather than dropped at teardown (Arc::strong_count is still 2 after drop(rt), with zero Drop calls), 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 test without it builds the std path, where the simulator API does not exist. Full suite: 40/40 pass under --cfg msim, std mode builds clean. Sui cargo simtest --test simtest (sui-benchmark) against this branch: 34/34 pass, and the transactional tests that originally aborted now pass instead of SIGABRT.

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.
@mystenmark
mystenmark requested a review from lxfind July 30, 2026 18:13
@mystenmark mystenmark changed the title Drop unrun blocking-pool jobs with intercepts disabled Drop teardown-time futures with intercepts disabled Jul 30, 2026
`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
mystenmark force-pushed the mlogan-drop-unrun-blocking-jobs branch from 78f5ba5 to 912090f Compare July 30, 2026 22:00
@mystenmark
mystenmark merged commit 6b8e1e8 into main Jul 30, 2026
12 checks passed
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>
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.

3 participants