From cebb0cc3f09bed10ef0c3164cc968c733c4240e5 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Mon, 14 Sep 2026 12:37:23 -0400 Subject: [PATCH] fix(pds)!: refuse a delete the log will not take A removal applied in memory alone came back at the next boot, after the caller was told it happened and subscribeRepos carried a delete op naming it; RecordStore::remove_repo can now refuse too, so erasure is retried. Change-Id: Iba11a5f570fad529be5cd333fb0b3e020a34d546 --- crates/didbot-pds/src/durable.rs | 90 +++++++++++------------- crates/didbot-pds/src/provision.rs | 37 +++++++--- crates/didbot-pds/src/records.rs | 17 +++-- crates/didbot-pds/tests/authored.rs | 4 +- crates/didbot-pds/tests/judged_record.rs | 4 +- crates/didbot-pds/tests/records.rs | 4 +- crates/didbot-pds/tests/refusal.rs | 62 ++++++++++++++++ 7 files changed, 152 insertions(+), 66 deletions(-) diff --git a/crates/didbot-pds/src/durable.rs b/crates/didbot-pds/src/durable.rs index 5c2ef9f9..a7d2d405 100644 --- a/crates/didbot-pds/src/durable.rs +++ b/crates/didbot-pds/src/durable.rs @@ -647,7 +647,10 @@ fn apply( // is not any one store's, and two entries for it could disagree after a // crash. if let Entry::RepoRemoved { did } = &entry { - records.remove_repo(did); + // The stores here hold what is in memory, and this is the replay of + // an entry that is already on the medium: there is nothing left for + // any of the three to refuse. + let _ = records.remove_repo(did); blobs.remove_repo(did); commits.forget(did); return; @@ -1362,10 +1365,11 @@ impl RecordStore for FileRecordStore { /// a history of what happened, and a no-op did not happen; logging it /// would grow the file every time a client made sure of something. /// - /// Synced rather than deferred, unlike [`RecordStore::put`]. The two are - /// not symmetrical: a lost write is one record of a stream the agent is - /// still producing, and a lost *deletion* is a record coming back from - /// the dead after the caller was told it was gone. + /// Synced rather than deferred, unlike [`RecordStore::put`], and refused + /// rather than applied when the log will not take it. The two are not + /// symmetrical: a lost write is one record of a stream the agent is still + /// producing, and a lost *deletion* is a record coming back from the dead + /// after the caller was told it was gone. fn remove( &self, did: &str, @@ -1382,21 +1386,14 @@ impl RecordStore for FileRecordStore { let Some(held) = held else { return Ok(false); }; - if let Err(error) = self - .wal + // Refused rather than applied in memory alone. A record dropped over + // an append that did not land comes back at the next boot, and by + // then the caller has been told it went and `subscribeRepos` has + // carried a `delete` op naming it — so this server would be serving a + // record the network believes is gone, with nothing that can say so. + self.wal .append(&removal(did, collection, rkey, held), Durability::Sync) - { - // The same posture as `remove_repo`: the trait cannot report - // this, and refusing to delete would be worse than a record that - // comes back on restart with a loud line saying why. - tracing::error!( - did, - collection, - rkey, - error = %backend(error), - "could not write a record deletion to the log; it will come back on restart" - ); - } + .map_err(record_backend)?; Ok(self.inner.delete(did, collection, rkey).is_some()) } @@ -1408,27 +1405,25 @@ impl RecordStore for FileRecordStore { self.inner.snapshot(did) } - fn remove_repo(&self, did: &str) { + /// Logged, then applied, and refused if the log will not take it. + /// + /// Dropping the repository in memory over an append that did not land + /// would be this deployment telling a caller the account was erased, + /// saying so on the wire, and serving every one of those records again + /// after the next restart with nothing to say it had. The erasure is + /// idempotent, so a refusal here is one a caller can act on by asking + /// again. + fn remove_repo(&self, did: &str) -> Result<(), RecordError> { let _write = self.write(); - // The only mutation here that cannot report a failure, because the - // trait says so — a deletion the caller is already committed to. A log - // failure is loud and the removal happens anyway: leaving the records - // behind would attribute them to a DID that no longer resolves, and - // the next startup would replay them into a repository with no - // account. - if let Err(error) = self.wal.append( - &Entry::RepoRemoved { - did: did.to_owned(), - }, - Durability::Sync, - ) { - tracing::error!( - did, - error = %backend(error), - "could not write a repository deletion to the log; it will come back on restart" - ); - } - self.inner.remove_repo(did); + self.wal + .append( + &Entry::RepoRemoved { + did: did.to_owned(), + }, + Durability::Sync, + ) + .map_err(record_backend)?; + self.inner.remove_repo(did) } fn stats(&self) -> RecordStats { @@ -1487,18 +1482,13 @@ impl RecordStore for FileRecordStore { } None => { if let Some(held) = self.inner.held_at(did, &collection, &rkey) { - if let Err(error) = self - .wal + // Refused, for the reason `RecordStore::remove` + // gives: a deletion that did not reach the log is one + // the next boot undoes, after the caller and the + // stream have both been told it happened. + self.wal .append(&removal(did, &collection, &rkey, held), Durability::Sync) - { - tracing::error!( - did, - collection, - rkey, - error = %backend(error), - "could not write a batched record deletion to the log; it will come back on restart" - ); - } + .map_err(record_backend)?; self.inner.delete(did, &collection, &rkey); } results.push(BatchOutcome::Deleted); diff --git a/crates/didbot-pds/src/provision.rs b/crates/didbot-pds/src/provision.rs index d6397a03..6cbb8a2f 100644 --- a/crates/didbot-pds/src/provision.rs +++ b/crates/didbot-pds/src/provision.rs @@ -2605,9 +2605,7 @@ where // Nothing external happened — no name was issued and no hostname // was published, because the apex is a name this deployment is // already served on — so the whole of the rollback is the seed. - self.records.remove_repo(did.as_str()); - self.history.forget(did.as_str()); - self.forget_repository(did.as_str()); + self.discard_repository(did.as_str()); tracing::error!(%error, "could not store the server account"); return Err(error.into()); } @@ -3425,6 +3423,26 @@ where Ok(fresh.to_car() == served.to_car()) } + /// Drops the repository of an account that was never made, on a path that + /// is already failing. + /// + /// The unwind, where the caller has an error to return whatever happens + /// here. A removal the log refuses leaves records under a DID no account + /// claims, which the line names so an operator can find them; nothing + /// outside this process was ever told the account existed, so there is no + /// consumer for the refusal to reach. + fn discard_repository(&self, did: &str) { + if let Err(error) = self.records.remove_repo(did) { + tracing::error!( + did, + %error, + "could not discard the repository of an account that was not made" + ); + } + self.history.forget(did); + self.forget_repository(did); + } + /// Drops what was built for `did`, because its records are gone. /// /// Called beside every `CommitStore::forget`. Belt and braces rather than @@ -5001,7 +5019,12 @@ where // included, so every signature this agent ever made stays checkable. // Every step here is idempotent, which is what lets a resumed // erasure run them again. - self.records.remove_repo(account.did.as_str()); + // Refused rather than applied in memory alone: the announcement + // below and the `Decommissioned` transition both follow this, and a + // repository that came back at the next boot would have been erased + // as far as every consumer is concerned. The erasure is idempotent, + // so the caller's remedy is to ask again. + self.records.remove_repo(account.did.as_str())?; self.history.forget(account.did.as_str()); self.forget_repository(account.did.as_str()); self.blobs.remove_repo(account.did.as_str()); @@ -5117,7 +5140,7 @@ where // on the store would otherwise have already discarded the records of // an account that still exists. Already gone for an erased account; // still here for one reaped before it was activated. - self.records.remove_repo(removed.did.as_str()); + self.records.remove_repo(removed.did.as_str())?; // And what this server remembered about committing to it. A history // outliving its repository would let a later account minted at the // same DID inherit a head naming blocks nothing holds. @@ -6239,9 +6262,7 @@ where Ok(deferred) => deferred, Err(error) => { tracing::info!(%error, "could not publish the registration record; provisioning nothing"); - self.records.remove_repo(did.as_str()); - self.history.forget(did.as_str()); - self.forget_repository(did.as_str()); + self.discard_repository(did.as_str()); unwind(&published); return Err(error); } diff --git a/crates/didbot-pds/src/records.rs b/crates/didbot-pds/src/records.rs index 9a285532..bf52a103 100644 --- a/crates/didbot-pds/src/records.rs +++ b/crates/didbot-pds/src/records.rs @@ -704,7 +704,12 @@ pub trait RecordStore: Send + Sync { /// Called when an account is deleted. Records outliving the identity that /// wrote them would be unattributable and unreachable: the DID no longer /// resolves, so nothing can say who wrote them or fetch them back. - fn remove_repo(&self, did: &str); + /// + /// Refuses rather than dropping them in memory alone. A durable store + /// that could not write the removal down would serve the repository again + /// at the next boot, and the caller has already been told the account was + /// erased and has said so on the wire. + fn remove_repo(&self, did: &str) -> Result<(), RecordError>; /// What this store holds, counted. fn stats(&self) -> RecordStats; @@ -1528,9 +1533,10 @@ impl RecordStore for MemoryRecordStore { .collect() } - fn remove_repo(&self, did: &str) { + fn remove_repo(&self, did: &str) -> Result<(), RecordError> { self.repos().remove(did); self.tombstones().remove_repo(did); + Ok(()) } fn stats(&self) -> RecordStats { @@ -1779,7 +1785,9 @@ mod tombstone_tests { for (which, store) in stores("repo") { put(store.as_ref(), "quernstone", "first"); delete(store.as_ref(), "quernstone"); - store.remove_repo(DID); + store + .remove_repo(DID) + .expect("a resident store drops what it holds"); assert_eq!(store.tombstone(DID, THING, "quernstone"), None, "{which}"); } } @@ -2568,9 +2576,10 @@ impl RecordStore for HeapRecordStore { .collect() } - fn remove_repo(&self, did: &str) { + fn remove_repo(&self, did: &str) -> Result<(), RecordError> { self.repos().remove(did); self.tombstones().remove_repo(did); + Ok(()) } /// What is held, counted — with the byte total counted as stored bytes. diff --git a/crates/didbot-pds/tests/authored.rs b/crates/didbot-pds/tests/authored.rs index 9ff1014a..d8551a95 100644 --- a/crates/didbot-pds/tests/authored.rs +++ b/crates/didbot-pds/tests/authored.rs @@ -406,7 +406,9 @@ fn dropping_a_whole_repository_still_takes_it() { .put_authored(DID, nsid::REGISTRATION, well_formed()) .expect("write"); assert_eq!(store.stats().records, 1); - store.remove_repo(DID); + store + .remove_repo(DID) + .expect("a resident store drops what it holds"); assert_eq!(store.stats().records, 0); } diff --git a/crates/didbot-pds/tests/judged_record.rs b/crates/didbot-pds/tests/judged_record.rs index 2ff9223f..8de1afd1 100644 --- a/crates/didbot-pds/tests/judged_record.rs +++ b/crates/didbot-pds/tests/judged_record.rs @@ -124,8 +124,8 @@ impl RecordStore for RacingStore { self.inner.snapshot(did) } - fn remove_repo(&self, did: &str) { - self.inner.remove_repo(did); + fn remove_repo(&self, did: &str) -> Result<(), RecordError> { + self.inner.remove_repo(did) } fn stats(&self) -> RecordStats { diff --git a/crates/didbot-pds/tests/records.rs b/crates/didbot-pds/tests/records.rs index 4c0d2312..f31e378a 100644 --- a/crates/didbot-pds/tests/records.rs +++ b/crates/didbot-pds/tests/records.rs @@ -634,7 +634,9 @@ fn the_store_validates_without_a_registry_in_front_of_it() { // A store that never saw a repository lists nothing rather than failing: // existence is a question about accounts, not about records. assert!(store.list(did, THING, &ListParams::new(10)).is_empty()); - store.remove_repo(did); + store + .remove_repo(did) + .expect("a resident store drops a repository"); assert_eq!(store.stats().records, 0); } diff --git a/crates/didbot-pds/tests/refusal.rs b/crates/didbot-pds/tests/refusal.rs index 4cf840a3..e63f27ba 100644 --- a/crates/didbot-pds/tests/refusal.rs +++ b/crates/didbot-pds/tests/refusal.rs @@ -213,6 +213,68 @@ fn a_full_log_refuses_and_replays_everything_it_acknowledged() { assert_eq!(stored(&durable), accepted + 1); } +/// A delete a full log cannot take is refused, not applied in memory. +/// +/// The direction that costs more than a lost write. A record dropped over an +/// append that did not land comes back at the next boot, and by then the +/// caller has been told it went and `subscribeRepos` has carried a `delete` +/// op naming it: this server would be serving a record the network believes +/// is gone, with nothing in either stream able to say so. +#[test] +fn a_delete_a_full_log_cannot_take_is_refused_rather_than_undone_by_a_restart() { + let dir = Dir::new("delete-full"); + let durable = open(&dir); + let records = durable.records(); + + let rkey = write(&*records, "quernstone").expect("a first write"); + let wal = durable.wal(); + // No room for anything at all, the removal entry included. + wal.set_capacity(Some(wal.len())); + + let refusal = records + .remove(DID, COLLECTION, &rkey, &Precondition::Unconditional) + .expect_err("a delete the log could not take was acknowledged"); + assert!( + matches!(refusal, RecordError::StorageFull { .. }), + "a full log refused a delete as {refusal:?} rather than as full" + ); + // And a repository removal, which is the same fact about a whole + // repository and the one the trait could not report at all. + let refusal = records + .remove_repo(DID) + .expect_err("a repository removal the log could not take was acknowledged"); + assert!( + matches!(refusal, RecordError::StorageFull { .. }), + "a full log refused a repository removal as {refusal:?} rather than as full" + ); + + // Still served, which is what makes the refusal true: this deployment + // says the record is there, and so does the one that restarts. + assert_eq!(stored(&durable), 1); + assert!(records.get(DID, COLLECTION, &rkey).is_some()); + + wal.set_capacity(Some(wal.len() + 100_000)); + wal.sync().expect("a sync"); + drop(durable); + + let durable = open(&dir); + assert_eq!( + stored(&durable), + 1, + "a restart and the caller disagree about whether a record was deleted" + ); + + // And it is not a one-way door: given room, the delete lands and stays. + durable + .records() + .remove(DID, COLLECTION, &rkey, &Precondition::Unconditional) + .expect("a delete once there is room again"); + assert_eq!(stored(&durable), 0); + durable.wal().sync().expect("a sync"); + drop(durable); + assert_eq!(stored(&open(&dir)), 0, "an acknowledged delete came back"); +} + /// Provisioning into a full deployment is refused, and nothing is half-made. /// /// The account store's stake is different from the record store's: a -- 2.51.2