fix: abort chain compaction cleanly on node shutdown - #3895
Conversation
Compaction could keep rewriting PMMR files after stop was requested, and killing the process mid-replace led to Invalid Root on next launch. - Thread StopState into compact from NetToChainAdapter and syncer - Skip starting compact when already stopping - Before replacing live PMMR files, if stopping, discard .tmp files and leave the existing chain data untouched Addresses mimblewimble#3842.
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
Good direction, but shutdown still needs to wait for the replace phase, and the test should exercise the actual tmp discard path.
| self.data_file.write_tmp_pruned(&pos_to_rm)?; | ||
| } | ||
|
|
||
| // Critical section: do not replace live files if we are shutting down. |
There was a problem hiding this comment.
A stop can arrive immediately after this check, while the files are replaced separately. Is shutdown guaranteed to wait until the whole replace section finishes?
There was a problem hiding this comment.
Done in b12bfca. Once the pre-replace abort check decides to proceed, replace always runs to completion (no further abort polls). Shutdown now joins the compactor thread (CompactorTracker / Server::stop) so the process does not exit mid-replace.
| debug!("compactor: not starting, node is stopping"); | ||
| return; | ||
| } | ||
| if let Err(e) = chain.compact_with_stop(Some(stop_state)) { |
There was a problem hiding this comment.
This compactor still runs on a detached thread, so shutdown cannot wait if it has already entered the replace phase. Could the thread be tracked and joined?
There was a problem hiding this comment.
Done in b12bfca. Added CompactorTracker that stores the JoinHandle. The adapter reaps finished handles and only starts one compact at a time; Server::stop joins the tracker after signalling stop so any in-flight compact can finish or abort cleanly.
|
|
||
| // Always abort: should not replace live files. | ||
| let done = backend | ||
| .check_compact_until(2, &Bitmap::new(), || true) |
There was a problem hiding this comment.
|| true trips the first check before any tmp file is written, so the discard path never runs. Could it return false first, then true, and verify that no tmp files remain?
There was a problem hiding this comment.
Done in b12bfca. The abort callback returns false on the first call (allow tmp write) and true on the second (abort before replace). The test asserts the second check ran and that no .tmp files remain under the data dir.
| /// * removes historical blocks and associated data from the db (unless archive mode) | ||
| /// | ||
| pub fn compact(&self) -> Result<(), Error> { | ||
| self.compact_with_stop(None) |
There was a problem hiding this comment.
The owner API still reaches this uncancellable path. Could an API-triggered compaction hit the same shutdown problem?
There was a problem hiding this comment.
Done in b12bfca. stop_state is plumbed into Owner / ChainCompactHandler; API compact uses compact_with_stop so a shutdown during an owner-triggered compact can abort before replace the same way as the background path.
| } | ||
|
|
||
| /// Drop any `.tmp` companion file without replacing the live file. | ||
| pub fn discard_tmp(&self) { |
There was a problem hiding this comment.
Could this return io::Result<()>? Otherwise the caller reports a clean abort even when tmp cleanup fails.
There was a problem hiding this comment.
Done in b12bfca. discard_tmp now returns io::Result<()>, and the abort path propagates cleanup failures with ? instead of treating them as a clean abort.
| /// Compact backend files, optionally aborting before live files are replaced | ||
| /// when `should_abort` returns true (e.g. node shutdown). Aborted compaction | ||
| /// discards `.tmp` files and leaves the live PMMR files untouched (#3842). | ||
| pub fn check_compact_until<F>( |
There was a problem hiding this comment.
Small naming thought, until sounds like a position limit. Would check_compact_with_abort describe this callback more clearly?
There was a problem hiding this comment.
Done in b12bfca. Renamed to check_compact_with_abort (and compact_with_abort on the txhashset) so it is clear the callback is an abort flag, not a position limit.
| teardown(data_dir); | ||
| } | ||
|
|
||
| /// Aborting compaction before replace must leave live files intact (#3842). |
There was a problem hiding this comment.
Could we keep this focused on the invariant and leave the issue reference in the PR?
There was a problem hiding this comment.
Done in b12bfca. Test comment now states the invariant only (abort after tmp write must discard tmp and leave live roots intact); the issue reference stays in the PR description.
Track and join the background compactor on shutdown so replace can finish, plumb stop_state into owner compact, rename the abort API, make discard_tmp fallible, and exercise the tmp-discard path in tests.
2 participants
Summary
Fixes #3842.
Long compaction runs (especially on low-RAM machines / startup) continued after the node was asked to stop. Killing the process while live PMMR files were being replaced left the chain with Invalid Root on the next launch.
Changes
NetToChainAdapterStopState; does not spawn compactors when stopped; passes stop into compactSyncercompact_with_stopon transition to NoSyncChain::compact_with_stopPMMRBackend::check_compact_until.tmpfiles, if aborting: discard tmp, do not replace live filesAppendOnlyFile::discard_tmp.tmpcompanionsChain::compact()remains for API/tests (no stop handle).Test plan
cargo test -p grin_store --test pmmr pmmr_compact_abort_before_replacecargo test -p grin_store --test pmmr -- --test-threads=1cargo test -p grin_chain --test mine_simple_chain compact -- --test-threads=1cargo test -p grin_servers --lib -- --test-threads=1