feat(remote): remote music source and user-data sync v2 (RFC-005) - #502
Draft
InstaZDLL wants to merge 14 commits into
Draft
feat(remote): remote music source and user-data sync v2 (RFC-005)#502InstaZDLL wants to merge 14 commits into
InstaZDLL wants to merge 14 commits into
Conversation
The desktop talks a protocol the server no longer implements. RFC-005 records the replacement and, more importantly, the decision that was written nowhere: synchronized state describes the server's playlists, favourites and ratings, which reference the server's tracks. Those have no local counterpart, and matching one is deliberately out of scope. Writing that projection into the local tables would leave two options, both wrong -- fabricate local rows for content that only exists on the server, or silently drop every entry. The first corrupts the library, the second makes a broken sync report success. So the projection gets its own reconstructible tables, and local playlists stop travelling between machines: a real capability lost, and one that cannot come back without the matching layer. Numbered 005 on purpose. Two documents were numbered RFC-003, one per repository, describing unrelated designs; the desktop's is now marked superseded and carries the warning in its header. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Lands the projection schema and the HTTP surface RFC-005 needs, behind a new off-by-default sync_v2 feature. Independent of crate::sync, which stays behind sync_v1: v2 does not synchronize local-entity CRUD, so the ~70 emit call sites keep resolving to the stub and are untouched. The v1 digest and backfill paths stay compilable on purpose -- until the snapshot bootstrap is proven they are the only way back from a divergence. The migration was replayed against a real database with foreign keys on, not a fixture: the CHECK constraints are half of what the binding relies on. Two of them are deliberately asymmetric. The favourite entity type carries no CHECK, because pinning today's list would turn a forward-compatible server change into an aborted transaction -- and an event the client cannot apply forces a fresh snapshot, whose replay would abort on the same row, looping forever. The rating range does carry one, because the apply path clamps before binding, so it can only fire on our own bug. Two traps paid for along the way. The v2 access tokens are opaque (wfa_/wfr_), so v1's "three dot-separated segments" check would reject every one of them. And refreshing held the profile pool lease across a 30-second request; it now captures the profile id and re-acquires against it, losing a rotated pair rather than writing one account's credentials into another profile's database. CI gained a job for the feature. Without it not one line of this tree would be compiled, and it would rot silently until the day it is switched on. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Binds a profile to a native server: probe the URL, run the handshake in the system browser, exchange the code, and store the session. Adds the five commands the Settings card will need and their typed wrappers. No UI is mounted yet -- the feature is off by default, and a card that cannot work in a stock build is worse than no card. Driven end to end against a live server rather than inferred from the spec, which corrected two things. The redirect URI is compared as a string at redemption, so changing only the port answers 401 even though shape validation ignores it; the listener therefore binds first and the URI is built once from the port obtained, making the two identical by construction. And the device name is capped at 120 bytes server-side, so a long hostname is truncated on a character boundary rather than failing the sign-in for a reason no user could guess. The same run confirmed the traps worth keeping in the docs: a wrong verifier burns the code, and presenting the correct one afterwards still fails; the refresh token rotates while the device survives it. Signing out and forgetting a server are deliberately different. Signing out drops the credentials only, so the cached library stays readable and a later sign-in resumes from its cursor. Forgetting drops the binding, the projection and any write that never left the machine -- and binding to a different account does the same, because a cursor is a position in that account's journal and a queued mutation names that account's resources. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Adds the read half of the protocol: replace the projection from GET /sync/snapshot, then walk GET /sync/changes page by page, applying and advancing the cursor in one transaction so a crash keeps both or neither. An event this build does not understand is skipped and the cursor keeps moving; a known event that fails to apply discards the projection and takes a fresh snapshot, because at that point local state no longer reflects the journal. The acknowledgement is fire-and-forget -- the protocol says it is not a prerequisite for reading, so a failing one must not stall anything. The rule the apply path is built around, and the reason it writes field by field rather than decoding into a struct of options: an absent key means unchanged, not null. Measured against a live server, creating a playlist emits [id, name, track_ids] while updating one emits five keys, and a share update omits track_ids entirely. Writing every field would blank a comment on every create-shaped replay and empty a share on every description edit. Two tests replay the server's own captured journal and assert it converges on the server's own snapshot. Those fixtures are bytes the server produced, not a shape we imagined, which is the only kind that can catch us having imagined the wrong one. remote_track is new because the feeds are asymmetric: a snapshot carries whole song objects, a change event carries bare identifiers. Missing ones are fetched afterwards, opportunistically -- ordering and identity are already stored, so a failed fetch costs a placeholder row rather than a wrong one. Ratings clamp instead of rejecting. An out-of-range value would abort the transaction, which forces a fresh snapshot, whose replay hits the same row -- an unbreakable loop over one bad field. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Subscribes to GET /sync/socket and asks the journal on every notice. The frame's cursor is deliberately not compared against ours before fetching: it is a hint, an empty page is cheap, and trusting the hint enough to skip the fetch would lose an event to a stale or reordered frame. Reconnecting triggers the same fetch, since whatever happened while we were away arrived through no frame at all. That makes the whole module an optimisation rather than a mechanism. Removed, a periodic pass still converges; what it buys is that an edit made on another device shows up in seconds. The UI event only fires when something actually changed, so an idle account does not re-render the library on a timer. Plain ws:// to a non-loopback host warns once per session -- self-hosted LAN is legitimate, but the access token rides in the upgrade headers. The rustls provider is installed here too rather than leaning on the v1 block: the two features are independent, and a sync_v2-only build would otherwise panic on its first wss:// upgrade. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
The write half. Each queued entry is a business call the drain re-issues verbatim under its operation identifier, so a lost response can be sent again without applying the change twice. Order is the constraint that shapes it. Strict FIFO, and the first retryable failure ends the pass so nothing overtakes what it depends on. A permanent failure is the exception -- it can never succeed, so that entry is marked and the drain continues rather than deadlocking every later change behind one bad request. A playlist created offline has no server identifier, so it gets a local: placeholder the projection uses as a real key, and it is visible and editable immediately. Resolving it taught two things the tests now pin. The row has to be copied, re-pointed and dropped rather than renamed: a primary key cannot be updated out from under the rows referencing it, so renaming first orphans the tracks and re-pointing first names a parent that does not exist. That was a real foreign-key failure, caught only because the test database has the constraint on. And a snapshot must not delete a placeholder playlist -- the server has never heard of it, so its snapshot is no evidence that it was deleted, and wiping it would destroy something the user made offline. Rewriting a queued payload is the one exception to entry immutability and it is sound for a narrow reason: FIFO guarantees an entry sitting behind an unlanded creation has never been presented, so there is no fingerprint for it to conflict with. Clearing a playlist comment is not expressible. The server coalesces a null onto the current value, so "leave it" and "empty it" are the same request; the mutation type says so instead of pretending otherwise. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Create, update and delete through the same replayable queue as everything else. Replaying a creation needs no special handling: the share token is derived deterministically from the identifier, so re-sending returns the same URL rather than minting a second share. Two things specific to shares. Their URL has exactly one moment -- the journal never carries it and it cannot be derived locally, since the token is keyed on a server-side secret. The creation response is the only time this device can learn the link, so a share created on another device stays link-less here permanently, and capturing it at creation is the only chance rather than an optimisation. And a snapshot must spare a placeholder share for the same reason it spares a placeholder playlist: the server has never heard of it. That fix also removed a duplicate unconditional delete that would have undone the preservation two lines later. Clearing an optional field remains inexpressible, and it is worse here than for playlists: expires_at is coalesced too, so a share given an expiry by mistake cannot have it removed through this API. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Re-captured the fixtures against a server seeded with six actual tracks and driven through every entity the protocol carries: two favourites (a track and an album), two ratings, two scrobbles, a saved queue, a playlist created then updated, and a share. The previous capture only had empty playlists, so favourites, ratings, scrobbles and the queue had never been observed -- only read out of the server's source. All ten event shapes match what was implemented. The convergence test now compares nine projections rather than one: it walks the journal, snapshots the same database, and asserts the two produce identical state. If they ever diverge, what a user sees would depend on whether they bootstrapped or caught up, which is undebuggable from a screenshot. Two things learned from the server side and applied. A 422 on an acknowledgement confounds an out-of-range cursor with an unknown or revoked device, and the second is both likelier and worse -- a revoked device fails every mutation too -- so the log now names it first. And the scrobble trade-off is documented precisely: plays are distinguishable in the journal but not in a snapshot, so walking the journal is the more faithful history, and the natural key is taken only because it is the one both feeds carry. The retention branch stays unwritten. Measured: a cursor beyond the last event returns an empty page, not an error, and there is no compaction contract yet -- so coding against an imagined status would be worse than leaving the path absent. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Three contract changes landed server-side and are implemented here, each verified against a rebuilt server rather than read from the source. Conflicts moved from 422 to 409 with code "conflict", which matters less for the verdict -- both were already permanent -- than for what shares that status. A read can answer 409 with code "cursor_expired", meaning the journal no longer reaches our position, and the two demand opposite reactions: an expired cursor treated as a conflict abandons a write that would have succeeded, a conflict treated as an expired cursor throws away a healthy projection. So the failure type now keeps the server's code as a structured field, and the catch-up branches on the code, never on the status. Folding it into the message would have made a destructive decision rest on a substring match. Clearing an optional field is now expressible, which closes a real dead end: a share given an expiry by mistake could not have it removed, and the owner's only recourse was deleting the share and publishing a different URL. An unrecognized field name is refused rather than ignored -- clear=["expiresAt"] answers 422, measured -- so the three accepted names are spelled in one place instead of inline at each call site. Setting a field and clearing it are distinct mutations that cannot share a replay identifier; the queue satisfies that already, since every enqueue draws a fresh one. The recovery branch is written even though it cannot fire: the journal is append-only, so a cursor beyond the last event still returns an empty page. It is written against a tested contract rather than an imagined status, which is the difference from the previous position. The queued payload flags default, so rows written before clearing existed still deserialize and mean "clear nothing". Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Commands and typed wrappers over the local tables: a counted overview, the playlists, one playlist's tracks, the saved queue. Pure queries -- they answer instantly and answer the same whether or not the server is reachable, which is the point of keeping a projection at all. Three decisions that shape what a user will see. A track whose metadata has not been fetched yet keeps its position and comes back with a null title, because an inner join would drop it and renumber everything after it, making a playlist look shorter than it is while a backfill is merely outstanding. A playlist summary sums only the durations it knows, so it understates rather than refusing to show a number. And a playlist still carrying a local placeholder is flagged, because it is the one that exists nowhere else and would vanish if the user forgot this server. The overview counts pending and permanently-failed changes apart. They mean different things: one is "not yet", the other is "never, and nobody will say so unless we do". No profile active answers empty rather than failing -- onboarding queries this before one exists. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
…human Mounts a Settings panel that identifies a server, signs in through the browser, synchronizes on demand and shows what the projection holds. This is the one thing unit tests cannot do for this feature: watch it run. Two decisions worth stating. The card is not localized, unlike every other string in the app -- the sync_v2 feature is off by default, so no shipped build can render it, and translating copy nobody can reach and that will still change would put a couple of hundred unreviewed strings into files that had a native-speaker pass. The keys land when the feature ships and the wording settles. And it hides itself rather than trusting a build flag TypeScript cannot see: it probes for its own backend command and renders nothing when the command is not registered, which is every build today. Forgetting a server is styled apart from signing out, because the two are not the same act -- one drops credentials, the other discards changes that never left the machine. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
The server now publishes full_hash on its tracks: BLAKE3, non-keyed, hex, over the whole file, pinned as part of its contract. The projection stores it and nothing reads it, which is deliberate -- matching a local file to a server track is out of scope -- but the value arrives free with every snapshot, and the alternative is re-downloading the catalogue on the day matching is designed. Checking whether it lines up with what we already have turned up the thing worth recording. It does not match track.file_hash, and the names invite exactly that assumption. The local digest is blake3( file_length_le_bytes || head_1MiB || tail_1MiB ) above 2 MiB, and length-prefixed whole content below it. The prefix alone means it can never equal a plain full-file digest at any size. That partial form is deliberate: full hashing was the scan's dominant cost, reading roughly 9 GB for 900 tracks. The comparable local value is hash_file_full, a plain whole-file BLAKE3 in hex -- equal to the server's on identical bytes. It is computed today only to confirm duplicates before a destructive delete, and stored nowhere. So matching on content means reading local files in full, which is the cost the scan exists to avoid. Both sides already expose size, which narrows candidates before anything is read. Recorded in the RFC as input to the matching design rather than as a design, since that remains someone else's RFC. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Closes the gap that has been open since the queue landed: nothing called enqueue. Starring, rating, and creating, renaming or deleting a playlist now each write the projection and queue the mutation in one transaction, and the command layer kicks the drain without making the caller wait -- the change is already durable, so there is nothing for the user to watch. The pairing is the point, and it is now tested rather than asserted in a comment. Splitting it fails in two directions: a projection updated without a queued mutation silently discards the change the next time a snapshot lands, and a mutation queued without the projection update makes the interface lie backwards, showing the old value while the new one is already travelling. One test rolls a transaction back and checks that neither half survives; another sweeps every gesture and counts both sides, so a future gesture that forgets one of them fails there. Each gesture is split into an in-tx half and a wrapper that owns the transaction and the profile lease. That is what makes it testable at all, since an AppState cannot be built in a unit test -- and it moved the rating range check somewhere every caller inherits it. Two behaviours worth naming. Deleting a playlist that never reached the server drops its queued creation instead of sending a create and a delete for something that existed for nobody. And asking to both set and clear a comment resolves to the clear: it is the more explicit intent, and sending the value alongside would let the server coalesce it straight back in. The diagnostics card grew a create field and per-playlist delete, so the whole loop can be watched: a playlist appears at once marked as unsent, travels on the next drain, and returns named by the server. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
…ares Shares now have commands, and so do scrobbles and the play queue. Every gesture keeps the same shape as the others: the projection write and the queued mutation commit together, and the drain is kicked without making the caller wait. What is deliberately absent is the wiring to playback. The local queue holds file paths and local row ids, and the scrobbler joins the local track table; the server validates identifiers and would reject every one of them. Hooking the player today would not ship an incomplete feature, it would ship a defect -- a steady stream of permanently-failed mutations piling up in a queue the user then has to be told about. The dependency is remote playback, which does not exist yet; until it does, these are the surface for a caller that already holds server identifiers. Two behaviours the tests pin. A "now playing" ping travels but does not enter the local history, because it is a transient state rather than a play and writing it would show a listen that never happened. And a share created offline is visible at once but carries no link: the token comes from a server-side secret, so it cannot exist before the creation lands. The sweep that checks every gesture writes both halves now covers all seven, so anything added later that forgets one of them fails there. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
InstaZDLL
added
scope: frontend
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.
Summary
remote_*tables rather than inplaylist/liked_track/track.rating. Writing it there would leave two options, both wrong — fabricate local rows for content that only exists on the server, or silently drop every entry.sync_v2feature, independent ofsync_v1. Nothing ships.Why it is safe to land while unfinished
sync_v1is not indefault, so the ~12 400 lines of v1 are already absent from every shipped binary, andsync_stub.rskeeps the ~70 CRUD emit call sites compiling. v2 is a separate tree (crate::remote) that touches none of them — correctly, since local-entity CRUD is no longer synchronized at all.digest/andbackfill/are deliberately not deleted. They become pointless, but until the snapshot bootstrap has been proven against a real deployment they are the only way back from a divergence. Removing them is the one slice left, and it waits on the manual test below.CI gained a job for the feature. Without it, not one line of this tree would be compiled and it would rot silently until the day it is switched on.
How I tested
Almost every real defect here came from something read rather than measured, so the protocol was driven against a live server throughout.
End to end, against a seeded instance (
waveflow-server/scripts/seed-dev-instance.sh, six real tracks):/oauth/authorizewith Bearer →/oauth/token), including the failure branches.409 conflict,clear: ["expires_at"]removes an expiry,clear: ["expiresAt"]answers422.Corrected by measuring rather than reading:
redirect_uriis compared byte for byte at redemption — changing only the port answers 401, even though shape validation ignores it. The listener now binds first and the URI is built once.device_nameis capped at 120 bytes server-side, so a long hostname failed sign-in for a reason no user could guess.[id, name, track_ids], updating one emits five keys. An apply that wrote every field would blank a comment on every create-shaped replay.Found by a test, not by review: resolving a local placeholder violated a foreign key — a primary key cannot be updated out from under the rows referencing it. Caught only because the test database has
foreign_keyson, which is now pinned by an assertion.107 tests on the module, plus
bun run lint/typecheck/buildandcargo check --workspace --all-targetsin both feature configurations.What is left, and why
The manual pass I cannot do. Settings → "Remote server": point it at a seeded server, Identify should answer
waveflow, sign in through the browser, Sync now fills the counters; create a playlist and watch it appear marked(not sent yet)then lose the mark after the drain; cut the network, create one, watch it wait, restore, watch it go. Those four confirm the bootstrap and unblock deleting v1.Two absences that are decisions, not oversights:
tracktable; the server validates identifiers and would reject all of them, so every such mutation would fail permanently and pile up in the queue. Wiring it today would ship a defect, not an incomplete feature. The missing dependency is remote playback, which nothing implements.Notes for the reviewer
409carries two opposite meanings —conflictandcursor_expired— so the client keeps the server's code as a structured field and never decides from the status. Confusing them abandons a write or discards a healthy projection.full_hashis captured and read by nothing. It does not match the localtrack.file_hash, which is a length-prefixed head+tail digest — a note for whoever designs the matching layer is in the RFC.