feat(depot): materialize capped database forks on bucket fork access

[NathanFlurry]

No description provided.

[NathanFlurry]

Stack for rivet-dev/rivet

• docs(depot): add high-level depot SQLite overview, consolidate docs #5224 sxlkwqvz · 2026-06-11 21:13:04
• feat(depot): materialize capped database forks on bucket fork access #5223 uouzzvly · 2026-06-11 21:13:03 👈
• fix(depot): teach doctor to diagnose reclaimed history via shard-seeded replay #5222 ulwkqwsz · 2026-06-11 21:13:02
• feat(depot): watermark-aligned delta reclaim and retention engine #5221 mkqlmnty · 2026-06-11 21:13:00
• test(depot): add branch history snapshot helper for exact retention assertions #5220 tlrrrnmu · 2026-06-11 21:12:58
• main

Get stack: forklift get 5223
Push local edits: forklift submit
Merge when ready: forklift merge 5223

[claude]

Review: feat(depot): materialize capped database forks on bucket fork access

This PR implements lazy database materialization for bucket forks — a clean and correct design. Reads through a forked bucket now freeze at the fork point, and the first write builds on inherited state rather than shadowing it with an empty branch. The test coverage is solid for the core scenarios. A few concerns to flag.

---

Bug: history pointer written with last_swapped_at_ms: 0 in lifecycle.rs

In both rollback code paths (rollback_database and rollback_database_to_target_tx), a synthetic DatabasePointer is constructed with last_swapped_at_ms: 0 and then encoded as the history pointer written to FDB:

let cur_ptr = DatabasePointer {
    current_branch: cur_branch,
    last_swapped_at_ms: 0,
};
// ...
let encoded_history_pointer = encode_database_pointer(cur_ptr.clone())?;
tx.informal().set(&keys::database_pointer_history_key(...), &encoded_history_pointer);

The original code used resolve_database_pointer which returned the real last_swapped_at_ms from FDB. Now the history key always records epoch 0. The field is only consumed by inspect.rs, so this doesn't affect runtime correctness, but it's incorrect data persisted in FDB. The fix is to read the pointer from FDB after materialization — materialize_inherited_database already writes it with the correct now_ms, so a resolve_database_pointer call after resolve_or_materialize_in_bucket_branch gives the right value.

---

Potential correctness gap: !bucket.initialized guard in branch_init.rs

if !bucket.initialized {
    if let Some(branch_id) = branch::resolve_or_materialize_in_bucket_branch(...) { ... }
}
// fallthrough -> new_v4(), database_initialized: true

When bucket.initialized = true, the parent-chain walk is skipped entirely. This is safe only if every database accessible through the fork was already materialized in a prior access. That invariant breaks in: fork bucket -> commit DB1 (bucket branch now initialized) -> commit DB2 for the first time (guard fires false, DB2's inherited state is skipped and an empty branch is created instead).

In practice, SQLite's read-before-write page pattern means get_pages through the read path (allow_materialize: true) would materialize DB2 before any commit. But that's an implicit invariant. Dropping the guard and always calling resolve_or_materialize_in_bucket_branch would be safer — on repeat commits to an already-materialized database the function hits the existing pointer at the top of the loop and returns immediately, so the extra FDB read is one RTT per commit:

if let Some(branch_id) = branch::resolve_or_materialize_in_bucket_branch(...).await? {
    return Ok(BranchResolution { branch_id, ..., database_initialized: false });
}

---

Missing test: multi-database bucket fork write path

No test covers: fork bucket -> commit DB1 (bucket becomes initialized) -> commit DB2 without a prior read (bucket already initialized, DB2 not yet materialized). This is the exact scenario that exercises the !bucket.initialized gap above. Worth adding to bucket_fork_access.rs.

---

Minor: tombstone check ordering on nil branch

In resolve_or_materialize_in_bucket_branch, the nil-branch early return precedes the tombstone check:

if current_branch_id == BucketBranchId::nil() {
    return Ok(None);
}
if tx.get(tombstone_key).await?.is_some() { ... }

Tombstones on the nil branch are never evaluated. This matches the original resolve_database_branch_in_bucket behavior so it's likely intentional, but a comment clarifying the deliberate precedence would help future readers.

---

What's well done

• snap_covered_target extraction is clean and reused correctly in materialize_inherited_database.
• allow_materialize: false in the doctor path is the right semantics — returning BranchNotReachable instead of stale source-branch data for a read-only diagnostic path.
• The five test scenarios cover the critical invariants: read freeze, COW write, post-fork invisibility, rollback isolation, and covered-point snap-down after reclaim.
• The module doc comment in materialize.rs clearly explains the lazy-materialization contract.