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

contacts D6: CardDAV PUT writes, durable href aliases and ETag preconditions #797

Closed
kayg wants to merge 12 commits from feat/carddav-writes into dev
pull from: feat/carddav-writes
merge into: kayg:dev
Conversation 10 Commits 12 Files changed 43 +5801 -22
kayg commented 2026-10-31 17:36:11 +00:00
Owner
Copy link

D6 CardDAV PUT writes

PUT creates and updates through the executor and contacts actions. canonical URLs and client aliases share a durable per-user href namespace. deleted contacts retain route tombstones: headerless replays are refused, If-Match on an absent resource returns 412, and explicit If-None-Match:* creation uses a fresh contact id. restoring the previous contact preserves a separate route if its former URL was reused.

ETag validation uses the coherent note/companion snapshot used for merging. the final room mutation compares that note version and returns 412 on any intervening ordinary note edit. conditional rename jobs check the version before changing paths or links. grouped labels belonging to retained unmodeled fields survive Apple omissions; modeled label deletions still work.

stack: #794 at 26edc6476b4b25e1a337190021f2f8dde77ca61a; head 8bcb2a1e08b9fe0496812e5044af2c2d0d66325e; target dev; never merge. review round 1 fixes are implemented and ready for independent re-review (maximum 3 rounds).

costs and failure handling

PUT card cap 256 KiB, transport cap 1 MiB, href cap 255 bytes. href operations are indexed O(log routes). migration visits historical ids; startup backfills handwritten contacts with an O(notes) scan. each live cached contact adds a small derived owner/route row. existing UID inventory scans and converter occurrence matching retain the previously documented scaling limits; no latency claim is added.

failure effect detection recovery regression
retired canonical/alias replay no implicit resurrection durable route owner plus Trash/departure restore explicitly, or create-only fresh id local/import/DAV lifecycle
canonical/alias collision no second contact committed single unique href namespace choose another URL action + migration + rebuild
note edit during conditional PUT no PUT edit, rename or link mutation room version compare-and-swap 412, GET and retry two deterministic pause points, rename and same path
crash after reservation reserved target remains invisible until contact exists durable pending id retry resumes that id reservation/reopen retry; no new process-kill claim
old events after URL reuse new owner remains projected cached note ownership replay journal or rebuild delayed delete/move regression
Apple omits grouped unknown field and label original metadata retained lossless converter binding modeled fields still permit deletion grouped omission + modeled label deletion

verification

all requested package/history tests, action contract, clippy -D warnings and fmt passed on dev-02 through slot/Nix; builds use -j4, package tests use --test-threads=1. the final control and VM sources match across 423 Rust/Cargo/contract files. complete commands, red/green evidence, test status table, timing-test observations and inherited delivery risks are recorded in the round 1 implementation comment.

native Apple capture/replay, DELETE, Lists/groups, perf-VM gates and protected recovery snapshots remain open inherited work. RFC 9110 strong/weak conditions and transformed-PUT ETag omission remain unchanged; headerless native requests remain an implementation assumption, not captured Apple evidence. scope does not include any PR merge.

# D6 CardDAV PUT writes PUT creates and updates through the executor and contacts actions. canonical URLs and client aliases share a durable per-user href namespace. deleted contacts retain route tombstones: headerless replays are refused, If-Match on an absent resource returns 412, and explicit If-None-Match:* creation uses a fresh contact id. restoring the previous contact preserves a separate route if its former URL was reused. ETag validation uses the coherent note/companion snapshot used for merging. the final room mutation compares that note version and returns 412 on any intervening ordinary note edit. conditional rename jobs check the version before changing paths or links. grouped labels belonging to retained unmodeled fields survive Apple omissions; modeled label deletions still work. stack: #794 at `26edc6476b4b25e1a337190021f2f8dde77ca61a`; head `8bcb2a1e08b9fe0496812e5044af2c2d0d66325e`; target dev; never merge. review round 1 fixes are implemented and ready for independent re-review (maximum 3 rounds). ## costs and failure handling PUT card cap 256 KiB, transport cap 1 MiB, href cap 255 bytes. href operations are indexed O(log routes). migration visits historical ids; startup backfills handwritten contacts with an O(notes) scan. each live cached contact adds a small derived owner/route row. existing UID inventory scans and converter occurrence matching retain the previously documented scaling limits; no latency claim is added. | failure | effect | detection | recovery | regression | |---|---|---|---|---| | retired canonical/alias replay | no implicit resurrection | durable route owner plus Trash/departure | restore explicitly, or create-only fresh id | local/import/DAV lifecycle | | canonical/alias collision | no second contact committed | single unique href namespace | choose another URL | action + migration + rebuild | | note edit during conditional PUT | no PUT edit, rename or link mutation | room version compare-and-swap | 412, GET and retry | two deterministic pause points, rename and same path | | crash after reservation | reserved target remains invisible until contact exists | durable pending id | retry resumes that id | reservation/reopen retry; no new process-kill claim | | old events after URL reuse | new owner remains projected | cached note ownership | replay journal or rebuild | delayed delete/move regression | | Apple omits grouped unknown field and label | original metadata retained | lossless converter binding | modeled fields still permit deletion | grouped omission + modeled label deletion | ## verification all requested package/history tests, action contract, clippy `-D warnings` and fmt passed on dev-02 through slot/Nix; builds use `-j4`, package tests use `--test-threads=1`. the final control and VM sources match across 423 Rust/Cargo/contract files. complete commands, red/green evidence, test status table, timing-test observations and inherited delivery risks are recorded in the round 1 implementation comment. native Apple capture/replay, DELETE, Lists/groups, perf-VM gates and protected recovery snapshots remain open inherited work. RFC 9110 strong/weak conditions and transformed-PUT ETag omission remain unchanged; headerless native requests remain an implementation assumption, not captured Apple evidence. scope does not include any PR merge.
kayg added 24 commits 2026-10-31 17:36:11 +00:00
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
web: regenerate action types
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
70ccd57cb9 
Fail-closed DAV URL user check, credential-first throttling, step-up for
minting, bounded cache with 503 on database errors, and small hardening.
# Conflicts:
#	crates/auth/src/app_password.rs
Finish scoped DAV transport gates and bounded CardDAV reads
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
26936341e0
docs: plan D6 CardDAV writes and recovery
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
50806ed27c
kayg changed title from WIP: contacts D6 CardDAV PUT writes and href aliases to contacts D6: CardDAV PUT writes, durable href aliases and ETag preconditions 2026-10-31 17:56:40 +00:00
kayg commented 2026-10-31 18:03:51 +00:00
Author
Owner
Copy link

Independent review, round 1

verdict: fix first. reviewed head fd829d36f11815178ca7104d598b1feca301f0f6, D6 diff against #794 at 9e95ad41. read the plan/failure table, ADRs 0063/0064/0068 and contacts decision memory. no P0 found.

findings

P1: retired canonical hrefs can resurrect locally created/imported contacts. crates/contacts-actions/src/write.rs:242–266, crates/contacts-actions/src/dav.rs:34–45. Create a contact through the ordinary contacts action, delete it, then replay a headerless PUT to its former <id>.vcf. There is no alias row for this route; create computes a different, href-derived id and checks Trash/departure against that new id. The deleted original therefore does not block creation. My regression expected refusal and got 201 Created with a new live contact id. Restoring the original can then produce two contacts claiming the same URL. Fix: durably reserve canonical routes as well as client aliases, including migration/tombstones for existing local/imported contacts; resolve retirement against the original route owner. Extend the existing Trash/restore/forget test to local/imported contacts, headerless and create-only PUTs.

P1: alias uniqueness excludes canonical resource names. crates/contacts-actions/src/write.rs:288–289, crates/contacts-actions/src/dav.rs:25–31, crates/notes/src/history/aliases.rs:9–35. With a local contact A, contacts.create {card: <different UID>, href: "<A-id>.vcf"} creates B successfully because A's implicit canonical route is absent from the alias table. Both now project to the same href. Cache upserts can replace A's wire representation with B; a later event for either contact can replace/delete the other's projection. This is reachable through the newly exposed action input, even though the normal HTTP PUT path checks the current cache first. My regression confirmed that the collision is accepted and both ids remain live. Fix: enforce one durable namespace for canonical and explicit routes under the contact writer lock, across every creation path; refuse collisions before committing a contact. Test action callers, cache rebuild and subsequent updates/deletes of both ids. This is within one user's Home; I found no cross-user route lookup.

P1: the ETag recheck is detached from the snapshot being committed. crates/contacts-actions/src/write.rs:351–375. The expected ETag is checked by one read(), then the action awaits UID/history work and obtains a second note snapshot and companion for its merge. Ordinary note edits use the note room (crates/notes/src/lib.rs:542–564), not person.writing. A note edit after the ETag read but before the second snapshot can therefore change a modeled field; the PUT accepts the newer snapshot as its patch base and can apply the phone-wins merge despite its now-stale If-Match. The existing test edits before entering the action and cannot exercise this window. Fix: evaluate the ETag from the exact coherent note/companion snapshot used for the merge, and bind validation to the final mutation's version check; return 412 if it changes. Add a deterministic interleaving test. This finding is from code tracing, not the three executed regressions.

P2: omitted labels on unmodeled grouped fields are lost. crates/contacts/src/merge.rs:271–275. Start with item1.X-UNMODELED:opaque and item1.X-ABLabel:Private field; let Apple omit both while changing TEL. The new preservation guard retains the unknown value but explicitly excludes X-ABLabel, so the saved/exported original loses its label. My converter regression failed with exactly that output. Fix: preserve the label and associated metadata of retained unmodeled occurrences; permit intentional label deletion for modeled fields. Extend the omission regression to grouped unknown properties and labels.

failure-table audit

row what the current tests establish / gap
two devices, same ETag two PUTs through one Book give 204/412; non-DAV in-flight snapshot race above is missing
stale/weak/create-only replay sequential header matrix is covered; no simultaneous If-None-Match:* creation test or action-level create condition recheck
reservation before contact manually seeded reservation + orderly reopen/retry proves durable-gap state recovery, not process-kill recovery
contact committed before reply/cache successful replay/restart and fresh cache rebuild are covered; no interruption at this boundary
duplicate unconditional PUT single-id replay covered
retired href DAV-created contact covered; local/imported canonical route fails as above
malformed/UID conflict sequential invalid/changed/duplicate UID cases covered; concurrent UID creation is not exercised
Apple omissions ungrouped unknown/TZ/SOURCE/LOGO covered; grouped metadata regression fails
cross-site/foreign user spoofed URL, hostile Origin/Fetch-Site, headerless and same-origin requests covered; not a two-real-user alias-isolation test
limits/unavailable projection both size caps covered; inherited projection-error tests exist, but no D6 PUT failure/retry test around a committed write

security, protocol and Apple evidence

Credential-derived user selection and per-Home alias storage look correctly scoped. Cross-site checks run before buffering; absent browser headers remain accepted. Strong If-Match / weak If-None-Match comparison and omission of ETag after transformed PUT are appropriate (RFC 9110 §9.3.4, RFC 6352 §6.3.2).

Contacts.app Origin/Sec-Fetch behavior is not verified by this PR or this review. Fetch Metadata specifies browser Fetch context; it is not evidence that every Contacts.app/CFNetwork version omits these headers. The cited macOS duplicate report also does not establish that header claim. Headerless native compatibility is a reasonable implementation assumption, not a measured Apple result. Retain the CSRF guard, but supply a redacted real macOS Contacts PUT capture/replay with the complete request headers before claiming native compatibility. ADR 0064 explicitly makes recorded macOS writes a D6 gate; that gate remains open. DELETE is also absent and remains an explicit D6-scope gap against that ADR.

review verification

On cua@calternal-dev-02-std, existing head fd829d36, Nix shell through ~/bin/slot:

cargo test -j4 -p calternal-carddav --test review_round1 review_r1_ --locked -- --nocapture

Temporary regression harness reused the existing write-test setup. 0 passed, 3 failed: review_r1_local_retired_href, review_r1_alias_canonical_collision, review_r1_unknown_group_label. These are deliberate assertions of the required safe behavior. Harness removed afterward; no production source edited, no code pushed. VM's existing Nix overlay files were left alone. Earlier broad gates are author-reported; I did not rerun them.

## Independent review, round 1 **verdict: fix first.** reviewed head `fd829d36f11815178ca7104d598b1feca301f0f6`, D6 diff against #794 at `9e95ad41`. read the plan/failure table, ADRs 0063/0064/0068 and contacts decision memory. no P0 found. ### findings **P1: retired canonical hrefs can resurrect locally created/imported contacts.** `crates/contacts-actions/src/write.rs:242–266`, `crates/contacts-actions/src/dav.rs:34–45`. Create a contact through the ordinary contacts action, delete it, then replay a headerless PUT to its former `<id>.vcf`. There is no alias row for this route; create computes a different, href-derived id and checks Trash/departure against that new id. The deleted original therefore does not block creation. My regression expected refusal and got **201 Created with a new live contact id**. Restoring the original can then produce two contacts claiming the same URL. Fix: durably reserve canonical routes as well as client aliases, including migration/tombstones for existing local/imported contacts; resolve retirement against the original route owner. Extend the existing Trash/restore/forget test to local/imported contacts, headerless and create-only PUTs. **P1: alias uniqueness excludes canonical resource names.** `crates/contacts-actions/src/write.rs:288–289`, `crates/contacts-actions/src/dav.rs:25–31`, `crates/notes/src/history/aliases.rs:9–35`. With a local contact A, `contacts.create {card: <different UID>, href: "<A-id>.vcf"}` creates B successfully because A's implicit canonical route is absent from the alias table. Both now project to the same href. Cache upserts can replace A's wire representation with B; a later event for either contact can replace/delete the other's projection. This is reachable through the newly exposed action input, even though the normal HTTP PUT path checks the current cache first. My regression confirmed that the collision is accepted and both ids remain live. Fix: enforce one durable namespace for canonical and explicit routes under the contact writer lock, across every creation path; refuse collisions before committing a contact. Test action callers, cache rebuild and subsequent updates/deletes of both ids. This is within one user's Home; I found no cross-user route lookup. **P1: the ETag recheck is detached from the snapshot being committed.** `crates/contacts-actions/src/write.rs:351–375`. The expected ETag is checked by one `read()`, then the action awaits UID/history work and obtains a second note snapshot and companion for its merge. Ordinary note edits use the note room (`crates/notes/src/lib.rs:542–564`), not `person.writing`. A note edit after the ETag read but before the second snapshot can therefore change a modeled field; the PUT accepts the newer snapshot as its patch base and can apply the phone-wins merge despite its now-stale If-Match. The existing test edits before entering the action and cannot exercise this window. Fix: evaluate the ETag from the exact coherent note/companion snapshot used for the merge, and bind validation to the final mutation's version check; return 412 if it changes. Add a deterministic interleaving test. This finding is from code tracing, not the three executed regressions. **P2: omitted labels on unmodeled grouped fields are lost.** `crates/contacts/src/merge.rs:271–275`. Start with `item1.X-UNMODELED:opaque` and `item1.X-ABLabel:Private field`; let Apple omit both while changing TEL. The new preservation guard retains the unknown value but explicitly excludes X-ABLabel, so the saved/exported original loses its label. My converter regression failed with exactly that output. Fix: preserve the label and associated metadata of retained unmodeled occurrences; permit intentional label deletion for modeled fields. Extend the omission regression to grouped unknown properties and labels. ### failure-table audit | row | what the current tests establish / gap | |---|---| | two devices, same ETag | two PUTs through one Book give 204/412; non-DAV in-flight snapshot race above is missing | | stale/weak/create-only replay | sequential header matrix is covered; no simultaneous If-None-Match:* creation test or action-level create condition recheck | | reservation before contact | manually seeded reservation + orderly reopen/retry proves durable-gap state recovery, not process-kill recovery | | contact committed before reply/cache | successful replay/restart and fresh cache rebuild are covered; no interruption at this boundary | | duplicate unconditional PUT | single-id replay covered | | retired href | DAV-created contact covered; local/imported canonical route fails as above | | malformed/UID conflict | sequential invalid/changed/duplicate UID cases covered; concurrent UID creation is not exercised | | Apple omissions | ungrouped unknown/TZ/SOURCE/LOGO covered; grouped metadata regression fails | | cross-site/foreign user | spoofed URL, hostile Origin/Fetch-Site, headerless and same-origin requests covered; not a two-real-user alias-isolation test | | limits/unavailable projection | both size caps covered; inherited projection-error tests exist, but no D6 PUT failure/retry test around a committed write | ### security, protocol and Apple evidence Credential-derived user selection and per-Home alias storage look correctly scoped. Cross-site checks run before buffering; absent browser headers remain accepted. Strong If-Match / weak If-None-Match comparison and omission of ETag after transformed PUT are appropriate ([RFC 9110 §9.3.4](https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.4), [RFC 6352 §6.3.2](https://www.rfc-editor.org/rfc/rfc6352.html#section-6.3.2)). **Contacts.app Origin/Sec-Fetch behavior is not verified by this PR or this review.** [Fetch Metadata](https://www.w3.org/TR/fetch-metadata/) specifies browser Fetch context; it is not evidence that every Contacts.app/CFNetwork version omits these headers. The cited [macOS duplicate report](https://github.com/nextcloud/server/issues/21944) also does not establish that header claim. Headerless native compatibility is a reasonable implementation assumption, not a measured Apple result. Retain the CSRF guard, but supply a redacted real macOS Contacts PUT capture/replay with the complete request headers before claiming native compatibility. ADR 0064 explicitly makes recorded macOS writes a D6 gate; that gate remains open. DELETE is also absent and remains an explicit D6-scope gap against that ADR. ### review verification On `cua@calternal-dev-02-std`, existing head `fd829d36`, Nix shell through `~/bin/slot`: ```sh cargo test -j4 -p calternal-carddav --test review_round1 review_r1_ --locked -- --nocapture ``` Temporary regression harness reused the existing write-test setup. **0 passed, 3 failed**: `review_r1_local_retired_href`, `review_r1_alias_canonical_collision`, `review_r1_unknown_group_label`. These are deliberate assertions of the required safe behavior. Harness removed afterward; no production source edited, no code pushed. VM's existing Nix overlay files were left alone. Earlier broad gates are author-reported; I did not rerun them.
kayg commented 2026-10-31 18:08:48 +00:00
Author
Owner
Copy link

round 1 implementation plan: re-merge #794, then replace implicit canonical URLs with reservations in the same durable per-user href namespace as aliases. backfill historical canonical ids (including departed ids); keep reservations after deletion. explicit create-only PUTs may create a fresh id at a retired route; headerless replays stay refused and If-Match stays 412. reserve routes before every contact create/import.

bind ETag validation to one note/companion snapshot and use the room version as a final compare-and-swap, including the first edit of rename jobs. a changed version fails before any rename/link mutation. retain grouped labels for omitted unmodeled fields.

cost: indexed href operations O(log routes), one-time historical backfill O(history ids); the existing UID scan and converter bounds remain. failure → effect → detection → recovery → test: retired replay → no write → durable route owner → explicit create-only/new id or restore → local/import/DAV deletion tests; namespace collision → no contact → unique(owner,href) → choose another URL → action/rebuild tests; note edit during PUT → no PUT mutation → room CAS → 412/re-GET → deterministic pause test. gates: package tests, notes regressions, contract, clippy -D warnings and fmt on dev-02 through slot/Nix. never merge.

round 1 implementation plan: re-merge #794, then replace implicit canonical URLs with reservations in the same durable per-user href namespace as aliases. backfill historical canonical ids (including departed ids); keep reservations after deletion. explicit create-only PUTs may create a fresh id at a retired route; headerless replays stay refused and If-Match stays 412. reserve routes before every contact create/import. bind ETag validation to one note/companion snapshot and use the room version as a final compare-and-swap, including the first edit of rename jobs. a changed version fails before any rename/link mutation. retain grouped labels for omitted unmodeled fields. cost: indexed href operations O(log routes), one-time historical backfill O(history ids); the existing UID scan and converter bounds remain. failure → effect → detection → recovery → test: retired replay → no write → durable route owner → explicit create-only/new id or restore → local/import/DAV deletion tests; namespace collision → no contact → unique(owner,href) → choose another URL → action/rebuild tests; note edit during PUT → no PUT mutation → room CAS → 412/re-GET → deterministic pause test. gates: package tests, notes regressions, contract, clippy -D warnings and fmt on dev-02 through slot/Nix. never merge.
kayg commented 2026-10-31 18:35:19 +00:00
Author
Owner
Copy link

round 1 fixes, ready for re-review

head 8bcb2a1e08b9fe0496812e5044af2c2d0d66325e; re-merged #794 at 26edc6476b4b25e1a337190021f2f8dde77ca61a. this is the implementation response to review round 1, of at most 3. no new independent review was started; never merged.

finding / gate status evidence
P1 retired canonical href recreation fixed review_r1_local_retired_href: local/imported, normal/missing legacy reservations, headerless refusal, stale If-Match 412, explicit create-only fresh ids, Trash/restore/forget
P1 alias/canonical collision fixed review_r1_alias_canonical_collision: caller refused before a second contact, reopen/rebuild, update and delete; history migration/namespace tests
P1 ETag race with ordinary note edits fixed review_r1_etag_and_commit_share_the_note_version: deterministic pauses after validation and before commit, same-path and rename; 412, local edit/card/path preserved
P2 grouped unknown label omission fixed review_r1_unknown_group_label and review_r1_modeled_group_label_can_still_be_deleted
simultaneous create-only attempts passed review_r1_simultaneous_create_only_actions_recheck_absence: one create and one BlockChanged/412
delayed events after retired URL reuse passed review_r1_delayed_retired_events_do_not_remove_reused_href: remove old cached route by note owner, retain the new owner, move cleanup
dav/carddav/contacts/contacts-actions/server passed complete package suite, serialized tests, -j4 builds
history aliases / migration passed 2 notes tests, including a v13 database with historical companion and forgotten-contact tombstones
action contract passed regenerated for optional create_only, checked again
clippy passed all targets of the five packages plus notes, -D warnings
fmt passed cargo fmt --all -- --check

all heavy commands ran on cua@calternal-dev-02-std, worktree /home/cua/Developer/Worktrees/carddav-writes, with ~/bin/slot nix develop -c bash -c .... final log ~/carddav797-r1-serial.log ends GATES_EXIT=0. SHA-256 matched all 423 Rust/Cargo/contract files between the control checkout and VM. no control-host builds, tests, formatting or installs.

cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server --locked --no-fail-fast -- --test-threads=1
cargo test -j4 -p calternal-notes history::aliases --locked
cargo test -j4 -p calternal --bin calternal contract --locked
cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server -p calternal-notes --all-targets --locked -- -D warnings
cargo fmt --all -- --check

red/green evidence: the two href regressions and grouped-label regression failed on the original implementation; the deterministic action race accepted the stale write and replaced the local phone edit before the fix. the delayed-projection regression also failed before the cache owner fix. logs: ~/carddav797-r1-red.log, ~/carddav797-r1-cas-red.log, ~/carddav797-r1-cache-red.log; green results are in the final package/history run.

decisions and remaining risks

  • canonical and client-selected names share resource_aliases, unique by (owner, href) in each user's history. creates/imports reserve before publishing; migration backfills canonical historical/departed ids. fallback lookup also checks Trash/departure records for missing legacy reservations. reservations survive forgetting.
  • headerless replay at a retired route is forbidden; If-Match against an absent route is 412. explicit If-None-Match: * may reuse a retired URL with a fresh contact id. if the old contact is later restored, it uses a distinct durable route, preserving both contacts. pending reservations still resume the reserved new id.
  • ETag and merge use one room-owned note/companion snapshot. ComparePatch checks that snapshot's note version inside the room before mutation; conditional rename jobs perform this edit before any rename/link step. existing unconditional patch/merge behavior remains.
  • disposable cache rows now track note ownership, so delayed deletes and moved restore routes cannot erase a new contact at a reused URL. pre-ownership caches rebuild once and receive a fresh sync epoch.
  • the inherited wall-clock transport ticker assertion failed intermittently, and the 65-second reader test once failed its reads > 400 count. the complete final suite passed with --test-threads=1; neither threshold nor body-validation assertion was weakened. the ticker failure message now includes its count.
  • native Apple PUT capture/replay, DELETE, Lists/groups, performance-VM gates and protected recovery snapshots remain inherited delivery gaps. this revision does not claim measured native Apple compatibility or process-kill recovery at the DAV action boundary.
## round 1 fixes, ready for re-review head `8bcb2a1e08b9fe0496812e5044af2c2d0d66325e`; re-merged #794 at `26edc6476b4b25e1a337190021f2f8dde77ca61a`. this is the implementation response to [review round 1](https://git.kayg.org/kayg/calternal/pulls/797#issuecomment-71132), of at most 3. no new independent review was started; never merged. | finding / gate | status | evidence | |---|---|---| | P1 retired canonical href recreation | fixed | `review_r1_local_retired_href`: local/imported, normal/missing legacy reservations, headerless refusal, stale If-Match 412, explicit create-only fresh ids, Trash/restore/forget | | P1 alias/canonical collision | fixed | `review_r1_alias_canonical_collision`: caller refused before a second contact, reopen/rebuild, update and delete; history migration/namespace tests | | P1 ETag race with ordinary note edits | fixed | `review_r1_etag_and_commit_share_the_note_version`: deterministic pauses after validation and before commit, same-path and rename; 412, local edit/card/path preserved | | P2 grouped unknown label omission | fixed | `review_r1_unknown_group_label` and `review_r1_modeled_group_label_can_still_be_deleted` | | simultaneous create-only attempts | passed | `review_r1_simultaneous_create_only_actions_recheck_absence`: one create and one BlockChanged/412 | | delayed events after retired URL reuse | passed | `review_r1_delayed_retired_events_do_not_remove_reused_href`: remove old cached route by note owner, retain the new owner, move cleanup | | dav/carddav/contacts/contacts-actions/server | passed | complete package suite, serialized tests, `-j4` builds | | history aliases / migration | passed | 2 notes tests, including a v13 database with historical companion and forgotten-contact tombstones | | action contract | passed | regenerated for optional `create_only`, checked again | | clippy | passed | all targets of the five packages plus notes, `-D warnings` | | fmt | passed | `cargo fmt --all -- --check` | all heavy commands ran on `cua@calternal-dev-02-std`, worktree `/home/cua/Developer/Worktrees/carddav-writes`, with `~/bin/slot nix develop -c bash -c ...`. final log `~/carddav797-r1-serial.log` ends `GATES_EXIT=0`. SHA-256 matched all **423** Rust/Cargo/contract files between the control checkout and VM. no control-host builds, tests, formatting or installs. ```sh cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server --locked --no-fail-fast -- --test-threads=1 cargo test -j4 -p calternal-notes history::aliases --locked cargo test -j4 -p calternal --bin calternal contract --locked cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server -p calternal-notes --all-targets --locked -- -D warnings cargo fmt --all -- --check ``` red/green evidence: the two href regressions and grouped-label regression failed on the original implementation; the deterministic action race accepted the stale write and replaced the local phone edit before the fix. the delayed-projection regression also failed before the cache owner fix. logs: `~/carddav797-r1-red.log`, `~/carddav797-r1-cas-red.log`, `~/carddav797-r1-cache-red.log`; green results are in the final package/history run. ### decisions and remaining risks - canonical and client-selected names share `resource_aliases`, unique by `(owner, href)` in each user's history. creates/imports reserve before publishing; migration backfills canonical historical/departed ids. fallback lookup also checks Trash/departure records for missing legacy reservations. reservations survive forgetting. - headerless replay at a retired route is forbidden; If-Match against an absent route is 412. explicit `If-None-Match: *` may reuse a retired URL with a **fresh contact id**. if the old contact is later restored, it uses a distinct durable route, preserving both contacts. pending reservations still resume the reserved new id. - ETag and merge use one room-owned note/companion snapshot. `ComparePatch` checks that snapshot's note version inside the room before mutation; conditional rename jobs perform this edit before any rename/link step. existing unconditional patch/merge behavior remains. - disposable cache rows now track note ownership, so delayed deletes and moved restore routes cannot erase a new contact at a reused URL. pre-ownership caches rebuild once and receive a fresh sync epoch. - the inherited wall-clock transport ticker assertion failed intermittently, and the 65-second reader test once failed its `reads > 400` count. the complete final suite passed with `--test-threads=1`; neither threshold nor body-validation assertion was weakened. the ticker failure message now includes its count. - native Apple PUT capture/replay, DELETE, Lists/groups, performance-VM gates and protected recovery snapshots remain inherited delivery gaps. this revision does not claim measured native Apple compatibility or process-kill recovery at the DAV action boundary.
kayg commented 2026-10-31 18:45:59 +00:00
Author
Owner
Copy link

Independent review, round 2

verdict: fix first. reviewed 8bcb2a1e08b9fe0496812e5044af2c2d0d66325e, against round 1 (#issuecomment-71132) and the fix report (#issuecomment-71231). no P0 found. no code pushed.

round-1 status

finding status does the regression prove it?
P1 retired canonical href recreation verified in the implementation; runtime coverage incomplete in this review route ownership now precedes id allocation, including local/imported canonical names and legacy departure fallback. review_r1_local_retired_href covers refusal, 412, explicit fresh-id creation, restore and forget, but my run failed at its initial GET's missing ETag (write.rs:605), before completing its assertions. do not count that run as green evidence.
P1 alias/canonical collisions verified for new writes; legacy migration gap below review_r1_alias_canonical_collision passed: rejected action, one live id, restart, update, delete. the history namespace test also passed. these prove prevention, not repair of already-colliding v13 rows.
P1 ETag race with ordinary edits verified review_r1_etag_and_commit_share_the_note_version passed. its deterministic pauses after validation and before commit cover both same-path and rename writes, asserting BlockChanged, retained local edit, unchanged card and path. this exercises the actual old race, unlike a pre-action stale-ETag test.
P2 omitted grouped unknown label verified both the unknown-group preservation and modeled-label deletion regressions passed. together they prove the requested distinction.

new findings

P1: migration can preserve a legacy collision and disable contacts recovery for the whole Home. crates/notes/src/history.rs:1179, crates/notes/src/history.rs:1182; recovery consequence at crates/contacts-actions/src/write.rs:190–196.

Scenario: a v13 Home contains local contact A and contact B whose client alias is <A-id>.vcf, a state the original implementation accepted. migration copies B's alias first, then INSERT OR IGNORE silently drops A's canonical reservation. A has no route. startup recovery attempts to reserve A's canonical URL, encounters B, and propagates IdempotencyConflict; Person.recovered cannot initialize, so contact operations/rebuild fail for that Home. I replayed the actual v14 migration SQL on dev-02: with a.vcf -> b, the resulting rows were only a.vcf -> b and b.vcf -> b; A had no route. the recovery failure is established by source tracing, not an executed integration regression.

Fix: explicitly reconcile conflicting existing aliases and canonical names during migration, retain every live/retired identity on a distinct durable route, and force the necessary cache resync. do not silently discard a required reservation. add a real v13 collision fixture that opens the upgraded contacts service and verifies both contacts, subsequent edits/deletes and rebuild.

P2: restoring the old contact after same-UID URL reuse creates duplicate live CardDAV UIDs. crates/contacts-actions/src/write.rs:272–280, crates/contacts-actions/src/write.rs:689–693.

Scenario: delete A with UID U; PUT If-None-Match: * with UID U at its retired href. the new policy permits fresh contact B because A is not live. restore A: restore checks no UID inventory, so A and B become live at distinct hrefs with the same UID. distinct href allocation fixes route ownership but leaves ambiguous CardDAV identity. the restore regression uses different UIDs (local / other), so it misses this case. finding from source tracing.

Fix: enforce the live UID invariant during restore under the same contact writer lock, returning an actionable conflict or explicitly reconciling identity without discarding either card. test reuse with the original UID, restore, restart and cache rebuild.

P2: permanent route/tombstone growth bypasses quota accounting with separate data/state roots. crates/notes/src/history/aliases.rs:39–60, crates/contacts-actions/src/write.rs:135–140, crates/contacts-actions/src/write.rs:328–337.

Scenario: repeatedly create, delete and forget contacts, reusing retired URLs with create-only PUTs. canonical reservations and replacement restore routes remain permanently in the state history DB. admission charges note/card growth; the separate-state provider reports only retained companion bytes, not route rows. after forgetting cards, billed usage can fall while permanent route metadata continues growing without a count/budget bound. no expiry preserves replay protection, but does not bound retained storage. finding from source tracing; no quota-load test run.

Fix: account for retained route metadata or impose an enforced per-Home metadata budget before reservation. preserve the retirement protection when that budget is exhausted; do not introduce a TTL that silently re-enables headerless resurrection. test repeated create/reuse/forget cycles with separate roots and a small quota.

concurrency and verification

review_r1_simultaneous_create_only_actions_recheck_absence passed: one creation, one BlockChanged, one live id. the action's absence check is inside the per-user writer lock, and the route transaction has a unique (owner,href) key. this supports two-device create-only uniqueness; it is an action-level test, not two independent network connections. ordinary restore without URL reuse retains its reservation; URL reuse allocates a separate restore route, subject to the UID gap above.

Executed on dev-02, isolated worktree at the reviewed SHA, Nix through ~/bin/slot, shared existing target directory:

cargo test -j4 -p calternal-carddav --test write review_r1_ --locked -- --test-threads=1
cargo test -j4 -p calternal-contacts-actions review_r1_etag --locked -- --test-threads=1
cargo test -j4 -p calternal-notes history::aliases --locked -- --test-threads=1
cargo test -j4 -p calternal-contacts --test break_merge review_r1_ --locked

Results: 7 tests passed; retired-href regression failed during setup as described above. log: ~/review797-r2.log. two additional integration probes were prepared but their corrected run remained queued behind occupied VM slots; I cancelled my queued job and removed the temporary harness. they are not claimed as executed evidence. migration SQL replay ran separately on the VM. no control-host builds/tests, no production access, no merge. inherited Apple capture/replay and DELETE delivery gaps remain as recorded in round 1.

## Independent review, round 2 **verdict: fix first.** reviewed `8bcb2a1e08b9fe0496812e5044af2c2d0d66325e`, against round 1 (#issuecomment-71132) and the fix report (#issuecomment-71231). no P0 found. no code pushed. ### round-1 status | finding | status | does the regression prove it? | |---|---|---| | P1 retired canonical href recreation | verified in the implementation; runtime coverage incomplete in this review | route ownership now precedes id allocation, including local/imported canonical names and legacy departure fallback. `review_r1_local_retired_href` covers refusal, 412, explicit fresh-id creation, restore and forget, but my run failed at its initial GET's missing ETag (`write.rs:605`), before completing its assertions. do not count that run as green evidence. | | P1 alias/canonical collisions | verified for new writes; legacy migration gap below | `review_r1_alias_canonical_collision` passed: rejected action, one live id, restart, update, delete. the history namespace test also passed. these prove prevention, not repair of already-colliding v13 rows. | | P1 ETag race with ordinary edits | verified | `review_r1_etag_and_commit_share_the_note_version` passed. its deterministic pauses after validation and before commit cover both same-path and rename writes, asserting BlockChanged, retained local edit, unchanged card and path. this exercises the actual old race, unlike a pre-action stale-ETag test. | | P2 omitted grouped unknown label | verified | both the unknown-group preservation and modeled-label deletion regressions passed. together they prove the requested distinction. | ### new findings **P1: migration can preserve a legacy collision and disable contacts recovery for the whole Home.** `crates/notes/src/history.rs:1179`, `crates/notes/src/history.rs:1182`; recovery consequence at `crates/contacts-actions/src/write.rs:190–196`. Scenario: a v13 Home contains local contact A and contact B whose client alias is `<A-id>.vcf`, a state the original implementation accepted. migration copies B's alias first, then `INSERT OR IGNORE` silently drops A's canonical reservation. A has no route. startup recovery attempts to reserve A's canonical URL, encounters B, and propagates IdempotencyConflict; `Person.recovered` cannot initialize, so contact operations/rebuild fail for that Home. I replayed the actual v14 migration SQL on dev-02: with `a.vcf -> b`, the resulting rows were only `a.vcf -> b` and `b.vcf -> b`; A had no route. the recovery failure is established by source tracing, not an executed integration regression. Fix: explicitly reconcile conflicting existing aliases and canonical names during migration, retain every live/retired identity on a distinct durable route, and force the necessary cache resync. do not silently discard a required reservation. add a real v13 collision fixture that opens the upgraded contacts service and verifies both contacts, subsequent edits/deletes and rebuild. **P2: restoring the old contact after same-UID URL reuse creates duplicate live CardDAV UIDs.** `crates/contacts-actions/src/write.rs:272–280`, `crates/contacts-actions/src/write.rs:689–693`. Scenario: delete A with UID U; PUT `If-None-Match: *` with UID U at its retired href. the new policy permits fresh contact B because A is not live. restore A: restore checks no UID inventory, so A and B become live at distinct hrefs with the same UID. distinct href allocation fixes route ownership but leaves ambiguous CardDAV identity. the restore regression uses different UIDs (`local` / `other`), so it misses this case. finding from source tracing. Fix: enforce the live UID invariant during restore under the same contact writer lock, returning an actionable conflict or explicitly reconciling identity without discarding either card. test reuse with the original UID, restore, restart and cache rebuild. **P2: permanent route/tombstone growth bypasses quota accounting with separate data/state roots.** `crates/notes/src/history/aliases.rs:39–60`, `crates/contacts-actions/src/write.rs:135–140`, `crates/contacts-actions/src/write.rs:328–337`. Scenario: repeatedly create, delete and forget contacts, reusing retired URLs with create-only PUTs. canonical reservations and replacement restore routes remain permanently in the state history DB. admission charges note/card growth; the separate-state provider reports only retained companion bytes, not route rows. after forgetting cards, billed usage can fall while permanent route metadata continues growing without a count/budget bound. no expiry preserves replay protection, but does not bound retained storage. finding from source tracing; no quota-load test run. Fix: account for retained route metadata or impose an enforced per-Home metadata budget before reservation. preserve the retirement protection when that budget is exhausted; do not introduce a TTL that silently re-enables headerless resurrection. test repeated create/reuse/forget cycles with separate roots and a small quota. ### concurrency and verification `review_r1_simultaneous_create_only_actions_recheck_absence` passed: one creation, one BlockChanged, one live id. the action's absence check is inside the per-user writer lock, and the route transaction has a unique `(owner,href)` key. this supports two-device create-only uniqueness; it is an action-level test, not two independent network connections. ordinary restore without URL reuse retains its reservation; URL reuse allocates a separate restore route, subject to the UID gap above. Executed on dev-02, isolated worktree at the reviewed SHA, Nix through `~/bin/slot`, shared existing target directory: ```sh cargo test -j4 -p calternal-carddav --test write review_r1_ --locked -- --test-threads=1 cargo test -j4 -p calternal-contacts-actions review_r1_etag --locked -- --test-threads=1 cargo test -j4 -p calternal-notes history::aliases --locked -- --test-threads=1 cargo test -j4 -p calternal-contacts --test break_merge review_r1_ --locked ``` Results: 7 tests passed; retired-href regression failed during setup as described above. log: `~/review797-r2.log`. two additional integration probes were prepared but their corrected run remained queued behind occupied VM slots; I cancelled my queued job and removed the temporary harness. they are not claimed as executed evidence. migration SQL replay ran separately on the VM. no control-host builds/tests, no production access, no merge. inherited Apple capture/replay and DELETE delivery gaps remain as recorded in round 1.
kayg commented 2026-10-31 18:48:58 +00:00
Author
Owner
Copy link

round 2 implementation plan (P1 migration only): merged origin/feat/dav-transport at 80ec1fa3, retaining route ownership in bounded rebuilds. reconcile legacy routes transactionally: canonical owners win; competing aliases use latest committed companion sequence, then note id. each displaced alias receives a fresh durable href and a resource_route_migrations report row. keep all historical/departed identities reserved and rotate disposable cache generation. reopen is a no-op. cost O(routes log routes + history), SQLite temporary tables plus streaming collision repair.

failure effect detection recovery test
legacy canonical/alias, alias/alias or cross-contact collisions no lost identity or blocked Home ranked shared namespace fresh loser href, durable report, full resync real legacy DB, recovery, GET, edit/delete, rebuild, reopen
interruption during upgrade no partial namespace SQLite transaction retry upgrade transactional migration plus reopen assertions

requested package/history/contract/clippy/fmt gates run on dev-02 through slot/Nix. the separate P2 UID-restore and quota findings remain follow-up scope. never merge.

round 2 implementation plan (P1 migration only): merged origin/feat/dav-transport at 80ec1fa3, retaining route ownership in bounded rebuilds. reconcile legacy routes transactionally: canonical owners win; competing aliases use latest committed companion sequence, then note id. each displaced alias receives a fresh durable href and a resource_route_migrations report row. keep all historical/departed identities reserved and rotate disposable cache generation. reopen is a no-op. cost O(routes log routes + history), SQLite temporary tables plus streaming collision repair. | failure | effect | detection | recovery | test | |---|---|---|---|---| | legacy canonical/alias, alias/alias or cross-contact collisions | no lost identity or blocked Home | ranked shared namespace | fresh loser href, durable report, full resync | real legacy DB, recovery, GET, edit/delete, rebuild, reopen | | interruption during upgrade | no partial namespace | SQLite transaction | retry upgrade | transactional migration plus reopen assertions | requested package/history/contract/clippy/fmt gates run on dev-02 through slot/Nix. the separate P2 UID-restore and quota findings remain follow-up scope. never merge.
kayg commented 2026-10-31 19:04:56 +00:00
Author
Owner
Copy link

round 2 P1 fixed, ready for FINAL review

head 0d55d2635d244ea87d709b700e57ebf1618802d7; re-merged origin/feat/dav-transport at 80ec1fa3 first (merge 764c8daa). responds to round 2. no merge, and no additional review launched.

finding / gate status evidence
P1 legacy href collisions block Home recovery fixed canonical owners win; alias-only conflicts choose greatest committed companion sequence, then note id. every displaced alias gets a fresh durable href and a resource_route_migrations row (owner, note, old/new href, version). no identity is silently discarded
v13 migration and existing v14 repair passed review_r2_repairs_v13_and_already_upgraded_v14_collisions, including retired identities and unchanged routes/report count after reopen
alias/canonical, alias/alias, cross-contact collisions passed review_r2_legacy_collisions_recover_and_rebuild: six real contacts; recovery, distinct advertised hrefs, GETs, four remap reports, restart, fresh cache rebuild, subsequent edits/deletes of every contact, unaffected contact preserved
deterministic alias winner passed alias_collision_keeps_latest_committed_contact_in_either_input_order
cache resync after remap passed route-cache version advances to 2; legacy-cache fixture has an already-current journal cursor and still rebuilds. rebuilt epoch invalidates prior sync tokens
dav/carddav/contacts/contacts-actions/server passed complete requested package suites, serialized tests, -j4; includes the previously interrupted review_r1_local_retired_href regression
history / migration passed all 24 history:: tests, including both migration generations
action contract passed contract_matches_the_server
clippy passed requested five packages plus notes, all targets, -D warnings
fmt passed cargo fmt --all -- --check

all gates ran on cua@calternal-dev-02-std, /home/cua/Developer/Worktrees/carddav-writes-r2, through ~/bin/slot nix develop -c bash ..., using the existing target directory. log ~/carddav797-r2-gates.log ends GATES_EXIT=0. SHA-256 matched all 451 Rust/Cargo/contract files with the pushed control checkout. no control-host builds, tests, formatting or installs.

cargo test -j4 -p calternal-carddav --test write review_r2_ --locked -- --test-threads=1
cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server --locked --no-fail-fast -- --test-threads=1
cargo test -j4 -p calternal-notes history:: --locked -- --test-threads=1
cargo test -j4 -p calternal --bin calternal contract --locked
cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server -p calternal-notes --all-targets --locked -- -D warnings
cargo fmt --all -- --check

red/green: the legacy integration fixture failed on the merged pre-fix migration with UNIQUE constraint failed: resource_aliases.owner, resource_aliases.href and passed after repair. a separate replay of the original migration SQL reproduced the strict-v13 canonical-owner loss (a.vcf -> b). the integration fixture deliberately relaxes legacy href uniqueness to additionally exercise duplicate alias imports; the history fixtures cover collisions compatible with normal v13 uniqueness. all red and green output is in the gate log except the separate SQL probe.

migration repair is transactional and uses SQLite temporary tables, indexed lookups and streamed loser rows: O(history + routes log routes), without loading card payloads. v15 also repairs Homes already upgraded by the original v14 implementation. reopening v15 does not rerun repair. transport's bounded disk rebuild now retains per-note route ownership and uses the advertised href in each batch.

scope/risks: clients perform a full resync and may see delete+add for remapped contacts, as authorized. the separate round-2 P2 same-UID restore and retained-metadata quota findings remain open follow-up scope. native Apple capture/replay, DELETE and other inherited delivery gaps remain unchanged. no new process-kill migration test or native Apple compatibility claim is made.

## round 2 P1 fixed, ready for FINAL review head `0d55d2635d244ea87d709b700e57ebf1618802d7`; re-merged `origin/feat/dav-transport` at `80ec1fa3` first (merge `764c8daa`). responds to [round 2](https://git.kayg.org/kayg/calternal/pulls/797#issuecomment-71264). no merge, and no additional review launched. | finding / gate | status | evidence | |---|---|---| | P1 legacy href collisions block Home recovery | fixed | canonical owners win; alias-only conflicts choose greatest committed companion sequence, then note id. every displaced alias gets a fresh durable href and a `resource_route_migrations` row (owner, note, old/new href, version). no identity is silently discarded | | v13 migration and existing v14 repair | passed | `review_r2_repairs_v13_and_already_upgraded_v14_collisions`, including retired identities and unchanged routes/report count after reopen | | alias/canonical, alias/alias, cross-contact collisions | passed | `review_r2_legacy_collisions_recover_and_rebuild`: six real contacts; recovery, distinct advertised hrefs, GETs, four remap reports, restart, fresh cache rebuild, subsequent edits/deletes of every contact, unaffected contact preserved | | deterministic alias winner | passed | `alias_collision_keeps_latest_committed_contact_in_either_input_order` | | cache resync after remap | passed | route-cache version advances to 2; legacy-cache fixture has an already-current journal cursor and still rebuilds. rebuilt epoch invalidates prior sync tokens | | dav/carddav/contacts/contacts-actions/server | passed | complete requested package suites, serialized tests, `-j4`; includes the previously interrupted `review_r1_local_retired_href` regression | | history / migration | passed | all 24 `history::` tests, including both migration generations | | action contract | passed | `contract_matches_the_server` | | clippy | passed | requested five packages plus notes, all targets, `-D warnings` | | fmt | passed | `cargo fmt --all -- --check` | all gates ran on `cua@calternal-dev-02-std`, `/home/cua/Developer/Worktrees/carddav-writes-r2`, through `~/bin/slot nix develop -c bash ...`, using the existing target directory. log `~/carddav797-r2-gates.log` ends `GATES_EXIT=0`. SHA-256 matched all **451** Rust/Cargo/contract files with the pushed control checkout. no control-host builds, tests, formatting or installs. ```sh cargo test -j4 -p calternal-carddav --test write review_r2_ --locked -- --test-threads=1 cargo test -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server --locked --no-fail-fast -- --test-threads=1 cargo test -j4 -p calternal-notes history:: --locked -- --test-threads=1 cargo test -j4 -p calternal --bin calternal contract --locked cargo clippy -j4 -p calternal-dav -p calternal-carddav -p calternal-contacts -p calternal-contacts-actions -p calternal-server -p calternal-notes --all-targets --locked -- -D warnings cargo fmt --all -- --check ``` red/green: the legacy integration fixture failed on the merged pre-fix migration with `UNIQUE constraint failed: resource_aliases.owner, resource_aliases.href` and passed after repair. a separate replay of the original migration SQL reproduced the strict-v13 canonical-owner loss (`a.vcf -> b`). the integration fixture deliberately relaxes legacy href uniqueness to additionally exercise duplicate alias imports; the history fixtures cover collisions compatible with normal v13 uniqueness. all red and green output is in the gate log except the separate SQL probe. migration repair is transactional and uses SQLite temporary tables, indexed lookups and streamed loser rows: O(history + routes log routes), without loading card payloads. v15 also repairs Homes already upgraded by the original v14 implementation. reopening v15 does not rerun repair. transport's bounded disk rebuild now retains per-note route ownership and uses the advertised href in each batch. scope/risks: clients perform a full resync and may see delete+add for remapped contacts, as authorized. the separate round-2 P2 same-UID restore and retained-metadata quota findings remain open follow-up scope. native Apple capture/replay, DELETE and other inherited delivery gaps remain unchanged. no new process-kill migration test or native Apple compatibility claim is made.
kayg commented 2026-10-31 19:14:29 +00:00
Author
Owner
Copy link

Independent review, round 3 (final)

verdict: stop and simplify. reviewed 0d55d2635d244ea87d709b700e57ebf1618802d7, stacked on #794 at 80ec1fa3, against reviews #issuecomment-71132 / #issuecomment-71264 and fix report #issuecomment-71309. no P0 found. the ordinary legacy collision fixtures pass, but another valid collision shape still blocks the Home. this reaches kayg's final-round stopping rule. no code pushed.

item status / evidence
canonical/alias collisions, cross-contact cycles, duplicate aliases verified for the supplied fixtures: review_r2_legacy_collisions_recover_and_rebuild passed independently; six contacts retained, distinct advertised hrefs, GET/edit/delete and fresh rebuild
v13 migration and existing v14 repair supplied history fixture passed for both versions, including departed identity reservations
reopen is a no-op supplied history and integration assertions passed: advertised routes and report rows remain stable
every collision shape migrates; recovery never blocks not satisfied: handwritten contacts without companion history are absent from the migration inventory; new P1 below
clients retain contact data after remap supplied fixture retains every contact's UID and permits later edits/deletes; source confirms route-cache version 2 forces rebuild and a fresh sync epoch rejects old tokens. full resync is the mechanism, rather than a recorded delete/add delta. native client behavior was not measured
prior P2: same-UID restore after URL reuse fix now, P2. Contacts::restore still calls notes restore without a live UID check. fresh B may reuse trashed A's UID, then restoring A publishes two live resources with that UID. guard restore under the writer lock, preserving both cards on conflict; cover restart/rebuild
prior P2: retained route metadata bypasses separate-root quota fix now, P2. quota provider still counts companion bytes, while reservations and migration-report rows persist separately. enforce a retained-metadata budget or account for these rows before new reservations. exhaustion must preserve existing replay protection and permit recovery; test create/reuse/forget with separate roots
inherited Apple capture/replay, DELETE and delivery gates remain open; no additional compatibility or delivery claim from this review

new findings, P0–P3

P1: the repair still omits live handwritten canonical owners, so an accepted legacy alias can disable all contact recovery.

Locations: crates/notes/src/history/route_migration.rs:24–27; crates/contacts-actions/src/write.rs:189–196.

Create handwritten contact note A (format: calternal-contact) through notes or an outside edit, without an original-card companion/history row. In a v13 Home, contact B can have explicit alias <A-id>.vcf; this is the same legacy implicit-canonical collision class the original implementation accepted. A need not have any alias, companion, or departure row.

The repair inventories only existing alias targets, contacts companions, and departures. It therefore never adds A as a canonical candidate, keeps A.vcf -> B, and emits no displacement report for it. During recovery, A is discovered by the live-note scan; alias_for(A) is empty, so reserve_alias(A.vcf,A) returns IdempotencyConflict. That error still escapes the per-person recovery initializer, blocking contact operations and CardDAV rebuilding for the Home. v15 will not retry reconciliation on subsequent opens.

Evidence: replayed the exact candidate/ranking SQL extracted from this head on dev-02, with a companion for B and legacy a.vcf -> b. Result: only a.vcf -> b and b.vcf -> b; A had zero routes. The recovery-equivalent insert/recheck still returned owner B. The service-wide consequence is established by source tracing, not an executed integration regression. The temporary handwritten-contact and same-UID integration probes remained queued behind occupied VM slots; I cancelled my queued job and removed their harness, so neither is claimed as run.

No other new P0/P1/P2/P3 finding is asserted.

concrete simpler design

Stop adding schema-only repairs that infer a complete contact inventory from companion history. Use one authoritative advertised route per contact, retaining retired route ownership for replay protection. Resolve requests from that table; remove implicit canonical-target inference and canonical reservations for unadvertised secondary names.

Perform one reconciliation after a settled Home inventory includes live handwritten contacts, Trash, historical/departed identities and pending reservations. Preserve existing unambiguous ownership; assign each unrepresented or displaced identity its canonical href if free, otherwise a fresh href, rather than failing the whole Home. Resolve actual duplicate legacy hrefs deterministically. Commit assignments, remap reports and a route generation together; rebuild the disposable cache when that generation changes. Reopen then only reads the persisted assignments. Keep this operation metadata-only: no card rewrite or identity merge.

Before reviving the branch, require fixtures covering a handwritten owner with no companion, the existing collision matrix, URL reuse/restore, old-token rejection, exact retained card bytes and reopen stability. This is a proposed replacement design, not another request to patch and start round 4.

independent verification

dev-02, isolated checkout /home/cua/Developer/Worktrees/review797-r3 at the reviewed SHA; Nix through ~/bin/slot, shared existing target directory:

cargo test -j4 -p calternal-carddav --test write review_r2_ --locked -- --test-threads=1
cargo test -j4 -p calternal-notes history:: --locked -- --test-threads=1

1 integration + 24 history tests passed, log ~/review797-r3-gates.log, EXIT=0. Exact migration SQL replay ran separately on the VM. Broader package/clippy/fmt gates remain author-reported, not independently rerun here. No control-host builds/tests, production access, code push or merge.

## Independent review, round 3 (final) **verdict: stop and simplify.** reviewed `0d55d2635d244ea87d709b700e57ebf1618802d7`, stacked on #794 at `80ec1fa3`, against reviews #issuecomment-71132 / #issuecomment-71264 and fix report #issuecomment-71309. no P0 found. the ordinary legacy collision fixtures pass, but another valid collision shape still blocks the Home. this reaches kayg's final-round stopping rule. no code pushed. | item | status / evidence | |---|---| | canonical/alias collisions, cross-contact cycles, duplicate aliases | verified for the supplied fixtures: `review_r2_legacy_collisions_recover_and_rebuild` passed independently; six contacts retained, distinct advertised hrefs, GET/edit/delete and fresh rebuild | | v13 migration and existing v14 repair | supplied history fixture passed for both versions, including departed identity reservations | | reopen is a no-op | supplied history and integration assertions passed: advertised routes and report rows remain stable | | every collision shape migrates; recovery never blocks | **not satisfied**: handwritten contacts without companion history are absent from the migration inventory; new P1 below | | clients retain contact data after remap | supplied fixture retains every contact's UID and permits later edits/deletes; source confirms route-cache version 2 forces rebuild and a fresh sync epoch rejects old tokens. full resync is the mechanism, rather than a recorded delete/add delta. native client behavior was not measured | | prior P2: same-UID restore after URL reuse | **fix now, P2**. `Contacts::restore` still calls notes restore without a live UID check. fresh B may reuse trashed A's UID, then restoring A publishes two live resources with that UID. guard restore under the writer lock, preserving both cards on conflict; cover restart/rebuild | | prior P2: retained route metadata bypasses separate-root quota | **fix now, P2**. quota provider still counts companion bytes, while reservations and migration-report rows persist separately. enforce a retained-metadata budget or account for these rows before new reservations. exhaustion must preserve existing replay protection and permit recovery; test create/reuse/forget with separate roots | | inherited Apple capture/replay, DELETE and delivery gates | remain open; no additional compatibility or delivery claim from this review | ### new findings, P0–P3 **P1: the repair still omits live handwritten canonical owners, so an accepted legacy alias can disable all contact recovery.** Locations: `crates/notes/src/history/route_migration.rs:24–27`; `crates/contacts-actions/src/write.rs:189–196`. Create handwritten contact note A (`format: calternal-contact`) through notes or an outside edit, without an original-card companion/history row. In a v13 Home, contact B can have explicit alias `<A-id>.vcf`; this is the same legacy implicit-canonical collision class the original implementation accepted. A need not have any alias, companion, or departure row. The repair inventories only existing alias targets, contacts companions, and departures. It therefore never adds A as a canonical candidate, keeps `A.vcf -> B`, and emits no displacement report for it. During recovery, A is discovered by the live-note scan; `alias_for(A)` is empty, so `reserve_alias(A.vcf,A)` returns IdempotencyConflict. That error still escapes the per-person recovery initializer, blocking contact operations and CardDAV rebuilding for the Home. v15 will not retry reconciliation on subsequent opens. **Evidence:** replayed the exact candidate/ranking SQL extracted from this head on dev-02, with a companion for B and legacy `a.vcf -> b`. Result: only `a.vcf -> b` and `b.vcf -> b`; A had zero routes. The recovery-equivalent insert/recheck still returned owner B. The service-wide consequence is established by source tracing, not an executed integration regression. The temporary handwritten-contact and same-UID integration probes remained queued behind occupied VM slots; I cancelled my queued job and removed their harness, so neither is claimed as run. No other new P0/P1/P2/P3 finding is asserted. ### concrete simpler design Stop adding schema-only repairs that infer a complete contact inventory from companion history. Use one authoritative advertised route per contact, retaining retired route ownership for replay protection. Resolve requests from that table; remove implicit canonical-target inference and canonical reservations for unadvertised secondary names. Perform one reconciliation after a settled Home inventory includes live handwritten contacts, Trash, historical/departed identities and pending reservations. Preserve existing unambiguous ownership; assign each unrepresented or displaced identity its canonical href if free, otherwise a fresh href, rather than failing the whole Home. Resolve actual duplicate legacy hrefs deterministically. Commit assignments, remap reports and a route generation together; rebuild the disposable cache when that generation changes. Reopen then only reads the persisted assignments. Keep this operation metadata-only: no card rewrite or identity merge. Before reviving the branch, require fixtures covering a handwritten owner with no companion, the existing collision matrix, URL reuse/restore, old-token rejection, exact retained card bytes and reopen stability. This is a proposed replacement design, not another request to patch and start round 4. ### independent verification dev-02, isolated checkout `/home/cua/Developer/Worktrees/review797-r3` at the reviewed SHA; Nix through `~/bin/slot`, shared existing target directory: ```sh cargo test -j4 -p calternal-carddav --test write review_r2_ --locked -- --test-threads=1 cargo test -j4 -p calternal-notes history:: --locked -- --test-threads=1 ``` **1 integration + 24 history tests passed**, log `~/review797-r3-gates.log`, `EXIT=0`. Exact migration SQL replay ran separately on the VM. Broader package/clippy/fmt gates remain author-reported, not independently rerun here. No control-host builds/tests, production access, code push or merge.
kayg commented 2026-10-31 19:20:03 +00:00
Author
Owner
Copy link

#797 is on hold after the round-3 “stop and simplify” verdict. No fourth repair round is planned here.

Replacement plan: #807, #807
Plan: https://git.kayg.org/kayg/calternal/src/branch/docs/carddav-writes-v2-plan/docs/plans/0064-carddav-writes-v2.md

The plan chooses one authoritative route table, stable client hrefs and deterministic reconciliation from the settled Home on every startup/recovery. It covers handwritten contacts without history, collision reports, retired ownership, crash tests, metadata accounting, salvage and implementation slices. RFC 6352 and HTTP Location research does not establish Apple support for immediate href replacement after PUT, so that behavior is not assumed. This is docs only; no implementation or merge.

#797 is on hold after the round-3 “stop and simplify” verdict. No fourth repair round is planned here. Replacement plan: #807, https://git.kayg.org/kayg/calternal/pulls/807 Plan: https://git.kayg.org/kayg/calternal/src/branch/docs/carddav-writes-v2-plan/docs/plans/0064-carddav-writes-v2.md The plan chooses one authoritative route table, stable client hrefs and deterministic reconciliation from the settled Home on every startup/recovery. It covers handwritten contacts without history, collision reports, retired ownership, crash tests, metadata accounting, salvage and implementation slices. RFC 6352 and HTTP Location research does not establish Apple support for immediate href replacement after PUT, so that behavior is not assumed. This is docs only; no implementation or merge.
kayg commented 2026-11-07 19:10:08 +00:00
Author
Owner
Copy link

housekeeping #1009: needs-decision.

Explicit three-round hold. CardDAV writes v2 plan #807 replaces the route/executor direction; retain href/ETag/precondition regression coverage.

original head: 0d55d2635d244ea87d709b700e57ebf1618802d7. against dev cd7f84432febfc63b788fbb9b9f9950452982d74, git cherry reports 22 unmatched and 0 equivalent patches. the landing audit lists each candidate missing commit and stable patch match in pr-797.md; unmatched patches can include superseded behavior.

left open, source branch unchanged. no rebase or new test claim for this decision lane.

housekeeping #1009: needs-decision. Explicit three-round hold. CardDAV writes v2 plan #807 replaces the route/executor direction; retain href/ETag/precondition regression coverage. original head: `0d55d2635d244ea87d709b700e57ebf1618802d7`. against dev `cd7f84432febfc63b788fbb9b9f9950452982d74`, git cherry reports 22 unmatched and 0 equivalent patches. the [landing audit](https://git.kayg.org/attachments/fe3c09dc-dea3-47a0-92e2-b44a777083e5) lists each candidate missing commit and stable patch match in `pr-797.md`; unmatched patches can include superseded behavior. left open, source branch unchanged. no rebase or new test claim for this decision lane.
kayg commented 2026-11-07 21:21:09 +00:00
Author
Owner
Copy link

Superseded by #807 (CardDAV writes v2). The branch and original tips are archived under archive/ (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.

Superseded by #807 (CardDAV writes v2). The branch and original tips are archived under `archive/` (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.
kayg closed this pull request 2026-11-07 21:21:10 +00:00
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

Pull request closed

Please reopen this pull request to perform a merge.
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!797
No description provided.