From 4e0f8ee5d977500640a3e25ab7adfe6ef60bb53c Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Thu, 3 Sep 2026 21:20:19 -0400 Subject: [PATCH] feat(pds)!: tally blobs by the state each one is in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BlobStats` carried a count and a byte total; it now carries a `BlobTally` per lifecycle state — held, referenced, unreferenced, collectable, and the two readings startup takes of the disk. `bot.did.stats` serves them under one `blobs` object, so the next blob condition worth reporting is a member of it rather than another top-level key. Breaking: `blobs` on that response is an object where it was a number, and `blobBytes` is now `blobs.held.bytes`. The write-ahead log is untouched, so `layout::SHAPE` is unchanged and a data directory needs nothing. Change-Id: I6d786f7ae36865a221a05045877d047e15e9f0f1 --- crates/didbot-pds/src/blobs.rs | 167 ++++++++++++-- crates/didbot-pds/src/durable.rs | 138 ++++++++--- crates/didbot-pds/src/lib.rs | 2 +- crates/didbot-pds/src/object_blobs.rs | 8 +- crates/didbot-pds/src/session.rs | 13 +- crates/didbot-pds/tests/blob_storage.rs | 45 ++-- crates/didbot-pds/tests/durability.rs | 6 +- crates/didbot-pds/tests/restore.rs | 51 ++++- crates/didbot-pds/tests/restore_report.rs | 2 +- .../assets/dashboard/dashboard.js | 3 +- crates/didbot-serve/src/auth.rs | 13 -- crates/didbot-serve/src/error.rs | 37 --- crates/didbot-serve/src/lib.rs | 10 +- crates/didbot-serve/src/routes.rs | 49 +--- crates/didbot-serve/src/tests.rs | 214 ++++++++---------- crates/didbot-serve/src/wire.rs | 112 +++++++-- crates/didbot/tests/conformance/wire.rs | 129 +---------- docs/conformance.md | 33 --- docs/operations.md | 4 +- plan/auth-types.md | 30 +-- plan/blob-storage-tiers.md | 17 +- plan/periodic-backups.md | 5 +- 22 files changed, 569 insertions(+), 519 deletions(-) diff --git a/crates/didbot-pds/src/blobs.rs b/crates/didbot-pds/src/blobs.rs index 8b1a107a..b1e127c3 100644 --- a/crates/didbot-pds/src/blobs.rs +++ b/crates/didbot-pds/src/blobs.rs @@ -209,15 +209,105 @@ impl BlobRef { } } -/// What a deployment's blobs come to, counted. -#[derive(Debug, Clone, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -pub struct BlobStats { - /// How many blobs exist across every repository. - pub blobs: usize, - /// What they occupy in total, in bytes. +/// How many blobs, and what they occupy. +/// +/// The pair rather than either alone. A thousand blobs and a thousand +/// mebibytes are two different problems — one is a listing that has grown, +/// the other is a disk — and every state in [`BlobStats`] is read to answer +/// one of those two questions. +#[derive( + Debug, Clone, Copy, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize, +)] +pub struct BlobTally { + /// How many blobs are in this state. + pub count: u64, + /// What they occupy, in bytes. pub bytes: u64, } +impl BlobTally { + /// Folds in one blob of `bytes`. + pub(crate) fn add_one(&mut self, bytes: u64) { + self.count = self.count.saturating_add(1); + self.bytes = self.bytes.saturating_add(bytes); + } + + /// Folds in another tally. + pub(crate) fn add(&mut self, other: BlobTally) { + self.count = self.count.saturating_add(other.count); + self.bytes = self.bytes.saturating_add(other.bytes); + } +} + +/// What a deployment's blobs come to, by the state each blob is in. +/// +/// Every figure is an aggregate over every repository. Which account holds +/// which blob is a fact about one account, and `bot.did.stats` serves this +/// to callers that have shown no credential. +/// +/// [`BlobStats::held`] is the whole of what the log names, and +/// [`BlobStats::referenced`], [`BlobStats::unreferenced`] and +/// [`BlobStats::collectable`] partition it: every held blob is in exactly +/// one of the three, and the three sum to `held`. The two `_at_boot` figures +/// are readings taken once, during startup, and are not part of that sum — +/// see each one for what it overlaps. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct BlobStats { + /// Every blob the log names, whatever state it is in. + pub held: BlobTally, + /// Blobs at least one live record points at. + /// + /// Counted from the reference count + /// [`BlobStore::mark_referenced`] raises and + /// [`BlobStore::unmark_referenced`] lowers, which + /// `crate::records::blob_refs` drives from the records themselves. These + /// are the blobs a collection pass will never take. + pub referenced: BlobTally, + /// Blobs no live record points at, still inside + /// [`BlobLimits::collection_grace`]. + /// + /// The ordinary state of every upload between `uploadBlob` and the + /// `createRecord` that names it, so a nonzero reading is a working + /// server rather than a problem. It is also where a blob lands when the + /// last record referencing it is deleted. + pub unreferenced: BlobTally, + /// Blobs no live record points at whose grace window has run out. + /// + /// Exactly [`BlobStore::collect_unreferenced`]'s candidates: what the + /// next collection pass will take, and the quota it will give back. A + /// figure that climbs and does not fall is a collector that is not + /// running, or one whose log appends are failing — the case that store's + /// doc says is skipped and retried. + pub collectable: BlobTally, + /// Blobs the log named whose bytes the disk did not have when this + /// process started. + /// + /// A boot-time reading held for the life of the process: what + /// [`FileBlobStore::reconcile`](crate::FileBlobStore::reconcile) counted + /// during startup. Nonzero means this deployment came back from an + /// incomplete copy and answers `404` for that many blobs. Zero means the + /// disk agreed with the log *then*; a blob deleted from under a running + /// server is not counted here, and a restart takes the reading again. + /// + /// These are also in [`BlobStats::held`] and in whichever of the three + /// states above their reference count puts them: the log names them, so + /// `listBlobs` still does. + /// + /// Zero for a store that never reconciled anything, which is every + /// in-memory run and every fresh data directory. + pub missing_at_boot: BlobTally, + /// Bytes the disk held that the log named none of, deleted during + /// startup. + /// + /// The other half of the same reading, and the other direction of the + /// same disagreement: an upload interrupted inside `.incoming/`, a blob + /// whose bytes were written and whose log entry never landed, or an + /// account directory the log no longer mentions. Nothing counted here is + /// in [`BlobStats::held`] — the index never named it, and it is gone + /// from the disk by the time this is read. + pub discarded_at_boot: BlobTally, +} + /// One blob a collection pass took, for a caller that wants to log or count /// what went. #[derive(Debug, Clone, PartialEq, Eq)] @@ -748,12 +838,22 @@ impl BlobIndex { Ok(None) } - /// Every CID one account holds, for a sweep that has to tell a stored - /// blob from a file nothing knows about. - pub(crate) fn cids(&self, did: &str) -> Vec { + /// Every CID one account holds, with the size the log recorded for it. + /// + /// A sweep needs the names, to tell a stored blob from a file nothing + /// knows about, and needs the sizes, because a name it does not find on + /// the disk is a missing blob whose bytes it has to report. Both come + /// out of one pass under one lock. + pub(crate) fn sizes(&self, did: &str) -> BTreeMap { self.lock() .get(did) - .map(|account| account.blobs.keys().cloned().collect()) + .map(|account| { + account + .blobs + .iter() + .map(|(cid, held)| (cid.clone(), held.reference.size)) + .collect() + }) .unwrap_or_default() } @@ -921,15 +1021,34 @@ impl BlobIndex { .collect() } - /// What the whole store holds. - pub(crate) fn stats(&self) -> BlobStats { + /// What the whole store holds, split by the state each blob is in. + /// + /// `cutoff` is the moment [`BlobLimits::collection_grace`] puts behind + /// the present — the same one [`BlobIndex::collect_candidates`] is + /// given, so that [`BlobStats::collectable`] counts exactly the blobs + /// the next collection pass would take rather than something near them. + /// + /// The boot-time figures are left at zero. The index is what the log + /// says; whether the disk agreed is a question only a store that has one + /// can answer, and `FileBlobStore` overlays what its startup reconcile + /// found. + pub(crate) fn stats(&self, cutoff: OffsetDateTime) -> BlobStats { let accounts = self.lock(); - BlobStats { - blobs: accounts.values().map(|account| account.blobs.len()).sum(), - bytes: accounts - .values() - .fold(0u64, |total, account| total.saturating_add(account.bytes)), + let mut stats = BlobStats::default(); + for account in accounts.values() { + for held in account.blobs.values() { + let bytes = held.reference.size; + stats.held.add_one(bytes); + if held.refs > 0 { + stats.referenced.add_one(bytes); + } else if held.uploaded_at <= cutoff { + stats.collectable.add_one(bytes); + } else { + stats.unreferenced.add_one(bytes); + } + } } + stats } } @@ -1058,7 +1177,9 @@ impl BlobStore for MemoryBlobStore { } fn stats(&self) -> BlobStats { - self.inner.index.stats() + self.inner + .index + .stats(OffsetDateTime::now_utc() - self.limits.collection_grace) } fn limits(&self) -> BlobLimits { @@ -1192,8 +1313,8 @@ mod tests { let store = MemoryBlobStore::new(); let one = upload(&store, "did:web:a.example", &[b"farthingale"]); upload(&store, "did:web:b.example", &[b"farthingale"]); - assert_eq!(store.stats().blobs, 2); - assert_eq!(store.stats().bytes, 22); + assert_eq!(store.stats().held.count, 2); + assert_eq!(store.stats().held.bytes, 22); // And deleting one account leaves the other's copy alone. store.remove_repo("did:web:a.example"); assert!(store.fetch("did:web:a.example", &one.cid).is_err()); @@ -1205,8 +1326,8 @@ mod tests { let store = MemoryBlobStore::new(); upload(&store, "did:web:a.example", &[b"kestrel"]); upload(&store, "did:web:a.example", &[b"kestrel"]); - assert_eq!(store.stats().blobs, 1, "one cid is one blob"); - assert_eq!(store.stats().bytes, 7, "and it spends the quota once"); + assert_eq!(store.stats().held.count, 1, "one cid is one blob"); + assert_eq!(store.stats().held.bytes, 7, "and it spends the quota once"); } #[test] @@ -1264,7 +1385,7 @@ mod tests { .expect("the upload should open"); upload.write(b"half a pict").expect("a chunk"); drop(upload); - assert_eq!(store.stats().blobs, 0); + assert_eq!(store.stats().held.count, 0); assert_eq!( store.list("did:web:a.example", 10, None).expect("listing"), (Vec::new(), None) diff --git a/crates/didbot-pds/src/durable.rs b/crates/didbot-pds/src/durable.rs index fd968573..207470d2 100644 --- a/crates/didbot-pds/src/durable.rs +++ b/crates/didbot-pds/src/durable.rs @@ -53,6 +53,7 @@ use time::OffsetDateTime; use crate::account::{AccountState, AccountStore, AgentAccount, MemoryAccountStore, StoreError}; use crate::blobs::{ canonical_cid, did_path, BlobError, BlobIndex, BlobLimits, BlobRef, BlobStats, BlobStore, + BlobTally, BlobUpload, CollectedBlob, Fetch, }; use crate::credential::{AgentTokenStore, IssuedToken, MemoryAgentTokenStore, TokenError}; @@ -244,7 +245,7 @@ impl Durable { let credential_count = credentials.snapshot().len() as u64; let live = accounts.len() as u64 + records.stats().records as u64 - + blob_stats.blobs as u64 + + blob_stats.held.count + ledger.entries() as u64 + commit_stats.repositories as u64 + (live_names + held_names) as u64 @@ -252,8 +253,8 @@ impl Durable { tracing::info!( accounts = accounts.len(), records = records.stats().records, - blobs = blob_stats.blobs, - blob_bytes = blob_stats.bytes, + blobs = blob_stats.held.count, + blob_bytes = blob_stats.held.bytes, ledgers = ledger.all().len(), ledger_entries = ledger.entries(), names = live_names, @@ -262,8 +263,8 @@ impl Durable { revisions = commit_stats.revisions, credentials = credential_count, history, - swept = reconciled.swept, - missing_blobs = reconciled.missing, + swept = reconciled.swept.bytes, + missing_blobs = reconciled.missing.count, "restored the deployment from its write-ahead log" ); @@ -1295,6 +1296,21 @@ struct BlobFiles { wal: Arc, /// Makes each incoming upload's filename unique within the process. next: AtomicU64, + /// What [`FileBlobStore::reconcile`] found, kept so that + /// [`BlobStats::missing_at_boot`] and + /// [`BlobStats::discarded_at_boot`] can report it for the life of the + /// process. Written once, during startup; see those fields' own docs for + /// why it is never refreshed. + boot: Mutex, +} + +/// The two boot-time figures [`BlobStats`] carries, as startup left them. +#[derive(Debug, Default, Clone, Copy)] +struct BootReconcile { + /// Fills [`BlobStats::missing_at_boot`]. + missing: BlobTally, + /// Fills [`BlobStats::discarded_at_boot`]. + discarded: BlobTally, } impl BlobFiles { @@ -1307,6 +1323,14 @@ impl BlobFiles { fn incoming_dir(&self, did: &str) -> PathBuf { self.account_dir(did).join(INCOMING_DIR) } + + /// The boot-time reading, recovering from a poisoned mutex. + fn boot(&self) -> BootReconcile { + *self + .boot + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } } /// Turns a log failure into the error a blob store is allowed to return. @@ -1340,6 +1364,7 @@ impl FileBlobStore { index: BlobIndex::default(), wal, next: AtomicU64::new(0), + boot: Mutex::new(BootReconcile::default()), }), limits, }) @@ -1419,7 +1444,7 @@ impl FileBlobStore { for entry in entries.flatten() { let path = entry.path(); if !path.is_dir() { - found.swept += remove_path(&path); + found.swept.add(remove_path(&path)); continue; } let name = entry.file_name().to_string_lossy().into_owned(); @@ -1427,53 +1452,69 @@ impl FileBlobStore { // account that was deleted, or one whose every upload was lost // with its entry. Either way nothing in it is a blob. let Some(did) = known.get(&name) else { - found.swept += remove_path(&path); + found.swept.add(remove_path(&path)); continue; }; visited.insert(name); - let cids: std::collections::BTreeSet = - self.inner.index.cids(did).into_iter().collect(); + // Drained as the walk finds each name, so what remains is the + // names the disk did not have — with the sizes the log recorded + // for them, which is the only place those bytes can be read from + // once the file is gone. + let mut absent = self.inner.index.sizes(did); let Ok(files) = std::fs::read_dir(&path) else { continue; }; - let mut on_disk = std::collections::BTreeSet::new(); for file in files.flatten() { let name = file.file_name().to_string_lossy().into_owned(); - if name == INCOMING_DIR || !cids.contains(&name) { - found.swept += remove_path(&file.path()); - continue; + if name == INCOMING_DIR || absent.remove(&name).is_none() { + found.swept.add(remove_path(&file.path())); } - on_disk.insert(name); } - for cid in cids.difference(&on_disk) { - found.note_missing(did, cid); + for (cid, size) in absent { + found.note_missing(did, &cid, size); } } for (dir, did) in &known { if visited.contains(dir) { continue; } - for cid in self.inner.index.cids(did) { - found.note_missing(did, &cid); + for (cid, size) in self.inner.index.sizes(did) { + found.note_missing(did, &cid, size); } } - if found.swept > 0 { + if found.swept.count > 0 { tracing::warn!( - bytes = found.swept, + blobs = found.swept.count, + bytes = found.swept.bytes, root = %self.inner.root.display(), "discarded blob bytes the log does not reference" ); } - if found.missing > 0 { + if found.missing.count > 0 { tracing::error!( - missing = found.missing, + missing = found.missing.count, + bytes = found.missing.bytes, sample = ?found.sample, root = %self.inner.root.display(), "the log names blobs whose bytes are not on the disk; \ this deployment will answer 404 for each of them" ); } + // Kept on the store rather than only returned, so that the answer + // outlives startup: `Durable::open` holds the return value, but + // `bot.did.stats` reads a `BlobStore`, and an operator asking "did + // this come back whole?" after the log has scrolled away is asking + // the route. Stored unconditionally — a healthy reconcile writing + // zero is what makes a later reading of zero mean something. + *self + .inner + .boot + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = BootReconcile { + missing: found.missing, + discarded: found.swept, + }; found } } @@ -1493,21 +1534,25 @@ pub const MISSING_SAMPLE: usize = 8; /// is a question asked after startup rather than during it. #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Reconciled { - /// Bytes deleted because the log named none of them. - pub swept: u64, - /// How many blobs the log names whose bytes are not on the disk. + /// What was deleted because the log named none of it. + /// + /// Fills [`BlobStats::discarded_at_boot`]. + pub swept: BlobTally, + /// What the log names whose bytes are not on the disk. /// /// Nonzero means this deployment is serving an incomplete restore: every - /// one of these is in `listBlobs` and answers `404` to a fetch. - pub missing: u64, + /// one of these is in `listBlobs` and answers `404` to a fetch. The + /// bytes are the sizes the log recorded, which is how much the restore + /// would have to put back. Fills [`BlobStats::missing_at_boot`]. + pub missing: BlobTally, /// Up to [`MISSING_SAMPLE`] of the missing ones, as `(did, cid)`. pub sample: Vec<(String, String)>, } impl Reconciled { /// Counts one missing blob, keeping its name while there is room. - fn note_missing(&mut self, did: &str, cid: &str) { - self.missing += 1; + fn note_missing(&mut self, did: &str, cid: &str, size: u64) { + self.missing.add_one(size); if self.sample.len() < MISSING_SAMPLE { self.sample.push((did.to_owned(), cid.to_owned())); } @@ -1537,18 +1582,28 @@ fn holds_blob_bytes(root: &Path) -> bool { false } -/// Deletes a file or a directory tree, answering how many bytes went. -fn remove_path(path: &Path) -> u64 { +/// Deletes a file or a directory tree, answering how many files went and +/// what they held. +/// +/// Files, not blobs: under the blob root a file is either a blob's bytes or +/// an upload that never finished, and this cannot tell which, so the caller +/// says what the count means. Directories are not counted, only walked. +fn remove_path(path: &Path) -> BlobTally { let size = std::fs::metadata(path).map(|meta| meta.len()).unwrap_or(0); if path.is_dir() { - let inner: u64 = std::fs::read_dir(path) - .map(|entries| entries.flatten().map(|e| remove_path(&e.path())).sum()) - .unwrap_or(0); + let mut inner = BlobTally::default(); + if let Ok(entries) = std::fs::read_dir(path) { + for entry in entries.flatten() { + inner.add(remove_path(&entry.path())); + } + } let _ = std::fs::remove_dir_all(path); return inner; } let _ = std::fs::remove_file(path); - size + let mut gone = BlobTally::default(); + gone.add_one(size); + gone } impl BlobStore for FileBlobStore { @@ -1655,7 +1710,7 @@ impl BlobStore for FileBlobStore { // written in the same breath: one deletion of one repository is one // fact, and two entries for it could disagree after a crash. let dir = self.inner.account_dir(did); - let bytes = remove_path(&dir); + let bytes = remove_path(&dir).bytes; tracing::info!( did, blobs = dropped.len(), @@ -1664,8 +1719,17 @@ impl BlobStore for FileBlobStore { ); } + /// The index's own tallies, plus the two figures the index cannot hold: + /// how much of what it names the disk did not have when this process + /// started, and how much the disk held that it names none of. fn stats(&self) -> BlobStats { - self.inner.index.stats() + let cutoff = time::OffsetDateTime::now_utc() - self.limits.collection_grace; + let boot = self.inner.boot(); + BlobStats { + missing_at_boot: boot.missing, + discarded_at_boot: boot.discarded, + ..self.inner.index.stats(cutoff) + } } fn limits(&self) -> BlobLimits { diff --git a/crates/didbot-pds/src/lib.rs b/crates/didbot-pds/src/lib.rs index dd4a57d3..1ba0b194 100644 --- a/crates/didbot-pds/src/lib.rs +++ b/crates/didbot-pds/src/lib.rs @@ -107,7 +107,7 @@ pub use account::{ }; pub use aturi::{AtUri, AtUriError}; pub use blobs::{ - BlobError, BlobLimits, BlobRef, BlobStats, BlobStore, BlobUpload, CollectedBlob, Fetch, + BlobError, BlobLimits, BlobRef, BlobStats, BlobStore, BlobTally, BlobUpload, CollectedBlob, Fetch, GraceTooShort, MemoryBlobStore, DEFAULT_ACCOUNT_QUOTA_BYTES, DEFAULT_BLOB_COLLECTION_GRACE, DEFAULT_MAX_BLOB_BYTES, DEFAULT_MIME_TYPE, MIN_BLOB_COLLECTION_GRACE, }; diff --git a/crates/didbot-pds/src/object_blobs.rs b/crates/didbot-pds/src/object_blobs.rs index 5828c492..8061c8f7 100644 --- a/crates/didbot-pds/src/object_blobs.rs +++ b/crates/didbot-pds/src/object_blobs.rs @@ -355,7 +355,9 @@ impl BlobStore for ObjectBlobStore { } fn stats(&self) -> BlobStats { - self.inner.index.stats() + self.inner + .index + .stats(time::OffsetDateTime::now_utc() - self.limits.collection_grace) } fn limits(&self) -> BlobLimits { @@ -762,7 +764,7 @@ mod tests { .expect_err("commit should fail when the backend is unreachable"); assert!(matches!(err, BlobError::Backend(_))); assert_eq!( - store.stats().blobs, + store.stats().held.count, 0, "a failed commit must not appear in the index" ); @@ -834,7 +836,7 @@ mod tests { assert!(backend .get(&format!("did%3Aweb%3Aa.example/{}", b.cid)) .is_none()); - assert_eq!(store.stats().blobs, 0); + assert_eq!(store.stats().held.count, 0); } /// The durability claim this module makes: the index survives a diff --git a/crates/didbot-pds/src/session.rs b/crates/didbot-pds/src/session.rs index adc5c761..7073402a 100644 --- a/crates/didbot-pds/src/session.rs +++ b/crates/didbot-pds/src/session.rs @@ -104,14 +104,11 @@ impl AppPasswordHash { /// Where app passwords live. /// /// One per account, the minimum this server needs: enough to authenticate -/// `createSession`, with no lifetime or naming beyond that. This trait is -/// the whole of the interface — a deployment writes an entry through it in -/// process, and `com.atproto.server.{create,list,revoke}AppPassword` answer -/// every caller that this server issues the agent token -/// `bot.did.provisionAgent` returns instead (`didbot_serve`'s -/// `ApiError::app_passwords_disabled`). So what an entry here has is a hash -/// and an account, and nothing that would need naming, listing or a -/// lifetime of its own. +/// `createSession`, with no lifetime or naming beyond that. Listing, naming +/// several and revoking one by name are `com.atproto.server.{create,list, +/// revoke}AppPassword`, deliberately not built here — see +/// `plan/auth-types.md`'s note that this is a second issuer with its own +/// lifetime, which those methods are. pub trait AppPasswordStore: Send + Sync { /// Sets (or replaces) the app password for `did`. fn set(&self, did: &str, hash: AppPasswordHash); diff --git a/crates/didbot-pds/tests/blob_storage.rs b/crates/didbot-pds/tests/blob_storage.rs index 5b877ef0..64411a43 100644 --- a/crates/didbot-pds/tests/blob_storage.rs +++ b/crates/didbot-pds/tests/blob_storage.rs @@ -67,6 +67,22 @@ fn files(root: &Path) -> Vec<(PathBuf, u64)> { found } +/// A whole `BlobStats` in which every blob is held and unreferenced. +/// +/// That is where an upload sits until a record names it, and no fixture in +/// this file writes a record, so it is where every blob here sits. Written +/// as a whole value rather than a field read, so that a figure this file +/// does not name has to be zero: a state that quietly started counting +/// something would fail here rather than pass unnoticed. +fn all_unreferenced(count: u64, bytes: u64) -> didbot_pds::BlobStats { + let tally = didbot_pds::BlobTally { count, bytes }; + didbot_pds::BlobStats { + held: tally, + unreferenced: tally, + ..Default::default() + } +} + /// Uploads `bytes` in one chunk. fn put(blobs: &FileBlobStore, did: &str, mime: &str, bytes: &[u8]) -> BlobRef { let mut upload = blobs @@ -166,7 +182,7 @@ fn a_lying_content_length_cannot_stream_past_the_per_blob_cap() { Vec::new(), "a refused upload leaves no bytes behind at all" ); - assert_eq!(blobs.stats().bytes, 0); + assert_eq!(blobs.stats().held.bytes, 0); let _ = std::fs::remove_dir_all(&dir); } @@ -279,10 +295,7 @@ fn eight_simultaneous_uploads_of_one_blob_land_once() { ); assert_eq!( blobs.stats(), - didbot_pds::BlobStats { - blobs: 1, - bytes: 10 - }, + all_unreferenced(1, 10), "one blob, and the quota spent once" ); assert_eq!( @@ -330,7 +343,7 @@ fn collection_gives_back_exactly_the_quota_an_abandoned_upload_took() { put(&blobs, did, "image/png", b"quernstone"); put(&blobs, did, "image/png", b"farthingal"); assert_eq!( - blobs.stats().bytes, + blobs.stats().held.bytes, 20, "the account is exactly at its quota" ); @@ -365,18 +378,18 @@ fn collection_gives_back_exactly_the_quota_an_abandoned_upload_took() { 20, "and collection reported back exactly what they had taken" ); - assert_eq!(blobs.stats().bytes, 0, "the quota is free again"); + assert_eq!(blobs.stats().held.bytes, 0, "the quota is free again"); // Which means the upload that was refused a moment ago now fits. put(&blobs, did, "image/png", b"kestrel"); - assert_eq!(blobs.stats().bytes, 7); + assert_eq!(blobs.stats().held.bytes, 7); durable.wal().sync().expect("flush"); } let (_durable, blobs) = open(&dir, limits); assert_eq!( blobs.stats(), - didbot_pds::BlobStats { blobs: 1, bytes: 7 }, + all_unreferenced(1, 7), "a restart replays the collections as well as the uploads" ); let root = dir.join(didbot_pds::BLOB_DIR); @@ -425,10 +438,7 @@ fn a_re_upload_answers_the_reference_the_store_will_serve() { assert_eq!(bytes, b"quernstone"); assert_eq!( blobs.stats(), - didbot_pds::BlobStats { - blobs: 1, - bytes: 10 - }, + all_unreferenced(1, 10), "a re-upload is still not a second blob" ); @@ -481,7 +491,7 @@ fn an_upload_cannot_land_in_an_account_deleted_while_it_streamed() { ); assert_eq!( blobs.stats(), - didbot_pds::BlobStats { blobs: 0, bytes: 0 }, + all_unreferenced(0, 0), "and it is charged for nothing" ); durable.wal().sync().expect("flush"); @@ -493,7 +503,7 @@ fn an_upload_cannot_land_in_an_account_deleted_while_it_streamed() { let (_durable, blobs) = open(&dir, BlobLimits::default()); assert_eq!( blobs.stats(), - didbot_pds::BlobStats { blobs: 0, bytes: 0 }, + all_unreferenced(0, 0), "a restart does not resurrect an upload into a deleted account" ); let root = dir.join(didbot_pds::BLOB_DIR); @@ -517,7 +527,10 @@ fn an_upload_cannot_land_in_an_account_deleted_while_it_streamed() { upload.commit().is_err(), "nor when the account held blobs at the moment it was deleted" ); - assert_eq!(blobs.stats(), didbot_pds::BlobStats { blobs: 0, bytes: 0 }); + assert_eq!( + blobs.stats(), + all_unreferenced(0, 0) + ); // And the DID is usable again afterwards, which is what makes the // refusal a check on that upload rather than a tombstone on the account. diff --git a/crates/didbot-pds/tests/durability.rs b/crates/didbot-pds/tests/durability.rs index a749fd87..d39c9a80 100644 --- a/crates/didbot-pds/tests/durability.rs +++ b/crates/didbot-pds/tests/durability.rs @@ -1511,7 +1511,11 @@ fn deleting_an_account_takes_its_blobs_with_it() { drop(durable); let (_durable, pds) = boot(&dir, None); - assert_eq!(pds.stats().blobs.blobs, 0, "and stay gone across a restart"); + assert_eq!( + pds.stats().blobs.held.count, + 0, + "and stay gone across a restart" + ); } /// A refused compare-and-swap must not reach the log. diff --git a/crates/didbot-pds/tests/restore.rs b/crates/didbot-pds/tests/restore.rs index c0731c23..5d8d7d7a 100644 --- a/crates/didbot-pds/tests/restore.rs +++ b/crates/didbot-pds/tests/restore.rs @@ -242,7 +242,7 @@ fn assert_self_consistent(pds: &Pds, durable: &Durable, dir: &Path, at: &str) { assert_eq!( durable.reconciled().missing, - 0, + didbot_pds::BlobTally::default(), "{at}: startup found blobs the log names and the disk lacks: {:?}", durable.reconciled().sample ); @@ -426,7 +426,11 @@ fn a_restore_that_lost_a_blobs_bytes_counts_it_serves_404_and_stays_up() { vec![(did.clone(), named.clone())], "startup did not name the blob it cannot serve" ); - assert_eq!(durable.reconciled().missing, 1, "and counted only that one"); + assert_eq!( + durable.reconciled().missing.count, + 1, + "and counted only that one" + ); let err = pds .fetch_blob(&did, &named) @@ -448,6 +452,47 @@ fn a_restore_that_lost_a_blobs_bytes_counts_it_serves_404_and_stays_up() { let _ = std::fs::remove_dir_all(©); } +/// The count a caller can reach, not just the one startup returned. +/// +/// `Durable::reconciled()` is held by whatever opened the log, and nothing +/// serving requests has that: a route asks the `Registry`. So the same number +/// has to survive the trip through `stats()`, or a deployment missing five +/// hundred blobs answers a caller identically to a healthy one. +/// +/// Both halves are load-bearing. The damaged restore proves the count arrives; +/// the healthy restore of the *same fixture* proves it is a reading and not a +/// constant, which is the only thing that makes a zero worth anything. +#[test] +fn a_restores_missing_blobs_are_counted_in_the_stats_a_caller_can_read() { + let dir = scratch("stats-missing"); + let damaged = scratch("stats-missing-damaged"); + let whole = scratch("stats-missing-whole"); + let (did, named, _loose) = seeded(&dir); + + restore(&dir, &damaged); + std::fs::remove_file(blob_path(&damaged, &did, &named)).expect("lose one blob's bytes"); + let (durable, pds) = boot(&damaged); + assert_eq!( + pds.stats().blobs.missing_at_boot.count, + 1, + "a restore that lost a blob's bytes reports a whole deployment" + ); + drop(durable); + + restore(&dir, &whole); + let (durable, pds) = boot(&whole); + assert_eq!( + pds.stats().blobs.missing_at_boot.count, + 0, + "a restore that lost nothing reports damage" + ); + drop(durable); + + let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&damaged); + let _ = std::fs::remove_dir_all(&whole); +} + /// A restore that brought the log and none of `blobs/` for one account. /// /// The directory is not on the disk at all, so the walk that finds a file @@ -467,7 +512,7 @@ fn a_restore_missing_an_accounts_whole_blob_directory_counts_every_blob_in_it() let (durable, pds) = boot(©); let (cids, _) = pds.list_blobs(&did, 100, None, None).expect("listing"); assert_eq!( - durable.reconciled().missing as usize, + durable.reconciled().missing.count as usize, cids.len(), "every blob of a directory that did not come back should be counted" ); diff --git a/crates/didbot-pds/tests/restore_report.rs b/crates/didbot-pds/tests/restore_report.rs index 846cc03d..c28a584c 100644 --- a/crates/didbot-pds/tests/restore_report.rs +++ b/crates/didbot-pds/tests/restore_report.rs @@ -116,7 +116,7 @@ fn a_restore_that_lost_bytes_names_them_at_startup() { rendered.contains("missing_blobs=1"), "the line that says what came back did not say what did not:\n{rendered}" ); - assert_eq!(durable.reconciled().missing, 1); + assert_eq!(durable.reconciled().missing.count, 1); drop(durable); let _ = std::fs::remove_dir_all(&dir); diff --git a/crates/didbot-serve/assets/dashboard/dashboard.js b/crates/didbot-serve/assets/dashboard/dashboard.js index 27d762b2..d235a433 100644 --- a/crates/didbot-serve/assets/dashboard/dashboard.js +++ b/crates/didbot-serve/assets/dashboard/dashboard.js @@ -70,7 +70,8 @@ async function refreshAbout() { ["zone", data.zone], ["accounts", String(data.stats.accounts)], ["records", String(data.stats.records)], - ["blobs", String(data.stats.blobs)], + ["blobs", String(data.stats.blobs.held.count)], + ["blob bytes", String(data.stats.blobs.held.bytes)], ]); }); } diff --git a/crates/didbot-serve/src/auth.rs b/crates/didbot-serve/src/auth.rs index c4615d17..b6548fdf 100644 --- a/crates/didbot-serve/src/auth.rs +++ b/crates/didbot-serve/src/auth.rs @@ -130,19 +130,6 @@ //! make this server work is the floor between two polls, which bounds the //! *outbound* rate rather than the inbound one; see //! `crate::ownership_poll::MIN_POLL_INTERVAL`. -//! - **Public because the route's whole answer is a decision, identical for -//! every caller.** `com.atproto.server.{createAppPassword, -//! listAppPasswords, revokeAppPassword}`. Each is served, and each answers -//! 403 `AppPasswordsDisabled` — see -//! [`crate::error::ApiError::app_passwords_disabled`] — naming the agent -//! token this server issues instead. `Public` is the honest declaration -//! because it is the true one: `routes::app_passwords_disabled` takes a -//! URI and nothing else, so no header is read, and requiring a credential -//! to be told a decision that applies to everybody would make the answer -//! vary with who asked — which is how a refusal becomes a way to find out -//! which accounts exist. This is the method-level shape of what -//! [`Presented::Disabled`] does one level up for a credential scheme, and -//! what `plan/auth-types.md` means by "recognised, not accepted". use axum::http::{header, HeaderMap, HeaderName, StatusCode}; use didbot_pds::{Registry, SessionAuth, TokenError}; diff --git a/crates/didbot-serve/src/error.rs b/crates/didbot-serve/src/error.rs index 7df0a451..546c1b7e 100644 --- a/crates/didbot-serve/src/error.rs +++ b/crates/didbot-serve/src/error.rs @@ -128,43 +128,6 @@ impl ApiError { ) } - /// The answer every `com.atproto.server.*AppPassword` method gives: this - /// server recognises the method and issues agent tokens instead. - /// - /// `AppPasswordsDisabled`, its own name in the family - /// `auth::scheme_disabled`'s `SchemeDisabled` and - /// `auth::disclosure_disabled`'s `DisclosureDisabled` already occupy: a - /// method this deployment serves, recognises, and answers with a - /// decision rather than with a credential check. The name is not - /// `MethodNotImplemented`, which `routes::unknown_route` answers for a - /// path this router mounts nothing at — a caller that cannot tell those - /// two apart cannot tell a decision from a typo, and only one of them is - /// worth reading the message of. - /// - /// 403, for the reason both of its siblings are 403: no credential - /// reaches past this, so a 401 would be inviting a retry that this - /// server answers identically. The handler behind it reads no header at - /// all — see `routes::app_passwords_disabled` — which is what makes the - /// answer the same for every caller and therefore useless for learning - /// which accounts exist here. - /// - /// The message names what this server does issue, so a client that - /// arrives holding the human credential shape learns where the machine - /// one comes from: `bot.did.provisionAgent` hands an account its agent - /// token once, bound to that one DID. See `plan/auth-types.md`. - pub fn app_passwords_disabled(nsid: &str) -> Self { - Self::new( - StatusCode::FORBIDDEN, - "AppPasswordsDisabled", - format!( - "this server serves `{nsid}` and answers it with this decision: the credential \ - an account here holds is the agent token `bot.did.provisionAgent` returns once, \ - bound to that one DID, and that is the credential every route on this server \ - accepts. Every caller receives this same answer, with or without one." - ), - ) - } - /// A caller has exceeded a rate limit; see `crate::rate_limit`. /// /// `RateLimitExceeded` names no error the lexicons declare — the same diff --git a/crates/didbot-serve/src/lib.rs b/crates/didbot-serve/src/lib.rs index ee7388f9..68529eb9 100644 --- a/crates/didbot-serve/src/lib.rs +++ b/crates/didbot-serve/src/lib.rs @@ -42,9 +42,8 @@ //! | `GET /xrpc/com.atproto.repo.getRecord` | one record by collection and key | //! | `GET /xrpc/com.atproto.repo.listRecords` | read a collection back, newest first | //! | `GET /xrpc/com.atproto.repo.describeRepo` | the account, its document and its collections | -//! | `GET /xrpc/bot.did.stats` | accounts, records, bytes, per collection | +//! | `GET /xrpc/bot.did.stats` | accounts, records, bytes per collection, blobs by lifecycle state | //! | `POST /xrpc/com.atproto.server.createSession` | a legacy session, from an app password | -//! | `com.atproto.server.{createAppPassword,listAppPasswords,revokeAppPassword}` | recognised, and answered `403 AppPasswordsDisabled`: an account here holds the agent token `bot.did.provisionAgent` returns | //! | `POST /xrpc/com.atproto.server.refreshSession` | rotate a session's tokens | //! | `POST /xrpc/com.atproto.server.deleteSession` | end a session | //! | `GET /xrpc/com.atproto.server.getSession` | the account a session answers for | @@ -59,7 +58,7 @@ //! Failures are returned in atproto's error shape, `{"error":..,"message":..}`, //! with the names the lexicons declare -- `RecordNotFound`, `RepoNotFound`, //! `InvalidRequest`, `InvalidRecord`, `MethodNotImplemented`. See [`ApiError`] -//! for the whole mapping, and for the conditions that get a name no lexicon +//! for the whole mapping, and for the two conditions that get a name no lexicon //! declares. #![forbid(unsafe_code)] @@ -113,12 +112,13 @@ pub use subscribe::{ DEFAULT_REPOS_CAPACITY, FUTURE_CURSOR, OUTDATED_CURSOR, }; pub use wire::{ - record_uri, refuse_skipped_validation, AgentSummary, ApplyWritesRequest, CreateRecordRequest, + record_uri, refuse_skipped_validation, AgentSummary, ApplyWritesRequest, BlobsView, + CreateRecordRequest, CreateRecordResponse, CreateSessionRequest, DeleteRecordRequest, DescribeRepoQuery, DescribeRepoResponse, DescribeServerResponse, DidRequest, FirehoseQuery, GetRecordQuery, GetRecordResponse, GetSessionResponse, ListRecordsQuery, ProvisionAgentRequest, PutRecordRequest, RecordView, RepoWrite, SessionResponse, SetPinnedRequest, StatsResponse, - Swap, WireRegistration, WriteResult, MAX_LIST_LIMIT, + Swap, TallyView, WireRegistration, WriteResult, MAX_LIST_LIMIT, }; use std::convert::Infallible; diff --git a/crates/didbot-serve/src/routes.rs b/crates/didbot-serve/src/routes.rs index 33c54644..c3626493 100644 --- a/crates/didbot-serve/src/routes.rs +++ b/crates/didbot-serve/src/routes.rs @@ -258,10 +258,7 @@ xrpc_methods! { /// "Why every public route is public" for why the read surface is /// `Public` on purpose: protocol-required for `com.atproto.sync.*` and /// `com.atproto.repo.*`'s reads, structurally required for - /// `createSession` and `describeServer`, and — for the three - /// `*AppPassword` methods — the credential-free shape of a route whose - /// answer is one decision for every caller alike; see - /// [`app_passwords_disabled`]. The write half takes the agent + /// `createSession` and `describeServer`. The write half takes the agent /// token `provisionAgent` hands back, and `routes::write_record` / /// `routes::apply_writes` are where the authenticated DID is checked /// against the repository a write names. @@ -276,14 +273,11 @@ xrpc_methods! { "com.atproto.repo.listRecords" => get(list_records) as Public, "com.atproto.repo.putRecord" => post(put_record) as AgentToken, "com.atproto.repo.uploadBlob" => post(crate::blobs::upload_blob) as AgentToken, - "com.atproto.server.createAppPassword" => post(app_passwords_disabled) as Public, "com.atproto.server.createSession" => post(create_session) as Public, "com.atproto.server.deleteSession" => post(delete_session) as LegacySession, "com.atproto.server.describeServer" => get(describe_server) as Public, "com.atproto.server.getSession" => get(get_session) as LegacySession, - "com.atproto.server.listAppPasswords" => get(app_passwords_disabled) as Public, "com.atproto.server.refreshSession" => post(refresh_session) as LegacySession, - "com.atproto.server.revokeAppPassword" => post(app_passwords_disabled) as Public, "com.atproto.sync.getBlob" => get(crate::blobs::get_blob) as Public, "com.atproto.sync.getBlocks" => get(get_blocks) as Public, "com.atproto.sync.getLatestCommit" => get(get_latest_commit) as Public, @@ -2717,36 +2711,6 @@ fn too_many_login_attempts() -> Response { ApiError::rate_limited("too many createSession attempts; slow down").into_response() } -/// The three vendored app-password methods — -/// `com.atproto.server.{createAppPassword, listAppPasswords, -/// revokeAppPassword}` — all answered by -/// [`ApiError::app_passwords_disabled`], 403 `AppPasswordsDisabled`. -/// -/// The three routes exist so that a client speaking the ordinary atproto -/// sign-in flow gets an answer with a reason in it. `plan/auth-types.md` -/// asks for exactly that shape — "recognised, not accepted" rather than a -/// bare refusal — and it is the same answer `Presented::Disabled` gives a -/// caller arriving with a `DPoP` or `ServiceAuth` header, one level up: the -/// thing the caller asked for is a thing this server knows the name of and -/// has decided about. -/// -/// The decision itself: an app password is a long-lived replayable secret -/// premised on a person typing it into a settings page, and the accounts -/// here are agents. What each one holds instead is the agent token -/// `bot.did.provisionAgent` returns once — a machine credential for a caller -/// that is never human, bound to one DID, which is the credential -/// `com.atproto.repo.*`'s write routes and `uploadBlob` already take. -/// -/// This handler takes a [`axum::http::Uri`] and nothing else. No -/// `HeaderMap`, no state, no body: the answer cannot vary with a credential -/// because there is no credential in scope to vary on, which is why -/// `Credential::Public` is the honest declaration in `xrpc_methods!` above -/// and why this route tells a stranger nothing about which accounts exist -/// here. -async fn app_passwords_disabled(uri: axum::http::Uri) -> Response { - ApiError::app_passwords_disabled(uri.path().trim_start_matches("/xrpc/")).into_response() -} - /// `POST /xrpc/com.atproto.server.createSession` /// /// Checks `identifier`'s app password and, on a match, mints a session in a @@ -2760,17 +2724,6 @@ async fn app_passwords_disabled(uri: axum::http::Uri) -> Response { /// paid for on the way to it (see [`NO_SUCH_ACCOUNT`]) — so a caller cannot /// use either the answer or how long it took to learn which accounts exist. /// -/// The password it checks comes from [`didbot_pds::AppPasswordStore`], which -/// this deployment writes in process; over HTTP the three app-password -/// methods answer [`app_passwords_disabled`]. So this route verifies a -/// credential this server holds a hash for, and its refusal stays -/// [`login_refused`] rather than the named `AppPasswordsDisabled` answer -/// those three give: they say one thing to everybody about a decision, and -/// this route must say one thing to everybody about an *account* — a wrong -/// password, an identifier nobody hosts and an account whose store holds no -/// password are one answer here, and naming which of the three happened is -/// precisely the enumeration this route is built not to permit. -/// /// Rate limited on two axes before any of the expensive work below runs — /// `resolve_identifier` is cheap, but `state.sessions.create` pays for a real /// Argon2id hash even for an identifier with no account at all (see diff --git a/crates/didbot-serve/src/tests.rs b/crates/didbot-serve/src/tests.rs index 9b96e558..9c5b45d8 100644 --- a/crates/didbot-serve/src/tests.rs +++ b/crates/didbot-serve/src/tests.rs @@ -19,8 +19,9 @@ use didbot_identity::{AgentDid, DidDocument, Zone}; use didbot_key::SigningKey; use didbot_pds::{ AccountState, AgentAccount, AgentLedger, AppPasswordHash, BatchOp, BatchOutcome, BlobLimits, - BlobStore, BlobUpload, Cid, CommitEvent, CommitStore, Estop, Fetch, LedgerEntry, LedgerEvent, - LifecycleEvent, LifecycleSink, ListParams, MemoryBlobStore, MemoryCommitStore, + BlobStats, BlobStore, BlobTally, BlobUpload, Cid, CommitEvent, CommitStore, Estop, Fetch, + LedgerEntry, + LedgerEvent, LifecycleEvent, LifecycleSink, ListParams, MemoryBlobStore, MemoryCommitStore, MemoryRecordStore, Minter, ProvisionError, ProvisionRequest, Provisioned, RecordStore, RegistrationFacts, Registry, RegistryStats, RepoHead, SessionAuth, Swap, Written, }; @@ -86,6 +87,23 @@ pub(crate) struct FakeRegistry { /// test can watch the credential stores beside the router be told, and /// not merely that this fake dropped a row. account_revoke_hook: Mutex>, + /// What a startup reconcile would have found, for a test that wants a + /// deployment restored short of some of its blobs. + /// + /// Plain numbers rather than a damaged `MemoryBlobStore`, because there + /// is no such thing: an in-memory store's bytes and its index are the + /// same map, and the disagreements these report are ones only a store + /// with a disk can have. `FileBlobStore` is where they are really + /// counted, and `crates/didbot-pds/tests/blob_storage.rs` is where that + /// counting is tested, one state at a time. + boot: BootReconcile, +} + +/// The two boot-time readings [`FakeRegistry`] hands out. +#[derive(Debug, Default, Clone, Copy)] +pub(crate) struct BootReconcile { + missing: BlobTally, + discarded: BlobTally, } impl FakeRegistry { @@ -103,6 +121,7 @@ impl FakeRegistry { history: MemoryCommitStore::new(), revisions: Minter::new(), account_revoke_hook: Mutex::new(None), + boot: BootReconcile::default(), } } @@ -134,6 +153,19 @@ impl FakeRegistry { } } + /// A registry that came back from a restore short of `missing` blobs + /// holding `bytes` between them, and that discarded `discarded` blobs' + /// worth of bytes the log named none of. + fn restored(missing: BlobTally, discarded: BlobTally) -> Self { + Self { + boot: BootReconcile { + missing, + discarded, + }, + ..Self::new() + } + } + /// A registry with the blob limits a test wants to see enforced. fn with_blob_limits(limits: BlobLimits) -> Self { Self { @@ -681,7 +713,11 @@ impl Registry for FakeRegistry { RegistryStats { accounts: self.accounts.lock().expect("poisoned").len(), records: self.records.stats(), - blobs: self.blobs.stats(), + blobs: BlobStats { + missing_at_boot: self.boot.missing, + discarded_at_boot: self.boot.discarded, + ..self.blobs.stats() + }, } } } @@ -3307,10 +3343,63 @@ async fn an_empty_deployment_reports_zeroes() { let (status, _, body) = call(Arc::new(FakeRegistry::new()), get("/xrpc/bot.did.stats")).await; assert_eq!(status, StatusCode::OK); + let zero = json!({ "count": 0, "bytes": 0 }); assert_eq!( body, json!({ "accounts": 0, "records": 0, "bytes": 0, "collections": {}, - "blobs": 0, "blobBytes": 0 }) + "blobs": { + "held": zero, + "referenced": zero, + "unreferenced": zero, + "collectable": zero, + "missingAtBoot": zero, + "discardedAtBoot": zero, + } }) + ); +} + +/// What `stats` says about the disk this deployment came back on: whether it +/// held the blobs the log names, and whether it held blobs the log does not. +/// +/// Without them a deployment missing five hundred blobs answers a caller +/// identically to a healthy one, and the only evidence is a startup log line +/// that has since scrolled away and a `404` nobody may make for months. +/// +/// The two readings are separate figures because they are opposite faults: +/// one is bytes a restore has to bring back, the other is bytes it brought +/// that nothing can name. A single "the restore was untidy" number would +/// send an operator looking in the wrong place half the time. +/// +/// Counts and sizes, and no names. This route takes no credential by +/// default, so "which repository lost which blob" would be reconnaissance; +/// the names go to the operator, in the log. +#[tokio::test] +async fn stats_report_both_halves_of_what_a_restore_left() { + let (status, _, body) = call( + Arc::new(FakeRegistry::restored( + BlobTally { + count: 3, + bytes: 90, + }, + BlobTally { + count: 2, + bytes: 40, + }, + )), + get("/xrpc/bot.did.stats"), + ) + .await; + + assert_eq!(status, StatusCode::OK); + assert_eq!(body["blobs"]["missingAtBoot"], json!({"count": 3, "bytes": 90})); + assert_eq!( + body["blobs"]["discardedAtBoot"], + json!({"count": 2, "bytes": 40}) + ); + assert_eq!( + body["blobs"].as_object().map(|blobs| blobs.len()), + Some(6), + "counts are all this route owes an anonymous caller: {body}" ); } @@ -7914,120 +8003,3 @@ async fn a_frozen_accounts_session_and_app_password_keep_answering() { .passwords() .verify(SEEDED_DID, "correct-horse-battery")); } - -// --------------------------------------------------------------------------- -// The app-password methods: served, recognised, and answered with a decision. -// -// `routes::app_passwords_disabled` is the handler behind all three, and -// `ApiError::app_passwords_disabled` is the answer. What the tests below pin -// is that each method is mounted (rather than reaching `unknown_route`), that -// the answer names the credential this server does issue, and that it is byte -// for byte the same answer whoever asks -- which is what stops a refusal from -// being a way to find out which accounts exist here. -// --------------------------------------------------------------------------- - -/// The three vendored app-password methods, with the HTTP verb each one's -/// document declares: `listAppPasswords` is a `query`, the other two are -/// `procedure`s. -const APP_PASSWORD_METHODS: &[(&str, bool)] = &[ - ("com.atproto.server.createAppPassword", true), - ("com.atproto.server.listAppPasswords", false), - ("com.atproto.server.revokeAppPassword", true), -]; - -/// Builds the request each app-password method takes, optionally carrying a -/// bearer token. -fn app_password_request(nsid: &str, is_procedure: bool, token: Option<&str>) -> Request { - let uri = format!("/xrpc/{nsid}"); - let mut builder = Request::builder().uri(&uri); - if is_procedure { - builder = builder - .method("POST") - .header(header::CONTENT_TYPE, "application/json"); - } - if let Some(token) = token { - builder = builder.header(header::AUTHORIZATION, format!("Bearer {token}")); - } - let body = if is_procedure { - Body::from(r#"{"name":"kestrel"}"#) - } else { - Body::empty() - }; - builder.body(body).expect("request builds") -} - -/// Each one answers 403 `AppPasswordsDisabled`, naming the credential this -/// server issues. -#[tokio::test] -async fn each_app_password_method_answers_with_the_credential_this_server_issues() { - for (nsid, is_procedure) in APP_PASSWORD_METHODS { - let registry = Arc::new(FakeRegistry::seeded("kestrel")); - let (status, _, body) = call( - registry, - app_password_request(nsid, *is_procedure, Some(&seeded_token())), - ) - .await; - assert_eq!(status, StatusCode::FORBIDDEN, "{nsid} answered {body}"); - assert_eq!(body["error"], json!("AppPasswordsDisabled"), "{nsid}"); - let message = body["message"].as_str().unwrap_or_default(); - assert!( - message.contains(nsid) && message.contains("bot.did.provisionAgent"), - "{nsid}'s answer must name the method asked for and the credential this \ - server issues instead: {message}" - ); - } -} - -/// Every app-password method is mounted, rather than reaching the router's -/// fallback for a path nothing serves. -/// -/// The verb the document does *not* declare is what separates the two: a -/// mounted procedure answers 405 to a `GET`, while a path this router mounts -/// nothing at answers `unknown_route`'s 501 on both verbs. Without this, a -/// deleted route would still look refused, because a fallback refuses too. -#[tokio::test] -async fn each_app_password_method_is_mounted_rather_than_falling_back() { - for (nsid, is_procedure) in APP_PASSWORD_METHODS { - let registry = Arc::new(FakeRegistry::seeded("kestrel")); - // The other verb: a GET at a procedure, a POST at a query. - let (status, _, body) = - call(registry, app_password_request(nsid, !*is_procedure, None)).await; - assert_eq!( - status, - StatusCode::METHOD_NOT_ALLOWED, - "{nsid} is served on one verb, so the other must be a 405 rather than \ - the fallback's 501: {body}" - ); - } -} - -/// The answer does not vary with what the caller presents. -/// -/// No credential, a real agent token for the seeded account, and a string -/// that is not a token at all: one status and one body across all three, so -/// the route tells a caller nothing it did not already know. The handler -/// reads no header, which is what makes this true rather than merely -/// currently so. -#[tokio::test] -async fn the_app_password_answer_is_the_same_for_every_caller() { - for (nsid, is_procedure) in APP_PASSWORD_METHODS { - let mut answers = Vec::new(); - for token in [None, Some(seeded_token()), Some("not-a-token".to_owned())] { - let registry = Arc::new(FakeRegistry::seeded("kestrel")); - let (status, _, body) = call( - registry, - app_password_request(nsid, *is_procedure, token.as_deref()), - ) - .await; - answers.push((status, body)); - } - let first = &answers[0]; - for other in &answers[1..] { - assert_eq!( - first, other, - "{nsid} answered differently depending on the credential presented, \ - which is a way to probe for accounts" - ); - } - } -} diff --git a/crates/didbot-serve/src/wire.rs b/crates/didbot-serve/src/wire.rs index 0d9e3aa8..c3770872 100644 --- a/crates/didbot-serve/src/wire.rs +++ b/crates/didbot-serve/src/wire.rs @@ -807,12 +807,11 @@ pub fn record_uri(did: &str, collection: &str, rkey: &str) -> String { /// Response of `bot.did.stats`. /// -/// Flat rather than nested: it is read by a person running `curl` and by a -/// health line in the log, and neither wants to reach through a wrapper. +/// One field per subject, and a subject with more than one figure to it +/// carries them in an object of its own rather than in a row of prefixed +/// keys: `blobs` is what this deployment's blobs come to, and every question +/// about them is answered inside it. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] -// Every field here was one word until blobs got a second figure, so this -// changes nothing that was already on the wire and keeps the next field from -// being the one that decides the convention. #[serde(rename_all = "camelCase")] pub struct StatsResponse { /// How many accounts are provisioned. @@ -823,15 +822,94 @@ pub struct StatsResponse { pub bytes: usize, /// How many records each collection holds. pub collections: BTreeMap, - /// How many blobs exist across every repository. - pub blobs: usize, - /// What those blobs occupy, in bytes. + /// What this deployment's blobs come to, by the state each blob is in. /// - /// A separate figure from `bytes` rather than folded into it: records are + /// Measured apart from `bytes` rather than folded into it: records are /// measured as compact JSON and blobs as themselves, and one number over - /// two units would be a number nothing could act on. An operator watching - /// a development run wants to know which of the two is growing. - pub blob_bytes: u64, + /// two units would be a number nothing could act on. An operator + /// watching a run wants to know which of the two is growing. + pub blobs: BlobsView, +} + +/// How many blobs, and what they occupy. +/// +/// Every member of [`BlobsView`] is one of these, so that a reader taking a +/// figure out of this response gets the same two words wherever it took it +/// from. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct TallyView { + /// How many blobs are in this state. + pub count: u64, + /// What they occupy, in bytes. + pub bytes: u64, +} + +impl From for TallyView { + fn from(tally: didbot_pds::BlobTally) -> Self { + Self { + count: tally.count, + bytes: tally.bytes, + } + } +} + +/// The `blobs` member of [`StatsResponse`]: this deployment's blobs, by the +/// state each one is in. +/// +/// Aggregates over every repository and nothing narrower. `bot.did.stats` is +/// open by default — `auth::Disclosure` — so a figure that said which +/// account held what would publish one account's content to anyone who +/// asked; a total cannot. The names of the blobs an incomplete restore lost +/// are in the startup log, where the operator is, and `didbot_pds::Reconciled` +/// keeps a sample of them for that line. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct BlobsView { + /// Every blob the log names, whatever state it is in. + /// + /// `held.count` and `held.bytes` are this deployment's blob inventory + /// and its blob footprint. The three states below partition this one and + /// sum back to it. + pub held: TallyView, + /// Blobs at least one live record points at. + pub referenced: TallyView, + /// Blobs no live record points at, still inside the collection grace + /// window. + /// + /// The ordinary state of every upload between `uploadBlob` and the + /// `createRecord` that names it, so a nonzero reading is a working + /// server rather than a problem. + pub unreferenced: TallyView, + /// Blobs no live record points at whose grace window has run out: what + /// the next collection pass will take, and the quota it will give back. + /// + /// A figure that climbs and does not fall is the one an operator acts + /// on — it says collection is not getting through. + pub collectable: TallyView, + /// Blobs the log named whose bytes the disk did not have when this + /// server last started. + /// + /// Named for when it was read, because that is the only way to read it: + /// it is what the startup reconcile counted, held for the life of the + /// process. Nonzero means this deployment came back from an incomplete + /// copy and will answer `404` for that many blobs until they are put + /// back, and `bytes` is how much a restore would have to bring. Zero + /// means the disk agreed with the log *at that start*, not that it + /// agrees now: nothing recounts while the server runs, and a restart is + /// what takes the reading again. + /// + /// These blobs are in `held` as well. The log names them, so `listBlobs` + /// still does. + pub missing_at_boot: TallyView, + /// Bytes the disk held that the log named none of, deleted during that + /// same startup. + /// + /// An upload interrupted before it was named, a blob whose bytes landed + /// and whose log entry did not, or an account directory the log no + /// longer mentions. Nonzero after a restore says the copy carried blob + /// bytes the log could not account for. Nothing counted here is in + /// `held`. + pub discarded_at_boot: TallyView, } impl StatsResponse { @@ -842,8 +920,14 @@ impl StatsResponse { records: stats.records.records, bytes: stats.records.bytes, collections: stats.records.collections.clone(), - blobs: stats.blobs.blobs, - blob_bytes: stats.blobs.bytes, + blobs: BlobsView { + held: stats.blobs.held.into(), + referenced: stats.blobs.referenced.into(), + unreferenced: stats.blobs.unreferenced.into(), + collectable: stats.blobs.collectable.into(), + missing_at_boot: stats.blobs.missing_at_boot.into(), + discarded_at_boot: stats.blobs.discarded_at_boot.into(), + }, } } } diff --git a/crates/didbot/tests/conformance/wire.rs b/crates/didbot/tests/conformance/wire.rs index d7bcf0ee..1547361f 100644 --- a/crates/didbot/tests/conformance/wire.rs +++ b/crates/didbot/tests/conformance/wire.rs @@ -251,26 +251,10 @@ impl Answer { /// The list exists so that "the document does not declare this" is a decision /// somebody wrote down rather than a check nobody runs. Every other /// undeclared name fails. -const EXCUSED_ERRORS: &[(&str, &str, &str)] = &[ - ( - "com.atproto.server.createAppPassword", - "AppPasswordsDisabled", - APP_PASSWORDS_DISABLED_REASON, - ), - ( - "com.atproto.server.listAppPasswords", - "AppPasswordsDisabled", - APP_PASSWORDS_DISABLED_REASON, - ), - ( - "com.atproto.server.revokeAppPassword", - "AppPasswordsDisabled", - APP_PASSWORDS_DISABLED_REASON, - ), - ( - "com.atproto.repo.getRecord", - "RepoNotFound", - "the document declares only `RecordNotFound`, which cannot distinguish a \ +const EXCUSED_ERRORS: &[(&str, &str, &str)] = &[( + "com.atproto.repo.getRecord", + "RepoNotFound", + "the document declares only `RecordNotFound`, which cannot distinguish a \ record that is absent from a repository that was never here — and those \ are different things to a caller, because retrying helps with neither but \ only one of them means the account is gone. `com.atproto.sync.getRecord` \ @@ -278,28 +262,7 @@ const EXCUSED_ERRORS: &[(&str, &str, &str)] = &[ protocol's rather than one invented here; a client that does not know it \ for this method sees a generic `XRPCError`, which is what it would see \ from a 404 with no name at all", - ), -]; - -/// Why all three app-password methods answer a name their documents do not -/// declare. -/// -/// Each document declares at most `AccountTakedown`, which is a moderation -/// state; what this server answers is a decision about which credential an -/// account here holds, and `AppPasswordsDisabled` is the name for it — see -/// `didbot_serve::ApiError::app_passwords_disabled`. The two names XRPC -/// defines for every method would each say something else: `InvalidRequest` -/// invites the caller to fix the request, and `MethodNotImplemented` is what -/// this router answers for a path it mounts nothing at, so a client could not -/// tell a served decision from a typo in a URL. A client that does not know -/// the name sees a 403 with a message naming the credential this server -/// issues instead, which is more than a declared name would have told it. -const APP_PASSWORDS_DISABLED_REASON: &str = "the documents declare only \ - `AccountTakedown`, a moderation state, and this answer is a decision \ - about which credential an account here holds: the agent token \ - `bot.did.provisionAgent` returns. `MethodNotImplemented` is this \ - router's answer for a path nothing is mounted at, so reusing it would \ - make a served decision indistinguishable from a mistyped URL"; +)]; /// Sends one request to the router. async fn call(app: &axum::Router, request: Request) -> Answer { @@ -1178,85 +1141,3 @@ async fn a_thrown_revoke_refuses_a_write_over_the_real_xrpc_surface() { ); assert_eq!(after.body["error"], "Halted"); } - -// --------------------------------------------------------------------------- -// The app-password methods -// --------------------------------------------------------------------------- - -/// Every vendored method whose NSID names an app password, read off disk. -/// -/// The list is the documents this repository vendors rather than one written -/// here: a document arriving in a later `scripts/refresh-atproto-lexicons.sh` -/// run joins this list on its own, and the assertions below then apply to it. -fn vendored_app_password_methods() -> Vec { - let mut found: Vec = json_files(&lexicon_dir()) - .into_iter() - .filter_map(|path| { - let text = std::fs::read_to_string(&path).expect("a vendored document reads"); - let doc: LexiconDoc = serde_json::from_str(&text).expect("it is a lexicon"); - doc.id.contains("AppPassword").then_some(doc.id) - }) - .collect(); - found.sort(); - assert!( - !found.is_empty(), - "no app-password documents under {} — run scripts/refresh-atproto-lexicons.sh", - lexicon_dir().display() - ); - found -} - -/// Every app-password document this repository vendors is a method this -/// server serves, and each one answers the same decision to every caller. -/// -/// Three assertions in one loop, and each is load-bearing. The route table -/// covers the vendored documents, so a method upstream defines is one an -/// arriving client gets an answer with a reason in it for. The answer is -/// `AppPasswordsDisabled` — the name [`EXCUSED_ERRORS`] carries this -/// server's reasoning for — under a 403. And it is the same answer with the -/// account's own agent token as with no credential at all, so a caller -/// cannot read anything about this deployment's accounts out of it. -#[tokio::test] -async fn every_vendored_app_password_method_answers_one_decision() { - let (app, _did, token) = server(); - - for nsid in vendored_app_password_methods() { - assert!( - didbot::serve::ATPROTO_METHODS.contains(&nsid.as_str()), - "{nsid} is vendored and this server's route table does not carry it" - ); - let method = upstream().method(&nsid).expect("a vendored method"); - - let mut bodies = Vec::new(); - for credential in [None, Some(token.as_str())] { - let answer = if method.kind == "procedure" { - procedure( - &app, - &nsid, - json!({ "name": "wire-conformance" }), - credential, - ) - .await - } else if let Some(credential) = credential { - query_authed(&app, credential, &nsid, "").await - } else { - query(&app, &nsid, "").await - }; - assert_eq!( - answer.status, - StatusCode::FORBIDDEN, - "{nsid} answered {}: {}", - answer.status, - answer.body - ); - let body = answer.body.clone(); - assert_eq!(answer.declared_error(&nsid), "AppPasswordsDisabled"); - bodies.push(body); - } - assert_eq!( - bodies[0], bodies[1], - "{nsid} answered the account's own credential differently from an \ - anonymous caller, which is a way to probe for accounts" - ); - } -} diff --git a/docs/conformance.md b/docs/conformance.md index bb3749c4..f40c113f 100644 --- a/docs/conformance.md +++ b/docs/conformance.md @@ -418,39 +418,6 @@ account is frozen reads this, not the firehose. There is no per-agent `get` route yet. When there is, it carries the same field for the same reason. -## App passwords: recognised, and answered with a reason - -`com.atproto.server.createAppPassword`, `listAppPasswords` and -`revokeAppPassword` are routes this server serves. Each answers 403 -`AppPasswordsDisabled`, and the message names the credential an account here -holds instead: the agent token `bot.did.provisionAgent` returns once, bound -to one DID. The accounts on this deployment are agents, and an app password -is a secret premised on a person typing it into a settings page — so the -answer is a decision this server states, in the shape -[auth-types](../plan/auth-types.md) asks for: recognised, not accepted. - -`AppPasswordsDisabled` is a name none of the three documents declares, so it -sits in `wire.rs`'s `EXCUSED_ERRORS` with the reasoning attached, the same -place every other deliberate departure from a document is written down. The -status separates it from `MethodNotImplemented`, which this router answers -for a path it mounts nothing at: a client can tell a decision from a mistyped -URL by the status and the name alike. - -The conformance suite reads the vendored documents to find these methods -rather than naming them: every document under `vendor/atproto-lexicons/` -whose NSID names an app password must be in the route table, and each is -driven through the real router twice — once with the account's own agent -token, once with nothing — and the two answers must be identical, so the -refusal carries no information about which accounts exist here. - -`com.atproto.server.createSession` verifies against -`didbot_pds::AppPasswordStore`, which this deployment writes in process, and -answers `InvalidRequest` under a 401 for a wrong password, an unknown -identifier and an account whose store holds no password alike. That single -answer is the account-enumeration bound, and it is why the named -`AppPasswordsDisabled` decision belongs on the three methods above and not -here. - ## Legacy sessions, checked against the lexicon schema `com.atproto.server.{create,refresh,delete,get}Session` are new surface, diff --git a/docs/operations.md b/docs/operations.md index 2a8d0e63..92f345e7 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -90,7 +90,9 @@ append in flight when the snapshot was taken. | Check | Confirms | |---|---| | `GET /health` returns `200` with `ticks` advancing between two calls a few seconds apart | The process is live and its ten-second tick is running | -| `GET /xrpc/bot.did.stats` | Accounts, records, bytes per collection, blobs and blob bytes match what the deployment had | +| `GET /xrpc/bot.did.stats` | Accounts, records and bytes per collection match what the deployment had, and `blobs.held` matches its blob inventory | +| `blobs.missingAtBoot` in that response reads `{"count": 0, "bytes": 0}` | Every blob the log names had its bytes on the disk this deployment started on. A nonzero `count` is that many `404`s waiting, and `bytes` is what the restore would have to bring back; the CIDs are in the startup `ERROR` line | +| `blobs.discardedAtBoot` reads `{"count": 0, "bytes": 0}` | The copy carried no blob bytes the log could not account for. A nonzero count is that many files deleted during startup | | `GET /xrpc/bot.did.listAgents` | The accounts that should be there are there | | One agent's `did.json` resolves | The signing keys came back | | Fetch one blob that account references | The bytes are on the disk; `getBlob` re-hashes what it reads and refuses on a mismatch | diff --git a/plan/auth-types.md b/plan/auth-types.md index a8d8d352..14124990 100644 --- a/plan/auth-types.md +++ b/plan/auth-types.md @@ -71,6 +71,10 @@ nothing to this server. an `aud` naming a service on a DID rather than the bare DID, and `lxm` required rather than optional. - [ ] **Per-principal rate limits.** Not built. +- [ ] **The caller-facing app-password lexicon methods.** `createSession` + works; `com.atproto.server.{create,list,revoke}AppPassword` do not + exist, so an operator sets the one password this server tracks per + account directly rather than a caller managing several. ### The write surface, and the credential that unlocked it @@ -261,24 +265,11 @@ it holds, or lifting the freeze would mean re-provisioning it. operator sets one through `didbot_pds::SessionAuth::passwords`, hashed with Argon2id (`AppPasswordHash`) and never stored reversibly. `createSession` refuses a wrong identifier and a wrong password - identically, so a caller cannot use it to enumerate accounts. The - caller-facing methods answer rather than issue — see "The - app-password methods answer" below. -- [x] **The app-password methods answer.** - `com.atproto.server.createAppPassword`, `listAppPasswords` and - `revokeAppPassword` are served, and each answers 403 - `AppPasswordsDisabled` naming the credential an account here holds - instead: the agent token `bot.did.provisionAgent` returns once, bound - to one DID. This is the method-level form of what this epic asks for a - credential scheme — recognised, not accepted — and it is the same - argument the agent token itself was introduced on: the callers here are - machines, and an app password is a long-lived replayable secret - premised on a person typing it, so issuing one would put a human - credential shape in a machine's hands. The three routes read no header, - so the answer is identical for every caller and carries nothing about - which accounts exist. `createSession` keeps verifying against - `AppPasswordStore` and keeps its one uniform refusal, which is where - the account-enumeration bound lives. + identically, so a caller cannot use it to enumerate accounts. Not + built: `com.atproto.server.{create,list,revoke}AppPassword` as + caller-facing methods — an operator sets the one password this server + tracks per account directly, a smaller surface than the full lexicon + set. - [x] **Say what an operator credential is: there is not one.** No scheme this server accepts authenticates an operator, and none should. A server learns which DID operates it by reading `bot.did.operator` out @@ -331,4 +322,5 @@ it holds, or lifting the freeze would mean re-provisioning it. same case, plus the cross-account `RepoMismatch` case, through the real router and the vendored lexicon documents. -Left open, on purpose: inter-service auth and per-principal rate limits. +Left open, on purpose: inter-service auth, per-principal rate limits, and +the caller-facing app-password lexicon methods. diff --git a/plan/blob-storage-tiers.md b/plan/blob-storage-tiers.md index ceade0b8..6aa751f3 100644 --- a/plan/blob-storage-tiers.md +++ b/plan/blob-storage-tiers.md @@ -146,13 +146,14 @@ an account holder asking for their data to be gone does not want. [`durable`](../crates/didbot-pds/src/durable.rs) is already the shape of that check; the proposal is that this configuration change is refused by name, with the migration being an explicit pass rather than a restart. -- [ ] **Two footprints, and `bot.did.stats` reports one.** `BlobStats` counts - blobs and bytes, and `StatsResponse` in - [`didbot-serve`](../crates/didbot-serve/src/wire.rs) publishes them as - `blobs` and `blobBytes`. A blob held on both tiers occupies a volume and - a bucket, and those are two operator-visible numbers with two prices — - see [cost](cost.md). Proposal, and a wire addition so it wants review: - `blobBytes` keeps meaning the disk figure it means today, and the object - figure is a field beside it. +- [ ] **Two footprints, and `bot.did.stats` reports one.** `BlobStats` tallies + blobs by lifecycle state, and `BlobsView` in + [`didbot-serve`](../crates/didbot-serve/src/wire.rs) publishes those + tallies under the response's `blobs` object. A blob held on both tiers + occupies a volume and a bucket, and those are two operator-visible + numbers with two prices — see [cost](cost.md). Proposal, and a wire + addition so it wants review: each tally keeps meaning the disk figure it + means today, and the object figures go in a sibling member of the same + `blobs` object rather than a second row of top-level keys. ## Done diff --git a/plan/periodic-backups.md b/plan/periodic-backups.md index 9e319afb..93cf2fda 100644 --- a/plan/periodic-backups.md +++ b/plan/periodic-backups.md @@ -140,8 +140,9 @@ twenty-four hours, and a copy that survives the account the vault is in. ## Saying whether it is working - [ ] **Backup health belongs on `bot.did.stats`.** `StatsResponse` in - [`didbot-serve`](../crates/didbot-serve/src/wire.rs) is flat, camelCase - and read by a person with `curl`. Proposal, and it is a wire addition so + [`didbot-serve`](../crates/didbot-serve/src/wire.rs) is camelCase, read + by a person with `curl`, and gives a subject with several figures to it + an object of its own — `blobs` is the one that has. Proposal, and it is a wire addition so it wants owner review before it is written: when the last successful backup completed and what offset it covered. An age is the figure that turns "backups are configured" into "backups are happening", and -- 2.51.2