feat(sync): rfc-003 phase a.1 — additive hlc fields on sync_op - #50
Conversation
First step of the RFC-003 Phase A migration: extend sync_op with the HLC pair (`hlc_wall`, `hlc_logical`) that §2's total order is defined on. Pre-existing rows backfill from `lamport_ts` per the RFC's Phase A rule — treat the legacy counter as the logical component, set wall = 0. The legacy `UNIQUE (user_id, device_id, lamport_ts)` stays in place; a new `UNIQUE (user_id, device_id, hlc_wall, hlc_logical)` enforces the same invariant on v2 wire-shape pushes. The `db::insert_op_returning` callsite is updated to write the derived HLC pair on every insert so the new NOT NULL columns are satisfied without a wire-shape change yet — the caller still passes only `lamport_ts`, and the SQL VALUES expression projects `(hlc_wall = 0, hlc_logical = lamport_ts)` to match the migration backfill. When A.2 lands and clients start emitting their own HLC, the signature gains an optional `hlc` parameter and the projection flips to use the client-provided values when present. The §2 total order's `origin_device_id` tiebreaker is the existing `sync_op.device_id` column (TEXT — unchanged at this layer). The stricter `origin_device_id UUID` typing lands in A.1.2 on the entity tables (`profile`, `library`, `track`, `playlist`, `liked_track`, `track_rating`) where the apply pipeline materialises the post-conflict-resolution state. `cargo check --all-targets --all-features` passes. `cargo test --all-features` 32 / 32 passes (sync + apply + share + artwork test suites all green against the new schema; the existing INSERT path automatically lands the derived HLC pair). `cargo clippy --all-targets --all-features -- -D warnings` clean. Migration dry-runs cleanly in a transaction against a live post-1.5.0 schema (one legacy `sync_op` row from a previous QA session backfilled successfully). Refs RFC-003 (PR #235 on InstaZDLL/WaveFlow). 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 (2)
📝 WalkthroughWalkthroughCette PR ajoute les colonnes HLC (hlc_wall, hlc_logical) au schéma ChangesExtension du schéma HLC pour sync_op
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@migrations/20260612000000_sync_op_hlc.sql`:
- Line 35: La colonne hlc_logical est INTEGER mais insert_op_returning
(src/db.rs) réutilise lamport_ts:i64 sans valider la plage 32-bit, ce qui peut
provoquer "integer out of range": dans src/db.rs (lines 123-123) ajoute un
contrôle explicite dans la fonction insert_op_returning pour vérifier que
lamport_ts est dans i32::MIN..=i32::MAX et retourner une erreur claire si hors
plage (ou clamp/convertir explicitement selon la politique), puis continue
d'utiliser la valeur validée/castée pour $4; pour
migrations/20260612000000_sync_op_hlc.sql (lines 35-35) ne change rien si vous
choisissez la validation Rust, sinon si vous voulez supporter des timestamps
>32-bit modifiez la colonne hlc_logical INTEGER → BIGINT et mettez à jour le
backfill SQL en conséquence.
🪄 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: cf32bbb0-1474-4bb9-9ce6-f65acc9ef30e
📒 Files selected for processing (2)
migrations/20260612000000_sync_op_hlc.sqlsrc/db.rs
Review finding on PR #50: `hlc_logical` is INTEGER (i32) per RFC-003 §2 but the migration backfill and the Rust INSERT both bound a raw i64 `lamport_ts` against it. Postgres would have raised SQLSTATE 22003 ("integer out of range") on any value > 2^31-1 — practically impossible in any current install (the legacy v1 Lamport counter is in the low thousands at most), but the silent-truncation risk on the SQL-side cast and the bare Postgres error on the Rust-side bind were both worth closing. Two changes, one PR: 1. Migration preflight + explicit cast. A DO block aborts the migration loudly if any sync_op.lamport_ts is outside [0, 2^31-1] before the UPDATE runs; the UPDATE itself now does an explicit `lamport_ts::INTEGER` cast so the contract between the wide source column and the narrow target column is visible in the SQL, not inferred. 2. Rust-side validation in `db::insert_op_returning`. A range check returns `sqlx::Error::Protocol` with a clear actionable message before binding, instead of letting Postgres surface bare 22003. The bound value is then narrowed to i32 explicitly (`lamport_ts as i32`) so the bind site type-matches the column. `cargo check --all-targets --all-features` clean. `cargo test --all-features` 32 / 32 green against a fresh DB running the new migration shape. Refs PR #50 review. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
What
First slice of RFC-003 Phase A — extend `sync_op` with the HLC pair (`hlc_wall`, `hlc_logical`) the §2 total order is defined on, so the wire shape can carry the new ordering primitive while every other moving part stays untouched. No semantic change yet — this PR is wire-shape additive only.
Migration
migrations/20260612000000_sync_op_hlc.sql:Backfill rule matches Phase A in the RFC: treat the legacy counter as the logical component, set wall = 0. That keeps the new unique invariant satisfied without needing a separate `WHERE` clause, and means any v2 op (with `hlc_wall > 0`) strictly outranks every legacy-shape row under the §2 total order — the intended LWW behaviour once A.2 lands.
The legacy `UNIQUE (user_id, device_id, lamport_ts)` stays in place. A v1 desktop pushing a stale-lamport replay still 23505s the same way it does today; v2 pushes 23505 against the new HLC constraint.
Code change
A single touchpoint in `src/db.rs` — `insert_op_returning` now projects `(hlc_wall = 0, hlc_logical = lamport_ts)` directly in the SQL VALUES clause so the new NOT NULL columns are satisfied without a caller-visible signature change. When A.2 lands and clients start emitting their own HLC, the signature gains an optional `hlc` parameter and the projection flips to use the client-provided values when present.
What this does NOT do
The point of slicing Phase A into A.1 → A.2 → A.3 → A.4 is exactly to keep each PR narrow enough that a rollback never has to unwind both schema and code at once.
Test plan
Refs
Summary by CodeRabbit