Skip to content

fix(gc): schedule automatic GC only when the node is in sync #6165

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.

Sign up for GitHub

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

Merged
hanabi1224 merged 4 commits into from
Oct 16, 2025
Merged

fix(gc): schedule automatic GC only when the node is in sync #6165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
863ddf8
fix(gc): schedule automatic GC only when the node is in sync
hanabi1224 Oct 15, 2025
da3f9f6
resolve comments
hanabi1224 Oct 15, 2025
4000863
comment on each condition
hanabi1224 Oct 15, 2025
710f042
simplify type
hanabi1224 Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions src/daemon/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ pub mod main;
use crate::blocks::Tipset;
use crate::chain::HeadChange;
use crate::chain::index::ResolveNullTipset;
use crate::chain_sync::ChainFollower;
use crate::chain_sync::network_context::SyncNetworkContext;
use crate::chain_sync::{ChainFollower, SyncStatusReport};
use crate::cli_shared::snapshot;
use crate::cli_shared::{
chain_path,
Expand All @@ -36,6 +36,7 @@ use crate::utils::{proofs_api::ensure_proof_params_downloaded, version::FOREST_V
use anyhow::{Context as _, bail};
use dialoguer::theme::ColorfulTheme;
use futures::{Future, FutureExt, select};
use parking_lot::RwLock;
use std::path::Path;
use std::sync::Arc;
use std::sync::OnceLock;
Expand Down Expand Up @@ -561,8 +562,9 @@ pub(super) async fn start(
_ = snap_gc_reboot_rx.recv_async() => {
snap_gc.cleanup_before_reboot().await;
}
result = start_services(start_time, &opts, config.clone(), shutdown_send.clone(), |ctx| {
result = start_services(start_time, &opts, config.clone(), shutdown_send.clone(), |ctx, sync_status| {
snap_gc.set_db(ctx.db.clone());
snap_gc.set_sync_status(sync_status);
snap_gc.set_car_db_head_epoch(ctx.db.heaviest_tipset().map(|ts|ts.epoch()).unwrap_or_default());
}) => {
break result
Expand All @@ -576,7 +578,7 @@ pub(super) async fn start_services(
opts: &CliOpts,
mut config: Config,
shutdown_send: mpsc::Sender<()>,
on_app_context_and_db_initialized: impl Fn(&AppContext),
on_app_context_and_db_initialized: impl Fn(&AppContext, Arc<RwLock<SyncStatusReport>>),
Comment thread
akaladarshi marked this conversation as resolved.
Outdated
) -> anyhow::Result<()> {
// Cleanup the collector prometheus metrics registry on start
crate::metrics::reset_collector_registry();
Expand Down Expand Up @@ -608,7 +610,7 @@ pub(super) async fn start_services(
services.shutdown().await;
return Ok(());
}
on_app_context_and_db_initialized(&ctx);
on_app_context_and_db_initialized(&ctx, chain_follower.sync_status.clone());
warmup_in_background(&ctx);
ctx.state_manager.populate_cache();
maybe_start_metrics_service(&mut services, &config, &ctx).await?;
Expand Down
31 changes: 23 additions & 8 deletions src/db/gc/snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ pub struct SnapshotGarbageCollector<DB> {
running: AtomicBool,
blessed_lite_snapshot: RwLock<Option<PathBuf>>,
db: RwLock<Option<Arc<DB>>>,
sync_status: RwLock<Option<Arc<RwLock<crate::chain_sync::SyncStatusReport>>>>,

@akaladarshi akaladarshi Oct 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this will be better here?

RwLock<Option<Arc<RwLock<crate::chain_sync::SyncStatusReport>>>> -> OnceLock<Arc<RwLock<crate::chain_sync::SyncStatusReport>>>

This way we can avoid doing: &*sync_status.read()

@hanabi1224 hanabi1224 Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's set more than once after every GC

@akaladarshi akaladarshi Oct 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I missed this GC restarts the node.

@elmattic elmattic Oct 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need two RwLocks here?

@elmattic elmattic Oct 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify this type?

@hanabi1224 hanabi1224 Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elmattic simplified type by adding pub type SyncStatus = Arc<RwLock<SyncStatusReport>>;

// On mainnet, it takes ~50MiB-200MiB RAM, depending on the time cost of snapshot export
memory_db: RwLock<Option<HashMap<Cid, Vec<u8>>>>,
memory_db_head_key: RwLock<Option<TipsetKey>>,
Expand Down Expand Up @@ -111,6 +112,7 @@ where
running: AtomicBool::new(false),
blessed_lite_snapshot: RwLock::new(None),
db: RwLock::new(None),
sync_status: RwLock::new(None),
memory_db: RwLock::new(None),
memory_db_head_key: RwLock::new(None),
exported_head_key: RwLock::new(None),
Expand All @@ -132,6 +134,10 @@ where
*self.car_db_head_epoch.write() = Some(epoch);
}

pub fn set_sync_status(&self, sync_status: Arc<RwLock<crate::chain_sync::SyncStatusReport>>) {
*self.sync_status.write() = Some(sync_status)
}

pub async fn event_loop(&self) {
while self.trigger_rx.recv_async().await.is_ok() {
if self.running.load(Ordering::Relaxed) {
Comment thread
akaladarshi marked this conversation as resolved.
Expand Down Expand Up @@ -170,18 +176,22 @@ where
);
loop {
if !self.running.load(Ordering::Relaxed)
&& let Some(db) = &*self.db.read()
&& let Some(car_db_head_epoch) = *self.car_db_head_epoch.read()
&& let Ok(head_key) = HeaviestTipsetKeyProvider::heaviest_tipset_key(db)
&& let Ok(head) = Tipset::load_required(db, &head_key)
&& let Some(sync_status) = &*self.sync_status.read()
{
let head_epoch = head.epoch();
if head_epoch - car_db_head_epoch >= snap_gc_interval_epochs
const IN_SYNC_EPOCH_THRESHOLD: i64 = 2;

@hanabi1224 hanabi1224 Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice sync_status.status does not refect the reality after a long catchup (potential bug?), so checking head_epoch + IN_SYNC_EPOCH_THRESHOLD >= network_head_epoch manually here

@akaladarshi akaladarshi Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a potential bug SyncStatusReport, I will file an issue for it.

@akaladarshi akaladarshi Oct 15, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because there we are using time diff only:

let time_diff = now_ts.saturating_sub(heaviest.min_timestamp());


if time_diff < seconds_per_epoch as u64 * SYNCED_EPOCH_THRESHOLD {
 NodeSyncStatus::Synced
} else {
  NodeSyncStatus::Syncing
}

But you can use

if sync_status.is_synced() && sync_status.active_forks.is_empty()

@hanabi1224 hanabi1224 Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment thread
akaladarshi marked this conversation as resolved.
Outdated
let sync_status = &*sync_status.read();
let network_head_epoch = sync_status.network_head_epoch;
let head_epoch = sync_status.current_head_epoch;
if head_epoch > 0

@elmattic elmattic Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this test condition is quite a beast, maybe it's time to break it down into functions and add some comments.

@hanabi1224 hanabi1224 Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added comment to each condition

&& head_epoch <= network_head_epoch
&& head_epoch + IN_SYNC_EPOCH_THRESHOLD >= network_head_epoch
&& head_epoch - car_db_head_epoch >= snap_gc_interval_epochs
&& self.trigger_tx.try_send(()).is_ok()
{
tracing::info!(%car_db_head_epoch, %head_epoch, %snap_gc_interval_epochs, "Snap GC scheduled");
tracing::info!(%car_db_head_epoch, %head_epoch, %network_head_epoch, %snap_gc_interval_epochs, "Snap GC scheduled");
} else {
tracing::trace!(%car_db_head_epoch, %head_epoch, %snap_gc_interval_epochs, "Snap GC not scheduled");
tracing::debug!(%car_db_head_epoch, %head_epoch, %network_head_epoch, %snap_gc_interval_epochs, "Snap GC not scheduled");
}
}
tokio::time::sleep(snap_gc_check_interval).await;
Expand Down Expand Up @@ -219,6 +229,7 @@ where
}
map
});
let start = Instant::now();
let (head_ts, _) = crate::chain::export_from_head::<Sha256>(
&db,
self.recent_state_roots,
Expand All @@ -235,7 +246,11 @@ where
head_ts.epoch()
));
temp_path.persist(&target_path)?;
tracing::info!("exported lite snapshot at {}", target_path.display());
tracing::info!(
"exported lite snapshot at {}, took {}",
target_path.display(),
humantime::format_duration(start.elapsed())
);
*self.blessed_lite_snapshot.write() = Some(target_path);
*self.exported_head_key.write() = Some(head_ts.key().clone());

Expand Down
Loading