feat(sync): rfc-003 phase a.2.2 — apply pipeline stamps hlc + origin_device_id - #55
Conversation
…device_id Phase A.2.2 — every entity write in the apply pipeline now stamps the §2 total-order triple (hlc_wall, hlc_logical, origin_device_id) the A.1.2 schema reserved. v1 ops derive `(0, lamport_ts)` exactly the way the A.1.1 sync_op backfill did; v2 ops echo the wire pair verbatim. The §2 tiebreaker rides through the existing sync_op.device_id TEXT (per A.1.1) parsed as Uuid — non-UUID strings (legacy v1 desktops) stamp NULL on the row. Scope: - New OpStamp type bundling effective Hlc + Option<Uuid> origin, computed once per op in apply_op and threaded to every handler. - New apply::effective_hlc + apply::parse_origin_device_id helpers with safe v1 narrowing (clamp to i32::MAX, defence-in-depth on the push handler's existing range guard). - apply_op signature gains device_id: &str; push handler updated. - profile_resolve::find_or_provision + library::insert/set_field + playlist::insert/set_field + track::insert (via TrackInput) + liked::insert + rating::set (UPSERT path) — all bind the three columns. INSERT, SET, and UPSERT-overwrite paths all stamp; the rating UPSERT also refreshes hlc on conflict so the row's tuple reflects the latest op, not the first one that landed. What this does NOT do: - payload_hash bind + metadata_digest_version bump — deferred to A.2.3 alongside the digest endpoint that consumes them. The payload_hash module (#54) stays unused until then. - LWW SQL gate (WHERE existing.hlc < incoming.hlc) — Phase A says "no behaviour change", LWW activation lands in Phase C alongside OR-Set / Fractional Index. - playlist_track / album / artist / track_artist sub-entity stamps — playlist_track is OR-Set territory (Phase C); album/artist are auto-materialised, HLC piggybacks on the source track row. - Delete handlers — no row left to stamp. Tests: 5 new cases in tests/apply.rs: - library_insert_v1_stamps_derived_hlc_and_uuid_origin (v1 path) - library_insert_v2_stamps_verbatim_hlc (v2 path) - library_insert_non_uuid_device_id_stamps_null_origin (legacy) - liked_insert_stamps_hlc_on_user_scoped_row (user-scoped) - rating_upsert_refreshes_hlc_on_overwrite (UPSERT path) Full suite: 235 tests pass (30 apply + 5 new + 200 others; one albums::list_album_tracks_wrong_library_id_returns_404 flapped under parallel load but passes in isolation — unrelated to this change). Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplémentation de la Phase A.2.2 RFC-003 : un ChangesPipeline de stamp HLC — RFC-003 Phase A.2.2
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/sync.rs (1)
347-360:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLa CI casse encore sur
cargo fmt.Le bloc
lamport_maxn’est pas au format attendu parrustfmt, donccargo fmt --all --checkéchoue déjà sur ce fichier. Lancecargo fmt --allavant merge.As per coding guidelines,
**/*.rs: Runcargo fmt --all --checkbefore committing Rust code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/sync.rs` around lines 347 - 360, The error handling block in the lamport_max function call in src/api/sync.rs does not conform to Rust formatting standards expected by rustfmt. Run cargo fmt --all to automatically reformat the code to meet the project's coding guidelines before merging.Sources: Coding guidelines, Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/apply.rs`:
- Around line 901-905: The INSERT statement for user_liked_track uses ON
CONFLICT (user_id, file_hash) DO NOTHING, which means that when a duplicate like
is attempted on the same user and file, the existing row retains its original
hlc_wall, hlc_logical, origin_device_id, and liked_at values instead of being
updated with the new ones. To ensure the materialized row always reflects the
most recent winning operation as required by this PR's contract, replace DO
NOTHING with DO UPDATE SET to update the hlc_wall, hlc_logical,
origin_device_id, and liked_at columns with the new values from the EXCLUDED
clause. This way, subsequent likes on the same user-file pair will refresh the
timestamp and device origin information to match the latest operation.
---
Outside diff comments:
In `@src/api/sync.rs`:
- Around line 347-360: The error handling block in the lamport_max function call
in src/api/sync.rs does not conform to Rust formatting standards expected by
rustfmt. Run cargo fmt --all to automatically reformat the code to meet the
project's coding guidelines before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d642b91-a425-46db-b9ee-1ac6fc86e57f
📒 Files selected for processing (4)
src/api/sync.rssrc/apply.rssrc/db.rstests/apply.rs
Two review fixes on #55: 1. apply::liked::apply — switched ON CONFLICT DO NOTHING to DO UPDATE so a repeat like on the same (user, file) refreshes hlc_wall / hlc_logical / origin_device_id / liked_at to the latest winning op. Without this, two devices liking the same file would converge on different row hashes once the A.2.3 digest endpoint goes live — the materialised row would keep the first-landing op's tuple while the canonical "winning" op (by sync_op.id under Phase A implicit LWW) would be the second. Mirrors the rating handler's existing UPSERT behaviour. 2. cargo fmt --all — fixes a formatting drift in api/sync.rs that #52 introduced and rustfmt would now reject on CI. Both fixes pass clippy + the apply suite's liked / rating / library stamp tests. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
What
Phase A.2.2 of the RFC-003 sync v2 rollout. Every entity write in the apply pipeline now stamps the §2 total-order triple
(hlc_wall, hlc_logical, origin_device_id)the A.1.2 schema reserved.(0, lamport_ts)exactly the way the A.1.1sync_opbackfill did — any v2 op withwall > 0strictly outranks every v1-derived row under §2.hlcpair verbatim.origin_device_idrides through the existingsync_op.device_idTEXT column (per A.1.1's design), parsed asUuid. Non-UUID strings (legacy v1 desktops) stampNULLon the row —payload_hash::HlcTriplealready hasNone < Some(any)so legacy rows lose to v2 ops on the §2 tiebreak.Follows #50 / #51 / #52 / #53 / #54.
Code change
OpStamp { hlc, origin_device_id }type insrc/apply.rs, computed once per op inapply_opand threaded by value (Copy) to every handler.apply::effective_hlc(op)+apply::parse_origin_device_id(device_id). The v1 narrowing ineffective_hlcclamps toi32::MAXas defence-in-depth on the push handler's existing range guard.apply_opsignature gainsdevice_id: &str;api/sync.rs::push_opsupdated to thread it.profile_resolve::find_or_provisionlibrary::insert/library::set_field(all four scalar fields)playlist::insert/playlist::set_field(all four scalar fields)track::insertviaTrackInput(3 new fields)liked::insertrating::setUPSERT — also refreshes HLC on conflict so the row's tuple reflects the latest op, not the first one that landedWhat this does NOT do
payload_hashbind +metadata_digest_versionbump — deferred to A.2.3 alongside the digest endpoint that actually consumes them. Thepayload_hashmodule (feat(sync): rfc-003 phase a.2.2.1 — payload_hash + hlc total-order helpers #54) stays unused until then; wiring them now without a consumer would be dead weight.WHERE existing.hlc < incoming.hlc) — Phase A says "no behaviour change". Phase C activates true LWW alongside OR-Set / Fractional Index.playlist_trackis OR-Set territory (Phase C);album/artist/track_artistare auto-materialised, HLC piggybacks on the sourcetrackrow.Test plan
cargo check --all-targets --all-features— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all-features --test apply— 35 pass (30 existing + 5 new):library_insert_v1_stamps_derived_hlc_and_uuid_origin— v1 path stamps(0, lamport_ts, uuid)library_insert_v2_stamps_verbatim_hlc— v2 path stamps(wall, logical, uuid)verbatimlibrary_insert_non_uuid_device_id_stamps_null_origin— legacy v1 with free-form device_id stampsNULLoriginliked_insert_stamps_hlc_on_user_scoped_row— user-scoped entity gets stamped toorating_upsert_refreshes_hlc_on_overwrite— UPSERT path refreshes the tuple, not just preserves the first onealbums::list_album_tracks_wrong_library_id_returns_404flapped under parallel load (passes in isolation, unrelated to this change).Refs
Summary by CodeRabbit
Notes de publication
Améliorations
Tests