Detect upstream changes before saving - #4
Conversation
Troche is often open on more than one machine. Saving was already per song and diff-based, so idle tabs never wrote and songs created elsewhere were never trashed — but a song edited in two places was overwritten by whichever tab saved last, silently, with wp-admin revisions as the only trace. Give the server a per-song version token (a hash of the stored JSON, so an identical re-save doesn't move it) and expose it from a new GET /library/state, which carries tokens only and no content. Clients never compute a token, only compare, so the hash is free to change shape later. The app checks that endpoint when a tab becomes visible — the moment a laptop is most likely stale — and again before each save, closing the debounce window. No background polling. Per-song granularity means most upstream activity isn't a conflict and doesn't need the user: a song changed elsewhere that this tab hasn't touched is adopted, one added elsewhere is pulled in, one trashed elsewhere is dropped. Only a song edited in both places, or edited here and trashed there, surfaces — as a notice naming the song with a choice, since a library-wide "reload" would discard local edits. Those songs are held back from saving until resolved; the rest of the library keeps autosaving. Also fixes an update to a song trashed elsewhere reporting itself as "Offline — changes kept locally". The 404 is now distinguished from a network failure and routed into the same reconcile.
|
Read the diff, ran 1. A re-conflicted song keeps a stale 2. Resolving a conflict leaves the offline buffer holding the discarded copy. ( 3. The core race is still open. The server accepts any PUT unconditionally, so two tabs saving inside the same window still clobber each other silently; the sync-before-save narrows the window but doesn't close it. Worth closing here since the client plumbing already exists: send the expected token with the PUT, 409 on mismatch, and route the 409 into the same reconcile path the 404 already uses. Tests.
Minor: PHP side otherwise I'd take as-is. Addressing 1–3 plus the test gaps on this branch. |
Review follow-up on three points. Syncing before a save narrows the clobber window but can't close it: the check and the write aren't one operation, so another machine can land a save in between. PUT now carries the token the client last saw in an X-Troche-Expect-Token header, and the server refuses with 409 if the song has moved since — checked immediately before the write. A refusal is routed into the same reconcile the 404 already used, so a lost race surfaces as the same per-song question rather than an overwrite. A custom header rather than If-Match on purpose: an If-Match a proxy decides to evaluate itself would fail the save outright, whereas a stripped custom header degrades to the unconditional write this endpoint has always done. Fix a re-flagged song keeping the copy it was first flagged with. If the other machine edited again while the notice sat unresolved, the reducer kept the original entry and dropped the newer one, so "Use theirs" adopted a stale copy — which then no longer matched the snapshot and was pushed straight back over their newer version on the next save, silently. Incoming entries now win, keeping their position in the strip; unmentioned flags still survive, so a song's save hold is never stranded. Lifted out of the component as mergeFlags() so it's directly testable. Fix resolving a conflict in the server's favour leaving the offline buffer holding the discarded copy. "Use theirs" and "Delete here too" correctly don't dirty the library — local already matches the server — but the buffer is otherwise only written by a save, so an offline reload resurrected the rejected version. Tests 97 -> 134. Covers the conditional write end to end (refused, accepted, and absent-header back-compat), a re-conflict carrying the newer copy, the diverged result for both 404 and 409 — a headline fix of the parent commit that had no test — the flag reducer's four rules, offline and expired probes, and that /library/state agrees with /library about trashed and unparseable posts, which is what makes deletions propagate at all.
|
Addressed in b94d514. Conditional writes. Stale Buffer on resolve. Tests 97 → 134. The conditional write end to end (refused / accepted / absent-header back-compat), a re-conflict carrying the newer copy, One thing I did not change, worth knowing about. The Left alone as noted: the |
|
Second pass, against the head including b94d514 — so the three items from the earlier review are in. Confirmed all three land: The design holds up. The gap is that it's a three-way merge for content and a two-way merge for existence — 1. A locally deleted song is resurrected by any unrelated upstream change
for (const server of unclaimed.values()) {
merged.push(server); // added on the other machine
pulled++;
}It can't have been — we knew about that song at the last sync, or it wouldn't be in const server = makeServer({ 1: song("a", "Alpha"), 2: song("b", "Beta"), 3: song("c", "Gamma") });
const storage = await freshStorage();
const lib = await storage.loadLibrary();
// This tab deletes Gamma; still inside the autosave debounce.
const afterDelete = { ...lib, songs: lib.songs.filter((s) => s.id !== "c") };
// Meanwhile the other laptop edits an entirely unrelated song.
server.edit(1, { bpm: 200 });
const result = await storage.syncUpstream(afterDelete);
// pulled: 2, library: [Alpha, Beta, Gamma]Gamma is back, counted as a silent pull ("Updated 2 songs from another device"), and the delete never reaches the server. The 6s undo toast is long gone. It doesn't need the other machine to have touched that song — any upstream movement at all trips it, because that's what gates the full fetch. Shape of a fix. Guard the loop on 2. A held song is trashed anyway if it's deleted locally
// Beta flagged as a conflict and held, then deleted locally:
const saved = await storage.saveLibrary(afterDelete);
// calls: ['DELETE /songs/2'] → the other laptop's edit is trashed
3. Flags outlive their songs
Worth noting 2 and 3 are both the same underlying thing: the hold is a set floating beside the library rather than a property of a song, so every path that removes a song has to remember to maintain it, and none of them do. Making it a field on the song, or a map that the trash diff is also required to consult, makes both unrepresentable. 4. An unreadable PUT response silently disables the guaranteeNew in b94d514. // PUT succeeds; response body can't be parsed (keepalive teardown, proxy, etc.)
const saved = await storage.saveLibrary(edited); // ok: true, server has the edit
const result = await storage.syncUpstream(edited); // pulled: 1, conflicts: 0
// ...with nothing changed on any other machine
5. Disagreement: "the race is closed, not just narrowed"Closed at the protocol level — a client can't overwrite a version it hasn't seen — and that's the part that matters. Two caveats on the stronger reading, and on the comment at It's a read-compare-write, not a compare-and-set. The conditional covers Minor
On the open follow-upsIndependently re-derived #5 and the Items 1 and 2 undo user work silently; I'd fix those here. 3 is a few lines in the same neighbourhood, and 4 is a one-line change to a regression this branch introduced. The delete-path cases have no coverage in |
1 participant
Troche is often open on more than one machine. Saving was already per song and diff-based, so idle tabs never wrote and songs created elsewhere were never trashed — but a song edited in two places was overwritten by whichever tab saved last, silently, with wp-admin revisions as the only trace.
Approach
Each song gets a version token: a hash of its stored JSON, so an identical re-save doesn't move it (
post_modified_gmtwas the obvious alternative but its one-second resolution makes two saves in the same second indistinguishable). Tokens are computed from the post as actually stored, after WordPress's save filters, so a filtered save can't mint a token that instantly reads as a conflict. Clients never compute one, only compare, so the hash is free to change shape later.A new
GET /library/stateserves those tokens and nothing else — no song content, so the payload stays flat as the library grows. The app checks it when a tab becomes visible (the moment a laptop is most likely stale) and again before each save, which closes the debounce window. No background polling.Per-song granularity means most upstream activity isn't a conflict and doesn't need the user:
Only the last two surface, and they surface per song naming the song — a library-wide "reload" would discard local edits. Flagged songs are held back from writing until resolved; the rest of the library keeps autosaving, so a conflict on one song doesn't stall the others.
Also fixes an update to a song trashed elsewhere reporting itself as "Offline — changes kept locally". The 404 is now distinguished from a network failure and routed into the same reconcile.
Scope
No merging. "Keep mine" still overwrites the other machine's copy — the change is that it's a decision made with the song named, rather than something that happens silently, and the losing version stays in that song's revisions.
Testing
97 passing (
npm test). Newtests/sync.mjs(29 tests, wired in astest:sync) drivesstorage.jsagainst a fake in-process WordPress and covers each row above plus the cases most likely to break: a quiet server costs exactly one token request and no content; a held song is neither written nor trashed by the diff; a song created this session isn't duplicated by a sync landing before the app adopts its handle; a kept orphan is re-created rather than PUT to a dead post. Plugin harness 35 → 46.Also exercised by hand in Playground, staging a real two-machine scenario:
No console errors.