feat(sync): rfc-003 phase a.2.1 — dual-shape ingest on /sync/ops - #52
Merged
Conversation
|
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 (1)
📝 WalkthroughWalkthroughCette PR ajoute HLC complet au protocole de synchronisation : nouveau type ChangesRFC-003 Phase A.2: Hybrid Logical Clock on Sync Operations
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as api::push_ops
participant DB as db::sync::insert_op_returning
participant Postgres
Client->>API: push SyncOpIn (hlc optional)
API->>API: validate hlc (wall/logical >= 0) or rollback -> 400
API->>DB: insert_op_returning(..., hlc_pair)
DB->>DB: derive/validate hlc or use provided pair
DB->>Postgres: INSERT ... RETURNING hlc_wall, hlc_logical
Postgres-->>DB: row with hlc_wall/hlc_logical
DB-->>API: inserted row
API-->>Client: 200 or 409(HlcRegression) / 400
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
type: feat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
272-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNe mappez pas tous les
23505à une régression Lamport.Depuis l’ajout de l’unicité sur
(user_id, device_id, hlc_wall, hlc_logical), cette branche couvre aussi les collisions HLC v2. Dans ce cas, répondrelamport_regressionavecstored_maxenvoie un signal de reprise faux au client. Il faut distinguer au moins la contrainte HLC viadb_err.constraint()et ne construirestored_maxque pour la vraie contrainte Lamport.💡 Direction de correction
- Err(sqlx::Error::Database(db_err)) if db_err.code().as_deref() == Some("23505") => { + Err(sqlx::Error::Database(db_err)) if db_err.code().as_deref() == Some("23505") => { + if db_err.constraint() == Some("sync_op_user_device_hlc_uniq") { + tx.rollback().await.ok(); + return (StatusCode::CONFLICT, "hlc_conflict").into_response(); + } // chemin lamport_regression existantAs per coding guidelines,
src/api/sync.rs: "Return 409 +stored_maxwhen a sync operation arrives with a lamport timestamp less than the last stored value (lamport regression)".🤖 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 272 - 299, The current match arm treats every SQL 23505 as a lamport regression; update it to inspect db_err.constraint() (via db_err.constraint().as_deref()) and only run the lamport-specific path (call db::sync::lamport_max(pool, user_id, device_id) and return the LamportRegression JSON with stored_max and offending_lamport_ts) when the constraint name matches the lamport unique index; for other constraint names (e.g. the HLC v2 constraint) return an appropriate 409 or other error response without computing stored_max (or delegate to the existing ON CONFLICT behavior), ensuring you still rollback the transaction as currently done and keep using the same symbols: db_err.constraint(), lamport_max, LamportRegression, and the 23505 match.Source: Coding guidelines
🤖 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/db.rs`:
- Around line 133-143: Dans le match qui construit (hlc_wall, hlc_logical) à
partir de hlc (le Some((wall, logical)) branch), ajoute une validation
symétrique à celle du chemin None : vérifier que logical >= 0 et que lamport_ts
est dans 0..=i64::from(i32::MAX) (ou que logical tient dans i32 si c'est
l'intention), et rejeter avec Err(sqlx::Error::Protocol(...)) le même style de
message si ces contraintes ne sont pas respectées; concrètement, dans le
Some((wall, logical)) arm (et toujours avant de retourner (wall, logical)), si
logical < 0 ou lamport_ts hors de l'intervalle, retourner l'erreur (de la même
forme que le cas None) afin d'empêcher la persistance de valeurs d'ordre
invalides et d'aligner le comportement v2 sur le chemin legacy.
---
Outside diff comments:
In `@src/api/sync.rs`:
- Around line 272-299: The current match arm treats every SQL 23505 as a lamport
regression; update it to inspect db_err.constraint() (via
db_err.constraint().as_deref()) and only run the lamport-specific path (call
db::sync::lamport_max(pool, user_id, device_id) and return the LamportRegression
JSON with stored_max and offending_lamport_ts) when the constraint name matches
the lamport unique index; for other constraint names (e.g. the HLC v2
constraint) return an appropriate 409 or other error response without computing
stored_max (or delegate to the existing ON CONFLICT behavior), ensuring you
still rollback the transaction as currently done and keep using the same
symbols: db_err.constraint(), lamport_max, LamportRegression, and the 23505
match.
🪄 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: e3673396-7db4-4946-bcfb-8ad9814f7c07
📒 Files selected for processing (4)
src/api/sync.rssrc/db.rssrc/sync.rstests/sync.rs
Phase A.2.1 of the RFC-003 sync v2 rollout. The /api/v1/sync/ops push
endpoint now accepts an optional v2 wire-shape `hlc: { wall, logical }`
field on each op; v1 clients keep working unchanged. Server prefers v2
when present; otherwise it derives the HLC pair from `lamport_ts` the
same way the A.1.1 backfill did (wall = 0, logical = lamport_ts).
The §2 total-order tiebreaker `origin_device_id` rides through the
existing `sync_op.device_id` TEXT column per A.1.1's design — no new
column, no separate wire field. V2 desktops format their device_id
as a UUID string; v1 desktops keep their free-form string. The apply
pipeline (A.2.2, follow-up PR) will parse `device_id` as UUID when
writing entity tables' UUID `origin_device_id` column.
Wire shape change is purely additive:
- SyncOpIn gains optional `hlc: Hlc { wall: i64, logical: i32 }`.
- SyncOp echo gains non-optional `hlc: Hlc` (server stamps the derived
pair on v1-shape rows so every read carries a usable total-order
tuple).
- insert_op_returning takes optional `hlc: Option<(i64, i32)>`; binds
the pair verbatim when Some, derives from lamport_ts when None.
- pull_ops_since + fetch_op_by_operation_id SELECTs now project
hlc_wall + hlc_logical so row_to_op can echo them.
- Push validates `hlc.wall >= 0` at the API boundary; negative wall
⇒ 400. Logical is i32 by the type, so no further narrowing check
needed on the v2 path (the v1 path's lamport_ts narrowing guard
stays in place).
Tests: 3 new round-trip cases in tests/sync.rs covering v1-only push
(derived hlc), v2 push (verbatim echo), and the negative-wall reject
boundary. Full suite green (175 tests).
What this does NOT do:
- Apply pipeline propagation onto entity tables — A.2.2 follow-up.
The new hlc columns stay default-zero on entity rows for now.
- payload_hash computation + metadata_digest_version bump — A.2.2.
- User-scoped digest routing for liked_track / track_rating — A.2.3.
Refs RFC-003 §2 (total order), §Migration plan / Phase A.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Review fixes on #52: 1. db.rs: the v2 path's Some((wall, logical)) arm now mirrors the v1 path's range guard — rejects negative wall OR negative logical with the same Protocol error shape. The API boundary already rejects wall < 0, but the helper is shared so a future caller bypassing the handler still gets a typed error instead of a row that breaks the §2 total-order invariant. Logical is i32 by the type so a negative is structurally legal but semantically wrong per RFC-003 §2 (unsigned-shaped counter). 2. api/sync.rs: 23505 path now inspects db_err.constraint() to discriminate the legacy lamport UNIQUE from the A.1.1 sync_op_user_device_hlc_uniq. A v2 HLC collision was getting reported as a misleading lamport_regression — the stored lamport_max is meaningless to a v2 client whose clock is the HLC pair, not lamport_ts. New HlcRegression body echoes the offending pair so the v2 client can resync its HLC instead of guessing. Test: push_v2_duplicate_hlc_returns_hlc_regression exercises the discrimination — different operation_id + advanced lamport_ts + same HLC pair lands on the HLC UNIQUE, not the lamport one. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
force-pushed
the
feat/sync-v2-phase-a-2-dual-shape-ingest
branch
from
June 13, 2026 21:29
1306d43 to
efccc3b
Compare
github-actions
Bot
added
type: feat
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/sync.rs`:
- Around line 217-222: La validation actuelle n'inspecte que hlc.wall; étendre
la vérification dans le bloc if let Some(hlc) = op_in.hlc pour aussi tester
hlc.logical < 0, appeler tx.rollback().await.ok(); et retourner
(StatusCode::BAD_REQUEST, "hlc.logical must be >= 0").into_response() (même
pattern que pour hlc.wall) afin de renvoyer 400 au lieu d'une erreur DB/500.
In `@tests/sync.rs`:
- Around line 841-866: Add a new test mirroring push_v2_negative_wall_rejected
that verifies negative hlc.logical is rejected: create an async sqlx::test named
push_v2_negative_logical_rejected using spawn_authenticated (e.g.
"hlc-neg-logical"), send the same POST to /api/v1/sync/ops via reqwest with an
ops entry that sets hlc: { "wall": 1_700_000_000_000_i64, "logical": -1_i32 },
and assert the response status is StatusCode::BAD_REQUEST; keep the same
structure/helpers (Uuid::new_v4(), lamport_ts, entity, field, payload) as in
push_v2_negative_wall_rejected.
🪄 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: 244f9bdd-b8d3-4afe-9620-ea708cc51ad4
📒 Files selected for processing (4)
src/api/sync.rssrc/db.rssrc/sync.rstests/sync.rs
Review fix on #52: the api/sync.rs push handler validated `hlc.wall < 0` but not `hlc.logical < 0` — a v2 client sending a negative logical would fall through to the db helper's defence-in- depth `Protocol` error and surface as 500. Symmetric `logical >= 0` check returns 400 the way `wall >= 0` already does. New test `push_v2_negative_logical_rejected` mirrors the existing `push_v2_negative_wall_rejected` shape. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
There was a problem hiding this comment.
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)
154-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation OpenAPI incomplète :
HlcRegressionmanque sur le 409.La route peut désormais renvoyer
HlcRegression(collision HLC) ouLamportRegression(régression lamport) sur un 409. L'annotationutoipane documente queLamportRegression, ce qui rend le contrat API imprécis pour les clients v2.🔧 Correctif proposé
(status = 400, description = "Empty `device_id`, oversized batch, or malformed op"), (status = 401, description = "Missing or invalid bearer token"), - (status = 409, description = "Lamport regression — stored max returned in body", body = LamportRegression), + (status = 409, description = "Lamport or HLC regression — body is LamportRegression or HlcRegression depending on the violated constraint"), (status = 500, description = "Database or internal failure"),Note :
utoipane supporte pas nativementoneOfpour les réponses, mais le texte de description clarifie au moins le contrat pour les consommateurs de l'API.🤖 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 154 - 159, La documentation Utoipa pour la route de push (l'attribut responses dans src/api/sync.rs) n'indique que LamportRegression pour le 409 alors que l'endpoint peut aussi renvoyer HlcRegression; modifiez le bloc responses (référence: PushBatchResponse / LamportRegression / HlcRegression) pour clarifier le 409 en mentionnant les deux cas (par ex. description = "Lamport regression or HLC collision — response body is LamportRegression or HlcRegression") ou, si possible, ajouter HlcRegression comme body alternatif; conservez la signature actuelle mais assurez-vous que la description explicite les deux types de réponse pour que le contrat API soit correct.
🤖 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.
Outside diff comments:
In `@src/api/sync.rs`:
- Around line 154-159: La documentation Utoipa pour la route de push (l'attribut
responses dans src/api/sync.rs) n'indique que LamportRegression pour le 409
alors que l'endpoint peut aussi renvoyer HlcRegression; modifiez le bloc
responses (référence: PushBatchResponse / LamportRegression / HlcRegression)
pour clarifier le 409 en mentionnant les deux cas (par ex. description =
"Lamport regression or HLC collision — response body is LamportRegression or
HlcRegression") ou, si possible, ajouter HlcRegression comme body alternatif;
conservez la signature actuelle mais assurez-vous que la description explicite
les deux types de réponse pour que le contrat API soit correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 49602af4-eae5-4190-bfd1-2b3f1c876a95
📒 Files selected for processing (2)
src/api/sync.rstests/sync.rs
Review fix on #52: the utoipa response block on POST /api/v1/sync/ops listed `LamportRegression` as the only 409 body, but after the HlcRegression addition the endpoint can return either shape discriminated on the `error` field. utoipa 5 has no concise `oneOf` for response bodies, so we list both shapes as sibling 409 entries and document the discriminating `error` value in each description. Also clarifies the 400 description to mention the new hlc.wall / hlc.logical < 0 rejection paths. No code-level behaviour change. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
type: feat
InstaZDLL
added a commit
that referenced
this pull request
Jun 14, 2026
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>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase A.2.1 of the RFC-003 sync v2 rollout. The
POST /api/v1/sync/opsendpoint accepts an optional v2hlc: { wall, logical }field on each op while keeping v1 clients working unchanged. The server prefers v2 when present; otherwise it derives the HLC pair fromlamport_tsexactly the way the A.1.1 backfill did.Follows #50 (A.1.1
sync_opHLC columns) + #51 (A.1.2 entity tables HLC columns +metadata_digest_version).Wire shape
Purely additive. v1 client payload still works verbatim:
{ "operation_id": "…", "lamport_ts": 1, "entity": "playlist", "entity_id": "pl-1", "op": "set", "field": "name", "payload": { "value": "Soirée" } }v2 client adds
hlc:{ "operation_id": "…", "lamport_ts": 1, "entity": "playlist", "entity_id": "pl-1", "op": "set", "field": "name", "payload": { "value": "Live" }, "hlc": { "wall": 1700000000000, "logical": 7 } }The §2 total-order tiebreaker
origin_device_idrides through the existingsync_op.device_idTEXT column per A.1.1's design — UUID-shaped TEXT round-trips losslessly, no new column needed at the sync_op layer. v2 desktops format theirdevice_idas a UUID string; v1 desktops keep their free-form one. The apply pipeline (A.2.2 follow-up) will parsedevice_idas UUID when writing entity tables' UUIDorigin_device_idcolumn.Code change
src/sync.rs— newHlc { wall: i64, logical: i32 }type;SyncOpIngains optionalhlc;SyncOpecho gains non-optionalhlc(server stamps the derived pair on v1-shape rows so every read carries a usable total-order tuple).src/db.rs::insert_op_returning— takes optionalhlc: Option<(i64, i32)>; binds the pair verbatim whenSome, derives(0, lamport_ts)whenNone. v1 path keeps the existinglamport_tsrange check + narrowing.src/db.rs—pull_ops_since+fetch_op_by_operation_idSELECTs now projecthlc_wall+hlc_logical.src/api/sync.rs::push_ops— validateshlc.wall >= 0at the API boundary (400 on negative wall);row_to_opreads + echoes the pair.What this does NOT do
profile/library/track/playlist/ etc. stay default-zero for now.payload_hashcomputation +metadata_digest_versionbump — A.2.2.liked_track/track_rating— A.2.3.Test plan
cargo check --all-targets --all-features— cleancargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all-features— full suite passes (175 tests)tests/sync.rs:push_v1_only_pulls_derived_hlc— push withouthlc, pull echoes{ wall: 0, logical: lamport_ts }push_v2_hlc_round_trips_verbatim— push withhlc, pull echoes verbatimpush_v2_negative_wall_rejected— push withhlc.wall = -1⇒ 400Refs
Summary by CodeRabbit
New Features
Bug Fixes / Comportement
Tests