Skip to content

Commit 216bc9d

Browse files
authored
sync: a pass that changed nothing must not rewrite state (#65)
Measured on the live fleet on 2026-08-25. Over one minute on Silber, with nothing changing: full_scans +7, inbound_noop_transactions +3, inbound_guarded_transactions +0, present +0, tombstones +0. In that same minute fabric rewrote a 45 MB state.json and a 25 MB manifest.json three times each: 213 MB a minute, 307 GB a day, on one idle machine. Across four machines it is about 2 TB a day. All of it rewriting what was already there. `sync_once` called `persist_entry` unguarded, once per pass. THIS FIX INVENTS NOTHING. The rule already existed on the other path. `prepare_inbound_entry` guards the identical call with a four-part condition and states the reason in its own comment: "an already durable no-op scan needs no rewrite". A test beside it asserts `persist_calls == 0` with "an already durable no-op generation must not rewrite state". So skipping the write on a no-op is not a change of contract. It is the contract, implemented on one of two paths. AND THE GUARD VALUE WAS ALREADY BEING COMPUTED AND DISCARDED. `scan_entry` returns `Result<bool>` saying whether the scan changed the node. This call site threw it away on the line above the write it should have gated. Each term of the guard earns its place: - `scan_changed`, which the scan already returned. - `observed != protected`, because materialization can move the disk receipt when the scan found nothing, for instance restoring a file deleted under catalog policy. Without this term such a restore would go unrecorded. - `durable_generation != generation`, for a watcher event not yet made durable. - `!state_path.exists()`, because a legacy entry has no state.json and its first pass must write even having changed nothing. Taken from the sibling. TESTS, AND I WATCHED THE FIRST ONE FAIL. Against the unguarded code, three no-op passes performed three persists; the test reports "left: 3, right: 0". The second test pins the property that makes this safe rather than the counter: after a real change followed by several no-op passes, the manifest ON DISK must still equal the one in memory. The dangerous version of this fix stops writing AND still marks the generation durable, so a crash loses what the skip declined to record. That test passes before the fix too, which is what makes it a control rather than a mirror. A RULE IMPLEMENTED IN ONE OF TWO PLACES IS WORTH LOOKING FOR ELSEWHERE IN THIS ENGINE. Agent: Silber.fabric
1 parent 007f891 commit 216bc9d

1 file changed

Lines changed: 124 additions & 2 deletions

File tree

src/sync/engine.rs

Lines changed: 124 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -787,9 +787,29 @@ impl<T: SyncTransport> SyncEngine<T> {
787787
let _operation = entry.operation.lock().await;
788788
let protected = entry.observed.lock().unwrap().clone();
789789
let generation = entry.work.mutation_generation.load(Ordering::Acquire);
790-
self.scan_entry(&entry).await?;
790+
let scan_changed = self.scan_entry(&entry).await?;
791791
self.materialize_entry_state(&entry, &protected).await?;
792-
self.persist_entry(&entry).await?;
792+
// A pass that changed nothing has nothing to record. This call was
793+
// unguarded while `prepare_inbound_entry` guards the identical one,
794+
// whose comment already states the rule: an already durable no-op
795+
// scan needs no rewrite. Applying it here, not inventing it.
796+
//
797+
// Each term earns its place:
798+
// - `scan_changed` is what the scan already returned and this call
799+
// site used to discard.
800+
// - `observed` can move when the scan found nothing, because
801+
// materialization restores a file deleted under catalog policy.
802+
// - the generation catches a watcher event not yet made durable.
803+
// - a legacy entry may have no state.json yet, so a first pass must
804+
// write even when it changed nothing.
805+
let observed_changed = { *entry.observed.lock().unwrap() != protected };
806+
if scan_changed
807+
|| observed_changed
808+
|| entry.work.durable_generation.load(Ordering::Acquire) != generation
809+
|| !self.state_path(&entry.config.name).exists()
810+
{
811+
self.persist_entry(&entry).await?;
812+
}
793813
entry.work.mark_generation_durable(generation);
794814
let baseline = entry.observed.lock().unwrap().clone();
795815
let manifest = entry.node.lock().await.manifest().clone();
@@ -3550,6 +3570,108 @@ mod tests {
35503570
write_named_sync(home, "catalog", folder, SyncPolicy::Catalog);
35513571
}
35523572

3573+
/// A pass that changes nothing must not rewrite state.
3574+
///
3575+
/// Measured on the live fleet on 2026-08-25: `sync_once` rewrote a 45 MB
3576+
/// state.json and a 25 MB manifest.json roughly every twenty seconds while
3577+
/// `full_scans` rose and `present`, `tombstones` and
3578+
/// `inbound_guarded_transactions` did not move at all. About 2 TB a day
3579+
/// across four machines, all of it re-writing what was already there.
3580+
///
3581+
/// The rule already existed on the inbound path, in
3582+
/// `prepare_inbound_entry`, whose own comment says "an already durable
3583+
/// no-op scan needs no rewrite". It was simply never applied here.
3584+
#[tokio::test]
3585+
async fn a_no_op_sync_pass_does_not_rewrite_state() {
3586+
let dir = tempfile::tempdir().unwrap();
3587+
let root = dir.path().join("resources");
3588+
std::fs::create_dir_all(&root).unwrap();
3589+
std::fs::write(root.join("a.md"), b"seed").unwrap();
3590+
write_bus_sync(dir.path(), &root);
3591+
3592+
let engine = SyncEngine::new(
3593+
FabricHome::new(dir.path()),
3594+
Author([1; 32]),
3595+
Arc::new(LoopbackTransport::default()),
3596+
CancellationToken::new(),
3597+
)
3598+
.await
3599+
.unwrap();
3600+
3601+
// The first pass discovers the file. It MUST persist.
3602+
engine.sync_once("bus").await.unwrap();
3603+
let entry = engine.entries.read().await.get("bus").cloned().unwrap();
3604+
assert!(
3605+
entry.work.persist_calls.load(Ordering::Relaxed) > 0,
3606+
"the first pass discovers a file and must write it"
3607+
);
3608+
entry.work.persist_calls.store(0, Ordering::Relaxed);
3609+
3610+
// Nothing changes on disk. These passes have nothing to record.
3611+
engine.sync_once("bus").await.unwrap();
3612+
engine.sync_once("bus").await.unwrap();
3613+
engine.sync_once("bus").await.unwrap();
3614+
3615+
assert_eq!(
3616+
entry.work.persist_calls.load(Ordering::Relaxed),
3617+
0,
3618+
"a pass that changed nothing must not rewrite state"
3619+
);
3620+
}
3621+
3622+
/// Skipping the write must not claim a durability we do not have.
3623+
///
3624+
/// The dangerous version of this fix is one that stops writing AND still
3625+
/// marks the generation durable, so a crash loses whatever the skipped
3626+
/// write would have carried. This asserts the property that matters rather
3627+
/// than the counter: after a no-op pass, what is ON DISK still equals what
3628+
/// is in memory.
3629+
#[tokio::test]
3630+
async fn a_skipped_write_leaves_the_persisted_state_still_correct() {
3631+
let dir = tempfile::tempdir().unwrap();
3632+
let root = dir.path().join("resources");
3633+
std::fs::create_dir_all(&root).unwrap();
3634+
std::fs::write(root.join("a.md"), b"seed").unwrap();
3635+
write_bus_sync(dir.path(), &root);
3636+
3637+
let engine = SyncEngine::new(
3638+
FabricHome::new(dir.path()),
3639+
Author([1; 32]),
3640+
Arc::new(LoopbackTransport::default()),
3641+
CancellationToken::new(),
3642+
)
3643+
.await
3644+
.unwrap();
3645+
engine.sync_once("bus").await.unwrap();
3646+
3647+
// A real local change, then several no-op passes on top of it.
3648+
std::fs::write(root.join("b.md"), b"second").unwrap();
3649+
engine.sync_once("bus").await.unwrap();
3650+
engine.sync_once("bus").await.unwrap();
3651+
engine.sync_once("bus").await.unwrap();
3652+
3653+
let in_memory = engine
3654+
.node_for("bus")
3655+
.await
3656+
.unwrap()
3657+
.lock()
3658+
.await
3659+
.manifest()
3660+
.clone();
3661+
let raw = std::fs::read(engine.state_path("bus")).unwrap();
3662+
let on_disk: PersistedEntryState = serde_json::from_slice(&raw).unwrap();
3663+
3664+
assert_eq!(
3665+
on_disk.manifest, in_memory,
3666+
"after skipping writes, the persisted manifest must still equal the \
3667+
live one, or a crash loses the change the skip decided not to record"
3668+
);
3669+
assert!(
3670+
on_disk.manifest.get("b.md").is_some(),
3671+
"the real change must be on disk, not merely in memory"
3672+
);
3673+
}
3674+
35533675
fn write_bus_sync(home: &Path, folder: &Path) {
35543676
write_named_sync(home, "bus", folder, SyncPolicy::Bus);
35553677
}

0 commit comments

Comments
 (0)