From 7dc5e82e1e0f1129d41f62263efa14fb02408cd6 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 19:58:59 +0300 Subject: [PATCH 01/11] docs: P36b+P36c implementation plan (checkpoints + bucket-backed serve) --- .../plans/2026-08-26-wal-bucket-p36b-p36c.md | 952 ++++++++++++++++++ 1 file changed, 952 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-26-wal-bucket-p36b-p36c.md diff --git a/docs/superpowers/plans/2026-08-26-wal-bucket-p36b-p36c.md b/docs/superpowers/plans/2026-08-26-wal-bucket-p36b-p36c.md new file mode 100644 index 0000000..cb0fe6c --- /dev/null +++ b/docs/superpowers/plans/2026-08-26-wal-bucket-p36b-p36c.md @@ -0,0 +1,952 @@ +# P36b + P36c: WAL Checkpoints and Bucket-Backed Serve Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** P36b — cold start over a bucket remote reads one checkpoint plus the log tail instead of replaying the whole log, with checkpoints folded opportunistically and coordinator-free; P36c — `sc serve --http|--stdio` can serve a bucket (`--store sc+wal://…|sc+s3://…`), making server instances disposable while tokens, TLS, and limits work unchanged. + +**Architecture:** P36b adds a third record kind to `walfmt` (`Checkpoint`: folded refs + cumulative pack list), teaches `BucketTransport::refresh()` to stop its parent-chain walk at `manifest.checkpoint_seq` and seed from the checkpoint, and hooks a best-effort fold into `update_ref` after a successful CAS. P36c generalizes the wire serve loop's transport from concrete `LocalTransport` to a two-variant `ServeTransport` enum (only `GetPack`/`PutPack` differ; the other verbs already dispatch through the `Transport` trait), threads an optional store URL through the HTTP path, and adds a `--store` CLI flag. The serve host keeps a local `.sc/` "serve home" for tokens/TLS/tmp spills — the bucket is the object source of truth. + +**Tech Stack:** Rust, existing crates only (`scl-objio`, `scl-repo`, `scl-cli`); no new dependencies. + +**Spec:** `docs/superpowers/specs/2026-08-26-wal-bucket-backend-design.md` (sections "Data flow → Checkpoints", "Clone / cold start", "Bucket-backed sc serve"). Prior phase: ADR-0046, plan `docs/superpowers/plans/2026-08-26-wal-bucket-backend-p36a.md`. + +## Global Constraints + +- Everything read from the bucket is untrusted: new checkpoint bytes go through the existing `capped()` guard (`crates/repo/src/bucket_transport.rs:61-79`) before decode; strict versioned decode fails closed; every count capped before allocation; branch names from checkpoints validated with `crate::refs::validate_branch_name`. +- Readers trust only what the manifest chain references: a checkpoint is authoritative only when `manifest.checkpoint_seq` names it AND its own `seq` field matches; a chain that bypasses the checkpoint (parent < checkpoint_seq without landing on it) is a loud `Error::Wal`. +- Checkpoint folding is opportunistic and best-effort: a fold failure or lost CAS must NEVER fail the push that triggered it (the commit already landed; the checkpoint is derived data any reader can rebuild). Threshold: `const CHECKPOINT_INTERVAL: u64 = 64` — one tunable constant (spec: "default 64 entries, one tunable constant"). +- P36c changes no wire protocol byte: `PROTOCOL_VERSION` stays 4; clients are untouched; `WirePolicy` read-only gates, P29 tokens, P31 limits, P32 TLS behave identically in `--store` mode. +- The serve home (`` arg) must contain `.sc/` exactly as today (404 gate at `http_transport.rs:996-999`, token load at `:1008`, TLS dir) — in `--store` mode its object store is simply never consulted. +- Errors: per-crate `thiserror`, lowercase, no trailing period; CLI uses `anyhow`. +- Every public type/fn gets an intent doc comment; tests live in `#[cfg(test)] mod tests` next to the code, clean up temp dirs, and assert the path is gone. +- Verification gate for EVERY task (CI runs fmt before tests): `cargo fmt --all -- --check` in addition to the task's tests and `cargo clippy` — a clippy-clean, test-green diff still fails CI if rustfmt is unhappy. +- Never silently drop data; refusals are loud and typed. + +--- + +### Task 1: `walfmt::Checkpoint` — third record kind + key helper + +**Files:** +- Modify: `crates/repo/src/walfmt.rs` (constants at :7-12, key helpers at :225-237, tests at :240+) + +**Interfaces:** +- Consumes: existing private `Cursor` (`take/u8/u32/u64/string/id/done`, walfmt.rs:17-66), `header()` (:70), `push_string()` (:47), consts `VERSION`, `MAX_NAME`, `MAX_LIST`, `MAX_HASH`. +- Produces (used by Tasks 2, 3): + - `pub struct Checkpoint { pub seq: u64, pub refs: Vec<(String, ObjectId)>, pub packs: Vec }` with `#[derive(Debug, Clone, PartialEq, Eq)]`, `pub fn encode(&self) -> Vec`, `pub fn decode(bytes: &[u8]) -> Result` + - `pub fn checkpoint_key(seq: u64) -> String` → `checkpoints/` + - New const `CHECKPOINT_MAGIC: &[u8; 4] = b"SCWC"` (private, beside the other two) + +- [ ] **Step 1: Write the failing tests** (append inside `mod tests`; helper `some_id` exists at walfmt.rs:244) + +```rust + #[test] + fn checkpoint_round_trips_and_rejects_garbage() { + let c = Checkpoint { + seq: 64, + refs: vec![ + ("feat".to_string(), some_id(2)), + ("main".to_string(), some_id(1)), + ], + packs: vec!["ab12".to_string(), "cd34".to_string()], + }; + let bytes = c.encode(); + assert_eq!(Checkpoint::decode(&bytes).unwrap(), c); + // wrong magic, truncated, future version, trailing junk: refused + assert!(Checkpoint::decode(b"XXXX").is_err()); + assert!(Checkpoint::decode(&bytes[..bytes.len() - 1]).is_err()); + let mut future = bytes.clone(); + future[4] = 0xFF; + assert!(Checkpoint::decode(&future).is_err()); + let mut junk = bytes.clone(); + junk.push(0); + assert!(Checkpoint::decode(&junk).is_err()); + // a log-entry buffer is not a checkpoint (magic mismatch, not a panic) + let entry = LogEntry { seq: 1, parent_seq: 0, packs: vec![], updates: vec![] }; + assert!(Checkpoint::decode(&entry.encode()).is_err()); + } + + #[test] + fn checkpoint_decode_caps_hostile_counts() { + // corrupt the refs count to u32::MAX: must fail fast, not allocate + let c = Checkpoint { seq: 1, refs: vec![("m".to_string(), some_id(1))], packs: vec![] }; + let mut evil = c.encode(); + // refs count sits right after magic(4)+version(4)+seq(8) = offset 16 + evil[16..20].copy_from_slice(&u32::MAX.to_le_bytes()); + assert!(Checkpoint::decode(&evil).is_err()); + } + + #[test] + fn checkpoint_key_is_stable() { + assert_eq!(checkpoint_key(64), "checkpoints/00000000000000000064"); + } +``` + +- [ ] **Step 2: Run to verify compile failure** + +Run: `cargo test -p scl-repo walfmt` +Expected: FAIL — `Checkpoint`, `checkpoint_key` not found. + +- [ ] **Step 3: Implement** + +Beside `ENTRY_MAGIC` (walfmt.rs:8): `const CHECKPOINT_MAGIC: &[u8; 4] = b"SCWC";`. Struct + codec mirroring `LogEntry`'s exact patterns (count capped against `MAX_LIST` BEFORE `Vec::with_capacity`; branch names via `c.string(MAX_NAME)?` then 32-byte `c.id()?`; pack hashes via `c.string(MAX_HASH)?`; end with `c.done()?`): + +```rust +/// A fold of the WAL at `seq`: every branch tip and every on-chain pack hash +/// accumulated from the chain's start through log entry `seq`. Cold start = +/// this + the log tail after `seq`, instead of replaying the whole chain. +/// Referenced (and made authoritative) only by `Manifest.checkpoint_seq`; +/// an unreferenced checkpoint object is garbage like any off-chain key. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Checkpoint { + pub seq: u64, + /// branch -> tip pairs, sorted by branch (BTreeMap iteration order). + pub refs: Vec<(String, ObjectId)>, + /// Cumulative pack hashes in chain order (oldest first). + pub packs: Vec, +} + +impl Checkpoint { + pub fn encode(&self) -> Vec { + let mut out = Vec::new(); + out.extend_from_slice(CHECKPOINT_MAGIC); + out.extend_from_slice(&VERSION.to_le_bytes()); + out.extend_from_slice(&self.seq.to_le_bytes()); + out.extend_from_slice(&(self.refs.len() as u32).to_le_bytes()); + for (branch, id) in &self.refs { + push_string(&mut out, branch); + out.extend_from_slice(id.as_bytes()); + } + out.extend_from_slice(&(self.packs.len() as u32).to_le_bytes()); + for hash in &self.packs { + push_string(&mut out, hash); + } + out + } + + pub fn decode(bytes: &[u8]) -> Result { + let mut c = header(bytes, CHECKPOINT_MAGIC, "checkpoint")?; + let seq = c.u64()?; + let nrefs = c.u32()? as usize; + if nrefs > MAX_LIST { + return Err(Error::Wal(format!("checkpoint ref count {nrefs} exceeds cap"))); + } + let mut refs = Vec::with_capacity(nrefs); + for _ in 0..nrefs { + let branch = c.string(MAX_NAME)?; + let id = c.id()?; + refs.push((branch, id)); + } + let npacks = c.u32()? as usize; + if npacks > MAX_LIST { + return Err(Error::Wal(format!("checkpoint pack count {npacks} exceeds cap"))); + } + let mut packs = Vec::with_capacity(npacks); + for _ in 0..npacks { + packs.push(c.string(MAX_HASH)?); + } + c.done()?; + Ok(Checkpoint { seq, refs, packs }) + } +} +``` +And beside `log_key` (:225): +```rust +/// `checkpoints/` zero-padded so lexical order == numeric order. +pub fn checkpoint_key(seq: u64) -> String { + format!("checkpoints/{seq:020}") +} +``` +(`id.as_bytes()` / `ObjectId::from_bytes` are the constructors the existing codec already uses — walfmt.rs:52, LogEntry encode.) + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-repo walfmt && cargo fmt --all -- --check && cargo clippy -p scl-repo --all-targets` +Expected: PASS / clean. + +- [ ] **Step 5: Commit** + +```bash +git add crates/repo/src/walfmt.rs +git commit -m "feat(repo): walfmt Checkpoint record kind + checkpoint_key (P36b)" +``` + +--- + +### Task 2: checkpoint-aware `refresh()` — cold start = checkpoint + log tail + +**Files:** +- Modify: `crates/repo/src/bucket_transport.rs` (`WalView` :18-28, `refresh()` :172-238, tests) + +**Interfaces:** +- Consumes: Task 1's `Checkpoint`, `checkpoint_key`; existing `capped()` (:61), `log_key`/`idx_key`, `crate::refs::validate_branch_name`, `parse_index`. +- Produces (used by Task 3): `WalView` gains `packs: Vec` (cumulative, chain order — checkpoint-seeded packs first, then tail packs); `refresh()` stops the chain walk at `checkpoint_seq` and seeds refs+packs from the checkpoint. Everything else about `WalView` (`tag`, `manifest`, `refs`, `index`) unchanged. + +- [ ] **Step 1: Write the failing tests** (in `mod tests`; helpers `pack_of` :523, `tiny_history` :538 exist; hand-building a WAL directly against `DirBucket` is the established pattern — see :588 and :652) + +```rust + /// Hand-build a WAL with `n` single-branch pushes; returns (bucket root, + /// final tip per branch map as Vec sorted, all pack hashes in order). + /// Each push i creates branch "b-" pointing at a distinct object. + fn hand_built_wal(tag: &str, n: u64) -> (std::path::PathBuf, Vec<(String, ObjectId)>, Vec) { + let broot = std::env::temp_dir().join(format!("scl-bt-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&broot); + let bucket = DirBucket::open(&broot).unwrap(); + let mut refs = Vec::new(); + let mut packs = Vec::new(); + for i in 1..=n { + let obj = scl_core::Object::Blob(scl_core::Blob { + bytes: format!("wal-entry-{i}").into_bytes().into(), + }); + let id = obj.id(); + let (hash, pack, idx) = pack_of(&[(id, obj.encode())]); + bucket.put_new(&pack_key(&hash), &pack).unwrap(); + bucket.put_new(&idx_key(&hash), &idx).unwrap(); + let entry = LogEntry { + seq: i, + parent_seq: i - 1, + packs: vec![hash.clone()], + updates: vec![RefUpdate { branch: format!("b-{i}"), old: None, new: id }], + }; + bucket.put_new(&log_key(i), &entry.encode()).unwrap(); + refs.push((format!("b-{i}"), id)); + packs.push(hash); + } + let m = Manifest { head_seq: n, checkpoint_seq: 0, head_branch: "b-1".into() }; + bucket.put_if_tag("manifest", &m.encode(), None).unwrap().unwrap(); + refs.sort(); + (broot, refs, packs) + } + // NOTE: adapt the Object construction line to however `pack_of`'s existing + // callers mint distinct blob objects in this test module (see + // tiny_history_distinct at :566) — the intent is "n distinct valid objects". + + #[test] + fn view_via_checkpoint_equals_full_replay_and_skips_folded_entries() { + let (broot, expected_refs, packs) = hand_built_wal("ckpt-eq", 6); + let bucket = DirBucket::open(&broot).unwrap(); + // fold through seq 4 by hand + let full = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + let full_refs = full.list_refs().unwrap(); + let ck = Checkpoint { + seq: 4, + refs: expected_refs.iter().filter(|(b, _)| { + let i: u64 = b.strip_prefix("b-").unwrap().parse().unwrap(); + i <= 4 + }).cloned().collect(), + packs: packs[..4].to_vec(), + }; + bucket.put_new(&checkpoint_key(4), &ck.encode()).unwrap(); + let Fetched::New { bytes, tag } = bucket.get("manifest", None).unwrap() else { panic!() }; + let mut m = Manifest::decode(&bytes).unwrap(); + m.checkpoint_seq = 4; + bucket.put_if_tag("manifest", &m.encode(), Some(&tag)).unwrap().unwrap(); + + // DELETE the folded log entries: a checkpoint-aware reader must not + // need them. (Direct file removal = simulated compaction.) + for seq in 1..=4u64 { + std::fs::remove_file(broot.join(log_key(seq))).unwrap(); + } + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!(t.list_refs().unwrap(), full_refs); + // objects from folded packs still readable (index seeded from checkpoint.packs) + let (b1, id1) = &expected_refs[0]; + assert!(b1.starts_with("b-")); + assert!(t.has_object(id1).unwrap()); + drop((t, full)); + std::fs::remove_dir_all(&broot).unwrap(); + } + + #[test] + fn corrupt_or_bypassing_checkpoints_fail_closed() { + let (broot, _refs, packs) = hand_built_wal("ckpt-bad", 3); + let bucket = DirBucket::open(&broot).unwrap(); + // (a) manifest names a checkpoint that does not exist + let Fetched::New { bytes, tag } = bucket.get("manifest", None).unwrap() else { panic!() }; + let mut m = Manifest::decode(&bytes).unwrap(); + m.checkpoint_seq = 2; + let tag = bucket.put_if_tag("manifest", &m.encode(), Some(&tag)).unwrap().unwrap(); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + // (b) checkpoint exists but its seq field lies + let ck = Checkpoint { seq: 1, refs: vec![], packs: packs[..2].to_vec() }; + bucket.put_new(&checkpoint_key(2), &ck.encode()).unwrap(); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + // (c) chain bypasses the checkpoint: entry at seq 3 has parent 1 (< 2) + let obj_end = { + // repair (b) first so the error is unambiguously the bypass + std::fs::remove_file(broot.join(checkpoint_key(2))).unwrap(); + let good = Checkpoint { seq: 2, refs: vec![], packs: packs[..2].to_vec() }; + bucket.put_new(&checkpoint_key(2), &good.encode()).unwrap(); + let bad_entry = LogEntry { seq: 3, parent_seq: 1, packs: vec![], updates: vec![] }; + std::fs::remove_file(broot.join(log_key(3))).unwrap(); + bucket.put_new(&log_key(3), &bad_entry.encode()).unwrap() + }; + assert!(obj_end); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + let _ = tag; + std::fs::remove_dir_all(&broot).unwrap(); + } +``` + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test -p scl-repo bucket_transport::tests::view_via` +Expected: FAIL — `checkpoint_key`/`Checkpoint` unimported at first, then (after imports) the equals test fails because refresh walks past the checkpoint into the deleted entries (`Error::Wal("log entry 4 referenced by chain but absent")` from `from_bucket`). + +- [ ] **Step 3: Implement** + +`WalView` gains the field (after `refs`): +```rust + /// Every on-chain pack hash in chain order (checkpoint fold first, then + /// the tail) — retained so a checkpoint fold (Task 3) is a pure copy. + packs: Vec, +``` +`refresh()`'s `Fetched::New` arm changes (current body verbatim at :186-231; the walk is `while seq != 0 { … }` then `entries.reverse()` then the refs/index build). New shape: + +```rust + let manifest = Manifest::decode(&bytes)?; + let stop = manifest.checkpoint_seq; + // Seed from the checkpoint when the manifest names one. The + // checkpoint is untrusted input like everything else here. + let (mut refs, mut packs): (BTreeMap, Vec) = + if stop != 0 { + let ck_bytes = match self.bucket.get(&checkpoint_key(stop), None)? { + Fetched::New { bytes, .. } => capped("checkpoint", bytes)?, + _ => { + return Err(Error::Wal(format!( + "checkpoint {stop} referenced by manifest but absent" + ))) + } + }; + let ck = Checkpoint::decode(&ck_bytes)?; + if ck.seq != stop { + return Err(Error::Wal(format!( + "checkpoint at {stop} claims seq {}", + ck.seq + ))); + } + let mut refs = BTreeMap::new(); + for (branch, id) in &ck.refs { + crate::refs::validate_branch_name(branch)?; + refs.insert(branch.clone(), *id); + } + (refs, ck.packs) + } else { + (BTreeMap::new(), Vec::new()) + }; + let mut entries = Vec::new(); + let mut seq = manifest.head_seq; + while seq != stop { + if seq < stop { + return Err(Error::Wal(format!( + "log chain bypasses checkpoint {stop} (reached {seq})" + ))); + } + let Fetched::New { bytes, .. } = self.bucket.get(&log_key(seq), None)? else { + return Err(Error::Wal(format!( + "log entry {seq} referenced by chain but absent" + ))); + }; + let e = LogEntry::decode(&capped("log entry", bytes)?)?; + // …seq/parent validation identical to today (:195-206)… + seq = e.parent_seq; + entries.push(e); + } + entries.reverse(); // oldest first + for e in &entries { + for u in &e.updates { + crate::refs::validate_branch_name(&u.branch)?; + refs.insert(u.branch.clone(), u.new); + } + for hash in &e.packs { + packs.push(hash.clone()); + } + } + // Index build: over the FULL cumulative pack list (checkpoint + // packs + tail packs), fetching each idx exactly as today. + let mut index = BTreeMap::new(); + for hash in &packs { + let Fetched::New { bytes, .. } = self.bucket.get(&idx_key(hash), None)? else { + return Err(Error::Wal(format!("pack {hash} on chain but idx absent"))); + }; + let bytes = capped("pack idx", bytes)?; + for IndexEntry { id, offset, length } in parse_index(&bytes)? { + index.insert(id, (hash.clone(), offset, length)); + } + } + *self.view.borrow_mut() = Some(WalView { tag, manifest, refs, index, packs }); +``` +Note the `head_seq == checkpoint_seq` case falls out naturally (`while seq != stop` runs zero times). Keep the existing seq-claims-vs-slot and parent-strictly-decreasing checks verbatim inside the loop. + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-repo bucket_transport && cargo fmt --all -- --check && cargo clippy -p scl-repo --all-targets` +Expected: all pass (the 13 existing tests prove no regression for `checkpoint_seq == 0`), clean. + +- [ ] **Step 5: Commit** + +```bash +git add crates/repo/src/bucket_transport.rs +git commit -m "feat(repo): checkpoint-aware refresh — cold start reads checkpoint + log tail (P36b)" +``` + +--- + +### Task 3: opportunistic checkpoint fold in `update_ref` + +**Files:** +- Modify: `crates/repo/src/bucket_transport.rs` (`update_ref` success arm :488-498, new helper, tests) + +**Interfaces:** +- Consumes: Task 2's `WalView.packs`; Task 1's `Checkpoint`/`checkpoint_key`; `Bucket::{put_new, put_if_tag}`. +- Produces: `const CHECKPOINT_INTERVAL: u64 = 64;` (module-level, doc-commented as the spec's single tunable) and private `fn maybe_fold_checkpoint(&self)` called after the successful CAS + refresh in `update_ref`. Fold is best-effort: all its errors are swallowed by the caller (`let _ = …`), with a doc comment stating why that is correct (derived data; next over-threshold push retries; losing the fold CAS to a racing pusher is the expected outcome, not a failure). + +- [ ] **Step 1: Write the failing tests** + +```rust + #[test] + fn pushes_past_the_interval_fold_a_checkpoint_and_cold_start_uses_it() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-bt-fold-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + let n = CHECKPOINT_INTERVAL + 2; + for i in 0..n { + let obj = /* distinct blob, same construction as fleet test :960 */; + t.put_object(&obj.id(), &obj.encode()).unwrap(); + t.update_ref(&format!("w-{i}"), &obj.id(), None).unwrap(); + } + // the bucket now carries a manifest whose checkpoint_seq > 0 and the + // matching checkpoints/ object + let bucket = DirBucket::open(&broot).unwrap(); + let Fetched::New { bytes, .. } = bucket.get("manifest", None).unwrap() else { panic!() }; + let m = Manifest::decode(&bytes).unwrap(); + assert!(m.checkpoint_seq > 0, "no fold happened after {n} pushes"); + let Fetched::New { bytes, .. } = bucket.get(&checkpoint_key(m.checkpoint_seq), None).unwrap() else { + panic!("manifest names checkpoint {} but object absent", m.checkpoint_seq) + }; + let ck = Checkpoint::decode(&bytes).unwrap(); + assert_eq!(ck.seq, m.checkpoint_seq); + assert!(!ck.refs.is_empty() && !ck.packs.is_empty()); + // cold start through it sees all n branches + let t2 = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!(t2.list_refs().unwrap().len(), n as usize); + drop((t, t2)); + std::fs::remove_dir_all(&broot).unwrap(); + } + + /// A bucket whose checkpoint writes always fail must not fail pushes. + struct FoldHostileBucket(DirBucket); + impl Bucket for FoldHostileBucket { + fn get(&self, key: &str, tag: Option<&str>) -> scl_objio::Result { + self.0.get(key, tag) + } + fn put_new(&self, key: &str, bytes: &[u8]) -> scl_objio::Result { + if key.starts_with("checkpoints/") { + return Err(scl_objio::Error::Backend("injected checkpoint write failure".into())); + } + self.0.put_new(key, bytes) + } + fn put_if_tag(&self, key: &str, bytes: &[u8], tag: Option<&str>) -> scl_objio::Result> { + self.0.put_if_tag(key, bytes, tag) + } + fn list(&self, prefix: &str) -> scl_objio::Result> { + self.0.list(prefix) + } + } + + #[test] + fn fold_failure_never_fails_the_push() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-bt-foldfail-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let t = BucketTransport::from_bucket(Box::new(FoldHostileBucket(DirBucket::open(&broot).unwrap()))).unwrap(); + for i in 0..(CHECKPOINT_INTERVAL + 2) { + let obj = /* distinct blob as above */; + t.put_object(&obj.id(), &obj.encode()).unwrap(); + t.update_ref(&format!("w-{i}"), &obj.id(), None).unwrap(); // must all be Ok + } + // no checkpoint could land; manifest still says 0 and reads still work + let t2 = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!(t2.list_refs().unwrap().len(), (CHECKPOINT_INTERVAL + 2) as usize); + drop((t, t2)); + std::fs::remove_dir_all(&broot).unwrap(); + } +``` + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test -p scl-repo bucket_transport::tests::pushes_past` +Expected: FAIL — `CHECKPOINT_INTERVAL` not found; then (once compiling) `m.checkpoint_seq > 0` assert fails because no fold exists. + +- [ ] **Step 3: Implement** + +Module-level, near `capped()`: +```rust +/// Fold a checkpoint once the log tail exceeds this many entries past the +/// last checkpoint (spec: "default 64 entries, one tunable constant"). +const CHECKPOINT_INTERVAL: u64 = 64; +``` +Private helper on `BucketTransport`: +```rust + /// Opportunistic, coordinator-free checkpoint fold. Called after a + /// successful commit; every failure path is deliberately non-fatal — + /// the checkpoint is derived data any reader can rebuild from the log, + /// a lost CAS just means a racing pusher's fold (or push) won, and the + /// next over-threshold push retries. The push that triggered this has + /// already durably landed. + fn maybe_fold_checkpoint(&self) -> Result<()> { + let (head_seq, checkpoint_seq, head_branch, tag, refs, packs) = { + let view = self.view.borrow(); + let Some(v) = view.as_ref() else { return Ok(()) }; + ( + v.manifest.head_seq, + v.manifest.checkpoint_seq, + v.manifest.head_branch.clone(), + v.tag.clone(), + v.refs.iter().map(|(b, id)| (b.clone(), *id)).collect::>(), + v.packs.clone(), + ) + }; + if head_seq - checkpoint_seq <= CHECKPOINT_INTERVAL { + return Ok(()); + } + let ck = Checkpoint { seq: head_seq, refs, packs }; + // Claim the object first (idempotent), then point the manifest at it. + self.bucket.put_new(&checkpoint_key(head_seq), &ck.encode())?; + let manifest = Manifest { head_seq, checkpoint_seq: head_seq, head_branch }; + // CAS from the tag our fresh post-commit view carries. A loss means + // someone advanced the WAL meanwhile — their problem to fold later. + if self.bucket.put_if_tag("manifest", &manifest.encode(), Some(&tag))?.is_some() { + self.refresh()?; + } + Ok(()) + } +``` +Call site — inside `update_ref`'s successful-CAS arm (currently `self.refresh()?; return Ok(());` at :496-497): +```rust + self.refresh()?; + let _ = self.maybe_fold_checkpoint(); // best-effort by design (see its doc) + return Ok(()); +``` + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-repo bucket_transport && cargo fmt --all -- --check && cargo clippy -p scl-repo --all-targets` +Expected: all pass — including the P36a race/fleet/crash proofs unchanged. The fleet test (8 pushes) stays under the interval, so folds don't interfere with it. + +- [ ] **Step 5: Commit** + +```bash +git add crates/repo/src/bucket_transport.rs +git commit -m "feat(repo): opportunistic coordinator-free checkpoint fold after commit (P36b)" +``` + +--- + +### Task 4: generalize the wire serve loop — `ServeTransport` + bucket stdio serve + +**Files:** +- Modify: `crates/repo/src/wire.rs` (`serve_with_policy` :692-731, `GetPack` arm :799-826, `PutPack` arm :827-856, read-only drain :757-786, `spill_pack_stream` :907-916) +- Modify: `crates/repo/src/transport.rs` (`TempPackGuard::new` :228-236 — add a dir-based constructor) +- Test: `#[cfg(test)] mod tests` in `wire.rs` (mirror its existing in-memory serve tests) + +**Interfaces:** +- Consumes: `BucketTransport` (implements `Transport` fully; `get_pack(wants, haves, filter, out)` and `put_pack(src)` on the trait, transport.rs:142-153); existing `LocalTransport` inherent fns (`layout` :98, `build_pack_tempfile` :121, `ingest_from` :208). +- Produces (used by Tasks 5, 6): + - `pub(crate) enum ServeTransport { Local(LocalTransport), Bucket { transport: BucketTransport, tmp: TempServeDir } }` in wire.rs, with `fn as_transport(&self) -> &dyn Transport` and `fn tmp_dir(&self) -> &Path`. + - `pub(crate) struct TempServeDir` — RAII temp dir under `std::env::temp_dir()` (`sc-serve-bucket--`), created on construction, best-effort removed on `Drop` (ephemeral-mode hygiene: serve spills must not outlive the session). + - `pub fn serve_bucket_with_policy(store_url: &str, r: &mut impl Read, w: &mut impl Write, policy: WirePolicy) -> Result<()>` — the bucket twin of `serve_with_policy` (:692). `serve_with_policy`'s signature and behavior are UNCHANGED. + - `pub(crate) fn TempPackGuard::new_in(dir: &std::path::Path) -> Result`; the existing `new(layout)` becomes a one-line wrapper reserving in `layout.tmp_dir()`. + - `spill_pack_stream(r, dir: &Path, max_bytes)` — parameter changes from `&Layout` to `&Path`; both call sites (:769 read-only drain, :828 PutPack) pass `transport.tmp_dir()`-equivalent. + +- [ ] **Step 1: Write the failing test** (wire.rs has in-memory serve tests — find its pattern, e.g. `serve_verb_errors_are_replies_not_session_teardown`, and mirror the pipe/duplex setup; the wire client half is `WireClient` from stdio_transport.rs:59) + +```rust + #[test] + fn bucket_stdio_serve_round_trips_refs_and_packs() { + // seed a bucket WAL with one commit via BucketTransport directly + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-wire-bucket-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let url = format!("sc+wal://{}", broot.display()); + { + let t = crate::bucket_transport::BucketTransport::open(&url).unwrap(); + // reuse however this test module (or bucket_transport's) mints a + // one-commit pack: build objects from a scratch repo, put_pack, + // update_ref "main" + /* seed as in bucket_transport::tests::push_via_trait_round_trips… */ + } + // serve it over an in-memory duplex exactly like the local serve tests + let (mut client_r, mut server_w) = /* this module's existing pipe pair helper */; + let (mut server_r, mut client_w) = /* … */; + let srv = std::thread::spawn(move || { + serve_bucket_with_policy(&url, &mut server_r, &mut server_w, WirePolicy::default()) + }); + let client = crate::stdio_transport::WireClient::handshake(&mut client_r, &mut client_w).unwrap(); + let refs = client.list_refs().unwrap(); + assert_eq!(refs.len(), 1); + assert_eq!(refs[0].0, "main"); + // GetPack streams; PutPack + UpdateRef land in the bucket + /* clone-style GetPack with wants=[tip], haves=[] and assert nonempty; + then push a second commit through PutPack + UpdateRef and assert a + fresh BucketTransport::open(&url) sees the moved tip */ + drop(client); + srv.join().unwrap().unwrap(); + std::fs::remove_dir_all(&broot).unwrap(); + } +``` +(The comment-marked seeding/piping lines are direction, not placeholders: the implementer copies the concrete duplex + seeding code from the named existing tests in the same two files — `wire.rs`'s serve tests and `bucket_transport.rs:702` — which are the authoritative in-repo patterns. New test must clean up and assert-gone.) + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test -p scl-repo wire::tests::bucket_stdio` +Expected: FAIL — `serve_bucket_with_policy` not found. + +- [ ] **Step 3: Implement** + +1. `TempPackGuard::new_in(dir)` in transport.rs — same body as `new` (:228-236) but reserving `dir.join(format!("pack-{pid}-{counter}.tmp"))` after `std::fs::create_dir_all(dir)?`; `new(layout)` delegates: `Self::new_in(&layout.tmp_dir())`. +2. `spill_pack_stream(r: &mut impl Read, tmp_dir: &Path, max_bytes: u64)` — replace the `layout` param (:907-916); body swaps `TempPackGuard::new(layout)` for `TempPackGuard::new_in(tmp_dir)`. +3. In wire.rs: +```rust +/// RAII scratch dir for a bucket-backed serve session's pack spills. +/// Removed (best-effort) on drop — a serve session leaves no residue. +pub(crate) struct TempServeDir(std::path::PathBuf); +impl TempServeDir { + fn create() -> Result { + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "sc-serve-bucket-{}-{n}", + std::process::id() + )); + std::fs::create_dir_all(&dir)?; + Ok(TempServeDir(dir)) + } +} +impl Drop for TempServeDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// The two transports the serve loop can sit on. Six verbs dispatch through +/// the `Transport` trait either way; only the pack verbs differ (local uses +/// the tempfile fast paths, bucket uses the trait's streaming methods). +pub(crate) enum ServeTransport { + Local(LocalTransport), + Bucket { + transport: crate::bucket_transport::BucketTransport, + tmp: TempServeDir, + }, +} +impl ServeTransport { + fn as_transport(&self) -> &dyn Transport { + match self { + ServeTransport::Local(t) => t, + ServeTransport::Bucket { transport, .. } => transport, + } + } + fn tmp_dir(&self) -> std::path::PathBuf { + match self { + ServeTransport::Local(t) => t.layout().tmp_dir(), + ServeTransport::Bucket { tmp, .. } => tmp.0.clone(), + } + } +} +``` +4. Extract the current post-handshake body of `serve_with_policy` (from the `LocalTransport::open` match at :721 to the end) into `fn serve_session(transport: ServeTransport, r, w, policy) -> Result<()>`, with these substitutions: + - Generic verbs (`ListRefs`…`UpdateRef` arm :857-887): call through `transport.as_transport()`. + - Read-only PutPack drain (:769): `spill_pack_stream(r, &transport.tmp_dir(), policy.ro_drain_cap)`. + - `GetPack` arm: `Local` keeps `build_pack_tempfile` verbatim; `Bucket` builds the same OK-before-stream shape by writing to a `TempPackGuard::new_in(&transport.tmp_dir())` file first: `transport.get_pack(&wants, &haves, filter_opt, &mut file)` then stream the file — same "fully succeeded before any wire byte" invariant as the comment at :600-603. + - `PutPack` arm: spill via `spill_pack_stream(r, &transport.tmp_dir(), policy.max_pack_size)`; then `Local` → `ingest_from(guard.path())` verbatim; `Bucket` → `transport.put_pack(&mut File::open(guard.path())?)` mapping to the same `ids_body` reply. +5. `serve_with_policy` becomes: handshake, `LocalTransport::open(root)` (unchanged error reply), `serve_session(ServeTransport::Local(t), …)`. New: +```rust +/// Serve a bucket WAL (`sc+wal://`/`sc+s3://`) over the wire protocol — +/// the disposable-instance mode (P36c): all durable state lives in the +/// bucket; this process keeps only an RAII scratch dir for pack spills. +pub fn serve_bucket_with_policy( + store_url: &str, + r: &mut impl Read, + w: &mut impl Write, + policy: WirePolicy, +) -> Result<()> { + // handshake identical to serve_with_policy (:698-720), then: + let transport = match crate::bucket_transport::BucketTransport::open(store_url) + .and_then(|t| Ok(ServeTransport::Bucket { transport: t, tmp: TempServeDir::create()? })) + { + Ok(t) => { + write_ok(w, &u32_body(PROTOCOL_VERSION))?; + t + } + Err(e) => { + let (code, msg) = err_to_wire(&e); + write_err(w, code, &msg)?; + return Ok(()); + } + }; + serve_session(transport, r, w, policy) +} +``` +(Factor the duplicated handshake into a small private helper if it keeps both entry fns readable — implementer's call; behavior is pinned by tests.) + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-repo wire && cargo test -p scl-repo stdio_transport && cargo test -p scl-repo http_transport && cargo fmt --all -- --check && cargo clippy -p scl-repo --all-targets` +Expected: new test passes; every existing serve/wire test passes unchanged (the refactor must be behavior-preserving for `Local`). + +- [ ] **Step 5: Commit** + +```bash +git add crates/repo/src +git commit -m "feat(repo): ServeTransport seam + serve_bucket_with_policy — wire serve over a bucket (P36c)" +``` + +--- + +### Task 5: bucket-backed HTTP serve — store threading + disposable-instance proof + +**Files:** +- Modify: `crates/repo/src/http_transport.rs` (`serve_http` :752, `serve_http_listener` :826, `handle_http_connection` :959-1063) +- Test: `#[cfg(test)] mod tests` there (mirror `spawn_real_http_server*` :1347-1374) + +**Interfaces:** +- Consumes: Task 4's `serve_bucket_with_policy`. +- Produces (used by Task 6): `serve_http`, `serve_http_listener`, and `handle_http_connection` each gain a trailing `store: Option<&str>` / owned `Option` parameter (threaded through the connection thread's `move` closure like `root`/`tls` at :865-866). Semantics: `None` = today's behavior byte-for-byte; `Some(url)` = the final hand-off (:1054-1063) calls `serve_bucket_with_policy(url, …)` instead of `serve_with_policy(root, …)` — everything before it (`.sc` presence gate, token load from the serve home, read-only floor, TLS, limits, timeouts) runs identically against `root`, which in store mode is the serve HOME, not the object source. + +- [ ] **Step 1: Write the failing test** + +```rust + fn spawn_bucket_http_server(home: std::path::PathBuf, store: String) -> u16 { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + serve_http_listener( + listener, + &home, + false, + false, + ServeLimits::default(), + None, + Some(store), + ) + .unwrap(); + }); + port + } + + #[test] + fn two_disposable_instances_serve_one_bucket_with_strict_consistency() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-http-bstore-{pid}")); + let home_a = tmp_repo("bstore-home-a"); // existing helper :1245 — an sc repo as serve home + let home_b = tmp_repo("bstore-home-b"); + let _ = std::fs::remove_dir_all(&broot); + let store = format!("sc+wal://{}", broot.display()); + // seed the bucket with one commit (same seeding as the wire test — a + // scratch repo pushed through BucketTransport::open(&store)) + /* seed one commit on "main" into the bucket */ + let port_a = spawn_bucket_http_server(home_a.clone(), store.clone()); + let port_b = spawn_bucket_http_server(home_b.clone(), store.clone()); + + // clone through instance A + let dst = std::env::temp_dir().join(format!("scl-http-bstore-dst-{pid}")); + let _ = std::fs::remove_dir_all(&dst); + let dst_repo = crate::repo::Repo::clone_url(&format!("sc+http://127.0.0.1:{port_a}/x"), &dst).unwrap(); + // push through instance A… + std::fs::write(dst.join("f2.txt"), b"instance hop").unwrap(); + let tip2 = dst_repo.commit("t", "c2").unwrap(); + dst_repo.push("origin").unwrap(); + drop(dst_repo); + // …and observe it through instance B with no propagation delay: + // strict consistency — "there is no eventually" (spec). + let dst2 = std::env::temp_dir().join(format!("scl-http-bstore-dst2-{pid}")); + let _ = std::fs::remove_dir_all(&dst2); + let d2 = crate::repo::Repo::clone_url(&format!("sc+http://127.0.0.1:{port_b}/x"), &dst2).unwrap(); + assert_eq!(d2.head_tip().unwrap(), Some(tip2)); + drop(d2); + for p in [&broot, &home_a, &home_b, &dst, &dst2] { + std::fs::remove_dir_all(p).unwrap(); + } + } + + #[test] + fn read_only_floor_holds_in_store_mode() { + // spawn with read_only=true + store; a push through it must fail with + // the ReadOnly wire error while clone still works — mirrors the + // existing server_read_only_floors_rw_token shape (:1719). + /* same setup as above, read_only: true; assert push Err, clone Ok */ + } +``` +(Seeding/`/* */` blocks: copy the concrete code from `bucket_transport.rs:702` (seed) and `http_transport.rs:1384` (clone/push over real socket) — in-repo authoritative patterns. All existing `serve_http_listener(...)` call sites in tests gain a trailing `None`.) + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test -p scl-repo http_transport::tests::two_disposable` +Expected: FAIL — `serve_http_listener` has no 7th parameter. + +- [ ] **Step 3: Implement** + +Signature changes (store LAST, after `tls`): +- `serve_http(addr, root, read_only, allow_public, limits, tls, store: Option<&str>)` +- `serve_http_listener(listener, root, read_only, mandatory_auth, limits, tls, store: Option)` +- `handle_http_connection(stream, root, server_read_only, mandatory_auth, limits, tls, store: Option<&str>)` + +Thread through the spawn closure exactly like `root` (:865-867): `let store = store.clone();` before the `move`. In `handle_http_connection`, the hand-off (:1053-1063) becomes: +```rust + let read_only = server_read_only || token_read_only; + let policy = crate::wire::WirePolicy { + read_only, + max_pack_size: limits.max_pack_size, + ro_drain_cap: crate::wire::RO_DRAIN_CAP, + }; + match store { + Some(url) => crate::wire::serve_bucket_with_policy(url, &mut reader, &mut writer, policy), + None => crate::wire::serve_with_policy(root, &mut reader, &mut writer, policy), + } +``` +`serve_http` forwards `store` to the listener after the existing gates — the bind gate, mandatory-auth computation, and token warning all keep reading the serve home's `.sc/`, unchanged. + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-repo http_transport && cargo fmt --all -- --check && cargo clippy -p scl-repo --all-targets` +Expected: 2 new tests pass; all ~38 existing http tests pass with their trailing `None`. + +- [ ] **Step 5: Commit** + +```bash +git add crates/repo/src/http_transport.rs +git commit -m "feat(repo): bucket-backed sc serve --http — disposable instances over one bucket (P36c)" +``` + +--- + +### Task 6: CLI `--store` flag + integration test + +**Files:** +- Modify: `crates/cli/src/main.rs` (`Cmd::Serve` :307-365, dispatch :939-981, `run_serve` :3480-3551) +- Create: test in `crates/cli/tests/bucket_remote.rs` (helpers `sc` :8, `tmp` :14 exist) + +**Interfaces:** +- Consumes: Tasks 4-5 (`serve_bucket_with_policy`, `serve_http(… store)`), `scl_repo::BucketUrl::parse` for fail-fast validation. +- Produces: `sc serve --stdio|--http --store `. `` stays required (serve home: `.sc/` for tokens/TLS/tmp). Malformed `--store` URL is refused before any bind. All other flags compose exactly as before. + +- [ ] **Step 1: Write the failing CLI test** (in `bucket_remote.rs`; readiness pattern copied from `crates/cli/tests/http_remote.rs:39-64` — the `listening on ` line) + +```rust +#[test] +fn serve_store_serves_a_bucket_and_second_instance_sees_pushes() { + let bucket = tmp("srv-bucket"); + let home = tmp("srv-home"); + assert!(sc(&home, &["init"]).status.success()); + let store = format!("sc+wal://{}", bucket.display()); + + // seed: a repo pushed straight to the bucket + let seed = tmp("srv-seed"); + assert!(sc(&seed, &["init"]).status.success()); + std::fs::write(seed.join("f.txt"), b"served from bucket").unwrap(); + assert!(sc(&seed, &["commit", "-m", "c1"]).status.success()); + assert!(sc(&seed, &["remote", "add", "origin", &store]).status.success()); + assert!(sc(&seed, &["push", "origin"]).status.success()); + + // malformed store URL refused before binding + let bad = sc(&home, &["serve", "--http", "127.0.0.1:0", "--store", "sc+s3://", home.to_str().unwrap()]); + assert!(!bad.status.success()); + + let (mut child, addr) = spawn_http_server_with(&home, &["--store", &store]); + let parent = tmp("srv-clone"); + let dst = parent.join("d"); + let url = format!("sc+http://{addr}/repo"); + assert!(sc(&parent, &["clone", &url, dst.to_str().unwrap()]).status.success()); + assert_eq!(std::fs::read(dst.join("f.txt")).unwrap(), b"served from bucket"); + // push through the server, then read it back via a SECOND instance + std::fs::write(dst.join("g.txt"), b"hop").unwrap(); + assert!(sc(&dst, &["commit", "-m", "c2"]).status.success()); + assert!(sc(&dst, &["push", "origin"]).status.success()); + child.kill().ok(); + let (mut child2, addr2) = spawn_http_server_with(&home, &["--store", &store]); + let parent2 = tmp("srv-clone2"); + let d2 = parent2.join("d2"); + assert!(sc(&parent2, &["clone", &format!("sc+http://{addr2}/repo"), d2.to_str().unwrap()]).status.success()); + assert_eq!(std::fs::read(d2.join("g.txt")).unwrap(), b"hop"); + child2.kill().ok(); + + for p in [&bucket, &home, &seed, &parent, &parent2] { + std::fs::remove_dir_all(p).unwrap(); + assert!(!p.exists()); + } +} +``` +Add a local `spawn_http_server_with(root, extra)` helper — copy `http_remote.rs:39-64` verbatim (same readiness line contract). Mirror the exact clap argv the existing tests in this file use for init/commit/clone/push. + +- [ ] **Step 2: Run to verify failure** + +Run: `cargo test -p scl-cli --test bucket_remote serve_store` +Expected: FAIL — `--store` is an unknown flag (clap error in the child's stderr → non-success where success asserted). + +- [ ] **Step 3: Implement** + +Clap (inside `Cmd::Serve`, after `tls_key`): +```rust + /// Serve a bucket WAL remote (`sc+wal://…` or `sc+s3://…`) instead of + /// this repo's own object store (P36c). `` remains the serve + /// home: its `.sc/` still provides access tokens, the TLS identity, + /// and scratch space — but all served content lives in the bucket, + /// making this instance disposable. + #[arg(long)] + store: Option, +``` +Dispatch (:939-981): pass `store` through to `run_serve` (12th param). In `run_serve`: +- Immediately after entry: `if let Some(url) = &store { scl_repo::BucketUrl::parse(url)?; }` (fail fast, before any bind — same idiom as `run_remote` :3650-3651). +- stdio arm (:3494-3517): replace the `serve_with_policy` call with +```rust + match &store { + Some(url) => scl_repo::wire::serve_bucket_with_policy(url, &mut stdin, &mut stdout, policy)?, + None => scl_repo::wire::serve_with_policy(&path, &mut stdin, &mut stdout, policy)?, + } +``` +- http arm (:3518-3547): `serve_http(&addr, &path, read_only, allow_public, limits, tls_mode, store.as_deref())?`. + +- [ ] **Step 4: Run tests** + +Run: `cargo test -p scl-cli && cargo fmt --all -- --check && cargo clippy --workspace --all-targets` +Expected: PASS across the CLI suites (14 test binaries), clean. + +- [ ] **Step 5: Commit** + +```bash +git add crates/cli crates/repo +git commit -m "feat(cli): sc serve --store — bucket-backed serving via CLI (P36c)" +``` + +--- + +### Task 7: docs + full gate — P36 complete + +**Files:** +- Modify: `docs/adr/0046-wal-bucket-remotes.md` (extend "As built" with P36b/P36c) +- Modify: `CLAUDE.md` (P36 capability row; standing-boundaries bullet) +- Modify: `ROADMAP.md` (Deferred: remove the P36b/P36c entries; add one new entry) +- Modify: `docs/THREAT-MODEL.md` (bucket section: serve-home note) + +**Interfaces:** +- Consumes: everything above, as actually built (verify claims against the code before writing them). +- Produces: docs matching reality; the workspace fully green. + +- [ ] **Step 1: Doc edits** + +- ADR-0046 "As built" gains a dated P36b/P36c paragraph: checkpoint record (`SCWC`, seq/refs/cumulative-packs), refresh stops at `checkpoint_seq` and fails closed on absent/lying/bypassed checkpoints, `CHECKPOINT_INTERVAL = 64` opportunistic best-effort fold after commit; `ServeTransport` seam, `serve_bucket_with_policy`, `--store` flag, serve home carries tokens/TLS/tmp, strict consistency across instances (two-instance tests named). +- CLAUDE.md P36 row: replace "P36a built … checkpoints (P36b) and bucket-backed serve (P36c) pending" with "Bucket WAL remotes (sc+wal://, sc+s3://): immutable packs + CAS'd manifest, checkpoints + log-tail cold start, bucket-backed `sc serve --store` with disposable instances" (keep the ADR link). Standing-boundaries bullet gains: "`sc serve --store` still requires a local serve home with `.sc/` — tokens, TLS identity, and pack spills live there; the bucket holds all served content." +- ROADMAP Deferred: delete the "Checkpoint fold (P36b)" and "Bucket-backed serve (P36c)" entries; keep compaction/gc, leases, partial-clone, static bundles, GCS, S3 streaming, incremental refresh, batched negotiation; add "**Serve-side persistent pack cache (P36c follow-on).** A bucket-backed serve instance re-downloads packs per connection; a content-addressed on-disk cache in the serve home would make warm instances cheap without affecting correctness." +- THREAT-MODEL bucket section: one added sentence — the serve-home split (bucket = content, home = access-control state) and that a bucket-backed serve enforces the same P29/P31 gates against clients while itself trusting the bucket only as far as BLAKE3 + strict WAL decode allow (same reader defenses as any client). + +- [ ] **Step 2: Full verification gate** + +Run: `cargo test --workspace && cargo clippy --workspace --all-targets && cargo fmt --all -- --check && cargo run --bin sc -- demo --agents 4` +Expected: all green; demo still proves zero residue. Paste the demo tail + workspace totals in the task report. + +- [ ] **Step 3: Commit** + +```bash +git add CLAUDE.md ROADMAP.md docs +git commit -m "docs: ADR-0046 As-built P36b/c, CLAUDE.md P36 complete, ROADMAP/THREAT-MODEL (P36b+c)" +``` From bd056882f2c7f46452df8238ecb85a10287eb570 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:01:57 +0300 Subject: [PATCH 02/11] feat(repo): walfmt Checkpoint record kind + checkpoint_key (P36b) --- crates/repo/src/walfmt.rs | 122 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/crates/repo/src/walfmt.rs b/crates/repo/src/walfmt.rs index a472d4d..0aa8a6f 100644 --- a/crates/repo/src/walfmt.rs +++ b/crates/repo/src/walfmt.rs @@ -6,6 +6,7 @@ use scl_core::ObjectId; const MANIFEST_MAGIC: &[u8; 4] = b"SCWM"; const ENTRY_MAGIC: &[u8; 4] = b"SCWE"; +const CHECKPOINT_MAGIC: &[u8; 4] = b"SCWC"; const VERSION: u32 = 1; const MAX_NAME: usize = 4096; const MAX_LIST: usize = 65536; @@ -221,11 +222,82 @@ impl LogEntry { } } +/// A fold of the WAL at `seq`: every branch tip and every on-chain pack hash +/// accumulated from the chain's start through log entry `seq`. Cold start = +/// this + the log tail after `seq`, instead of replaying the whole chain. +/// Referenced (and made authoritative) only by `Manifest.checkpoint_seq`; +/// an unreferenced checkpoint object is garbage like any off-chain key. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Checkpoint { + pub seq: u64, + /// branch -> tip pairs, sorted by branch (BTreeMap iteration order). + pub refs: Vec<(String, ObjectId)>, + /// Cumulative pack hashes in chain order (oldest first). + pub packs: Vec, +} + +impl Checkpoint { + /// Serialize to the on-bucket wire format: magic, version, then fields + /// in declaration order, all little-endian. + pub fn encode(&self) -> Vec { + let mut out = Vec::new(); + out.extend_from_slice(CHECKPOINT_MAGIC); + out.extend_from_slice(&VERSION.to_le_bytes()); + out.extend_from_slice(&self.seq.to_le_bytes()); + out.extend_from_slice(&(self.refs.len() as u32).to_le_bytes()); + for (branch, id) in &self.refs { + push_string(&mut out, branch); + out.extend_from_slice(id.as_bytes()); + } + out.extend_from_slice(&(self.packs.len() as u32).to_le_bytes()); + for hash in &self.packs { + push_string(&mut out, hash); + } + out + } + + /// Strictly decode a checkpoint: bad magic, unknown version, any length + /// that overruns the buffer, or trailing bytes are all refused. + pub fn decode(bytes: &[u8]) -> Result { + let mut c = header(bytes, CHECKPOINT_MAGIC, "checkpoint")?; + let seq = c.u64()?; + let nrefs = c.u32()? as usize; + if nrefs > MAX_LIST { + return Err(Error::Wal(format!( + "checkpoint ref count {nrefs} exceeds cap" + ))); + } + let mut refs = Vec::with_capacity(nrefs); + for _ in 0..nrefs { + let branch = c.string(MAX_NAME)?; + let id = c.id()?; + refs.push((branch, id)); + } + let npacks = c.u32()? as usize; + if npacks > MAX_LIST { + return Err(Error::Wal(format!( + "checkpoint pack count {npacks} exceeds cap" + ))); + } + let mut packs = Vec::with_capacity(npacks); + for _ in 0..npacks { + packs.push(c.string(MAX_HASH)?); + } + c.done()?; + Ok(Checkpoint { seq, refs, packs }) + } +} + /// `log/` zero-padded so lexical order == numeric order. pub fn log_key(seq: u64) -> String { format!("log/{seq:020}") } +/// `checkpoints/` zero-padded so lexical order == numeric order. +pub fn checkpoint_key(seq: u64) -> String { + format!("checkpoints/{seq:020}") +} + /// `packs/.pack` — the packfile object for a given content hash. pub fn pack_key(hash: &str) -> String { format!("packs/{hash}.pack") @@ -320,4 +392,54 @@ mod tests { assert_eq!(pack_key("abcd"), "packs/abcd.pack"); assert_eq!(idx_key("abcd"), "packs/abcd.idx"); } + + #[test] + fn checkpoint_round_trips_and_rejects_garbage() { + let c = Checkpoint { + seq: 64, + refs: vec![ + ("feat".to_string(), some_id(2)), + ("main".to_string(), some_id(1)), + ], + packs: vec!["ab12".to_string(), "cd34".to_string()], + }; + let bytes = c.encode(); + assert_eq!(Checkpoint::decode(&bytes).unwrap(), c); + // wrong magic, truncated, future version, trailing junk: refused + assert!(Checkpoint::decode(b"XXXX").is_err()); + assert!(Checkpoint::decode(&bytes[..bytes.len() - 1]).is_err()); + let mut future = bytes.clone(); + future[4] = 0xFF; + assert!(Checkpoint::decode(&future).is_err()); + let mut junk = bytes.clone(); + junk.push(0); + assert!(Checkpoint::decode(&junk).is_err()); + // a log-entry buffer is not a checkpoint (magic mismatch, not a panic) + let entry = LogEntry { + seq: 1, + parent_seq: 0, + packs: vec![], + updates: vec![], + }; + assert!(Checkpoint::decode(&entry.encode()).is_err()); + } + + #[test] + fn checkpoint_decode_caps_hostile_counts() { + // corrupt the refs count to u32::MAX: must fail fast, not allocate + let c = Checkpoint { + seq: 1, + refs: vec![("m".to_string(), some_id(1))], + packs: vec![], + }; + let mut evil = c.encode(); + // refs count sits right after magic(4)+version(4)+seq(8) = offset 16 + evil[16..20].copy_from_slice(&u32::MAX.to_le_bytes()); + assert!(Checkpoint::decode(&evil).is_err()); + } + + #[test] + fn checkpoint_key_is_stable() { + assert_eq!(checkpoint_key(64), "checkpoints/00000000000000000064"); + } } From 48ee93c4a926df2c9a6dc0f0b55e70a98be4be54 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:12:09 +0300 Subject: [PATCH 03/11] =?UTF-8?q?feat(repo):=20checkpoint-aware=20refresh?= =?UTF-8?q?=20=E2=80=94=20cold=20start=20reads=20checkpoint=20+=20log=20ta?= =?UTF-8?q?il=20(P36b)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/repo/src/bucket_transport.rs | 281 ++++++++++++++++++++++++++-- 1 file changed, 268 insertions(+), 13 deletions(-) diff --git a/crates/repo/src/bucket_transport.rs b/crates/repo/src/bucket_transport.rs index 7a0fdce..b6255fe 100644 --- a/crates/repo/src/bucket_transport.rs +++ b/crates/repo/src/bucket_transport.rs @@ -4,7 +4,9 @@ use crate::error::{Error, Result}; use crate::transport::Transport; -use crate::walfmt::{idx_key, log_key, pack_key, LogEntry, Manifest, RefUpdate}; +use crate::walfmt::{ + checkpoint_key, idx_key, log_key, pack_key, Checkpoint, LogEntry, Manifest, RefUpdate, +}; use scl_core::pack::{parse_index, read_object_at_bounded, IndexEntry, PackWriter}; use scl_core::{Object, ObjectId}; use scl_objio::{Bucket, Fetched}; @@ -25,6 +27,11 @@ struct WalView { refs: BTreeMap, /// object id -> (pack hash, offset, length), from every on-chain pack's idx. index: BTreeMap, + /// Every on-chain pack hash in chain order (checkpoint fold first, then + /// the tail) — retained so a checkpoint fold (P36b Task 3, bucket-side + /// checkpoint writer) is a pure copy. Not yet read anywhere in this task. + #[allow(dead_code)] + packs: Vec, } /// A [`Transport`] whose object graph and refs live entirely in an @@ -183,9 +190,43 @@ impl BucketTransport { Fetched::New { bytes, tag } => { let bytes = capped("manifest", bytes)?; let manifest = Manifest::decode(&bytes)?; + let stop = manifest.checkpoint_seq; + // Seed from the checkpoint when the manifest names one. The + // checkpoint is untrusted input like everything else here. + let (mut refs, mut packs): (BTreeMap, Vec) = if stop != 0 + { + let ck_bytes = match self.bucket.get(&checkpoint_key(stop), None)? { + Fetched::New { bytes, .. } => capped("checkpoint", bytes)?, + _ => { + return Err(Error::Wal(format!( + "checkpoint {stop} referenced by manifest but absent" + ))) + } + }; + let ck = Checkpoint::decode(&ck_bytes)?; + if ck.seq != stop { + return Err(Error::Wal(format!( + "checkpoint at {stop} claims seq {}", + ck.seq + ))); + } + let mut refs = BTreeMap::new(); + for (branch, id) in &ck.refs { + crate::refs::validate_branch_name(branch)?; + refs.insert(branch.clone(), *id); + } + (refs, ck.packs) + } else { + (BTreeMap::new(), Vec::new()) + }; let mut entries = Vec::new(); let mut seq = manifest.head_seq; - while seq != 0 { + while seq != stop { + if seq < stop { + return Err(Error::Wal(format!( + "log chain bypasses checkpoint {stop} (reached {seq})" + ))); + } let Fetched::New { bytes, .. } = self.bucket.get(&log_key(seq), None)? else { return Err(Error::Wal(format!( "log entry {seq} referenced by chain but absent" @@ -208,22 +249,25 @@ impl BucketTransport { entries.push(e); } entries.reverse(); // oldest first - let mut refs = BTreeMap::new(); - let mut index = BTreeMap::new(); for e in &entries { for u in &e.updates { crate::refs::validate_branch_name(&u.branch)?; refs.insert(u.branch.clone(), u.new); } for hash in &e.packs { - let Fetched::New { bytes, .. } = self.bucket.get(&idx_key(hash), None)? - else { - return Err(Error::Wal(format!("pack {hash} on chain but idx absent"))); - }; - let bytes = capped("pack idx", bytes)?; - for IndexEntry { id, offset, length } in parse_index(&bytes)? { - index.insert(id, (hash.clone(), offset, length)); - } + packs.push(hash.clone()); + } + } + // Index build: over the FULL cumulative pack list (checkpoint + // packs + tail packs), fetching each idx exactly as today. + let mut index = BTreeMap::new(); + for hash in &packs { + let Fetched::New { bytes, .. } = self.bucket.get(&idx_key(hash), None)? else { + return Err(Error::Wal(format!("pack {hash} on chain but idx absent"))); + }; + let bytes = capped("pack idx", bytes)?; + for IndexEntry { id, offset, length } in parse_index(&bytes)? { + index.insert(id, (hash.clone(), offset, length)); } } *self.view.borrow_mut() = Some(WalView { @@ -231,6 +275,7 @@ impl BucketTransport { manifest, refs, index, + packs, }); Ok(()) } @@ -515,7 +560,9 @@ impl Transport for BucketTransport { mod tests { use super::*; use crate::transport::Transport; - use crate::walfmt::{idx_key, log_key, pack_key, LogEntry, Manifest, RefUpdate}; + use crate::walfmt::{ + checkpoint_key, idx_key, log_key, pack_key, Checkpoint, LogEntry, Manifest, RefUpdate, + }; use scl_core::ObjectId; use scl_objio::{Bucket, DirBucket}; @@ -1167,4 +1214,212 @@ mod tests { std::fs::remove_dir_all(&a_root).unwrap(); assert!(!broot.exists() && !a_root.exists()); } + + /// Hand-build a WAL with `n` single-branch pushes; returns (bucket root, + /// final tip per branch map as Vec sorted, all pack hashes in order). + /// Each push i creates branch "b-" pointing at a distinct object. + fn hand_built_wal( + tag: &str, + n: u64, + ) -> (std::path::PathBuf, Vec<(String, ObjectId)>, Vec) { + let broot = std::env::temp_dir().join(format!("scl-bt-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&broot); + let bucket = DirBucket::open(&broot).unwrap(); + let mut refs = Vec::new(); + let mut packs = Vec::new(); + for i in 1..=n { + let obj = Object::blob(format!("wal-entry-{i}").into_bytes()); + let id = obj.id(); + let (hash, pack, idx) = pack_of(&[(id, obj.encode())]); + bucket.put_new(&pack_key(&hash), &pack).unwrap(); + bucket.put_new(&idx_key(&hash), &idx).unwrap(); + let entry = LogEntry { + seq: i, + parent_seq: i - 1, + packs: vec![hash.clone()], + updates: vec![RefUpdate { + branch: format!("b-{i}"), + old: None, + new: id, + }], + }; + bucket.put_new(&log_key(i), &entry.encode()).unwrap(); + refs.push((format!("b-{i}"), id)); + packs.push(hash); + } + let m = Manifest { + head_seq: n, + checkpoint_seq: 0, + head_branch: "b-1".into(), + }; + bucket + .put_if_tag("manifest", &m.encode(), None) + .unwrap() + .unwrap(); + refs.sort(); + (broot, refs, packs) + } + + #[test] + fn view_via_checkpoint_equals_full_replay_and_skips_folded_entries() { + let (broot, expected_refs, packs) = hand_built_wal("ckpt-eq", 6); + let bucket = DirBucket::open(&broot).unwrap(); + // fold through seq 4 by hand + let full = + BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + let full_refs = full.list_refs().unwrap(); + let ck = Checkpoint { + seq: 4, + refs: expected_refs + .iter() + .filter(|(b, _)| { + let i: u64 = b.strip_prefix("b-").unwrap().parse().unwrap(); + i <= 4 + }) + .cloned() + .collect(), + packs: packs[..4].to_vec(), + }; + bucket.put_new(&checkpoint_key(4), &ck.encode()).unwrap(); + let Fetched::New { bytes, tag } = bucket.get("manifest", None).unwrap() else { + panic!() + }; + let mut m = Manifest::decode(&bytes).unwrap(); + m.checkpoint_seq = 4; + bucket + .put_if_tag("manifest", &m.encode(), Some(&tag)) + .unwrap() + .unwrap(); + + // DELETE the folded log entries: a checkpoint-aware reader must not + // need them. (Direct file removal = simulated compaction.) + for seq in 1..=4u64 { + std::fs::remove_file(broot.join(log_key(seq))).unwrap(); + } + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!(t.list_refs().unwrap(), full_refs); + // objects from folded packs still readable (index seeded from checkpoint.packs) + let (b1, id1) = &expected_refs[0]; + assert!(b1.starts_with("b-")); + assert!(t.has_object(id1).unwrap()); + drop((t, full)); + std::fs::remove_dir_all(&broot).unwrap(); + } + + #[test] + fn corrupt_or_bypassing_checkpoints_fail_closed() { + let (broot, _refs, packs) = hand_built_wal("ckpt-bad", 3); + let bucket = DirBucket::open(&broot).unwrap(); + // (a) manifest names a checkpoint that does not exist + let Fetched::New { bytes, tag } = bucket.get("manifest", None).unwrap() else { + panic!() + }; + let mut m = Manifest::decode(&bytes).unwrap(); + m.checkpoint_seq = 2; + let tag = bucket + .put_if_tag("manifest", &m.encode(), Some(&tag)) + .unwrap() + .unwrap(); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + // (b) checkpoint exists but its seq field lies + let ck = Checkpoint { + seq: 1, + refs: vec![], + packs: packs[..2].to_vec(), + }; + bucket.put_new(&checkpoint_key(2), &ck.encode()).unwrap(); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + // (c) chain bypasses the checkpoint: entry at seq 3 has parent 1 (< 2) + let obj_end = { + // repair (b) first so the error is unambiguously the bypass + std::fs::remove_file(broot.join(checkpoint_key(2))).unwrap(); + let good = Checkpoint { + seq: 2, + refs: vec![], + packs: packs[..2].to_vec(), + }; + bucket.put_new(&checkpoint_key(2), &good.encode()).unwrap(); + let bad_entry = LogEntry { + seq: 3, + parent_seq: 1, + packs: vec![], + updates: vec![], + }; + std::fs::remove_file(broot.join(log_key(3))).unwrap(); + bucket.put_new(&log_key(3), &bad_entry.encode()).unwrap() + }; + assert!(obj_end); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); + let _ = tag; + std::fs::remove_dir_all(&broot).unwrap(); + + // (d) checkpoint's refs carry a branch name the ref grammar rejects + // ("a/b" is proven invalid by repo.rs's own switch()/validate tests). + let (broot2, refs2, packs2) = hand_built_wal("ckpt-badname", 2); + let bucket2 = DirBucket::open(&broot2).unwrap(); + let bad_id = refs2[0].1; + let Fetched::New { bytes, tag } = bucket2.get("manifest", None).unwrap() else { + panic!() + }; + let mut m2 = Manifest::decode(&bytes).unwrap(); + m2.checkpoint_seq = 2; + bucket2 + .put_if_tag("manifest", &m2.encode(), Some(&tag)) + .unwrap() + .unwrap(); + let bad_ck = Checkpoint { + seq: 2, + refs: vec![("a/b".to_string(), bad_id)], + packs: packs2, + }; + bucket2 + .put_new(&checkpoint_key(2), &bad_ck.encode()) + .unwrap(); + assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot2).unwrap())).is_err()); + std::fs::remove_dir_all(&broot2).unwrap(); + } + + #[test] + fn update_ref_preserves_checkpoint_seq_across_a_push() { + // A push against a bucket that already has a checkpoint must not + // reset `manifest.checkpoint_seq` back to 0 — that would strand the + // checkpoint (its packs/refs still readable) while the very next + // cold `refresh()` walked the *full* chain looking for now-compacted + // log entries, reproducing the failure the equals test above guards. + let (broot, _refs, packs) = hand_built_wal("ckpt-carry", 3); + let bucket = DirBucket::open(&broot).unwrap(); + let ck = Checkpoint { + seq: 3, + refs: vec![], + packs: packs.clone(), + }; + bucket.put_new(&checkpoint_key(3), &ck.encode()).unwrap(); + let Fetched::New { bytes, tag } = bucket.get("manifest", None).unwrap() else { + panic!() + }; + let mut m = Manifest::decode(&bytes).unwrap(); + m.checkpoint_seq = 3; + bucket + .put_if_tag("manifest", &m.encode(), Some(&tag)) + .unwrap() + .unwrap(); + + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + let obj = Object::blob(b"carry-push".to_vec()); + let (tip, bytes) = (obj.id(), obj.encode()); + t.put_object(&tip, &bytes).unwrap(); + t.update_ref("carried", &tip, None).unwrap(); + drop(t); + + let bucket2 = DirBucket::open(&broot).unwrap(); + let Fetched::New { bytes, .. } = bucket2.get("manifest", None).unwrap() else { + panic!() + }; + let after = Manifest::decode(&bytes).unwrap(); + assert_eq!( + after.checkpoint_seq, 3, + "checkpoint_seq must survive a push" + ); + std::fs::remove_dir_all(&broot).unwrap(); + } } From 613e3fce392ca50e29603d1b5042582845d84c8d Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:18:33 +0300 Subject: [PATCH 04/11] fix(repo): assert temp-dir removal in the four new checkpoint tests (P36b review) --- crates/repo/src/bucket_transport.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/repo/src/bucket_transport.rs b/crates/repo/src/bucket_transport.rs index b6255fe..9ed2e49 100644 --- a/crates/repo/src/bucket_transport.rs +++ b/crates/repo/src/bucket_transport.rs @@ -1304,6 +1304,7 @@ mod tests { assert!(t.has_object(id1).unwrap()); drop((t, full)); std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); } #[test] @@ -1352,6 +1353,7 @@ mod tests { assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).is_err()); let _ = tag; std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); // (d) checkpoint's refs carry a branch name the ref grammar rejects // ("a/b" is proven invalid by repo.rs's own switch()/validate tests). @@ -1377,6 +1379,7 @@ mod tests { .unwrap(); assert!(BucketTransport::from_bucket(Box::new(DirBucket::open(&broot2).unwrap())).is_err()); std::fs::remove_dir_all(&broot2).unwrap(); + assert!(!broot2.exists()); } #[test] @@ -1421,5 +1424,6 @@ mod tests { "checkpoint_seq must survive a push" ); std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); } } From c99256a34971dcec967432480ec5b6e18320956b Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:23:19 +0300 Subject: [PATCH 05/11] feat(repo): opportunistic coordinator-free checkpoint fold after commit (P36b) --- crates/repo/src/bucket_transport.rs | 154 +++++++++++++++++++++++++++- 1 file changed, 151 insertions(+), 3 deletions(-) diff --git a/crates/repo/src/bucket_transport.rs b/crates/repo/src/bucket_transport.rs index 9ed2e49..98e09d4 100644 --- a/crates/repo/src/bucket_transport.rs +++ b/crates/repo/src/bucket_transport.rs @@ -28,9 +28,9 @@ struct WalView { /// object id -> (pack hash, offset, length), from every on-chain pack's idx. index: BTreeMap, /// Every on-chain pack hash in chain order (checkpoint fold first, then - /// the tail) — retained so a checkpoint fold (P36b Task 3, bucket-side - /// checkpoint writer) is a pure copy. Not yet read anywhere in this task. - #[allow(dead_code)] + /// the tail) — retained so a checkpoint fold (`maybe_fold_checkpoint`) + /// is a pure copy: it never needs to re-walk the log or re-derive the + /// pack list, just snapshot this field into the new checkpoint object. packs: Vec, } @@ -85,6 +85,10 @@ fn capped(what: &str, bytes: Vec) -> Result> { Ok(bytes) } +/// Fold a checkpoint once the log tail exceeds this many entries past the +/// last checkpoint (spec: "default 64 entries, one tunable constant"). +const CHECKPOINT_INTERVAL: u64 = 64; + /// Which bucket backend a [`BucketUrl`] names. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BucketScheme { @@ -341,6 +345,58 @@ impl BucketTransport { } } } + + /// Opportunistic, coordinator-free checkpoint fold. Called after a + /// successful commit; every failure path is deliberately non-fatal — + /// the checkpoint is derived data any reader can rebuild from the log, + /// a lost CAS just means a racing pusher's fold (or push) won, and the + /// next over-threshold push retries. The push that triggered this has + /// already durably landed. + fn maybe_fold_checkpoint(&self) -> Result<()> { + let (head_seq, checkpoint_seq, head_branch, tag, refs, packs) = { + let view = self.view.borrow(); + let Some(v) = view.as_ref() else { + return Ok(()); + }; + ( + v.manifest.head_seq, + v.manifest.checkpoint_seq, + v.manifest.head_branch.clone(), + v.tag.clone(), + v.refs + .iter() + .map(|(b, id)| (b.clone(), *id)) + .collect::>(), + v.packs.clone(), + ) + }; + if head_seq - checkpoint_seq <= CHECKPOINT_INTERVAL { + return Ok(()); + } + let ck = Checkpoint { + seq: head_seq, + refs, + packs, + }; + // Claim the object first (idempotent), then point the manifest at it. + self.bucket + .put_new(&checkpoint_key(head_seq), &ck.encode())?; + let manifest = Manifest { + head_seq, + checkpoint_seq: head_seq, + head_branch, + }; + // CAS from the tag our fresh post-commit view carries. A loss means + // someone advanced the WAL meanwhile — their problem to fold later. + if self + .bucket + .put_if_tag("manifest", &manifest.encode(), Some(&tag))? + .is_some() + { + self.refresh()?; + } + Ok(()) + } } /// `ObjectSource` over the bucket for reachability walks (`get_pack`'s @@ -539,6 +595,7 @@ impl Transport for BucketTransport { // pull the fresh view (cheap: one conditional GET, since our // own write just changed the tag). self.refresh()?; + let _ = self.maybe_fold_checkpoint(); // best-effort by design (see its doc) return Ok(()); } // Lost the manifest CAS: someone else's append won the race. Our @@ -1426,4 +1483,95 @@ mod tests { std::fs::remove_dir_all(&broot).unwrap(); assert!(!broot.exists()); } + + #[test] + fn pushes_past_the_interval_fold_a_checkpoint_and_cold_start_uses_it() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-bt-fold-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + let n = CHECKPOINT_INTERVAL + 2; + for i in 0..n { + let obj = Object::blob(format!("fold-{i}").into_bytes()); + t.put_object(&obj.id(), &obj.encode()).unwrap(); + t.update_ref(&format!("w-{i}"), &obj.id(), None).unwrap(); + } + // the bucket now carries a manifest whose checkpoint_seq > 0 and the + // matching checkpoints/ object + let bucket = DirBucket::open(&broot).unwrap(); + let Fetched::New { bytes, .. } = bucket.get("manifest", None).unwrap() else { + panic!() + }; + let m = Manifest::decode(&bytes).unwrap(); + assert!(m.checkpoint_seq > 0, "no fold happened after {n} pushes"); + let Fetched::New { bytes, .. } = + bucket.get(&checkpoint_key(m.checkpoint_seq), None).unwrap() + else { + panic!( + "manifest names checkpoint {} but object absent", + m.checkpoint_seq + ) + }; + let ck = Checkpoint::decode(&bytes).unwrap(); + assert_eq!(ck.seq, m.checkpoint_seq); + assert!(!ck.refs.is_empty() && !ck.packs.is_empty()); + // cold start through it sees all n branches + let t2 = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!(t2.list_refs().unwrap().len(), n as usize); + drop((t, t2)); + std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); + } + + /// A bucket whose checkpoint writes always fail must not fail pushes. + struct FoldHostileBucket(DirBucket); + impl Bucket for FoldHostileBucket { + fn get(&self, key: &str, tag: Option<&str>) -> scl_objio::Result { + self.0.get(key, tag) + } + fn put_new(&self, key: &str, bytes: &[u8]) -> scl_objio::Result { + if key.starts_with("checkpoints/") { + return Err(scl_objio::Error::Backend( + "injected checkpoint write failure".into(), + )); + } + self.0.put_new(key, bytes) + } + fn put_if_tag( + &self, + key: &str, + bytes: &[u8], + tag: Option<&str>, + ) -> scl_objio::Result> { + self.0.put_if_tag(key, bytes, tag) + } + fn list(&self, prefix: &str) -> scl_objio::Result> { + self.0.list(prefix) + } + } + + #[test] + fn fold_failure_never_fails_the_push() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-bt-foldfail-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let t = BucketTransport::from_bucket(Box::new(FoldHostileBucket( + DirBucket::open(&broot).unwrap(), + ))) + .unwrap(); + for i in 0..(CHECKPOINT_INTERVAL + 2) { + let obj = Object::blob(format!("foldfail-{i}").into_bytes()); + t.put_object(&obj.id(), &obj.encode()).unwrap(); + t.update_ref(&format!("w-{i}"), &obj.id(), None).unwrap(); // must all be Ok + } + // no checkpoint could land; manifest still says 0 and reads still work + let t2 = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + assert_eq!( + t2.list_refs().unwrap().len(), + (CHECKPOINT_INTERVAL + 2) as usize + ); + drop((t, t2)); + std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); + } } From 3fbf99804f886cab61d5caa1d87f3be60a268d3e Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:35:53 +0300 Subject: [PATCH 06/11] =?UTF-8?q?feat(repo):=20ServeTransport=20seam=20+?= =?UTF-8?q?=20serve=5Fbucket=5Fwith=5Fpolicy=20=E2=80=94=20wire=20serve=20?= =?UTF-8?q?over=20a=20bucket=20(P36c)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/repo/src/transport.rs | 12 +- crates/repo/src/wire.rs | 356 +++++++++++++++++++++++++++++++---- 2 files changed, 329 insertions(+), 39 deletions(-) diff --git a/crates/repo/src/transport.rs b/crates/repo/src/transport.rs index 291d8b4..7a3dc2d 100644 --- a/crates/repo/src/transport.rs +++ b/crates/repo/src/transport.rs @@ -227,9 +227,17 @@ impl TempPackGuard { /// file itself is not created here — callers open/create it themselves /// (as a writer for a fresh spill, or a reader once written). pub(crate) fn new(layout: &Layout) -> Result { + Self::new_in(&layout.tmp_dir()) + } + + /// Like [`TempPackGuard::new`], but reserves the path directly under an + /// arbitrary directory rather than deriving it from a repo `Layout` — the + /// seam a bucket-backed serve session (P36c) uses, since a bucket remote + /// has no `.sc/tmp/` of its own to spool into and spills instead into an + /// RAII scratch dir for the session's lifetime. + pub(crate) fn new_in(dir: &std::path::Path) -> Result { static COUNTER: AtomicU64 = AtomicU64::new(0); - let dir = layout.tmp_dir(); - std::fs::create_dir_all(&dir)?; + std::fs::create_dir_all(dir)?; let n = COUNTER.fetch_add(1, Ordering::Relaxed); let path = dir.join(format!("pack-{}-{n}.tmp", std::process::id())); Ok(TempPackGuard { path }) diff --git a/crates/repo/src/wire.rs b/crates/repo/src/wire.rs index 150b3a0..431ef56 100644 --- a/crates/repo/src/wire.rs +++ b/crates/repo/src/wire.rs @@ -621,7 +621,8 @@ pub fn decode_refs_body(b: &[u8]) -> Result> { Ok(out) } -use crate::transport::{LocalTransport, Transport}; +use crate::bucket_transport::BucketTransport; +use crate::transport::{LocalTransport, TempPackGuard, Transport}; /// Default cap on an incoming `PutPack` spool when no operator override is /// configured (P31): 16 GiB. Threaded through [`WirePolicy::max_pack_size`]; @@ -676,6 +677,128 @@ pub fn validate_max_pack_size(max: u64) -> Result<()> { Ok(()) } +/// Read and validate the session's opening `HELLO` frame — shared by +/// [`serve_with_policy`] and [`serve_bucket_with_policy`] regardless of which +/// backend ends up serving the session: version skew or a non-`HELLO` first +/// frame get a typed error reply here, before either backend is ever opened. +/// Returns `Ok(true)` when the caller should proceed to open its transport; +/// `Ok(false)` when the session is already fully handled (an error was +/// replied, or the peer hung up immediately) and the caller should return +/// `Ok(())` without touching a transport at all. +fn handshake_hello(r: &mut impl Read, w: &mut impl Write) -> Result { + let first = match read_frame_opt(r)? { + Some(f) => f, + None => return Ok(false), // peer connected and immediately hung up + }; + match Request::decode(&first) { + Ok(Request::Hello { version }) if version == PROTOCOL_VERSION => Ok(true), + Ok(Request::Hello { version }) => { + write_err( + w, + EC_PROTOCOL, + &format!( + "unsupported protocol version {version} (server speaks {PROTOCOL_VERSION})" + ), + )?; + Ok(false) + } + Ok(_) | Err(_) => { + write_err(w, EC_PROTOCOL, "expected HELLO as the first request")?; + Ok(false) + } + } +} + +/// RAII scratch dir for a bucket-backed serve session's pack spills (P36c). +/// A bucket remote has no `.sc/tmp/` of its own to spool into, so a bucket +/// session gets one disposable directory under `std::env::temp_dir()` for its +/// lifetime instead — removed (best-effort) on drop, so the ephemeral-mode +/// disk invariant (zero residue after the session ends) holds for +/// bucket-backed serve exactly as it does for every other ephemeral session. +pub(crate) struct TempServeDir(std::path::PathBuf); + +impl TempServeDir { + fn create() -> Result { + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let n = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("sc-serve-bucket-{}-{n}", std::process::id())); + std::fs::create_dir_all(&dir)?; + Ok(TempServeDir(dir)) + } +} + +impl Drop for TempServeDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// The two transports [`serve_session`] can sit on (P36c). Every verb except +/// the pack pair dispatches identically through the `Transport` trait (via +/// [`ServeTransport::as_transport`]); `GetPack`/`PutPack` diverge because +/// `LocalTransport` has its own bounded tempfile fast paths +/// (`build_pack_tempfile`/`ingest_from`, sharing the repo's own `.sc/tmp/`) +/// while a bucket only exposes the trait's streaming `get_pack`/`put_pack`, +/// spooling through its own [`TempServeDir`] instead — so those two verbs are +/// handled per-variant in `serve_session`. +pub(crate) enum ServeTransport { + Local(LocalTransport), + Bucket { + transport: BucketTransport, + tmp: TempServeDir, + }, +} + +impl ServeTransport { + fn as_transport(&self) -> &dyn Transport { + match self { + ServeTransport::Local(t) => t, + ServeTransport::Bucket { transport, .. } => transport, + } + } + + /// Scratch dir this session's pack spills go into: the repo's own + /// `.sc/tmp/` for a local transport, this session's [`TempServeDir`] for + /// a bucket one. + fn tmp_dir(&self) -> std::path::PathBuf { + match self { + ServeTransport::Local(t) => t.layout().tmp_dir(), + ServeTransport::Bucket { tmp, .. } => tmp.0.clone(), + } + } +} + +/// Bucket twin of `LocalTransport::build_pack_tempfile`: builds a `GetPack` +/// response's temp pack file so the same "fully succeeded before any wire +/// byte" invariant (the comment on the `GetPack` arm in [`serve_session`]) +/// holds regardless of backend — just built from the trait's streaming +/// `get_pack` (spooled into a fresh guard under the session's +/// [`TempServeDir`]) instead of a local store's own reachability walk. +fn bucket_get_pack_tempfile( + bt: &BucketTransport, + tmp_dir: &std::path::Path, + wants: &[ObjectId], + haves: &[ObjectId], + filter: Option<&[String]>, +) -> Result { + let guard = TempPackGuard::new_in(tmp_dir)?; + let mut f = std::fs::File::create(guard.path())?; + bt.get_pack(wants, haves, filter, &mut f)?; + Ok(guard) +} + +/// Bucket twin of `LocalTransport::ingest_from`: hand the already-spilled +/// pack file (produced by [`spill_pack_stream`]) to the trait's `put_pack`, +/// which re-verifies every record and uploads a fresh pack to the bucket — +/// mirroring the local path's own re-verify-then-ingest contract. +fn bucket_put_pack_from_file( + bt: &BucketTransport, + path: &std::path::Path, +) -> Result> { + let mut f = std::fs::File::open(path)?; + bt.put_pack(&mut f) +} + /// Serve the repo at `root` to one wire-protocol peer until `Bye`/EOF. /// /// This is the whole server: every verb dispatches onto [`LocalTransport`], @@ -697,28 +820,50 @@ pub fn serve_with_policy( ) -> Result<()> { // Handshake: HELLO must come first, and versions must match, before any // repo access happens. - let first = match read_frame_opt(r)? { - Some(f) => f, - None => return Ok(()), // peer connected and immediately hung up - }; - match Request::decode(&first) { - Ok(Request::Hello { version }) if version == PROTOCOL_VERSION => {} - Ok(Request::Hello { version }) => { - write_err( - w, - EC_PROTOCOL, - &format!( - "unsupported protocol version {version} (server speaks {PROTOCOL_VERSION})" - ), - )?; - return Ok(()); + if !handshake_hello(r, w)? { + return Ok(()); + } + let transport = match LocalTransport::open(root) { + Ok(t) => { + write_ok(w, &u32_body(PROTOCOL_VERSION))?; + ServeTransport::Local(t) } - Ok(_) | Err(_) => { - write_err(w, EC_PROTOCOL, "expected HELLO as the first request")?; + Err(e) => { + let (code, msg) = err_to_wire(&e); + write_err(w, code, &msg)?; return Ok(()); } + }; + serve_session(transport, r, w, policy) +} + +/// Open a [`BucketTransport`] plus this session's [`TempServeDir`] as one +/// [`ServeTransport::Bucket`] — a single fallible step so +/// [`serve_bucket_with_policy`] only needs one match to decide whether to +/// reply OK or a typed error. +fn open_bucket_serve_transport(store_url: &str) -> Result { + let transport = BucketTransport::open(store_url)?; + let tmp = TempServeDir::create()?; + Ok(ServeTransport::Bucket { transport, tmp }) +} + +/// Serve a bucket WAL (`sc+wal://`/`sc+s3://`) over the same wire protocol +/// `serve_with_policy` speaks for a local repo (P36c): same handshake, same +/// `PROTOCOL_VERSION`, same read-only gate and pack-spool caps — every verb +/// except the pack pair goes through [`Transport`] identically either way. +/// All durable state lives in the bucket; this process holds only an RAII +/// scratch dir ([`TempServeDir`]) for pack spills, removed when the session +/// ends. +pub fn serve_bucket_with_policy( + store_url: &str, + r: &mut impl Read, + w: &mut impl Write, + policy: WirePolicy, +) -> Result<()> { + if !handshake_hello(r, w)? { + return Ok(()); } - let transport = match LocalTransport::open(root) { + let transport = match open_bucket_serve_transport(store_url) { Ok(t) => { write_ok(w, &u32_body(PROTOCOL_VERSION))?; t @@ -729,7 +874,21 @@ pub fn serve_with_policy( return Ok(()); } }; + serve_session(transport, r, w, policy) +} +/// Serve one wire-protocol peer over an already-opened [`ServeTransport`] +/// until `Bye`/EOF — the shared body of [`serve_with_policy`] and +/// [`serve_bucket_with_policy`] once each has finished its own handshake and +/// opened its own backend. See `serve_with_policy`'s doc comment for the +/// full verb-by-verb contract (read-only gate, pack spool caps); it applies +/// identically here no matter which transport variant is behind it. +fn serve_session( + transport: ServeTransport, + r: &mut impl Read, + w: &mut impl Write, + policy: WirePolicy, +) -> Result<()> { loop { let frame = match read_frame_opt(r)? { Some(f) => f, @@ -766,7 +925,7 @@ pub fn serve_with_policy( // the normal arm's larger `max_pack_size` — a read-only // push is discarded regardless, so there's no reason to // spool an attacker-sized pack just to reject it. - match spill_pack_stream(r, transport.layout(), policy.ro_drain_cap) { + match spill_pack_stream(r, &transport.tmp_dir(), policy.ro_drain_cap) { Ok(guard) => { drop(guard); let (code, msg) = err_to_wire(&Error::ReadOnly); @@ -806,13 +965,22 @@ pub fn serve_with_policy( } else { Some(filter.as_slice()) }; - match transport.build_pack_tempfile(&wants, &haves, filter_opt) { + let result = match &transport { + ServeTransport::Local(t) => t.build_pack_tempfile(&wants, &haves, filter_opt), + ServeTransport::Bucket { transport: bt, tmp } => { + bucket_get_pack_tempfile(bt, &tmp.0, &wants, &haves, filter_opt) + } + }; + match result { Ok(guard) => { // Building the temp pack file (bounded RAM: one - // object at a time via PackWriter) fully succeeded - // before any wire byte for this response was sent, - // so an OK/ERR split here is still clean — no - // partial stream can ever follow an ERR. + // object at a time) fully succeeded before any wire + // byte for this response was sent, so an OK/ERR + // split here is still clean — no partial stream can + // ever follow an ERR. True for both variants: Local's + // `build_pack_tempfile` and the bucket path's + // `bucket_get_pack_tempfile` each finish writing (and + // return Err on any failure) before we touch `w`. write_ok(w, &[])?; // empty body: "stream follows" let mut f = std::fs::File::open(guard.path())?; write_pack_stream(w, &mut f, pack_chunk_size())?; @@ -825,9 +993,15 @@ pub fn serve_with_policy( } } Request::PutPack => { - match spill_pack_stream(r, transport.layout(), policy.max_pack_size) { + match spill_pack_stream(r, &transport.tmp_dir(), policy.max_pack_size) { Ok(guard) => { - match transport.ingest_from(guard.path()) { + let ingest_result = match &transport { + ServeTransport::Local(t) => t.ingest_from(guard.path()), + ServeTransport::Bucket { transport: bt, .. } => { + bucket_put_pack_from_file(bt, guard.path()) + } + }; + match ingest_result { Ok(ids) => write_ok(w, &ids_body(&ids))?, Err(e) => { let (code, msg) = err_to_wire(&e); @@ -859,18 +1033,27 @@ pub fn serve_with_policy( Request::Hello { .. } => { Err(Error::Protocol("unexpected HELLO mid-session".into())) } - Request::ListRefs => transport.list_refs().map(|refs| refs_body(&refs)), - Request::HeadBranch => transport.head_branch().map(|s| str_body(&s)), - Request::HasObject(id) => transport.has_object(&id).map(bool_body), - Request::GetObject(id) => transport.get_object(&id), - Request::PutObject { id, bytes } => { - transport.put_object(&id, &bytes).map(|()| Vec::new()) + Request::ListRefs => transport + .as_transport() + .list_refs() + .map(|refs| refs_body(&refs)), + Request::HeadBranch => { + transport.as_transport().head_branch().map(|s| str_body(&s)) } + Request::HasObject(id) => { + transport.as_transport().has_object(&id).map(bool_body) + } + Request::GetObject(id) => transport.as_transport().get_object(&id), + Request::PutObject { id, bytes } => transport + .as_transport() + .put_object(&id, &bytes) + .map(|()| Vec::new()), Request::UpdateRef { branch, id, expected_old, } => transport + .as_transport() .update_ref(&branch, &id, expected_old.as_ref()) .map(|()| Vec::new()), Request::Bye | Request::GetPack { .. } | Request::PutPack => { @@ -903,13 +1086,15 @@ pub fn serve(root: &std::path::Path, r: &mut impl Read, w: &mut impl Write) -> R /// created before any read, so a stream that errors partway (a malformed /// frame, a dropped connection) still leaves nothing behind — `Drop` removes /// whatever was written so far. `max_bytes` bounds the spool (0 = unlimited, -/// P31) — see [`read_pack_stream`]. +/// P31) — see [`read_pack_stream`]. `tmp_dir` (P36c) is the caller's scratch +/// dir — a repo's `.sc/tmp/` for a local serve session, a [`TempServeDir`] +/// for a bucket one — so this function itself stays backend-agnostic. fn spill_pack_stream( r: &mut impl Read, - layout: &crate::layout::Layout, + tmp_dir: &std::path::Path, max_bytes: u64, -) -> Result { - let guard = crate::transport::TempPackGuard::new(layout)?; +) -> Result { + let guard = TempPackGuard::new_in(tmp_dir)?; let mut f = std::fs::File::create(guard.path())?; read_pack_stream(r, &mut f, max_bytes)?; Ok(guard) @@ -1587,4 +1772,101 @@ mod tests { assert!(!tmp.exists() || std::fs::read_dir(&tmp).unwrap().next().is_none()); let _ = std::fs::remove_dir_all(&root); } + + /// Mint a one-commit object set the same way + /// `bucket_transport::tests::tiny_history` does (blob -> tree -> snapshot + /// via a real scratch repo), returning `(tip, objects)`. `tag` + /// disambiguates the scratch repo path between call sites sharing this + /// process id. + fn tiny_history_for_bucket(tag: &str, content: &[u8]) -> (ObjectId, Vec<(ObjectId, Vec)>) { + let root = + std::env::temp_dir().join(format!("scl-wire-bkhist-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let repo = crate::repo::Repo::init(&root).unwrap(); + std::fs::write(root.join("f.txt"), content).unwrap(); + let tip = repo.commit("t", "c").unwrap(); + let store_arc = repo.vfs().store(); + let mut store = store_arc.lock().unwrap(); + let ids = crate::reachable::reachable_objects(&mut *store, &[tip]).unwrap(); + let objects: Vec<(ObjectId, Vec)> = ids + .iter() + .map(|id| (*id, store.get(id).unwrap().encode())) + .collect(); + drop(store); + drop(repo); + std::fs::remove_dir_all(&root).unwrap(); + assert!(!root.exists()); + (tip, objects) + } + + /// `serve_bucket_with_policy` speaks the exact same wire protocol as + /// `serve_with_policy` against a bucket WAL instead of a local `.sc/`: + /// handshake, `ListRefs`, a streamed `GetPack`, and a `PutPack` + + /// `UpdateRef` that lands a second commit — verified by a fresh + /// `BucketTransport::open` on the bucket after the session ends. + #[test] + fn bucket_stdio_serve_round_trips_refs_and_packs() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-wire-bucket-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let url = format!("sc+wal://{}", broot.display()); + + // Seed a one-commit bucket WAL directly via BucketTransport — same + // shape as bucket_transport::tests::push_via_trait_round_trips_into_a_fresh_bucket. + let (tip, objects) = tiny_history_for_bucket("seed", b"hello wal"); + { + let t = crate::bucket_transport::BucketTransport::open(&url).unwrap(); + let (pack, _idx) = scl_core::pack::build_pack(&objects).unwrap(); + t.put_pack(&mut std::io::Cursor::new(pack)).unwrap(); + t.update_ref("main", &tip, None).unwrap(); + } + + // Serve it over an in-memory duplex, exactly like the local serve + // tests' `spawn_wire_pair_with_policy` pipe setup. + let (client_read, mut server_write) = std::io::pipe().unwrap(); + let (mut server_read, client_write) = std::io::pipe().unwrap(); + let url_for_server = url.clone(); + let srv = std::thread::spawn(move || { + serve_bucket_with_policy( + &url_for_server, + &mut server_read, + &mut server_write, + WirePolicy::default(), + ) + }); + let client = + crate::stdio_transport::WireClient::handshake(client_read, client_write).unwrap(); + + let refs = client.list_refs().unwrap(); + assert_eq!(refs.len(), 1); + assert_eq!(refs[0].0, "main"); + assert_eq!(refs[0].1, tip); + + // GetPack streams a nonempty pack covering the tip's full closure. + let mut out = Vec::new(); + client.get_pack(&[tip], &[], None, &mut out).unwrap(); + assert!(!out.is_empty()); + let got = scl_core::pack::parse_pack(&out).unwrap(); + assert_eq!(got.len(), objects.len()); + + // PutPack + UpdateRef land a second commit through the same session. + let (tip2, objects2) = tiny_history_for_bucket("seed2", b"second commit"); + let (pack2, _idx2) = scl_core::pack::build_pack(&objects2).unwrap(); + let ids2 = client.put_pack(&mut std::io::Cursor::new(pack2)).unwrap(); + assert_eq!(ids2.len(), objects2.len()); + client.update_ref("main", &tip2, Some(&tip)).unwrap(); + + client.bye().unwrap(); + drop(client); + srv.join().unwrap().unwrap(); + + // A fresh BucketTransport sees the moved tip. + let t2 = crate::bucket_transport::BucketTransport::open(&url).unwrap(); + assert_eq!(t2.list_refs().unwrap(), vec![("main".to_string(), tip2)]); + drop(t2); + + std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); + } } From a02297566a61eb3de1b18eeeb7f81c11f3f7774f Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:48:45 +0300 Subject: [PATCH 07/11] =?UTF-8?q?feat(repo):=20bucket-backed=20sc=20serve?= =?UTF-8?q?=20--http=20=E2=80=94=20disposable=20instances=20over=20one=20b?= =?UTF-8?q?ucket=20(P36c)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/cli/src/main.rs | 1 + crates/repo/src/http_transport.rs | 183 ++++++++++++++++++++++++++++-- 2 files changed, 173 insertions(+), 11 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 8674691..33c669c 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -3542,6 +3542,7 @@ fn run_serve( allow_public, limits, tls_mode, + None, )?; Ok(()) } diff --git a/crates/repo/src/http_transport.rs b/crates/repo/src/http_transport.rs index 2565004..294e7d3 100644 --- a/crates/repo/src/http_transport.rs +++ b/crates/repo/src/http_transport.rs @@ -756,6 +756,7 @@ pub fn serve_http( allow_public: bool, limits: ServeLimits, tls: TlsMode, + store: Option<&str>, ) -> Result<()> { crate::wire::validate_max_pack_size(limits.max_pack_size)?; let tls_config = resolve_tls(root, &tls)?; @@ -801,6 +802,7 @@ pub fn serve_http( mandatory_auth, limits, tls_config, + store.map(str::to_string), ) } @@ -830,6 +832,7 @@ pub fn serve_http_listener( mandatory_auth: bool, limits: ServeLimits, tls: Option, + store: Option, ) -> Result<()> { let live = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); let mut backoff = AcceptBackoff::new(); @@ -864,6 +867,7 @@ pub fn serve_http_listener( }; let root = root.to_path_buf(); let tls = tls.clone(); + let store = store.clone(); let spawn_result = std::thread::Builder::new().spawn(move || { let _guard = guard; // slot held for the connection's lifetime if let Err(e) = handle_http_connection( @@ -873,6 +877,7 @@ pub fn serve_http_listener( mandatory_auth, limits, tls.as_ref(), + store.as_deref(), ) { eprintln!("sc serve --http: connection error: {e}"); } @@ -963,6 +968,7 @@ fn handle_http_connection( mandatory_auth: bool, limits: ServeLimits, tls: Option<&scl_tlsio::TlsServerConfig>, + store: Option<&str>, ) -> Result<()> { stream .set_read_timeout(Some(OPENING_READ_TIMEOUT)) @@ -1051,16 +1057,15 @@ fn handle_http_connection( .map_err(|e| Error::ConnectionLost(format!("sc+http set session timeouts: {e}")))?; let read_only = server_read_only || token_read_only; - crate::wire::serve_with_policy( - root, - &mut reader, - &mut writer, - crate::wire::WirePolicy { - read_only, - max_pack_size: limits.max_pack_size, - ro_drain_cap: crate::wire::RO_DRAIN_CAP, - }, - ) + let policy = crate::wire::WirePolicy { + read_only, + max_pack_size: limits.max_pack_size, + ro_drain_cap: crate::wire::RO_DRAIN_CAP, + }; + match store { + Some(url) => crate::wire::serve_bucket_with_policy(url, &mut reader, &mut writer, policy), + None => crate::wire::serve_with_policy(root, &mut reader, &mut writer, policy), + } } #[cfg(test)] @@ -1367,6 +1372,7 @@ mod tests { mandatory_auth, ServeLimits::default(), None, + None, ) .unwrap(); }); @@ -1534,6 +1540,7 @@ mod tests { false, ServeLimits::default(), TlsMode::Off, + None, ) .unwrap_err(); assert!( @@ -1863,7 +1870,7 @@ mod tests { let addr = listener.local_addr().unwrap(); let root = root.to_path_buf(); std::thread::spawn(move || { - let _ = serve_http_listener(listener, &root, false, false, limits, None); + let _ = serve_http_listener(listener, &root, false, false, limits, None, None); }); addr } @@ -2072,6 +2079,7 @@ mod tests { false, ServeLimits::default(), Some(cfg), + None, ); }); (addr, spki) @@ -2268,4 +2276,157 @@ mod tests { let _ = std::fs::remove_file(&policy.known_hosts); std::fs::remove_dir_all(&root).unwrap(); } + + // ── Task 5 (P36c): bucket-backed serve — the `store` parameter threads + // through unchanged gates (`.sc` presence, tokens, read-only floor, TLS, + // limits, timeouts) against `root` (the serve HOME), and only the final + // hand-off routes to `serve_bucket_with_policy` instead of + // `serve_with_policy`. The headline property: two disposable server + // instances, each with its own HOME, serving the SAME bucket, see each + // other's writes with no propagation delay (strict consistency — there + // is no "eventually" for a bucket WAL). ── + + /// Seed a one-commit bucket WAL directly via `BucketTransport` — same + /// shape as `bucket_transport::tests::push_via_trait_round_trips_into_a_fresh_bucket` + /// and `wire::tests::bucket_stdio_serve_round_trips_refs_and_packs`'s + /// seeding: a scratch repo mints the objects, then a raw + /// `BucketTransport::open` pushes them in — never through an HTTP server. + fn seed_bucket_history(store: &str, tag: &str, content: &[u8]) -> ObjectId { + let root = + std::env::temp_dir().join(format!("scl-http-bkseed-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let repo = crate::repo::Repo::init(&root).unwrap(); + std::fs::write(root.join("f.txt"), content).unwrap(); + let tip = repo.commit("t", "c").unwrap(); + let store_arc = repo.vfs().store(); + let mut objstore = store_arc.lock().unwrap(); + let ids = crate::reachable::reachable_objects(&mut *objstore, &[tip]).unwrap(); + let objects: Vec<(ObjectId, Vec)> = ids + .iter() + .map(|id| (*id, objstore.get(id).unwrap().encode())) + .collect(); + drop(objstore); + drop(repo); + std::fs::remove_dir_all(&root).unwrap(); + + let t = crate::bucket_transport::BucketTransport::open(store).unwrap(); + let (pack, _idx) = scl_core::pack::build_pack(&objects).unwrap(); + t.put_pack(&mut std::io::Cursor::new(pack)).unwrap(); + t.update_ref("main", &tip, None).unwrap(); + tip + } + + /// Spawn a `serve_http_listener` whose `root` is a plain sc repo used + /// only as the serve HOME (`.sc` presence gate, tokens) while every + /// object read/write routes to `store` instead. + fn spawn_bucket_http_server_policy( + home: std::path::PathBuf, + store: String, + read_only: bool, + ) -> u16 { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + serve_http_listener( + listener, + &home, + read_only, + false, + ServeLimits::default(), + None, + Some(store), + ) + .unwrap(); + }); + port + } + + fn spawn_bucket_http_server(home: std::path::PathBuf, store: String) -> u16 { + spawn_bucket_http_server_policy(home, store, false) + } + + /// Two disposable server instances (`home_a`, `home_b`), each with its + /// own serve HOME, both serving the SAME bucket: a push landed through + /// instance A is visible through instance B on the very next connection + /// — no coordinator, no replication lag, no "eventually" (ADR-0046's + /// strict-consistency contract, now exercised end to end through the + /// HTTP server rather than just `BucketTransport`/`wire::serve_bucket_with_policy`). + #[test] + fn two_disposable_instances_serve_one_bucket_with_strict_consistency() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-http-bstore-{pid}")); + let home_a = tmp_repo("bstore-home-a"); // existing helper — an sc repo as serve home + let home_b = tmp_repo("bstore-home-b"); + let _ = std::fs::remove_dir_all(&broot); + let store = format!("sc+wal://{}", broot.display()); + + // seed the bucket with one commit on "main" + let tip1 = seed_bucket_history(&store, "seed", b"first commit"); + + let port_a = spawn_bucket_http_server(home_a.clone(), store.clone()); + let port_b = spawn_bucket_http_server(home_b.clone(), store.clone()); + + // clone through instance A + let dst = std::env::temp_dir().join(format!("scl-http-bstore-dst-{pid}")); + let _ = std::fs::remove_dir_all(&dst); + let dst_repo = + crate::repo::Repo::clone_url(&format!("sc+http://127.0.0.1:{port_a}/x"), &dst).unwrap(); + assert_eq!(dst_repo.head_tip().unwrap(), Some(tip1)); + + // push through instance A… + std::fs::write(dst.join("f2.txt"), b"instance hop").unwrap(); + let tip2 = dst_repo.commit("t", "c2").unwrap(); + dst_repo.push("origin").unwrap(); + drop(dst_repo); + + // …and observe it through instance B with no propagation delay: + // strict consistency — "there is no eventually" (spec). + let dst2 = std::env::temp_dir().join(format!("scl-http-bstore-dst2-{pid}")); + let _ = std::fs::remove_dir_all(&dst2); + let d2 = crate::repo::Repo::clone_url(&format!("sc+http://127.0.0.1:{port_b}/x"), &dst2) + .unwrap(); + assert_eq!(d2.head_tip().unwrap(), Some(tip2)); + drop(d2); + + for p in [&broot, &home_a, &home_b, &dst, &dst2] { + std::fs::remove_dir_all(p).unwrap(); + } + } + + /// The read-only floor (`server_read_only || token_read_only`) holds in + /// store mode exactly as it does for a local repo — mirrors + /// `server_read_only_floors_rw_token` (:1719), but the write attempt + /// goes through `Repo::push` against a bucket-backed server instead of a + /// raw `Transport::put_object` call: a push must fail with the wire + /// `ReadOnly` error while a plain clone still succeeds. + #[test] + fn read_only_floor_holds_in_store_mode() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-http-bstore-ro-{pid}")); + let home = tmp_repo("bstore-home-ro"); + let _ = std::fs::remove_dir_all(&broot); + let store = format!("sc+wal://{}", broot.display()); + let tip1 = seed_bucket_history(&store, "seed-ro", b"read-only seed"); + + let port = spawn_bucket_http_server_policy(home.clone(), store.clone(), true); + + // clone still works under the read-only floor. + let dst = std::env::temp_dir().join(format!("scl-http-bstore-ro-dst-{pid}")); + let _ = std::fs::remove_dir_all(&dst); + let dst_repo = + crate::repo::Repo::clone_url(&format!("sc+http://127.0.0.1:{port}/x"), &dst).unwrap(); + assert_eq!(dst_repo.head_tip().unwrap(), Some(tip1)); + + // a push is rejected with the wire ReadOnly error. + std::fs::write(dst.join("blocked.txt"), b"should never land").unwrap(); + dst_repo.commit("t", "blocked commit").unwrap(); + let err = dst_repo.push("origin").unwrap_err(); + assert!(matches!(err, Error::ReadOnly), "{err:?}"); + drop(dst_repo); + + for p in [&broot, &home, &dst] { + std::fs::remove_dir_all(p).unwrap(); + } + } } From 2d826b3e4960c0554584d6ed50bc66e2864e55b6 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 20:57:49 +0300 Subject: [PATCH 08/11] =?UTF-8?q?feat(cli):=20sc=20serve=20--store=20?= =?UTF-8?q?=E2=80=94=20bucket-backed=20serving=20via=20CLI=20(P36c)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/cli/src/main.rs | 28 +++++++- crates/cli/tests/bucket_remote.rs | 111 +++++++++++++++++++++++++++++- 2 files changed, 135 insertions(+), 4 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 33c669c..19b898e 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -359,6 +359,13 @@ enum Cmd { /// --tls-cert (enforced in run_serve, not by clap). #[arg(long)] tls_key: Option, + /// Serve a bucket WAL remote (`sc+wal://…` or `sc+s3://…`) instead of + /// this repo's own object store (P36c). `` remains the serve + /// home: its `.sc/` still provides access tokens, the TLS identity, + /// and scratch space — but all served content lives in the bucket, + /// making this instance disposable. + #[arg(long)] + store: Option, /// Repo root to serve (the directory containing `.sc/`). Required /// unless a `token`/`fingerprint` subcommand is used. path: Option, @@ -948,6 +955,7 @@ fn main() -> Result<()> { tls, tls_cert, tls_key, + store, path, } => match sub { Some(ServeSub::Token { op }) => run_serve_token(op), @@ -975,6 +983,7 @@ fn main() -> Result<()> { tls, tls_cert, tls_key, + store, path, ) } @@ -3476,7 +3485,11 @@ fn run_clone_git(url: &str, dst: &std::path::Path) -> Result<()> { /// `--read-only`/`--allow-public` are `--http`-only (P29): `--stdio` /// delegates auth/access entirely to ssh, so combining them is refused /// rather than silently ignored. `--max-connections`/`--timeout` are also -/// `--http`-only (P31). `--max-pack-size` applies to both (P31). +/// `--http`-only (P31). `--max-pack-size` applies to both (P31). `--store` +/// (P36c) redirects served content to a bucket WAL remote; `path` remains +/// the serve home (tokens/TLS identity/scratch) either way. A malformed +/// `--store` URL is validated up front, before any bind — same fail-fast +/// idiom as `run_remote`'s `BucketUrl::parse` check. fn run_serve( stdio: bool, http: Option, @@ -3488,8 +3501,12 @@ fn run_serve( tls: bool, tls_cert: Option, tls_key: Option, + store: Option, path: PathBuf, ) -> Result<()> { + if let Some(url) = &store { + scl_repo::BucketUrl::parse(url)?; // fail fast on malformed URLs, before any bind + } match (stdio, http) { (true, None) => { if read_only || allow_public { @@ -3512,7 +3529,12 @@ fn run_serve( }; let mut stdin = std::io::stdin().lock(); let mut stdout = std::io::stdout().lock(); - scl_repo::wire::serve_with_policy(&path, &mut stdin, &mut stdout, policy)?; + match &store { + Some(url) => { + scl_repo::wire::serve_bucket_with_policy(url, &mut stdin, &mut stdout, policy)? + } + None => scl_repo::wire::serve_with_policy(&path, &mut stdin, &mut stdout, policy)?, + } Ok(()) } (false, Some(addr)) => { @@ -3542,7 +3564,7 @@ fn run_serve( allow_public, limits, tls_mode, - None, + store.as_deref(), )?; Ok(()) } diff --git a/crates/cli/tests/bucket_remote.rs b/crates/cli/tests/bucket_remote.rs index 94f5079..ff307d0 100644 --- a/crates/cli/tests/bucket_remote.rs +++ b/crates/cli/tests/bucket_remote.rs @@ -2,8 +2,9 @@ //! is proven in scl-repo's bucket_transport tests; this exercises CLI //! plumbing: remote add validation, push, clone, fetch. +use std::io::BufRead; use std::path::{Path, PathBuf}; -use std::process::{Command, Output}; +use std::process::{Child, Command, Output, Stdio}; fn sc(dir: &Path, args: &[&str]) -> Output { let mut cmd = Command::new(env!("CARGO_BIN_EXE_sc")); @@ -18,6 +19,39 @@ fn tmp(tag: &str) -> PathBuf { d } +/// Spawn `sc serve --http 127.0.0.1:0 ` and return the child +/// plus the OS-assigned `host:port` it reports on its first stdout line +/// (`listening on `). Copied from `crates/cli/tests/http_remote.rs`'s +/// `spawn_http_server` — same readiness contract (the announce line prints +/// only after `TcpListener::bind` returns) — parameterized with `extra` so +/// this file's tests can pass `--store `. +fn spawn_http_server_with(root: &Path, extra: &[&str]) -> (Child, String) { + let mut args = vec!["serve", "--http", "127.0.0.1:0"]; + args.extend_from_slice(extra); + args.push(root.to_str().unwrap()); + let mut child = Command::new(env!("CARGO_BIN_EXE_sc")) + .args(&args) + .stdout(Stdio::piped()) + .spawn() + .expect("spawn sc serve --http"); + let stdout = child.stdout.take().expect("child stdout is piped"); + let mut reader = std::io::BufReader::new(stdout); + let mut line = String::new(); + let n = reader + .read_line(&mut line) + .expect("read serve startup line"); + if n == 0 { + let status = child.wait().ok(); + panic!("sc serve --http exited before announcing a bound address: {status:?}"); + } + let addr = line + .trim() + .strip_prefix("listening on ") + .unwrap_or_else(|| panic!("unexpected serve startup line: {line:?}")) + .to_string(); + (child, addr) +} + #[test] fn bucket_clone_push_fetch_round_trip_and_url_validation() { let a = tmp("a"); @@ -51,3 +85,78 @@ fn bucket_clone_push_fetch_round_trip_and_url_validation() { assert!(!d.exists()); } } + +/// `sc serve --http --store ` serves a bucket instead of the +/// serve-home's own object store (P36c): a repo pushed straight to the +/// bucket is clonable through the server, and a push through the server is +/// visible to a completely separate server instance pointed at the same +/// bucket (proving durable state lives in the bucket, not the server +/// process). A malformed `--store` URL must be refused before any bind. +#[test] +fn serve_store_serves_a_bucket_and_second_instance_sees_pushes() { + let bucket = tmp("srv-bucket"); + let home = tmp("srv-home"); + assert!(sc(&home, &["init"]).status.success()); + let store = format!("sc+wal://{}", bucket.display()); + + // seed: a repo pushed straight to the bucket + let seed = tmp("srv-seed"); + assert!(sc(&seed, &["init"]).status.success()); + std::fs::write(seed.join("f.txt"), b"served from bucket").unwrap(); + assert!(sc(&seed, &["commit", "-m", "c1"]).status.success()); + assert!(sc(&seed, &["remote", "add", "origin", &store]) + .status + .success()); + assert!(sc(&seed, &["push", "origin"]).status.success()); + + // malformed store URL refused before binding + let bad = sc( + &home, + &[ + "serve", + "--http", + "127.0.0.1:0", + "--store", + "sc+s3://", + home.to_str().unwrap(), + ], + ); + assert!(!bad.status.success()); + + let (mut child, addr) = spawn_http_server_with(&home, &["--store", &store]); + let parent = tmp("srv-clone"); + let dst = parent.join("d"); + let url = format!("sc+http://{addr}/repo"); + assert!(sc(&parent, &["clone", &url, dst.to_str().unwrap()]) + .status + .success()); + assert_eq!( + std::fs::read(dst.join("f.txt")).unwrap(), + b"served from bucket" + ); + // push through the server, then read it back via a SECOND instance + std::fs::write(dst.join("g.txt"), b"hop").unwrap(); + assert!(sc(&dst, &["commit", "-m", "c2"]).status.success()); + assert!(sc(&dst, &["push", "origin"]).status.success()); + child.kill().ok(); + let (mut child2, addr2) = spawn_http_server_with(&home, &["--store", &store]); + let parent2 = tmp("srv-clone2"); + let d2 = parent2.join("d2"); + assert!(sc( + &parent2, + &[ + "clone", + &format!("sc+http://{addr2}/repo"), + d2.to_str().unwrap() + ] + ) + .status + .success()); + assert_eq!(std::fs::read(d2.join("g.txt")).unwrap(), b"hop"); + child2.kill().ok(); + + for p in [&bucket, &home, &seed, &parent, &parent2] { + std::fs::remove_dir_all(p).unwrap(); + assert!(!p.exists()); + } +} From 6b04760328b3601a450e2482f77b2c05eb4e1bee Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 21:12:02 +0300 Subject: [PATCH 09/11] docs: ADR-0046 As-built P36b/c, CLAUDE.md P36 complete, ROADMAP/THREAT-MODEL (P36b+c) --- CLAUDE.md | 5 ++- ROADMAP.md | 39 ++++++++++---------- docs/THREAT-MODEL.md | 17 +++++++-- docs/adr/0046-wal-bucket-remotes.md | 56 +++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6b8b5dc..0f05fa0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -150,7 +150,7 @@ the code, those win. | P33 | Randomized protected sealing (fresh DEK + nonce; `RANDOMIZED` perms bit); dual-read of pre-P33 convergent ciphertext; per-checkout keyed stat cache; `sc rewrap` upgrades convergent blobs at the tip | [0043](docs/adr/0043-randomized-protected-encryption.md) | | P34 | Private branches: ref points at a sealed-branch manifest; every commit/tree/blob individually sealed (copy-on-write) under a per-branch KEK wrapped per recipient + escrow; `sc branch --private/grant/revoke/publish`; opaque to non-recipients (content, paths, messages); grant O(1), revoke rotates the KEK; publish replays to public with a scanner gate; git bridge + private→public integration refused; `PROTOCOL_VERSION` 4 | [0044](docs/adr/0044-per-branch-access-control.md) | | P35 | Native Tauri desktop browser: opens `.sc` repositories through `scl-repo`, shows local/remote refs, all-parent snapshot DAG + provenance, public trees and first-parent diffs; protected content is locked and private branches remain opaque; no mutation or identity surface | [0045](docs/adr/0045-native-desktop-read-model.md) | -| P36 | P36a built: bucket WAL remotes (sc+wal://, sc+s3://) — immutable packs + CAS'd manifest, multi-writer safe, no coordinator; checkpoints (P36b) and bucket-backed serve (P36c) pending | [0046](docs/adr/0046-wal-bucket-remotes.md) | +| P36 | Bucket WAL remotes (sc+wal://, sc+s3://): immutable packs + CAS'd manifest, checkpoints + log-tail cold start, bucket-backed `sc serve --store` with disposable instances | [0046](docs/adr/0046-wal-bucket-remotes.md) | ## Standing boundaries & gotchas @@ -172,6 +172,9 @@ transport-adjacent. The rest, imperatively: - **Bucket remotes hold public content plaintext at rest** — bucket ACL is the perimeter (sealed content stays ciphertext, unchanged); partial-clone `filter` against bucket remotes is refused. +- **`sc serve --store` still requires a local serve home with `.sc/`** — + tokens, TLS identity, and pack spills live there; the bucket holds all + served content. - **Protected sealing is randomized since P33.** Pre-P33 convergent ciphertext dual-reads forever and stays equality-confirmable forever (rotation ≠ erasure). Identical independent edits on two branches now genuinely diff --git a/ROADMAP.md b/ROADMAP.md index ad758ed..4bd448d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -930,24 +930,15 @@ scale-&-reach horizon): - **Bucket compaction/gc (P36a follow-on).** The WAL log (`log/` keys, each a hand-rolled versioned binary `walfmt` entry, not protobuf) grows - unboundedly with no compaction — every reader walks the full parent chain - back from `head_seq`, and no entry or superseded pack is ever removed. - Deferred until checkpointing (below) gives a safe compaction cutoff. + unboundedly with no compaction — a cold-start reader now stops at the + latest checkpoint (P36b) instead of walking to `0`, but no log entry, + checkpoint, or superseded pack is ever actually removed from the bucket. + Deferred until a safe pruning cutoff (e.g. "no reader can still need + anything before checkpoint N") is designed. - **Leases (P36a follow-on).** The only cross-writer coordination today is the manifest's compare-and-swap; there is no lease/TTL primitive for operations that need to hold exclusive intent across more than one bucket round-trip (e.g. a long-running compaction). Deferred. -- **Checkpoint fold (P36b, next).** `walfmt::Manifest.checkpoint_seq` - already reserves a field for a future compaction cutoff, but nothing - writes a `checkpoints//` object yet — cold start is always a full - log walk from `head_seq`. Folding the log into a periodic ref snapshot so - cold start becomes snapshot + short tail is the next bucket-remote phase. -- **Bucket-backed `sc serve` (P36c, next).** `sc serve` cannot host a - bucket as its backing store today — a bucket remote is written to - directly by every client's `sc push`/`sc fetch`, not brokered through a - server process. Wiring `BucketTransport` in as an `sc serve` backend - (so a bucket remote can also sit behind access control / resource limits - the way `.sc/` repos do via P29/P31) is deferred to a follow-on phase. - **Partial clone from bucket remotes (P36a follow-on).** `sc clone --filter` against `sc+wal://`/`sc+s3://` is refused outright (`BucketTransport` has no per-prefix negotiation); teaching the WAL format @@ -964,17 +955,23 @@ scale-&-reach horizon): individually, but a transfer moves a whole pack at a time, so memory use is pack-sized, not object-sized. Streaming the S3 request/response bodies instead of buffering them whole is deferred. -- **Incremental `refresh()` for bucket remotes (P36a follow-on).** - `BucketTransport::refresh` short-circuits on an unchanged manifest tag, - but whenever the manifest *has* changed it re-walks the full parent chain - from `head_seq` and re-fetches every `idx_key` on that chain from - scratch — O(chain) GETs per manifest change, which gets expensive under - fleet-frequency pushes. An incremental refresh that picks up from the - last-seen manifest/seq instead of re-walking from scratch is deferred. +- **Incremental `refresh()` for bucket remotes (P36a follow-on, narrowed by + P36b).** `BucketTransport::refresh` short-circuits on an unchanged + manifest tag, but whenever the manifest *has* changed it re-walks the log + chain from `head_seq` back to the manifest's checkpoint (P36b bounds this + to the tail instead of the full chain to `0`) and still re-fetches every + `idx_key` in the cumulative pack list from scratch each time — expensive + under fleet-frequency pushes even with the checkpoint bound. An + incremental refresh that picks up from the last-seen manifest/seq instead + of rebuilding the whole index every time is deferred. - **Bucket-aware push negotiation (P36a follow-on).** Push negotiation today issues a `has_object` round trip per object over S3; batching those probes into one `refresh()` plus local index lookups (instead of one S3 round trip per object) is deferred. +- **Serve-side persistent pack cache (P36c follow-on).** A bucket-backed + serve instance re-downloads packs per connection; a content-addressed + on-disk cache in the serve home would make warm instances cheap without + affecting correctness. ## How a phase gets built diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index 3c44e22..307c13c 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -352,12 +352,25 @@ too, not just the src-control-side metadata. controls — but a bucket has no `sc`-native access-control layer at all (no bearer tokens, no `--read-only`, no loopback-bind gate). **Bucket ACL is the entire confidentiality perimeter for public content on a bucket - remote**; an operator who needs `sc`-native read/write scoping for public - content should use `sc serve --http`/`--https` instead. + remote** written to directly by `sc push`/`sc fetch`; an operator who + needs `sc`-native read/write scoping for public content should front it + with `sc serve --http`/`--https` (including via `--store`, serving that + very bucket — see below). - **Partial clone (`--filter`) against a bucket remote is refused**, not silently ignored — the WAL format has no per-prefix negotiation yet, so there is no partial-fetch code path to reason about for a bucket remote at all. +- **`sc serve --store ` (P36c) splits content from + access-control state.** The bucket holds all served content; the serve + **home** (`path`, still a plain `.sc/` directory) holds tokens, TLS + identity/pins, and pack-spool scratch. A bucket-backed serve instance + enforces the exact same P29/P31 gates against its clients as a + local-store instance — bearer tokens, `--read-only`, the loopback-bind + default, connection/timeout/pack-size limits — while itself trusting the + bucket no further than any other reader does: every object's BLAKE3 id is + re-verified and the WAL metadata is strict-decoded fail-closed exactly as + described above, so a hostile or corrupted bucket gets no more leverage + against a serve instance than it would against a direct `sc` client. ## Untrusted-input hardening (DoS) — ADR-0039 diff --git a/docs/adr/0046-wal-bucket-remotes.md b/docs/adr/0046-wal-bucket-remotes.md index 5bc60c5..f1bffe1 100644 --- a/docs/adr/0046-wal-bucket-remotes.md +++ b/docs/adr/0046-wal-bucket-remotes.md @@ -147,3 +147,59 @@ round-trip, racing/fleet pushes, crash-mid-push recovery, and the `MAX_OBJECT_SIZE` cap. CLI plumbing (remote-add validation, push, clone, fetch through the real binary) proven in `crates/cli/tests/bucket_remote.rs`. + +## As built (P36b/P36c, 2026-08-26) + +**P36b — checkpoint fold.** `walfmt::Checkpoint` (magic `SCWC`; a `seq`, a +sorted branch→tip `refs` list, and a cumulative `packs` hash list, all +strict-decoded fail-closed like every other WAL record) and +`checkpoint_key(seq)` join the existing `Manifest`/`LogEntry` kinds. +`BucketTransport::refresh()` now seeds from `manifest.checkpoint_seq` when +non-zero: it fetches that checkpoint, takes its refs/packs as the fold base, +then walks only the log tail from `head_seq` down to `checkpoint_seq` +(rather than to `0`) before rebuilding the object index over the +checkpoint's cumulative packs plus the tail's. Every checkpoint input is +untrusted like the rest of the WAL and fails closed: a manifest naming a +checkpoint that's absent, a checkpoint object claiming a different `seq` +than the key it was fetched at, and a log chain that steps past +`checkpoint_seq` without landing on it exactly are all refused +(`Error::Wal`), never best-effort recovered. After a successful commit, +`maybe_fold_checkpoint` opportunistically folds once +`head_seq - checkpoint_seq > CHECKPOINT_INTERVAL` (64): it claims +`checkpoints/` via `put_new` (idempotent — a racing folder's +duplicate claim is a no-op, not an error), then CASes the manifest to point +at it. A lost manifest CAS (someone else advanced the WAL meanwhile) drops +the fold silently — a checkpoint is derived data any reader can refold from +the log later, so there is nothing to retry — and the push that triggered +the fold attempt has already durably landed either way. + +**P36c — bucket-backed serve.** A new `wire::ServeTransport` enum +(`Local(LocalTransport)` / `Bucket { transport: BucketTransport, tmp: +TempServeDir }`) lets `serve_session` dispatch every verb except the +`GetPack`/`PutPack` pair identically regardless of backend. +`serve_bucket_with_policy(store_url, …)` mirrors `serve_with_policy` +verb-for-verb — same handshake, same `PROTOCOL_VERSION`, same P29/P31 +read-only gate and pack-spool caps — but opens a `BucketTransport` instead +of a local repo. `TempServeDir` is an RAII scratch directory under +`std::env::temp_dir()` (created per session, removed best-effort on drop) +standing in for a local repo's `.sc/tmp/`, since a bucket has no scratch directory of +its own — this keeps the ephemeral-mode zero-residue invariant intact for +bucket-backed serve too. `sc serve --http`/`--stdio` gained `--store `: +`path` remains the serve **home** (`.sc/` — tokens, TLS identity/pins, +scratch), while served content routes to the bucket at `` instead of +the home's own object store. A malformed `--store` URL is rejected via +`BucketUrl::parse` before any bind (`run_serve`'s fail-fast check, mirroring +`run_remote`'s). Proven by `two_disposable_instances_serve_one_bucket_with_strict_consistency` +and `read_only_floor_holds_in_store_mode` in +`crates/repo/src/http_transport.rs` (two independent `sc serve --http +--store` server instances, each with its own serve home, observe each +other's pushes to the shared bucket with no propagation delay — the +manifest-CAS strict-consistency contract from P36a, now exercised end to +end over HTTP) and +`serve_store_serves_a_bucket_and_second_instance_sees_pushes` in +`crates/cli/tests/bucket_remote.rs` (the same property across two real, +separately-spawned `sc` **processes**). + +This supersedes the two now-stale Consequences bullets above about +checkpoint folding and `sc serve` being unable to host a bucket as its +backing store — both are built as of P36b/P36c. From d30d826c80d7e0be19b63723c294244c9997ac88 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 21:42:17 +0300 Subject: [PATCH 10/11] =?UTF-8?q?fix:=20final-review=20fixes=20=E2=80=94?= =?UTF-8?q?=20fold=20MAX=5FLIST=20guard,=20serve=20spills=20under=20home?= =?UTF-8?q?=20.sc/tmp,=20fold=20dedup=20+=20docs=20(P36b+c)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ROADMAP.md | 7 +- crates/cli/src/main.rs | 10 +- crates/cli/tests/bucket_remote.rs | 2 + crates/repo/src/bucket_transport.rs | 151 ++++++++++++++++++++++++++-- crates/repo/src/http_transport.rs | 4 +- crates/repo/src/walfmt.rs | 7 +- crates/repo/src/wire.rs | 90 ++++++++++++++--- 7 files changed, 243 insertions(+), 28 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 4bd448d..279e9ee 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -934,7 +934,12 @@ scale-&-reach horizon): latest checkpoint (P36b) instead of walking to `0`, but no log entry, checkpoint, or superseded pack is ever actually removed from the bucket. Deferred until a safe pruning cutoff (e.g. "no reader can still need - anything before checkpoint N") is designed. + anything before checkpoint N") is designed. Compaction is also what + relieves `walfmt::MAX_LIST` (65536): a checkpoint fold with more refs or + packs than that cap is skipped outright (`maybe_fold_checkpoint`'s guard, + P36b review) rather than writing an object `Checkpoint::decode` would then + refuse to read back, so a remote whose live ref/pack count grows past the + cap loses folding entirely until compaction can retire entries below it. - **Leases (P36a follow-on).** The only cross-writer coordination today is the manifest's compare-and-swap; there is no lease/TTL primitive for operations that need to hold exclusive intent across more than one bucket diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 19b898e..9a695a3 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -3530,9 +3530,13 @@ fn run_serve( let mut stdin = std::io::stdin().lock(); let mut stdout = std::io::stdout().lock(); match &store { - Some(url) => { - scl_repo::wire::serve_bucket_with_policy(url, &mut stdin, &mut stdout, policy)? - } + Some(url) => scl_repo::wire::serve_bucket_with_policy( + url, + &path, + &mut stdin, + &mut stdout, + policy, + )?, None => scl_repo::wire::serve_with_policy(&path, &mut stdin, &mut stdout, policy)?, } Ok(()) diff --git a/crates/cli/tests/bucket_remote.rs b/crates/cli/tests/bucket_remote.rs index ff307d0..a5317db 100644 --- a/crates/cli/tests/bucket_remote.rs +++ b/crates/cli/tests/bucket_remote.rs @@ -139,6 +139,7 @@ fn serve_store_serves_a_bucket_and_second_instance_sees_pushes() { assert!(sc(&dst, &["commit", "-m", "c2"]).status.success()); assert!(sc(&dst, &["push", "origin"]).status.success()); child.kill().ok(); + let _ = child.wait(); let (mut child2, addr2) = spawn_http_server_with(&home, &["--store", &store]); let parent2 = tmp("srv-clone2"); let d2 = parent2.join("d2"); @@ -154,6 +155,7 @@ fn serve_store_serves_a_bucket_and_second_instance_sees_pushes() { .success()); assert_eq!(std::fs::read(d2.join("g.txt")).unwrap(), b"hop"); child2.kill().ok(); + let _ = child2.wait(); for p in [&bucket, &home, &seed, &parent, &parent2] { std::fs::remove_dir_all(p).unwrap(); diff --git a/crates/repo/src/bucket_transport.rs b/crates/repo/src/bucket_transport.rs index 98e09d4..306928f 100644 --- a/crates/repo/src/bucket_transport.rs +++ b/crates/repo/src/bucket_transport.rs @@ -5,7 +5,7 @@ use crate::error::{Error, Result}; use crate::transport::Transport; use crate::walfmt::{ - checkpoint_key, idx_key, log_key, pack_key, Checkpoint, LogEntry, Manifest, RefUpdate, + checkpoint_key, idx_key, log_key, pack_key, Checkpoint, LogEntry, Manifest, RefUpdate, MAX_LIST, }; use scl_core::pack::{parse_index, read_object_at_bounded, IndexEntry, PackWriter}; use scl_core::{Object, ObjectId}; @@ -37,11 +37,14 @@ struct WalView { /// A [`Transport`] whose object graph and refs live entirely in an /// object-storage bucket (S3-compatible or a local directory), read through /// the parent-linked WAL log format `walfmt` defines. Readers walk the log -/// backward from the manifest's `head_seq` via `parent_seq` links — a log -/// entry's own claimed `seq` is never trusted for reachability, only for -/// self-consistency (it must match the slot it was read from and its parent -/// must strictly precede it). An entry not on that chain (e.g. a losing -/// racer's orphaned append) is invisible to every read method here, by +/// backward from the manifest's `head_seq` via `parent_seq` links, stopping +/// at `checkpoint_seq` (seeded from the checkpoint object it names, P36b) — +/// a cold start no longer replays to `0`, only the tail past the last fold. +/// A log entry's own claimed `seq` is never trusted for reachability, only +/// for self-consistency (it must match the slot it was read from and its +/// parent must strictly precede it, down to `checkpoint_seq`). An entry not +/// on that chain (e.g. a losing racer's orphaned append) is invisible to +/// every read method here, by /// construction. pub struct BucketTransport { bucket: Box, @@ -89,6 +92,16 @@ fn capped(what: &str, bytes: Vec) -> Result> { /// last checkpoint (spec: "default 64 entries, one tunable constant"). const CHECKPOINT_INTERVAL: u64 = 64; +/// True when a checkpoint fold of `nrefs` refs and `npacks` packs would +/// exceed `walfmt::MAX_LIST` on either axis — i.e. would write a checkpoint +/// object `Checkpoint::decode` then refuses to read back. Pure predicate +/// (no bucket I/O) so `maybe_fold_checkpoint`'s guard can be unit-tested +/// directly against the exact boundary `Checkpoint::decode` enforces, +/// without constructing a real 65536+-ref/pack `WalView` end to end. +fn fold_would_overflow(nrefs: usize, npacks: usize) -> bool { + nrefs > MAX_LIST || npacks > MAX_LIST +} + /// Which bucket backend a [`BucketUrl`] names. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BucketScheme { @@ -181,8 +194,10 @@ impl BucketTransport { } /// One conditional GET of the manifest; on change, rebuild refs + index - /// by walking parent links head -> 0 (seq numbers are claims; the chain - /// is the truth — off-chain entries are garbage). + /// by walking parent links from `head_seq` down to `checkpoint_seq` + /// (P36b) — seeded from that checkpoint's own `refs`/`packs` rather than + /// replayed from `0` — then folding the tail entries on top (seq numbers + /// are claims; the chain is the truth — off-chain entries are garbage). fn refresh(&self) -> Result<()> { let cached_tag = self.view.borrow().as_ref().map(|v| v.tag.clone()); match self.bucket.get("manifest", cached_tag.as_deref())? { @@ -373,12 +388,45 @@ impl BucketTransport { if head_seq - checkpoint_seq <= CHECKPOINT_INTERVAL { return Ok(()); } + // Two pushers landing identical packs (e.g. two racers whose staged + // objects happened to build the same pack bytes) leave duplicate + // hashes in the cumulative `packs` list; don't copy the duplicates + // forward into every future checkpoint. Order-preserving: chain + // order (oldest first) has no semantic meaning here, but there's no + // reason to churn it either. + let mut seen = std::collections::HashSet::new(); + let packs: Vec = packs + .into_iter() + .filter(|h| seen.insert(h.clone())) + .collect(); + if fold_would_overflow(refs.len(), packs.len()) { + // A fold at or under `walfmt::MAX_LIST` on both axes round-trips + // through `Checkpoint::decode` cleanly. One that overflows would + // write a checkpoint object that decode then REFUSES to read + // back — bricking the remote: every subsequent `refresh` by any + // reader (every client and every `serve` instance, including + // this same process) fails closed on `Manifest.checkpoint_seq` + // naming an undecodable checkpoint, with no way back short of + // hand-editing the bucket. Skip the fold instead — it is + // best-effort by design (see the doc above) — and let every + // reader keep doing the slower but correct full-tail walk back + // to the last checkpoint that DID fit. Durable relief is + // compaction (ROADMAP "Bucket compaction/gc"), which can retire + // refs/packs instead of letting them accumulate into ever-larger + // checkpoints forever. + return Ok(()); + } let ck = Checkpoint { seq: head_seq, refs, packs, }; - // Claim the object first (idempotent), then point the manifest at it. + // Claim the object first (idempotent), then point the manifest at + // it. `put_new`'s discarded `bool` (already-existed vs. freshly + // written) is safe to ignore here: two racing folds off the SAME + // head hold byte-identical views (same refs, same deduped packs, + // same `head_seq`), so a colliding claim is content-identical, not + // a conflict — there is nothing to redo either way. self.bucket .put_new(&checkpoint_key(head_seq), &ck.encode())?; let manifest = Manifest { @@ -1574,4 +1622,89 @@ mod tests { std::fs::remove_dir_all(&broot).unwrap(); assert!(!broot.exists()); } + + /// Boundary-exact unit test for the fold guard's pure predicate: it must + /// agree exactly with `Checkpoint::decode`'s own cap (`nrefs`/`npacks` + /// each allowed up to and including `MAX_LIST`, refused strictly above + /// it) — a fold that passes this guard must always be decodable, and a + /// fold that would overflow must always be caught before it ever reaches + /// the bucket. + #[test] + fn fold_would_overflow_matches_checkpoint_decodes_cap_exactly() { + assert!(!fold_would_overflow(0, 0)); + assert!(!fold_would_overflow(MAX_LIST, 0)); + assert!(!fold_would_overflow(0, MAX_LIST)); + assert!(!fold_would_overflow(MAX_LIST, MAX_LIST)); + assert!(fold_would_overflow(MAX_LIST + 1, 0)); + assert!(fold_would_overflow(0, MAX_LIST + 1)); + assert!(fold_would_overflow(MAX_LIST + 1, MAX_LIST + 1)); + } + + /// Regression for the "over-cap fold bricks the remote" review finding: + /// `maybe_fold_checkpoint` must skip the fold (return `Ok`, write + /// nothing) once the view it would fold exceeds `walfmt::MAX_LIST` on + /// either axis, rather than writing a checkpoint object + /// `Checkpoint::decode` then refuses to read back. + /// + /// Driving this through real traffic would need 65537+ actual pushes — + /// far too slow for a unit test, and the boundary case is exercised + /// precisely above. Instead this hand-builds an over-cap `WalView` + /// directly (this test module is `bucket_transport::tests`, a + /// descendant of the defining module, so it may reach `BucketTransport`'s + /// private `view` field and construct a `WalView` — the same private + /// types `refresh()` itself builds) and calls the private + /// `maybe_fold_checkpoint` method directly, proving the guard fires + /// before any bucket write — no checkpoint object lands and the + /// manifest is untouched. `pushes_past_the_interval_fold_a_checkpoint_and_cold_start_uses_it` + /// above is the complementary proof that an ordinary (under-cap) fold + /// still happens. + #[test] + fn fold_is_skipped_and_the_triggering_push_still_succeeds_when_the_view_exceeds_the_cap() { + let pid = std::process::id(); + let broot = std::env::temp_dir().join(format!("scl-bt-foldcap-{pid}")); + let _ = std::fs::remove_dir_all(&broot); + let t = BucketTransport::from_bucket(Box::new(DirBucket::open(&broot).unwrap())).unwrap(); + + // Hand-build a view whose ref count exceeds MAX_LIST — the exact + // shape `refresh()` would eventually produce after enough real + // pushes, minus actually performing 65537 of them. + let mut refs = BTreeMap::new(); + for i in 0..=MAX_LIST { + let branch = format!("b-{i}"); + refs.insert(branch.clone(), ObjectId::of(branch.as_bytes())); + } + *t.view.borrow_mut() = Some(WalView { + tag: "fake-tag".to_string(), + manifest: Manifest { + head_seq: CHECKPOINT_INTERVAL + 100, // well past the fold threshold + checkpoint_seq: 0, + head_branch: "main".to_string(), + }, + refs, + index: BTreeMap::new(), + packs: Vec::new(), + }); + + // The triggering push's own commit already landed (that's what put + // this over-cap view in place); the fold itself must be a no-op — + // best-effort by design — not an error. + t.maybe_fold_checkpoint().unwrap(); + + // Nothing was written: no checkpoint object, and (since we never + // actually pushed the fake manifest to the bucket, only mutated the + // in-memory view) the manifest key is still absent. + let bucket = DirBucket::open(&broot).unwrap(); + assert!(matches!( + bucket.get("manifest", None).unwrap(), + Fetched::Absent + )); + assert!( + bucket.list("checkpoints/").unwrap().is_empty(), + "an over-cap fold must not write any checkpoint object" + ); + + drop(t); + std::fs::remove_dir_all(&broot).unwrap(); + assert!(!broot.exists()); + } } diff --git a/crates/repo/src/http_transport.rs b/crates/repo/src/http_transport.rs index 294e7d3..7ae71fa 100644 --- a/crates/repo/src/http_transport.rs +++ b/crates/repo/src/http_transport.rs @@ -1063,7 +1063,9 @@ fn handle_http_connection( ro_drain_cap: crate::wire::RO_DRAIN_CAP, }; match store { - Some(url) => crate::wire::serve_bucket_with_policy(url, &mut reader, &mut writer, policy), + Some(url) => { + crate::wire::serve_bucket_with_policy(url, root, &mut reader, &mut writer, policy) + } None => crate::wire::serve_with_policy(root, &mut reader, &mut writer, policy), } } diff --git a/crates/repo/src/walfmt.rs b/crates/repo/src/walfmt.rs index 0aa8a6f..67a5c25 100644 --- a/crates/repo/src/walfmt.rs +++ b/crates/repo/src/walfmt.rs @@ -9,7 +9,12 @@ const ENTRY_MAGIC: &[u8; 4] = b"SCWE"; const CHECKPOINT_MAGIC: &[u8; 4] = b"SCWC"; const VERSION: u32 = 1; const MAX_NAME: usize = 4096; -const MAX_LIST: usize = 65536; +/// Cap on the number of entries in any length-prefixed list this format +/// encodes (a `LogEntry`'s `packs`/`updates`, a `Checkpoint`'s `refs`/ +/// `packs`). `pub(crate)` so `bucket_transport`'s checkpoint fold can check +/// against the same cap `Checkpoint::decode` enforces, rather than +/// duplicating the literal — see `maybe_fold_checkpoint`'s guard. +pub(crate) const MAX_LIST: usize = 65536; const MAX_HASH: usize = 128; /// A bounds-checked cursor over a decode buffer. Every read either advances diff --git a/crates/repo/src/wire.rs b/crates/repo/src/wire.rs index 431ef56..a15e39d 100644 --- a/crates/repo/src/wire.rs +++ b/crates/repo/src/wire.rs @@ -711,17 +711,30 @@ fn handshake_hello(r: &mut impl Read, w: &mut impl Write) -> Result { /// RAII scratch dir for a bucket-backed serve session's pack spills (P36c). /// A bucket remote has no `.sc/tmp/` of its own to spool into, so a bucket -/// session gets one disposable directory under `std::env::temp_dir()` for its -/// lifetime instead — removed (best-effort) on drop, so the ephemeral-mode -/// disk invariant (zero residue after the session ends) holds for -/// bucket-backed serve exactly as it does for every other ephemeral session. +/// session gets one disposable directory under the serve HOME's own +/// `.sc/tmp/` for its lifetime instead — matching CLAUDE.md's and +/// THREAT-MODEL.md's documented "pack-spool scratch lives in the serve +/// home" contract (and local-mode's own `.sc/tmp` convention) rather than +/// the process-wide `std::env::temp_dir()`, which is shared, unbounded, and +/// not scoped to this repo's access control at all. Removed (best-effort) +/// on drop, so the ephemeral-mode disk invariant (zero residue after the +/// session ends) holds for bucket-backed serve exactly as it does for every +/// other ephemeral session. pub(crate) struct TempServeDir(std::path::PathBuf); impl TempServeDir { - fn create() -> Result { + /// Create the scratch dir under `/.sc/tmp/serve-bucket--`. + /// `home` is the serve session's HOME directory (the same `path` a local + /// `serve_with_policy` session materializes into) — callers must have + /// already confirmed it has a `.sc/` (the same gate `handle_http_connection` + /// and `LocalTransport::open` apply) before calling this. + fn create_in(home: &std::path::Path) -> Result { static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); let n = COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let dir = std::env::temp_dir().join(format!("sc-serve-bucket-{}-{n}", std::process::id())); + let dir = home + .join(".sc") + .join("tmp") + .join(format!("serve-bucket-{}-{n}", std::process::id())); std::fs::create_dir_all(&dir)?; Ok(TempServeDir(dir)) } @@ -840,10 +853,12 @@ pub fn serve_with_policy( /// Open a [`BucketTransport`] plus this session's [`TempServeDir`] as one /// [`ServeTransport::Bucket`] — a single fallible step so /// [`serve_bucket_with_policy`] only needs one match to decide whether to -/// reply OK or a typed error. -fn open_bucket_serve_transport(store_url: &str) -> Result { +/// reply OK or a typed error. `home` is the serve session's HOME directory +/// (see [`TempServeDir::create_in`]) — the spool dir is created under its +/// `.sc/tmp/`, not the process-wide temp dir. +fn open_bucket_serve_transport(store_url: &str, home: &std::path::Path) -> Result { let transport = BucketTransport::open(store_url)?; - let tmp = TempServeDir::create()?; + let tmp = TempServeDir::create_in(home)?; Ok(ServeTransport::Bucket { transport, tmp }) } @@ -852,10 +867,17 @@ fn open_bucket_serve_transport(store_url: &str) -> Result { /// `PROTOCOL_VERSION`, same read-only gate and pack-spool caps — every verb /// except the pack pair goes through [`Transport`] identically either way. /// All durable state lives in the bucket; this process holds only an RAII -/// scratch dir ([`TempServeDir`]) for pack spills, removed when the session -/// ends. +/// scratch dir ([`TempServeDir`]) for pack spills, created under `home`'s own +/// `.sc/tmp/` (CLAUDE.md / THREAT-MODEL.md's "pack-spool scratch lives in the +/// serve home" contract) and removed when the session ends. `home` is the +/// same serve-HOME directory `serve_with_policy` materializes a local repo +/// into — callers must have already confirmed it has a `.sc/` before calling +/// this (both current callers, `handle_http_connection` and the CLI's +/// `--stdio` path, already do, via the same gate `LocalTransport::open` +/// would apply). pub fn serve_bucket_with_policy( store_url: &str, + home: &std::path::Path, r: &mut impl Read, w: &mut impl Write, policy: WirePolicy, @@ -863,7 +885,7 @@ pub fn serve_bucket_with_policy( if !handshake_hello(r, w)? { return Ok(()); } - let transport = match open_bucket_serve_transport(store_url) { + let transport = match open_bucket_serve_transport(store_url, home) { Ok(t) => { write_ok(w, &u32_body(PROTOCOL_VERSION))?; t @@ -1804,13 +1826,19 @@ mod tests { /// `serve_with_policy` against a bucket WAL instead of a local `.sc/`: /// handshake, `ListRefs`, a streamed `GetPack`, and a `PutPack` + /// `UpdateRef` that lands a second commit — verified by a fresh - /// `BucketTransport::open` on the bucket after the session ends. + /// `BucketTransport::open` on the bucket after the session ends. Also + /// pins the fix for a review finding: the session's pack-spool scratch + /// dir must live under the serve HOME's own `.sc/tmp/`, not the + /// process-wide `std::env::temp_dir()` — asserted below by checking that + /// `home/.sc/tmp/` is the only place a `serve-bucket-*` dir ever + /// appears, and that it's gone again once the session ends. #[test] fn bucket_stdio_serve_round_trips_refs_and_packs() { let pid = std::process::id(); let broot = std::env::temp_dir().join(format!("scl-wire-bucket-{pid}")); let _ = std::fs::remove_dir_all(&broot); let url = format!("sc+wal://{}", broot.display()); + let home = tmp_repo("bucket-serve-home"); // an sc repo used only as the serve HOME // Seed a one-commit bucket WAL directly via BucketTransport — same // shape as bucket_transport::tests::push_via_trait_round_trips_into_a_fresh_bucket. @@ -1827,9 +1855,11 @@ mod tests { let (client_read, mut server_write) = std::io::pipe().unwrap(); let (mut server_read, client_write) = std::io::pipe().unwrap(); let url_for_server = url.clone(); + let home_for_server = home.clone(); let srv = std::thread::spawn(move || { serve_bucket_with_policy( &url_for_server, + &home_for_server, &mut server_read, &mut server_write, WirePolicy::default(), @@ -1866,7 +1896,41 @@ mod tests { assert_eq!(t2.list_refs().unwrap(), vec![("main".to_string(), tip2)]); drop(t2); + // The session's TempServeDir (which the PutPack/GetPack verbs above + // spooled through) has already been dropped by the time `srv.join` + // returned — zero residue under the serve HOME, same ephemeral-mode + // guarantee as every other session kind. + let home_tmp = home.join(".sc").join("tmp"); + assert!( + !home_tmp.exists() || std::fs::read_dir(&home_tmp).unwrap().next().is_none(), + "no serve-bucket-* spool dir may survive the session" + ); + std::fs::remove_dir_all(&broot).unwrap(); + std::fs::remove_dir_all(&home).unwrap(); assert!(!broot.exists()); } + + /// Regression for a review finding: a bucket-backed serve session's pack + /// spool dir must live under the serve HOME's own `.sc/tmp/`, not the + /// process-wide `std::env::temp_dir()` (CLAUDE.md / THREAT-MODEL.md's + /// "pack-spool scratch lives in the serve home" contract) — and must be + /// removed again on drop, exactly like every other ephemeral scratch dir + /// in this codebase. + #[test] + fn temp_serve_dir_lives_under_home_sc_tmp_and_is_removed_on_drop() { + let home = tmp_repo("bucket-tmpdir-home"); + let path = { + let guard = TempServeDir::create_in(&home).unwrap(); + let p = guard.0.clone(); + assert!( + p.starts_with(home.join(".sc").join("tmp")), + "spool dir {p:?} must live under the serve home's .sc/tmp/" + ); + assert!(p.is_dir()); + p + }; + assert!(!path.exists(), "TempServeDir must be removed on drop"); + std::fs::remove_dir_all(&home).unwrap(); + } } From bfac074403797e4ad2238258d86b875b11cf51c7 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Wed, 26 Aug 2026 21:49:36 +0300 Subject: [PATCH 11/11] fix(cli): fail closed when sc serve --stdio --store home lacks .sc (P36c review) --- crates/cli/src/main.rs | 18 ++++++++++++ crates/cli/tests/bucket_remote.rs | 46 +++++++++++++++++++++++++++++++ crates/repo/src/wire.rs | 13 ++++++--- 3 files changed, 73 insertions(+), 4 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 9a695a3..3a576e9 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -3520,6 +3520,24 @@ fn run_serve( "--tls applies only to --http (ssh already provides --stdio's confidential channel)" ); } + // `--http`'s `handle_http_connection` gates on `.sc/` presence + // unconditionally (404 before any dispatch, store mode included). + // `--stdio` has no such gate upstream — enforce it here, before + // touching stdin, so a `--store` session never auto-vivifies + // `/.sc/tmp/` under an uninitialized directory (which + // `TempServeDir::create_in` would otherwise do via + // `create_dir_all`) and never leaves an empty `.sc/tmp/` behind + // after teardown (its `Drop` only removes the leaf spool dir). + // Local (non-`--store`) `--stdio` needs no separate check here: + // `LocalTransport::open` already fails closed on a missing + // `.sc/` inside `serve_with_policy` itself. + if store.is_some() && !path.join(".sc").is_dir() { + anyhow::bail!( + "sc serve --store requires an initialized serve home (run `sc init` in {} first): \ + tokens, TLS identity, and pack spills live under its .sc/", + path.display() + ); + } let max_pack = max_pack_size.unwrap_or(scl_repo::wire::DEFAULT_MAX_PACK_SIZE); scl_repo::wire::validate_max_pack_size(max_pack)?; let policy = scl_repo::wire::WirePolicy { diff --git a/crates/cli/tests/bucket_remote.rs b/crates/cli/tests/bucket_remote.rs index a5317db..5b10364 100644 --- a/crates/cli/tests/bucket_remote.rs +++ b/crates/cli/tests/bucket_remote.rs @@ -162,3 +162,49 @@ fn serve_store_serves_a_bucket_and_second_instance_sees_pushes() { assert!(!p.exists()); } } + +/// Regression (P36c review): `sc serve --stdio --store ` must +/// fail closed when `` has no `.sc/` yet, exactly like the `--http` +/// path's unconditional 404 gate — not silently `create_dir_all` one into +/// existence via `TempServeDir::create_in`'s spool-dir creation and leave an +/// empty `.sc/tmp/` behind after teardown. The check runs before any stdin +/// read, so the child exits immediately on its own (no hang, no need to +/// feed it a HELLO frame). +#[test] +fn stdio_serve_with_store_refuses_an_uninitialized_serve_home() { + let bucket = tmp("stdio-uninit-bucket"); + let store = format!("sc+wal://{}", bucket.display()); + // `tmp()` creates the directory itself but never runs `sc init` in it — + // exactly the "uninitialized dir" this gate must reject. + let home = tmp("stdio-uninit-home"); + assert!(!home.join(".sc").exists()); + + let out = sc( + &home, + &[ + "serve", + "--stdio", + "--store", + &store, + home.to_str().unwrap(), + ], + ); + assert!( + !out.status.success(), + "must refuse an uninitialized serve home: {out:?}" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("serve home"), + "stderr must name the serve home as the problem: {stderr}" + ); + assert!( + !home.join(".sc").exists(), + "refusing must never auto-vivify .sc/ under the uninitialized home" + ); + + for p in [&bucket, &home] { + std::fs::remove_dir_all(p).unwrap(); + assert!(!p.exists()); + } +} diff --git a/crates/repo/src/wire.rs b/crates/repo/src/wire.rs index a15e39d..5a4cbfa 100644 --- a/crates/repo/src/wire.rs +++ b/crates/repo/src/wire.rs @@ -871,10 +871,15 @@ fn open_bucket_serve_transport(store_url: &str, home: &std::path::Path) -> Resul /// `.sc/tmp/` (CLAUDE.md / THREAT-MODEL.md's "pack-spool scratch lives in the /// serve home" contract) and removed when the session ends. `home` is the /// same serve-HOME directory `serve_with_policy` materializes a local repo -/// into — callers must have already confirmed it has a `.sc/` before calling -/// this (both current callers, `handle_http_connection` and the CLI's -/// `--stdio` path, already do, via the same gate `LocalTransport::open` -/// would apply). +/// into — callers MUST have already confirmed it has a `.sc/` before calling +/// this, or [`TempServeDir::create_in`] will silently `create_dir_all` one +/// into existence under an uninitialized directory. Both current callers do: +/// `handle_http_connection` gates unconditionally on `.sc/` presence (a 404 +/// before any dispatch, store mode included) and the CLI's `run_serve` +/// checks `path.join(".sc").is_dir()` itself before reaching this function +/// (P36c review — `--stdio` has no upstream gate the way `--http` does, so +/// it cannot lean on `LocalTransport::open`'s own check the way a +/// non-`--store` `--stdio` session implicitly does). pub fn serve_bucket_with_policy( store_url: &str, home: &std::path::Path,