kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 191 Pull requests 61 Projects Releases Packages Wiki Activity Actions 20

Keep CardDAV rebuilds and cold reads within their budgets, rebased #777

Open
kayg wants to merge 2 commits from issue/carddav-read-perf-fix-r2 into issue/carddav-read-r2
pull from: issue/carddav-read-perf-fix-r2
merge into: kayg:issue/carddav-read-r2
Conversation 9 Commits 2 Files changed 22 +2449
kayg commented 2026-10-31 05:14:13 +00:00
Owner
Copy link

Round 2 shared epoch fix implemented, FINAL review pending

Head 670a883c. Status table, exact gates and limits. Held-reader tokens are validated against committed epochs; mid-read replacement fails closed. Updated #777 is re-merged into #794 at 21e678b2, pushed after green gates. Never merged into dev.

Earlier implementation records below are historical.

Round 1 fixes implemented, review pending

Head 3c8ddf53: disk-backed staging, bounded card/ID batches and inherited token expiry. Plan and bounds; status table, exact gates and open risks. All scoped VM gates passed. Never merged into dev.

The original implementation record below is historical; its unbounded-retention/rebuild gaps are superseded by this round.

Replacement for #739 (same change, rebased on the replacement for #734). Refs #657. This branch contains the replacement #776 commit as well, so the diff against dev shows both; once that PR merges only the perf commit remains. Measurements in #739 are from the earlier base.

Gates (cua@calternal-dev-02-std, aarch64, Nix dev shell, via slot, -j4)

  • cargo fmt --all -- --check: clean
  • cargo clippy -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions --all-targets -- -D warnings: clean
  • cargo test -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions: all pass (carddav 7 cache + 2 journal + 10 read + 1 recovery = 20; contacts suites incl. wire_index, hostile, break_*; contacts-actions 130)
  • Run on head 370d170e. No conflicts and no test breakage from the rebase.
  • Not re-run here: perf examples on a perf VM (see gaps).

Review against ADR 0064 (independent; not the author)

Matches: bridge never writes files (read-only methods, writes get 405); vCard 3.0 precomputed with strong ETags, reads never run the converter; stable user-id URLs and /.well-known/carddav redirect; paged sync with fixed snapshot, continuation tokens and 507 marker (RFC 6578 3.6); deletes reported as 404; expired/foreign tokens give valid-sync-token; rebuild is atomic and rotates the epoch; routes unmounted and an authenticator is required; depth infinity refused.

Gaps and risks (none block landing; D4 and later should pick them up):

  1. No macOS 27 replay. Piece 4 gate asks for replayed Apple traffic; only RFC fixtures exist. D5 recorder covers this.
  2. Perf gates are still hand-run examples, not a CI gate on the perf VM (ADR "Speed is a gate", 0005). Numbers in #739 were measured on the older base and not re-measured after this rebase.
  3. revisions is append-only and never pruned. Each edit adds a full card row; disk grows with edit count and only a rebuild compacts it. Needs a compaction rule (rebuild also rotates the epoch, so devices resync).
  4. changed_ids maps a fixed list of journal kinds. A new journal kind that touches a contact (for example Trash or tier moves) would be missed until a rebuild. Needs a test that enumerates note-affecting kinds, or a fallback rebuild on unknown kinds. Companion cards (FO1/D2) are not covered yet.
  5. PROPFIND depth 1 loads all summaries (O(n), not O(p)) and multiget has no href count cap beyond the 1 MiB body. Fine at 1,000 cards; ADR 0067 reply cap of 1,000 resources is not enforced.
  6. Cold rebuild stages the whole book in memory (O(n)); stated in #739, acceptable for 20k-card import but unbounded for photo books.
  7. Blocking SQLite work runs under an async mutex in the request path; one slow rebuild stalls that user's DAV reads (not other users).
  8. Missing in the read surface by design (later pieces): Lists/group cards, photos, writes, aliases for phone-chosen hrefs (hrefs are <id>.vcf only; D6 must add the alias table), If-Match/412.
  9. Cache tests use fake time-free fixtures only; no kill-9 test of a process dying mid-rebuild (backup install is atomic by design, covered by the rollback test, not by a real crash).
## Round 2 shared epoch fix implemented, FINAL review pending Head **670a883c**. [Status table, exact gates and limits](https://git.kayg.org/kayg/calternal/pulls/777#issuecomment-71338). Held-reader tokens are validated against committed epochs; mid-read replacement fails closed. Updated #777 is re-merged into #794 at **21e678b2**, pushed after green gates. Never merged into dev. Earlier implementation records below are historical. ## Round 1 fixes implemented, review pending Head **3c8ddf53**: disk-backed staging, bounded card/ID batches and inherited token expiry. [Plan and bounds](https://git.kayg.org/kayg/calternal/pulls/777#issuecomment-71175); [status table, exact gates and open risks](https://git.kayg.org/kayg/calternal/pulls/777#issuecomment-71217). All scoped VM gates passed. Never merged into dev. The original implementation record below is historical; its unbounded-retention/rebuild gaps are superseded by this round. Replacement for #739 (same change, rebased on the replacement for #734). Refs #657. This branch contains the replacement #776 commit as well, so the diff against dev shows both; once that PR merges only the perf commit remains. Measurements in #739 are from the earlier base. ## Gates (cua@calternal-dev-02-std, aarch64, Nix dev shell, via slot, -j4) - `cargo fmt --all -- --check`: clean - `cargo clippy -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions --all-targets -- -D warnings`: clean - `cargo test -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions`: all pass (carddav 7 cache + 2 journal + 10 read + 1 recovery = 20; contacts suites incl. wire_index, hostile, break_*; contacts-actions 130) - Run on head 370d170e. No conflicts and no test breakage from the rebase. - Not re-run here: perf examples on a perf VM (see gaps). ## Review against ADR 0064 (independent; not the author) Matches: bridge never writes files (read-only methods, writes get 405); vCard 3.0 precomputed with strong ETags, reads never run the converter; stable user-id URLs and `/.well-known/carddav` redirect; paged sync with fixed snapshot, continuation tokens and 507 marker (RFC 6578 3.6); deletes reported as 404; expired/foreign tokens give `valid-sync-token`; rebuild is atomic and rotates the epoch; routes unmounted and an authenticator is required; depth infinity refused. Gaps and risks (none block landing; D4 and later should pick them up): 1. No macOS 27 replay. Piece 4 gate asks for replayed Apple traffic; only RFC fixtures exist. D5 recorder covers this. 2. Perf gates are still hand-run examples, not a CI gate on the perf VM (ADR "Speed is a gate", 0005). Numbers in #739 were measured on the older base and not re-measured after this rebase. 3. `revisions` is append-only and never pruned. Each edit adds a full card row; disk grows with edit count and only a rebuild compacts it. Needs a compaction rule (rebuild also rotates the epoch, so devices resync). 4. `changed_ids` maps a fixed list of journal kinds. A new journal kind that touches a contact (for example Trash or tier moves) would be missed until a rebuild. Needs a test that enumerates note-affecting kinds, or a fallback rebuild on unknown kinds. Companion cards (FO1/D2) are not covered yet. 5. PROPFIND depth 1 loads all summaries (O(n), not O(p)) and multiget has no href count cap beyond the 1 MiB body. Fine at 1,000 cards; ADR 0067 reply cap of 1,000 resources is not enforced. 6. Cold rebuild stages the whole book in memory (O(n)); stated in #739, acceptable for 20k-card import but unbounded for photo books. 7. Blocking SQLite work runs under an async mutex in the request path; one slow rebuild stalls that user's DAV reads (not other users). 8. Missing in the read surface by design (later pieces): Lists/group cards, photos, writes, aliases for phone-chosen hrefs (hrefs are `<id>.vcf` only; D6 must add the alias table), `If-Match`/412. 9. Cache tests use fake time-free fixtures only; no kill-9 test of a process dying mid-`rebuild` (backup install is atomic by design, covered by the rollback test, not by a real crash).
Add journal-backed CardDAV cache and read bridge
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
49ccd9a338
Reduce CardDAV cache rebuild and cold read I/O
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
370d170e1b
kayg commented 2026-10-31 18:12:41 +00:00
Author
Owner
Copy link

Independent review, round 1

Reviewed head 370d170e1b2f441c7621c2697c5e27c7d0169303, focusing on the delta over #776 (49ccd9a3), ADRs 0063/0064 and the original #734/#739 performance/migration discussions. Transport admission, auth and caps belong to #794.

Findings

  • P2: rebuild duplicates the entire projected book in unbounded process memory. crates/carddav/src/cache.rs:205 creates the staging database in memory and :216 copies every wire payload/property index into it while the caller's complete Vec<Update> remains alive (crates/carddav/src/source.rs:98). A photo-heavy book accumulated across multiple valid imports can require gigabytes for each copy; a per-request/response cap does not bound the full book, and separate users can rebuild concurrently. This changes the previous disk-backed transaction into a server OOM risk during a cold rebuild. Fix: use a disk-backed temporary SQLite staging database on the state tier, or enforce a measured staging-memory budget with a safe disk-backed fallback before constructing the full in-memory image. Preserve the atomic backup install, old cache on failure and epoch rotation. Gate with a photo-heavy full-book rebuild and concurrent per-user rebuilds under a memory limit, not only small text-card fixtures.
  • P2, inherited from #776: crates/carddav/src/cache.rs:181 still retains a full card/index for every edit indefinitely. The new live revision pointer removes the current-row duplicate, not historical payload growth. See the round 1 finding on #776; it must be addressed in the stack.

Checks and limits

The migration keeps revision sequence, cursor and epoch together in a transaction; its test exercises an in-progress initial sync and a later deletion. The committed head is updated with the live pointer/cursor. The backup replacement updates the in-memory epoch only after StepResult::Done; the existing failed/busy rebuild tests check rollback and retry. I found no additional concrete ETag, cross-user or migration omission in this delta. Crash safety follows the SQLite replacement transaction, but there is no actual kill/reopen test during backup installation.

Static review only. No build/test/install ran on the control host. git diff --check 49ccd9a3 370d170e passed. The historical perf samples and reported VM gates were inspected, not independently rerun on this rebased head.

Verdict: fix first. No P0/P1 finding in this delta; bound rebuild memory and resolve the inherited retention issue.

## Independent review, round 1 Reviewed head `370d170e1b2f441c7621c2697c5e27c7d0169303`, focusing on the delta over #776 (`49ccd9a3`), ADRs 0063/0064 and the original #734/#739 performance/migration discussions. Transport admission, auth and caps belong to #794. ### Findings - **P2: rebuild duplicates the entire projected book in unbounded process memory.** `crates/carddav/src/cache.rs:205` creates the staging database in memory and `:216` copies every wire payload/property index into it while the caller's complete `Vec<Update>` remains alive (`crates/carddav/src/source.rs:98`). A photo-heavy book accumulated across multiple valid imports can require gigabytes for each copy; a per-request/response cap does not bound the full book, and separate users can rebuild concurrently. This changes the previous disk-backed transaction into a server OOM risk during a cold rebuild. **Fix:** use a disk-backed temporary SQLite staging database on the state tier, or enforce a measured staging-memory budget with a safe disk-backed fallback before constructing the full in-memory image. Preserve the atomic backup install, old cache on failure and epoch rotation. Gate with a photo-heavy full-book rebuild and concurrent per-user rebuilds under a memory limit, not only small text-card fixtures. - **P2, inherited from #776:** `crates/carddav/src/cache.rs:181` still retains a full card/index for every edit indefinitely. The new live revision pointer removes the current-row duplicate, not historical payload growth. See the round 1 finding on #776; it must be addressed in the stack. ### Checks and limits The migration keeps revision sequence, cursor and epoch together in a transaction; its test exercises an in-progress initial sync and a later deletion. The committed head is updated with the live pointer/cursor. The backup replacement updates the in-memory epoch only after `StepResult::Done`; the existing failed/busy rebuild tests check rollback and retry. I found no additional concrete ETag, cross-user or migration omission in this delta. Crash safety follows the SQLite replacement transaction, but there is no actual kill/reopen test during backup installation. Static review only. No build/test/install ran on the control host. `git diff --check 49ccd9a3 370d170e` passed. The historical perf samples and reported VM gates were inspected, not independently rerun on this rebased head. **Verdict: fix first.** No P0/P1 finding in this delta; bound rebuild memory and resolve the inherited retention issue.
kayg commented 2026-10-31 18:16:28 +00:00
Author
Owner
Copy link

round 1 implementation plan (no dev merge):

  • #776: default 1,024 wire-changing revisions of delta/snapshot history, configurable through Cache::open_with_retention. advance a durable floor in the same transaction as cursor and pruning. keep one predecessor per href at the floor plus newer revisions, so accepted paged snapshots remain coherent. expire older snapshot high-water marks and delta positions with RFC 6578 valid-sync-token; an empty token starts a full resync. no-op projection does not consume the window. disk pages are reused.
  • #777: disk-backed staging on the same state tier, bounded SQLite page caches, incremental card batches and paged source IDs. install the finished image atomically through SQLite backup, rotate epoch only after success. O(batch bytes + SQLite cache) additional memory, O(book bytes) staging disk; retention O(live cards + window) payloads, prune indexed by href/seq.
failure effect detection recovery gate
expired delta/snapshot no partial or false continuity durable floor 403 valid-sync-token, empty-token full resync churn, paged edits/deletes, reopen and HTTP tests
conversion/staging error old cache/cursor/epoch stay intact source/SQLite error discard staging, retry failure at every batch
process killed mid-build/install old or complete new generation reopen SQLite rollback/retry real killed child at batch boundaries and backup contention
large/concurrent photo books bounded process memory isolated RSS limit staging stays on disk 50k synthetic cards and concurrent photo rebuilds

then push #776 and #777 in order, merge updated #777 into #794, rerun its specified gates and push only if green. #797 remains a downstream consumer; its independent write fixes are outside this task. cheap P2s will be fixed where established; remaining delivery gaps will be listed. review round 1 of maximum 3.

round 1 implementation plan (no dev merge): - #776: default 1,024 wire-changing revisions of delta/snapshot history, configurable through Cache::open_with_retention. advance a durable floor in the same transaction as cursor and pruning. keep one predecessor per href at the floor plus newer revisions, so accepted paged snapshots remain coherent. expire older snapshot high-water marks and delta positions with RFC 6578 valid-sync-token; an empty token starts a full resync. no-op projection does not consume the window. disk pages are reused. - #777: disk-backed staging on the same state tier, bounded SQLite page caches, incremental card batches and paged source IDs. install the finished image atomically through SQLite backup, rotate epoch only after success. O(batch bytes + SQLite cache) additional memory, O(book bytes) staging disk; retention O(live cards + window) payloads, prune indexed by href/seq. | failure | effect | detection | recovery | gate | |---|---|---|---|---| | expired delta/snapshot | no partial or false continuity | durable floor | 403 valid-sync-token, empty-token full resync | churn, paged edits/deletes, reopen and HTTP tests | | conversion/staging error | old cache/cursor/epoch stay intact | source/SQLite error | discard staging, retry | failure at every batch | | process killed mid-build/install | old or complete new generation | reopen SQLite | rollback/retry | real killed child at batch boundaries and backup contention | | large/concurrent photo books | bounded process memory | isolated RSS limit | staging stays on disk | 50k synthetic cards and concurrent photo rebuilds | then push #776 and #777 in order, merge updated #777 into #794, rerun its specified gates and push only if green. #797 remains a downstream consumer; its independent write fixes are outside this task. cheap P2s will be fixed where established; remaining delivery gaps will be listed. review round 1 of maximum 3.
kayg commented 2026-10-31 18:28:51 +00:00
Author
Owner
Copy link

round 1 implementation status at 3c8ddf53 (includes #776 f3d6361d, merged in stack order). pushed, never merged into dev. independent review remains pending.

finding / gate status evidence
unbounded rebuild memory fixed disk staging beside carddav.db; 2 MiB SQLite page cache, no staging mmap, file-backed temp store; production stable-ID keyset pages and 32-card batches
inherited unbounded revisions fixed #776 count window, durable floor, baseline retention and RFC 6578 token expiry
50,000 synthetic cards pass isolated process 11,228 KiB peak RSS; all 50k rows installed and final card readable
concurrent photo-heavy rebuilds pass two per-user books, 1,000 cards each with 224 KiB PHOTO; 45,340 KiB peak RSS; both finish with identical wire bytes
process memory budget pass both isolated cases assert peak RSS <192 MiB; original #777 failed at 754,248 KiB on the 50k case (exit 101)
kill/reopen at every batch pass real killed child after 0/1/2/3 committed 32-card batches; old bytes/cursor/token remain usable, active stage lock refuses a second builder, reopen/retry removes orphan stage and installs exactly 96 cards
failure atomicity / busy replacement / migration pass retained existing duplicate/busy rollback and legacy migration/paged-sync tests
production ID paging pass real notes keyset page test; real journal/contact projection tests remain green
package / lint / formatting pass all carddav, contacts and 130 contacts-actions tests; notes projection scan regression; clippy -D warnings and fmt

VM: cua@calternal-dev-02-std, existing /home/cua/Developer/Worktrees/carddav-r2, Nix shell through ~/bin/slot. logs ~/carddav-rebuild-r1-red.log (expected baseline failure), ~/carddav-rebuild-r1-green2.log (all tests pass; initial clippy rejected the test's marker-file write), and ~/carddav-rebuild-r1-final.log (test-only lint annotation, kill/reopen rerun, clippy and fmt all exit 0). no user files use that test marker helper.

cargo test -j4 -p calternal-carddav --locked --no-fail-fast -- --nocapture
cargo test -j4 -p calternal-notes projection_id_scan --locked
cargo test -j4 -p calternal-contacts -p calternal-contacts-actions --locked --no-fail-fast
cargo test -j4 -p calternal-carddav --test rebuild_crash --locked
cargo clippy -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-notes --all-targets --locked -- -D warnings
cargo fmt --all -- --check

Decisions: rebuild uses O(batch payload + bounded SQLite caches) additional process memory and O(book bytes) staging disk, followed by SQLite's atomic backup install. epoch/cursor change only after installation succeeds. WAL NORMAL policy is unchanged. a fixed staging name plus an OS file lock bounds abandoned staging to one image and prevents another live builder from deleting it; the next rebuild discards crash leftovers. production scans IDs in indexed immutable order and replays the pre-scan journal cursor, so moving/renaming notes cannot move them behind a title/time cursor. legacy custom Source implementations retain a compatibility fallback that lists IDs; production JournalSource overrides it.

Remaining work/limits: native Apple replay and perf-VM latency gates were not run; groups/photos product delivery and future journal-kind exhaustiveness remain outside this review. existing incremental journal replay still buffers its affected-card set, and the per-book readiness lock can delay HTTP reads during rebuild; this change bounds the cold rebuild, not every source plugin/recovery allocation. SQLite reuses freed history pages but does not shrink a previously oversized cache file. kill tests cover all tested staging batch boundaries, not power failure or a deterministic interruption inside SQLite backup. no extra review round was launched. #794 downstream integration/gates are running separately; #797's independent write fixes remain on its own branch.

round 1 implementation status at **3c8ddf53** (includes #776 **f3d6361d**, merged in stack order). pushed, never merged into dev. independent review remains pending. | finding / gate | status | evidence | |---|---|---| | unbounded rebuild memory | fixed | disk staging beside carddav.db; 2 MiB SQLite page cache, no staging mmap, file-backed temp store; production stable-ID keyset pages and 32-card batches | | inherited unbounded revisions | fixed | #776 count window, durable floor, baseline retention and RFC 6578 token expiry | | 50,000 synthetic cards | pass | isolated process **11,228 KiB peak RSS**; all 50k rows installed and final card readable | | concurrent photo-heavy rebuilds | pass | two per-user books, 1,000 cards each with 224 KiB PHOTO; **45,340 KiB peak RSS**; both finish with identical wire bytes | | process memory budget | pass | both isolated cases assert peak RSS <192 MiB; original #777 failed at **754,248 KiB** on the 50k case (exit 101) | | kill/reopen at every batch | pass | real killed child after 0/1/2/3 committed 32-card batches; old bytes/cursor/token remain usable, active stage lock refuses a second builder, reopen/retry removes orphan stage and installs exactly 96 cards | | failure atomicity / busy replacement / migration | pass | retained existing duplicate/busy rollback and legacy migration/paged-sync tests | | production ID paging | pass | real notes keyset page test; real journal/contact projection tests remain green | | package / lint / formatting | pass | all carddav, contacts and 130 contacts-actions tests; notes projection scan regression; clippy -D warnings and fmt | VM: `cua@calternal-dev-02-std`, existing `/home/cua/Developer/Worktrees/carddav-r2`, Nix shell through `~/bin/slot`. logs `~/carddav-rebuild-r1-red.log` (expected baseline failure), `~/carddav-rebuild-r1-green2.log` (all tests pass; initial clippy rejected the test's marker-file write), and `~/carddav-rebuild-r1-final.log` (test-only lint annotation, kill/reopen rerun, clippy and fmt all exit 0). no user files use that test marker helper. ```sh cargo test -j4 -p calternal-carddav --locked --no-fail-fast -- --nocapture cargo test -j4 -p calternal-notes projection_id_scan --locked cargo test -j4 -p calternal-contacts -p calternal-contacts-actions --locked --no-fail-fast cargo test -j4 -p calternal-carddav --test rebuild_crash --locked cargo clippy -j4 -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-notes --all-targets --locked -- -D warnings cargo fmt --all -- --check ``` Decisions: rebuild uses O(batch payload + bounded SQLite caches) additional process memory and O(book bytes) staging disk, followed by SQLite's atomic backup install. epoch/cursor change only after installation succeeds. WAL NORMAL policy is unchanged. a fixed staging name plus an OS file lock bounds abandoned staging to one image and prevents another live builder from deleting it; the next rebuild discards crash leftovers. production scans IDs in indexed immutable order and replays the pre-scan journal cursor, so moving/renaming notes cannot move them behind a title/time cursor. legacy custom Source implementations retain a compatibility fallback that lists IDs; production JournalSource overrides it. Remaining work/limits: native Apple replay and perf-VM latency gates were not run; groups/photos product delivery and future journal-kind exhaustiveness remain outside this review. existing incremental journal replay still buffers its affected-card set, and the per-book readiness lock can delay HTTP reads during rebuild; this change bounds the cold rebuild, not every source plugin/recovery allocation. SQLite reuses freed history pages but does not shrink a previously oversized cache file. kill tests cover all tested staging batch boundaries, not power failure or a deterministic interruption inside SQLite backup. no extra review round was launched. #794 downstream integration/gates are running separately; #797's independent write fixes remain on its own branch.
kayg commented 2026-10-31 18:39:54 +00:00
Author
Owner
Copy link

downstream integration complete: updated #777 was merged into #794 at 80ec1fa3, all requested gates passed before push. Exact commands, status table and source identity. #797 unchanged; never merged into dev. round 1 implementation complete, independent review pending.

downstream integration complete: updated #777 was merged into #794 at **80ec1fa3**, all requested gates passed before push. [Exact commands, status table and source identity](https://git.kayg.org/kayg/calternal/pulls/794#issuecomment-71242). #797 unchanged; never merged into dev. round 1 implementation complete, independent review pending.
kayg commented 2026-10-31 18:52:58 +00:00
Author
Owner
Copy link

Independent review, round 2

Reviewed 3c8ddf534f, round-1 #issuecomment-71159, implementation #issuecomment-71217, and merge into #794 80ec1fa39f (#issuecomment-71242).

round-1 finding status does its test prove it?
P2: unbounded rebuild process memory verified fixed Yes for the production paged rebuild path: 32-ID/card batches, disk staging, 2 MiB page cache, staging mmap disabled, file temp store. Independently reran the isolated 50k-card and two concurrent 1,000-card/224 KiB-photo tests on the merged tree: 10,528 / 32,328 KiB peak RSS, below 192 MiB. Legacy custom Source fallback and caller-owned rebuild(&[Update]) inventories remain outside that bounded production path.
P2: inherited unbounded revisions verified fixed Retention/floor/baseline pruning is preserved. All 13 merged cache regressions pass, including photo churn, floor snapshot, tombstones, reopen and zero window. The tests prove bounded edit history and correct ordinary expiry, not cross-handle rebuild token validity.
crash/reopen evidence verified within its stated scope Independently reran both crash-test functions. Actual child kills after 0/1/2/3 staging batches preserve old bytes/cursor/token, exclude another builder, clean orphan staging on retry and install 96 cards. This does not prove interruption inside backup installation or power-loss durability.

new finding: R2-1, P2: a held reader can silently miss a complete replacement

Locations: sync.rs:19, cache.rs:263. The SQLite replacement is atomic, but epoch validity is cached separately in each Cache object. Only the writer refreshes that field.

Independent failing probe on source-identical #794: seed a.vcf at sequence 1; keep a second Cache open and save its token; writer rebuilds a one-card book containing only b.vcf. Writer rejects the saved token; held reader returns Ok(Page { rows: [], token: <old epoch>:d:1, more: false }). A client following that reader is told nothing changed, despite both a deletion and addition. The crash test reads the old generation while staging, then reopens/uses the installing handle; it misses this post-install case.

Fix in the base stack: obtain epoch/head/floor and rows in one SQLite read snapshot when validating/issuing tokens, including token/ctag, or enforce a single-handle storage invariant. Add a successful-replacement regression with an independently held reader. Inherited from the base cache; the sequence reset here makes the empty-delta manifestation immediate.

P2 rationale: the mounted adapter currently shares a single mutex-protected Book per user. No production single-Book HTTP failure was reproduced. This is a demonstrated public-cache multi-handle correctness gap, rather than a new admission escape or established deployed P1.

rebuild and integration checks

  • During staging, the OS lock protects the single fixed staging image; old committed data remains readable. After crash, cleanup happens on the next rebuild, not merely on Cache open. The committed kill test proves that distinction.
  • Same-Book HTTP readers wait behind readiness/cache locking; they are not served partial staging. The documented readiness delay remains.
  • SQLite backup installs rows/cursor/epoch together, with in-memory epoch changed only after completion. Duplicate-stage and busy-target rollback/retry regressions pass; deterministic kill during backup remains untested.
  • Staging disk-full: independent ENOSPC injection passes: failed stage apply, stage removed on drop, original card/cursor/token unchanged, then small rebuild succeeds. The test uses a child file-size limit and a small preload shim mapping EFBIG to ENOSPC only for staging-file writes; it does not fill a shared VM filesystem. SQLite reports database or disk is full. This exercises staging apply failure, not ENOSPC during destination backup installation.
  • Expiry: independent just-inside-window paged-deletion and i64::MAX fail-closed probes pass. Clock jumps do not affect sequence-based expiry.
  • #794 merge: no admission/limit regression found. Relative to 26edc647, no changes to crates/dav, adapter/properties or transport/network tests; merged sync retains resource and aggregate byte guards. Earlier source-matched transport gate results were inspected, not independently rerun as a new transport review.

verification

Fresh independent runtime checks used dev-01 ~/Developer/Worktrees/dav794-retention-r1; SHA-256 comparison confirms 445/445 Rust/Cargo files match 80ec1fa3. Its Git HEAD remains the pre-merge commit with the source-matched merge changes in the working tree. #777's staging/retention/source implementation is preserved in that merge; its runtime checks here are on the merged tree, not a separate pristine 3c8ddf53 checkout.

In Nix through ~/bin/slot, with -j4:

cargo test -j4 -p calternal-carddav --test reviewer_round2 --test cache --test rebuild_crash --test rebuild_memory --locked --no-fail-fast -- --nocapture
LD_PRELOAD=/home/claude/review-carddav-enospc.so cargo test -j4 -p calternal-carddav --test reviewer_round2 reviewer_disk_full --locked -- --nocapture

Committed tests: 13 cache + 2 crash functions + 1 memory parent pass. New window/deletion and sequence probes pass. Held-reader assertion fails, reproducing R2-1. The first RLIMIT-only disk probe failed its FULL-error discriminator because EFBIG yields SQLITE_IOERR; after stage-only ENOSPC injection, cleanup/retry passes. Thus the broad probe command exits 101; it is not an all-green gate. Logs: ~/review-carddav-r2b.log, ~/review-carddav-r2-enospc-b.log (exit 0). Untracked probe/shim files remain on the VM; tracked sources are unchanged.

Read-only git diff --check passed for the retention update, rebuild delta and #794 merge. Native Apple replay, perf-VM latency and kill during backup were not rerun or claimed.

verdict: fix first, for R2-1 in the base stack. New findings: P0 0, P1 0, P2 1, P3 0 (same shared finding as #776, not two independent defects). No code pushed, merge or production access.

## Independent review, round 2 Reviewed **3c8ddf534f35851276f85cce93e3c29907618654**, round-1 #issuecomment-71159, implementation #issuecomment-71217, and merge into #794 **80ec1fa39fe22af52f8ccddbd6bbe144a59d43eb** (#issuecomment-71242). | round-1 finding | status | does its test prove it? | |---|---|---| | P2: unbounded rebuild process memory | **verified fixed** | Yes for the production paged rebuild path: 32-ID/card batches, disk staging, 2 MiB page cache, staging mmap disabled, file temp store. Independently reran the isolated 50k-card and two concurrent 1,000-card/224 KiB-photo tests on the merged tree: **10,528 / 32,328 KiB** peak RSS, below 192 MiB. Legacy custom Source fallback and caller-owned `rebuild(&[Update])` inventories remain outside that bounded production path. | | P2: inherited unbounded revisions | **verified fixed** | Retention/floor/baseline pruning is preserved. All 13 merged cache regressions pass, including photo churn, floor snapshot, tombstones, reopen and zero window. The tests prove bounded edit history and correct ordinary expiry, not cross-handle rebuild token validity. | | crash/reopen evidence | **verified within its stated scope** | Independently reran both crash-test functions. Actual child kills after 0/1/2/3 staging batches preserve old bytes/cursor/token, exclude another builder, clean orphan staging on retry and install 96 cards. This does not prove interruption inside backup installation or power-loss durability. | ### new finding: R2-1, P2: a held reader can silently miss a complete replacement Locations: [sync.rs:19](https://git.kayg.org/kayg/calternal/src/commit/3c8ddf53/crates/carddav/src/cache/sync.rs#L19), [cache.rs:263](https://git.kayg.org/kayg/calternal/src/commit/3c8ddf53/crates/carddav/src/cache.rs#L263). The SQLite replacement is atomic, but epoch validity is cached separately in each `Cache` object. Only the writer refreshes that field. Independent failing probe on source-identical #794: seed `a.vcf` at sequence 1; keep a second Cache open and save its token; writer rebuilds a one-card book containing only `b.vcf`. Writer rejects the saved token; held reader returns **`Ok(Page { rows: [], token: <old epoch>:d:1, more: false })`**. A client following that reader is told nothing changed, despite both a deletion and addition. The crash test reads the old generation while staging, then reopens/uses the installing handle; it misses this post-install case. Fix in the base stack: obtain epoch/head/floor and rows in one SQLite read snapshot when validating/issuing tokens, including `token`/`ctag`, or enforce a single-handle storage invariant. Add a successful-replacement regression with an independently held reader. Inherited from the base cache; the sequence reset here makes the empty-delta manifestation immediate. **P2 rationale:** the mounted adapter currently shares a single mutex-protected Book per user. No production single-Book HTTP failure was reproduced. This is a demonstrated public-cache multi-handle correctness gap, rather than a new admission escape or established deployed P1. ### rebuild and integration checks - During staging, the OS lock protects the single fixed staging image; old committed data remains readable. After crash, cleanup happens on the **next rebuild**, not merely on Cache open. The committed kill test proves that distinction. - Same-Book HTTP readers wait behind readiness/cache locking; they are not served partial staging. The documented readiness delay remains. - SQLite backup installs rows/cursor/epoch together, with in-memory epoch changed only after completion. Duplicate-stage and busy-target rollback/retry regressions pass; deterministic kill during backup remains untested. - **Staging disk-full:** independent ENOSPC injection passes: failed stage apply, stage removed on drop, original card/cursor/token unchanged, then small rebuild succeeds. The test uses a child file-size limit and a small preload shim mapping EFBIG to ENOSPC **only for staging-file writes**; it does not fill a shared VM filesystem. SQLite reports `database or disk is full`. This exercises staging apply failure, not ENOSPC during destination backup installation. - Expiry: independent just-inside-window paged-deletion and `i64::MAX` fail-closed probes pass. Clock jumps do not affect sequence-based expiry. - #794 merge: **no admission/limit regression found**. Relative to **26edc647**, no changes to `crates/dav`, adapter/properties or transport/network tests; merged sync retains resource and aggregate byte guards. Earlier source-matched transport gate results were inspected, not independently rerun as a new transport review. ### verification Fresh independent runtime checks used dev-01 `~/Developer/Worktrees/dav794-retention-r1`; SHA-256 comparison confirms **445/445 Rust/Cargo files** match 80ec1fa3. Its Git HEAD remains the pre-merge commit with the source-matched merge changes in the working tree. #777's staging/retention/source implementation is preserved in that merge; its runtime checks here are on the merged tree, not a separate pristine 3c8ddf53 checkout. In Nix through `~/bin/slot`, with `-j4`: ```sh cargo test -j4 -p calternal-carddav --test reviewer_round2 --test cache --test rebuild_crash --test rebuild_memory --locked --no-fail-fast -- --nocapture LD_PRELOAD=/home/claude/review-carddav-enospc.so cargo test -j4 -p calternal-carddav --test reviewer_round2 reviewer_disk_full --locked -- --nocapture ``` Committed tests: **13 cache + 2 crash functions + 1 memory parent pass**. New window/deletion and sequence probes pass. Held-reader assertion fails, reproducing R2-1. The first RLIMIT-only disk probe failed its FULL-error discriminator because EFBIG yields SQLITE_IOERR; after stage-only ENOSPC injection, cleanup/retry passes. Thus the broad probe command exits 101; it is **not** an all-green gate. Logs: `~/review-carddav-r2b.log`, `~/review-carddav-r2-enospc-b.log` (exit 0). Untracked probe/shim files remain on the VM; tracked sources are unchanged. Read-only `git diff --check` passed for the retention update, rebuild delta and #794 merge. Native Apple replay, perf-VM latency and kill during backup were not rerun or claimed. **verdict: fix first**, for R2-1 in the base stack. New findings: **P0 0, P1 0, P2 1, P3 0** (same shared finding as #776, not two independent defects). No code pushed, merge or production access.
kayg commented 2026-10-31 18:55:48 +00:00
Author
Owner
Copy link

R2-1 implementation plan (shared #776/#777): remove the per-handle cached epoch. Sync reads epoch/head/floor/rows in one SQLite read transaction, then releases that snapshot and checks the current committed epoch before returning. Token/ctag use the same guard. Old tokens return RFC 6578 invalid-token; a rebuild observed during a read fails closed.

Bounds: constant metadata/epoch work per request, existing bounded sync page and retention costs unchanged; one read transaction, no book-sized copy. No new lock across rebuild staging.

failure effect detection recovery test
held handle after rebuild old token refused durable epoch mismatch empty-token full resync independent reader, delta and initial continuation
rebuild during snapshot discard old reply committed epoch differs after snapshot release retry/full resync deterministic interleaved rebuild
sqlite read error request fails propagated error retry existing rollback/rebuild suites

Gates: reproduce held-reader failure first; cache/read tests on #776, inherited tests plus rebuild suites on #777; re-merge #777 into #794 and run requested dav/carddav/auth/server, mount, contract, clippy -D warnings and fmt before pushing #794. No dev merge or additional review round.

R2-1 implementation plan (shared #776/#777): remove the per-handle cached epoch. Sync reads epoch/head/floor/rows in one SQLite read transaction, then releases that snapshot and checks the current committed epoch before returning. Token/ctag use the same guard. Old tokens return RFC 6578 invalid-token; a rebuild observed during a read fails closed. Bounds: constant metadata/epoch work per request, existing bounded sync page and retention costs unchanged; one read transaction, no book-sized copy. No new lock across rebuild staging. | failure | effect | detection | recovery | test | |---|---|---|---|---| | held handle after rebuild | old token refused | durable epoch mismatch | empty-token full resync | independent reader, delta and initial continuation | | rebuild during snapshot | discard old reply | committed epoch differs after snapshot release | retry/full resync | deterministic interleaved rebuild | | sqlite read error | request fails | propagated error | retry | existing rollback/rebuild suites | Gates: reproduce held-reader failure first; cache/read tests on #776, inherited tests plus rebuild suites on #777; re-merge #777 into #794 and run requested dav/carddav/auth/server, mount, contract, clippy -D warnings and fmt before pushing #794. No dev merge or additional review round.
kayg commented 2026-10-31 19:12:50 +00:00
Author
Owner
Copy link

R2-1 implementation status: ready for FINAL review

Shared finding from #776#issuecomment-71275 / #777#issuecomment-71278 implemented in the base and propagated in stack order. Heads: #776 113af4f819, #777 670a883c93. Re-merged #777 into #794: 21e678b258, pushed only after every requested gate passed. Never merged into dev. No additional independent review round started.

Removed the per-handle cached epoch. Each sync pins epoch/head/floor/rows in one SQLite read transaction. After releasing that snapshot, it queries the current committed epoch again and discards the result with InvalidToken if it changed. token and ctag use the same guard. Every emitted sync token carries the epoch read for that snapshot. Existing HTTP mapping returns RFC 6578 403 valid-sync-token, followed by empty-token full resync. A rebuild after the final epoch check is ordered after that read; the returned old-epoch token is then rejected on the next request.

finding / gate status evidence
held reader after successful rebuild pass Independent writer replaces a/c with b/d at the same revision count; held reader rejects both delta and initial continuation tokens as InvalidToken, issues current token/ctag, and fully resyncs replacement rows. Equal revision counts exercise #777's former silent empty delta.
rebuild during a sync read pass Deterministic WAL interleaving commits another handle's rebuild after epoch pinning but before sync-row reads. Both initial and delta cases observe the old snapshot internally, then discard it as InvalidToken after the current committed epoch check.
HTTP invalidation/full resync pass One-card a-to-b replacement with independently held reader and matching source cursor: 403 valid-sync-token, then 207 with b.vcf and fresh token.
#776 tests / clippy / fmt pass 24 CardDAV tests, 0 failures; all targets, -D warnings; workspace fmt.
#777 tests / clippy / fmt pass 31 top-level CardDAV tests, 0 failures; includes staging crash/reopen, retention, WAL reads and rebuild rollback. 50k-card peak RSS 10,380 KiB; concurrent photo books 32,272 KiB, below existing 192 MiB assertions.
#794 dav/carddav/auth/server tests pass All package tests pass, existing ignores retained; includes socket admission/deadlines/slow reading and the epoch regressions.
#794 dav_is_mounted / contract pass Real scoped-auth/contact projection mount and server contract tests both pass.
#794 clippy / fmt pass Requested packages/all targets with -D warnings; workspace fmt clean.
tested source identity pass SHA-256 agrees for all 422 / 426 / 446 Rust/Cargo files in #776 / #777 / #794 respectively; VM-only Nix files excluded and never committed.

VM: claude@calternal-dev-01-compute02. Worktrees: ~/Developer/Worktrees/carddav-epoch-r2, carddav-epoch-stack-r2, dav794-epoch-r2. All builds/tests/fmt used Nix, with heavy work through one ~/bin/slot, -j4. Reused target: /home/claude/Developer/Worktrees/dav794-retention-r1/target; log records recompilation from each checked worktree. Authoritative log ~/carddav-epoch-final2.log contains BASE_GREEN, STACK_GREEN, TRANSPORT_GREEN; ~/carddav-epoch-final2.exit = 0.

Exact commands in the Nix shell, inside the slot. On each of #776 and #777:

cargo test -j4 -p calternal-carddav --locked
cargo clippy -j4 -p calternal-carddav --all-targets --locked -- -D warnings
cargo fmt --all -- --check

On the refreshed #794:

cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-auth -p calternal-server --locked --no-fail-fast
cargo test -j4 -p calternal --bin calternal dav_is_mounted --locked
cargo test -j4 -p calternal --bin calternal contract --locked
cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-auth -p calternal-server -p calternal --all-targets --locked -- -D warnings
cargo fmt --all -- --check

Failing-before evidence: ~/carddav-epoch-red.log, baseline 407eabf0 (original base plus regression), exits 101 because the held reader accepts the obsolete epoch. Two subsequent test-fixture mistakes were corrected: an accidentally changed existing retention expectation, and the HTTP fake source cursor triggering an unrelated second rebuild. The final source above passes all gates; earlier runs are not claimed green.

Decisions/limits: constant extra epoch queries and a read transaction; existing bounded page/retention/staging behavior preserved. No new book-sized allocation or rebuild lock. Under repeated concurrent rebuilds a request may fail and require full resync instead of returning stale success. Native Apple replay, perf-VM latency budgets and deterministic kill/power loss inside backup installation remain the previously documented limits; none is claimed here. FINAL review remains pending.

## R2-1 implementation status: ready for FINAL review Shared finding from #776#issuecomment-71275 / #777#issuecomment-71278 implemented in the base and propagated in stack order. Heads: #776 **113af4f81970cba5d7c588de856d6b3d6bfd80e9**, #777 **670a883c9359c64cc73ec7ee847adccf0e5c9bfd**. Re-merged #777 into #794: **21e678b258f8fe7bb765897fc227d95e2b11abaf**, pushed only after every requested gate passed. Never merged into dev. No additional independent review round started. Removed the per-handle cached epoch. Each sync pins epoch/head/floor/rows in one SQLite read transaction. After releasing that snapshot, it queries the current committed epoch again and discards the result with `InvalidToken` if it changed. `token` and `ctag` use the same guard. Every emitted sync token carries the epoch read for that snapshot. Existing HTTP mapping returns RFC 6578 **403 `valid-sync-token`**, followed by empty-token full resync. A rebuild after the final epoch check is ordered after that read; the returned old-epoch token is then rejected on the next request. | finding / gate | status | evidence | |---|---|---| | held reader after successful rebuild | pass | Independent writer replaces a/c with b/d at the same revision count; held reader rejects both delta and initial continuation tokens as `InvalidToken`, issues current token/ctag, and fully resyncs replacement rows. Equal revision counts exercise #777's former silent empty delta. | | rebuild during a sync read | pass | Deterministic WAL interleaving commits another handle's rebuild after epoch pinning but before sync-row reads. Both initial and delta cases observe the old snapshot internally, then discard it as `InvalidToken` after the current committed epoch check. | | HTTP invalidation/full resync | pass | One-card a-to-b replacement with independently held reader and matching source cursor: 403 `valid-sync-token`, then 207 with b.vcf and fresh token. | | #776 tests / clippy / fmt | pass | 24 CardDAV tests, 0 failures; all targets, `-D warnings`; workspace fmt. | | #777 tests / clippy / fmt | pass | 31 top-level CardDAV tests, 0 failures; includes staging crash/reopen, retention, WAL reads and rebuild rollback. 50k-card peak RSS 10,380 KiB; concurrent photo books 32,272 KiB, below existing 192 MiB assertions. | | #794 dav/carddav/auth/server tests | pass | All package tests pass, existing ignores retained; includes socket admission/deadlines/slow reading and the epoch regressions. | | #794 `dav_is_mounted` / `contract` | pass | Real scoped-auth/contact projection mount and server contract tests both pass. | | #794 clippy / fmt | pass | Requested packages/all targets with `-D warnings`; workspace fmt clean. | | tested source identity | pass | SHA-256 agrees for all 422 / 426 / 446 Rust/Cargo files in #776 / #777 / #794 respectively; VM-only Nix files excluded and never committed. | VM: **claude@calternal-dev-01-compute02**. Worktrees: `~/Developer/Worktrees/carddav-epoch-r2`, `carddav-epoch-stack-r2`, `dav794-epoch-r2`. All builds/tests/fmt used Nix, with heavy work through one `~/bin/slot`, `-j4`. Reused target: `/home/claude/Developer/Worktrees/dav794-retention-r1/target`; log records recompilation from each checked worktree. Authoritative log **`~/carddav-epoch-final2.log`** contains `BASE_GREEN`, `STACK_GREEN`, `TRANSPORT_GREEN`; **`~/carddav-epoch-final2.exit` = 0**. Exact commands in the Nix shell, inside the slot. On each of #776 and #777: ```sh cargo test -j4 -p calternal-carddav --locked cargo clippy -j4 -p calternal-carddav --all-targets --locked -- -D warnings cargo fmt --all -- --check ``` On the refreshed #794: ```sh cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-auth -p calternal-server --locked --no-fail-fast cargo test -j4 -p calternal --bin calternal dav_is_mounted --locked cargo test -j4 -p calternal --bin calternal contract --locked cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-auth -p calternal-server -p calternal --all-targets --locked -- -D warnings cargo fmt --all -- --check ``` Failing-before evidence: `~/carddav-epoch-red.log`, baseline **407eabf0** (original base plus regression), exits 101 because the held reader accepts the obsolete epoch. Two subsequent test-fixture mistakes were corrected: an accidentally changed existing retention expectation, and the HTTP fake source cursor triggering an unrelated second rebuild. The final source above passes all gates; earlier runs are not claimed green. Decisions/limits: constant extra epoch queries and a read transaction; existing bounded page/retention/staging behavior preserved. No new book-sized allocation or rebuild lock. Under repeated concurrent rebuilds a request may fail and require full resync instead of returning stale success. Native Apple replay, perf-VM latency budgets and deterministic kill/power loss inside backup installation remain the previously documented limits; none is claimed here. FINAL review remains pending.
kayg commented 2026-10-31 19:21:03 +00:00
Author
Owner
Copy link

Independent review, round 3 (final)

Reviewed #777 at 670a883c93, the shared round-2 finding (#776#issuecomment-71275 / #777#issuecomment-71278), implementation status (#776#issuecomment-71333 / #777#issuecomment-71338), and propagation into #794 21e678b2.

verdict: ready. New findings: P0 0, P1 0, P2 0, P3 0. The shared R2-1 finding is closed. No new P0/P1 appeared, so kayg's final-round “stop and simplify” trigger is not met.

check status evidence
R2-1: held reader after rebuild verified fixed Epoch is read from SQLite rather than cached per handle. Held delta and initial-continuation tokens are rejected after a same-sized replacement; current token/ctag agree with the writer.
Old token to full resync verified fixed HTTP regression requires 403 valid-sync-token, followed by an empty-token 207 containing replacement b.vcf. No successful empty delta under the obsolete token.
Rebuild during read verified fixed Deterministic WAL interleaving covers initial and delta reads. Old rows are visible inside the pinned snapshot, then discarded as InvalidToken after the transaction ends and the current committed epoch differs.
New transaction boundary no new defect found Epoch/head/floor/rows share one read transaction, including the empty-delta fast path. Error exits drop/roll back the transaction. Token and ctag use the same guard. A rebuild after the final check is ordered after that read; the next request rejects its old-epoch token.
#794 integration no merge regression found 80ec1fa3..21e678b2 changes only cache implementation and regression tests. DAV transport, adapter/properties, auth, admission, response reservations/byte guards, deadlines and socket tests are unchanged.

New findings

None at P0–P3 within this review's scope.

Verification and limits

Fresh independent checks on dev-01, in Nix through ~/bin/slot, reused target /home/claude/Developer/Worktrees/dav794-retention-r1/target:

cargo test -j4 -p calternal-carddav --lib --test cache --test read --locked -- --nocapture

#776: 21 passed, #777: 25 passed, no failures. Both runs include the held-reader cache/HTTP regressions and the deterministic mid-read rebuild test. Logs: ~/review-carddav-r3-final/gates.log (through TREE_GREEN=carddav-epoch-r2) and ~/review-carddav-r3-final/remainder.log (through TREE_GREEN=carddav-epoch-stack-r2). After those passes, #794's attempted rerun remained queued for a slot and was cancelled before Cargo started; no fresh merged-tree or transport runtime pass is claimed.

Independent SHA-256 comparison matches 422/422, 426/426, 446/446 Rust/Cargo files against the specified #776/#777/#794 heads. The #794 VM worktree has an older Git HEAD but its compared source matches 21e678b2. Inspected prior source-matched ~/carddav-epoch-final2.log and exit file (0): all three epoch regressions pass on each tree, with BASE_GREEN, STACK_GREEN, TRANSPORT_GREEN; clippy/fmt and broader transport/auth/socket gates are prior implementation evidence, not fresh runs in this review. Read-only git diff --check passes for f3d6361d..113af4f8, 3c8ddf53..670a883c and 80ec1fa3..21e678b2.

Native Apple replay, perf-VM latency gates and interruption inside backup installation remain the documented limits. No new allocation proportional to book size, write lock or transport-policy change was introduced by this fix. No code changed/pushed, merge or production access.

## Independent review, round 3 (final) Reviewed #777 at **670a883c9359c64cc73ec7ee847adccf0e5c9bfd**, the shared round-2 finding (#776#issuecomment-71275 / #777#issuecomment-71278), implementation status (#776#issuecomment-71333 / #777#issuecomment-71338), and propagation into #794 **21e678b2**. **verdict: ready. New findings: P0 0, P1 0, P2 0, P3 0.** The shared R2-1 finding is closed. No new P0/P1 appeared, so kayg's final-round “stop and simplify” trigger is not met. | check | status | evidence | |---|---|---| | R2-1: held reader after rebuild | verified fixed | Epoch is read from SQLite rather than cached per handle. Held delta and initial-continuation tokens are rejected after a same-sized replacement; current token/ctag agree with the writer. | | Old token to full resync | verified fixed | HTTP regression requires 403 `valid-sync-token`, followed by an empty-token 207 containing replacement b.vcf. No successful empty delta under the obsolete token. | | Rebuild during read | verified fixed | Deterministic WAL interleaving covers initial and delta reads. Old rows are visible inside the pinned snapshot, then discarded as `InvalidToken` after the transaction ends and the current committed epoch differs. | | New transaction boundary | no new defect found | Epoch/head/floor/rows share one read transaction, including the empty-delta fast path. Error exits drop/roll back the transaction. Token and ctag use the same guard. A rebuild after the final check is ordered after that read; the next request rejects its old-epoch token. | | #794 integration | no merge regression found | 80ec1fa3..21e678b2 changes only cache implementation and regression tests. DAV transport, adapter/properties, auth, admission, response reservations/byte guards, deadlines and socket tests are unchanged. | ### New findings None at P0–P3 within this review's scope. ### Verification and limits Fresh independent checks on dev-01, in Nix through `~/bin/slot`, reused target `/home/claude/Developer/Worktrees/dav794-retention-r1/target`: ```sh cargo test -j4 -p calternal-carddav --lib --test cache --test read --locked -- --nocapture ``` #776: **21 passed**, #777: **25 passed**, no failures. Both runs include the held-reader cache/HTTP regressions and the deterministic mid-read rebuild test. Logs: `~/review-carddav-r3-final/gates.log` (through `TREE_GREEN=carddav-epoch-r2`) and `~/review-carddav-r3-final/remainder.log` (through `TREE_GREEN=carddav-epoch-stack-r2`). After those passes, #794's attempted rerun remained queued for a slot and was cancelled before Cargo started; no fresh merged-tree or transport runtime pass is claimed. Independent SHA-256 comparison matches **422/422**, **426/426**, **446/446** Rust/Cargo files against the specified #776/#777/#794 heads. The #794 VM worktree has an older Git HEAD but its compared source matches 21e678b2. Inspected prior source-matched `~/carddav-epoch-final2.log` and exit file (0): all three epoch regressions pass on each tree, with `BASE_GREEN`, `STACK_GREEN`, `TRANSPORT_GREEN`; clippy/fmt and broader transport/auth/socket gates are prior implementation evidence, not fresh runs in this review. Read-only `git diff --check` passes for f3d6361d..113af4f8, 3c8ddf53..670a883c and 80ec1fa3..21e678b2. Native Apple replay, perf-VM latency gates and interruption inside backup installation remain the documented limits. No new allocation proportional to book size, write lock or transport-policy change was introduced by this fix. No code changed/pushed, merge or production access.
kayg changed target branch from dev to issue/carddav-read-r2 2026-11-07 20:25:00 +00:00
kayg commented 2026-11-07 20:26:02 +00:00
Author
Owner
Copy link

housekeeping #1009: rebased+failing, left open for review.

restacked CardDAV read fixes on #776; ids_page uses current db.reader, and dev restart tests remain.

head: 89e9ed150f204128b8f5db02d5cb427371906de9; based on dev cd7f84432febfc63b788fbb9b9f9950452982d74, pushed with the original exact force-with-lease.

frontend frozen install, check, lint, build and unit tests passed. bundle gate passed. first-paint brotli vs dev: JS 230777 B (+0 B), CSS 18386 B (+0 B).

Rust gate results (Mac VM):

exit command
0 cargo fmt -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes -- --check
0 cargo check -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes --all-targets
0 cargo clippy -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes --all-targets -- -D warnings
101 cargo test -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes

the Notes suite was stopped after six minutes with two executor tests still pending; other CardDAV/contact suites completed. this is an incomplete run, not an assertion failure or a performance measurement.

all executed gates used the VM Nix environment, shared Mac lock and slot. exact commands, source heads, logs and dev baseline. frontend inputs are identical to the tested frontend tree when a Rust-only repair changed the head.

original-head cherry/patch inventory, pr-777.md; unmatched entries are missing-patch candidates, with semantic overlap/replacements noted above.

housekeeping #1009: rebased+failing, left open for review. restacked CardDAV read fixes on #776; ids_page uses current db.reader, and dev restart tests remain. head: `89e9ed150f204128b8f5db02d5cb427371906de9`; based on dev `cd7f84432febfc63b788fbb9b9f9950452982d74`, pushed with the original exact force-with-lease. frontend frozen install, check, lint, build and unit tests passed. bundle gate passed. first-paint brotli vs dev: JS 230777 B (+0 B), CSS 18386 B (+0 B). Rust gate results (Mac VM): | exit | command | |---|---| | 0 | `cargo fmt -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes -- --check` | | 0 | `cargo check -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes --all-targets` | | 0 | `cargo clippy -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes --all-targets -- -D warnings` | | 101 | `cargo test -j4 --locked -p calternal-carddav -p calternal-contacts-actions -p calternal-contacts -p calternal-notes` | the Notes suite was stopped after six minutes with two executor tests still pending; other CardDAV/contact suites completed. this is an incomplete run, not an assertion failure or a performance measurement. all executed gates used the VM Nix environment, shared Mac lock and slot. [exact commands, source heads, logs and dev baseline](https://git.kayg.org/attachments/bdb8d284-b80d-4185-b0be-6fa74f48a191). frontend inputs are identical to the tested frontend tree when a Rust-only repair changed the head. [original-head cherry/patch inventory](https://git.kayg.org/attachments/e05a38dc-c028-4d77-8655-efa9d5c8ea50), `pr-777.md`; unmatched entries are missing-patch candidates, with semantic overlap/replacements noted above.
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue/carddav-read-perf-fix-r2:issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-perf-fix-r2

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch issue/carddav-read-r2
git merge --no-ff issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-perf-fix-r2
git rebase issue/carddav-read-r2
git switch issue/carddav-read-r2
git merge --ff-only issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-perf-fix-r2
git rebase issue/carddav-read-r2
git switch issue/carddav-read-r2
git merge --no-ff issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-r2
git merge --squash issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-r2
git merge --ff-only issue/carddav-read-perf-fix-r2
git switch issue/carddav-read-r2
git merge issue/carddav-read-perf-fix-r2
git push origin issue/carddav-read-r2
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!777
No description provided.