Harden rocksdb shutdown, fix sporadic SIGSEGV in tests - #5116
Harden rocksdb shutdown, fix sporadic SIGSEGV in tests#5116AhmedSoliman wants to merge 3 commits into
Conversation
50a2c06 to
cd71c03
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd71c03349
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Wait for storage tasks that are already running before draining `self.dbs`: a database | ||
| // that is still being opened has not been registered there yet (see `open_db`), so it | ||
| // would otherwise escape the close loop entirely. | ||
| self.join_storage_pools().await; |
There was a problem hiding this comment.
async_spawn checks shutting_down before separately calling ThreadPool::execute, so a submitter descheduled between those operations can resume after both joins have observed an empty pool. In that race, shutdown can drain the databases, join the RocksDB environment, and return before the delayed task—potentially DB::Open—is enqueued, preserving the post-shutdown RocksDB access and SIGSEGV this change is intended to prevent. Protect the admission check and enqueue/drain with the same synchronization mechanism.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this is a valid sequence in which the SIGSEGV can still occur. We probably would want to check whether we are shutting_down in the background task that runs the open db operation and additionally check whether we have shut down before adding the newly opened db to self.dbs.
| /// an unsealed safe one. | ||
| /// | ||
| /// *Since v1.7.3* | ||
| pub(crate) const SEAL_MARKER: u64 = 11; |
There was a problem hiding this comment.
This marker is an unknown FSM key to pre-1.7.3 servers, while crates/types/src/cluster_marker.rs:36-39 still permits binaries as old as 1.6.0 to open the data directory. After an ahead-of-log store is sealed and an operator force-seals the log to a tail beyond the store's applied LSN, rolling back allows the older processor's ordinary tail check to pass and it will use the potentially divergent store because it cannot see this marker. Either make this storage change forward-incompatible or encode an unsafe state older supported binaries will reject, and document the rollback constraint.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
| - A parked processor stays down until an operator intervenes, the node restarts, or the node leaves | ||
| the partition's replica set. Previously it would keep retrying (and keep failing). |
There was a problem hiding this comment.
A node restart does not recover this processor: the seal marker remains in RocksDB, and spawn_processor_task.rs:132-140 reads it on the next startup and returns the same PartitionAheadOfLog error, causing the manager to park it again. This tells operators that restarting is an intervention when availability actually remains unchanged; list only actions that discard or replace the sealed store.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
## What - New `ProcessorState::Broken`. The PPM parks a processor on `ProcessorError::PartitionAheadOfLog` rather than restarting it with `RestartDelay::MaxBackoff`. Other blocked states (version/migration barrier, missing snapshot) keep retrying — those can resolve on their own. - Broken entries have no runtime task backing them, so they are dropped directly when the node leaves the partition's replica set and before `await_processors_termination` on shutdown. - Reported through the new `PartitionProcessorStatus::broken_reason` (`BrokenReason`, bilrost tag 18 / proto field 18). ## Why A sealed store cannot be repaired by retrying: the local data has to be dropped and replaced from a snapshot. Retrying every 30s only produced log noise and made a permanently broken partition indistinguishable from a flapping one. ## Observability - `restatectl partition list` → `Broken (ahead-of-log)` in `STATUS` - `restatectl status` → `2 (1 broken)` under `FOLLOWERS` - `sys_partition_state.broken_reason` (NULL while healthy)
## Why Storage-pool threads could still be inside `rocksdb::DBImpl::Open` when the process called `exit()`. C++ static destructors then freed rocksdb's option-registry statics - the enum lookup maps that `OptionTypeInfo` holds by pointer - and the in-flight open read freed memory while re-parsing the `OPTIONS-*` file it had just written. The result was a SIGSEGV on an `rs:io-lo` thread, seen as `proper_partition_processor_lifecycle` crashing roughly 1 run in 15 under parallel load. `shutdown()` did not prevent this. It closed the databases in `self.dbs`, waited for `close_db_tasks`, and joined rocksdb's *env* threads, but never joined our own storage pools. A database that is still being opened is also not yet registered in `self.dbs` - `open_db` inserts it only after `RocksDb::open` returns - so it escaped the close loop as well. ## What - `shutdown()` sets `shutting_down` itself instead of relying on `DbWatchdog` having observed the TaskCenter shutdown watch first. Direct callers (tests, `lite`) never set it, so opens were still being accepted. - New `join_storage_pools()` runs before draining `self.dbs`, so in-flight opens complete and register their database in time to be closed, and again after the close tasks, so no storage task is inside rocksdb when `shutdown()` returns. - Bounded by `Configuration::common.shutdown_grace_period()` (default 60s). On timeout it warns and continues: a stalled write should not hang the process, even though returning early re-opens the window. Note this makes shutdown wait for in-flight storage IO where it previously did not, so a busy node can take longer to stop. ## Evidence 0 failures and 0 new crash reports over 60 runs of the reproducing loop, against 1 in 15 before. Beyond that, no pool job can be queued or running once `shutdown()` returns, which is the precondition the crash needs.
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for hardening the shutdown logic of the RocksDB manager @AhmedSoliman. I think it makes the case less likely to happen while it is still possible because the threadpool can still accept more work after shutdown returns if a caller passed the shutting_down check before it's set to true and then waits to open the db until after the shutdown method returns. So the claim in the commit message is probably a bit strong but since this is only for tests, I think it's good to go.
| // Stop accepting new work. Submitters using the `*_unchecked` variants can still get | ||
| // through, which is why we join the pools below instead of relying on this alone. | ||
| self.shutting_down | ||
| .store(true, std::sync::atomic::Ordering::Release); |
There was a problem hiding this comment.
Relaxed is probably enough as we aren't publishing any other memory fields with this one. However, keeping it like it, because it's consistently used in db_manager.rs.
| // Wait for storage tasks that are already running before draining `self.dbs`: a database | ||
| // that is still being opened has not been registered there yet (see `open_db`), so it | ||
| // would otherwise escape the close loop entirely. | ||
| self.join_storage_pools().await; |
There was a problem hiding this comment.
I think this is a valid sequence in which the SIGSEGV can still occur. We probably would want to check whether we are shutting_down in the background task that runs the open db operation and additionally check whether we have shut down before adding the newly opened db to self.dbs.
Why
Storage-pool threads could still be inside
rocksdb::DBImpl::Openwhen theprocess called
exit(). C++ static destructors then freed rocksdb'soption-registry statics - the enum lookup maps that
OptionTypeInfoholds bypointer - and the in-flight open read freed memory while re-parsing the
OPTIONS-*file it had just written. The result was a SIGSEGV on anrs:io-lothread, seen as
proper_partition_processor_lifecyclecrashing roughly 1 run in15 under parallel load.
shutdown()did not prevent this. It closed the databases inself.dbs, waitedfor
close_db_tasks, and joined rocksdb's env threads, but never joined our ownstorage pools. A database that is still being opened is also not yet registered in
self.dbs-open_dbinserts it only afterRocksDb::openreturns - so itescaped the close loop as well.
What
shutdown()setsshutting_downitself instead of relying onDbWatchdoghaving observed the TaskCenter shutdown watch first. Direct callers (tests,
lite) never set it, so opens were still being accepted.join_storage_pools()runs before drainingself.dbs, so in-flight openscomplete and register their database in time to be closed, and again after the
close tasks, so no storage task is inside rocksdb when
shutdown()returns.Configuration::common.shutdown_grace_period()(default 60s). Ontimeout it warns and continues: a stalled write should not hang the process,
even though returning early re-opens the window.
Note this makes shutdown wait for in-flight storage IO where it previously did
not, so a busy node can take longer to stop.
Evidence
0 failures and 0 new crash reports over 60 runs of the reproducing loop, against
1 in 15 before. Beyond that, no pool job can be queued or running once
shutdown()returns, which is the precondition the crash needs.Stack created with Sapling. Best reviewed with ReviewStack.
restatectl partition drop-store#5114