-
Notifications
You must be signed in to change notification settings - Fork 980
fix: abort chain compaction cleanly on node shutdown #3895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ | |
| //! Adapters connecting new block, new transaction, and accepted transaction | ||
| //! events to consumers of those events. | ||
|
|
||
| use crate::util::RwLock; | ||
| use crate::util::{RwLock, StopState}; | ||
| use std::collections::HashMap; | ||
| use std::fs::File; | ||
| use std::net::SocketAddr; | ||
|
|
@@ -76,6 +76,8 @@ where | |
| config: ServerConfig, | ||
| hooks: Vec<Box<dyn NetEvents + Send + Sync>>, | ||
| header_segment_requests: RwLock<HashMap<SocketAddr, (DateTime<Utc>, usize)>>, | ||
| /// Shared shutdown flag so compaction can abort cleanly (#3842). | ||
| stop_state: Arc<StopState>, | ||
| tx: mpsc::SyncSender<NetAdapterWorkerMessage>, | ||
| } | ||
|
|
||
|
|
@@ -698,6 +700,7 @@ where | |
| tx_pool: Arc<RwLock<pool::TransactionPool<B, P>>>, | ||
| config: ServerConfig, | ||
| hooks: Vec<Box<dyn NetEvents + Send + Sync>>, | ||
| stop_state: Arc<StopState>, | ||
| ) -> Self { | ||
| let (tx, rx) = mpsc::sync_channel(WORKER_CHANNEL_BUFFER_SIZE); | ||
| let adapter = NetToChainAdapter { | ||
|
|
@@ -708,6 +711,7 @@ where | |
| config, | ||
| hooks, | ||
| header_segment_requests: RwLock::new(HashMap::new()), | ||
| stop_state, | ||
| tx, | ||
| }; | ||
| adapter.spawn_net_adapter_worker(Arc::downgrade(&chain), rx); | ||
|
|
@@ -966,15 +970,25 @@ where | |
| } | ||
|
|
||
| fn check_compact(&self) { | ||
| // Do not start long-running compaction while the node is shutting down. | ||
| if self.stop_state.is_stopped() { | ||
| return; | ||
| } | ||
|
|
||
| // Roll the dice to trigger compaction at 1/COMPACTION_CHECK chance per block, | ||
| // uses a different thread to avoid blocking the caller thread (likely a peer) | ||
| let mut rng = thread_rng(); | ||
| if 0 == rng.gen_range(0, global::COMPACTION_CHECK) { | ||
| let chain = self.chain(); | ||
| let stop_state = self.stop_state.clone(); | ||
| let _ = thread::Builder::new() | ||
| .name("compactor".to_string()) | ||
| .spawn(move || { | ||
| if let Err(e) = chain.compact() { | ||
| if stop_state.is_stopped() { | ||
| debug!("compactor: not starting, node is stopping"); | ||
| return; | ||
| } | ||
| if let Err(e) = chain.compact_with_stop(Some(stop_state)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| error!("Could not compact chain: {:?}", e); | ||
| } | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -414,8 +414,28 @@ impl<T: PMMRable> PMMRBackend<T> { | |
| /// will have a suitable output_pos. This is used to enforce a horizon | ||
| /// after which the local node should have all the data to allow rewinding. | ||
| pub fn check_compact(&mut self, cutoff_pos: u64, rewind_rm_pos: &Bitmap) -> io::Result<bool> { | ||
| self.check_compact_until(cutoff_pos, rewind_rm_pos, || false) | ||
| } | ||
|
|
||
| /// 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>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small naming thought, until sounds like a position limit. Would check_compact_with_abort describe this callback more clearly? |
||
| &mut self, | ||
| cutoff_pos: u64, | ||
| rewind_rm_pos: &Bitmap, | ||
| should_abort: F, | ||
| ) -> io::Result<bool> | ||
| where | ||
| F: Fn() -> bool, | ||
| { | ||
| assert!(self.prunable, "Trying to compact a non-prunable PMMR"); | ||
|
|
||
| if should_abort() { | ||
| debug!("compact: aborted before writing tmp files"); | ||
| return Ok(false); | ||
| } | ||
|
|
||
| // Calculate the sets of leaf positions and node positions to remove based | ||
| // on the cutoff_pos provided. | ||
| let (leaves_removed, pos_to_rm) = self.pos_to_rm(cutoff_pos, rewind_rm_pos); | ||
|
|
@@ -447,6 +467,15 @@ impl<T: PMMRable> PMMRBackend<T> { | |
| self.data_file.write_tmp_pruned(&pos_to_rm)?; | ||
| } | ||
|
|
||
| // Critical section: do not replace live files if we are shutting down. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| // Leaving a half-applied replace causes Invalid Root on next start (#3842). | ||
| if should_abort() { | ||
| info!("compact: aborted before replace; discarding tmp files"); | ||
| self.hash_file.discard_tmp(); | ||
| self.data_file.discard_tmp(); | ||
| return Ok(false); | ||
| } | ||
|
|
||
| // Replace hash and data files with compact copies. | ||
| // Rebuild and intialize from the new files. | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ use crate::grin_core::ser::{ | |
| self, BinWriter, DeserializationMode, ProtocolVersion, Readable, Reader, StreamingReader, | ||
| Writeable, Writer, | ||
| }; | ||
| use log::warn; | ||
| use std::fmt::Debug; | ||
| use std::fs::{self, File, OpenOptions}; | ||
| use std::io::{self, BufReader, BufWriter, Seek, SeekFrom, Write}; | ||
|
|
@@ -160,6 +161,11 @@ where | |
| self.file.write_tmp_pruned(prune_idx.as_slice()) | ||
| } | ||
|
|
||
| /// Discard any temporary pruned file without applying it. | ||
| pub fn discard_tmp(&self) { | ||
| self.file.discard_tmp() | ||
| } | ||
|
|
||
| /// Replace with file at tmp path. | ||
| /// Rebuild and initialize from new file. | ||
| pub fn replace_with_tmp(&mut self) -> io::Result<()> { | ||
|
|
@@ -521,6 +527,16 @@ where | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Drop any `.tmp` companion file without replacing the live file. | ||
| pub fn discard_tmp(&self) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this return io::Result<()>? Otherwise the caller reports a clean abort even when tmp cleanup fails. |
||
| let tmp = self.tmp_path(); | ||
| if tmp.exists() { | ||
| if let Err(e) = fs::remove_file(&tmp) { | ||
| warn!("discard_tmp: failed to remove {:?}: {}", tmp, e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Replace the underlying file with the file at tmp path. | ||
| /// Rebuild and initialize from the new file. | ||
| pub fn replace_with_tmp(&mut self) -> io::Result<()> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,6 +214,36 @@ fn pmmr_compact_leaf_sibling() { | |
| teardown(data_dir); | ||
| } | ||
|
|
||
| /// Aborting compaction before replace must leave live files intact (#3842). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we keep this focused on the invariant and leave the issue reference in the PR? |
||
| #[test] | ||
| fn pmmr_compact_abort_before_replace() { | ||
| let (data_dir, elems) = setup("compact_abort"); | ||
| { | ||
| let mut backend = | ||
| store::pmmr::PMMRBackend::new(data_dir.to_string(), true, ProtocolVersion(1), None) | ||
| .unwrap(); | ||
| let mmr_size = load(0, &elems[..], &mut backend); | ||
| backend.sync().unwrap(); | ||
| let root_before = { | ||
| let pmmr = PMMR::at(&mut backend, mmr_size); | ||
| pmmr.root().unwrap() | ||
| }; | ||
|
|
||
| // Always abort: should not replace live files. | ||
| let done = backend | ||
| .check_compact_until(2, &Bitmap::new(), || true) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. || 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? |
||
| .unwrap(); | ||
| assert!(!done); | ||
|
|
||
| let root_after = { | ||
| let pmmr = PMMR::at(&mut backend, mmr_size); | ||
| pmmr.root().unwrap() | ||
| }; | ||
| assert_eq!(root_before, root_after); | ||
| } | ||
| teardown(data_dir); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pmmr_prune_compact() { | ||
| let (data_dir, elems) = setup("prune_compact"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The owner API still reaches this uncancellable path. Could an API-triggered compaction hit the same shutdown problem?