fix(sync): server-authoritative device initialized state (stop already-synced devices re-syncing)#13
Merged
Conversation
…s already-synced devices asking to re-sync) Root cause of the regression: 'does this device need an initial transfer?' was decided client-side. The PR #12 backfill read callbacks.getNotes() during startSync, but App runs initSync() right after loadFromVault() before React re-renders, so notesRef.current was the stale empty array -> every already-synced device was marked 'needs transfer' permanently, so established devices prompted each other and the bogus transfer hung. Move the decision to authoritative server state: add devices.initialized (existing rows default to 1 via guarded ALTER, so already-paired devices are instantly correct). Founder (no other devices) registers initialized=1; a joiner registers 0 and is marked initialized on transfer-complete. isNewDevice is computed from the DB. The client deletes its whole flag layer and just trusts welcome.needsTransfer. Guards kept/added: a device needing a transfer never shows an approval prompt, never approves its own request, and never requests from itself (client+server); approving dismisses the prompt immediately.
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
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.
Regression
Two already-synced devices started asking each other to "allow sync," and approving did nothing (the prompt hung).
Why it kept regressing — the real root cause
"Does this device need an initial transfer?" was being decided on the client — across the last several fixes it was inferred from a localStorage flag, note count, or the server cursor. All of those are unreliable:
migrateInitFlagIfNeededbackfill (PR fix(sync): the requesting device must never show an approval prompt (+ backfill legacy initialized flag) #12) readcallbacks.getNotes()duringstartSync, butAppcallsinitSync()immediately afterawait loadFromVault()— before React re-renders — sonotesRef.currentwas still the stale empty array. Every already-synced device therefore read "0 notes," was marked "needs transfer," and the marker made it permanent. Hence established devices demanded a re-sync, prompted each other, and the bogus transfer hung.Client state races with note-loading, gets wiped on logout, and differs across app versions. It is the wrong place for this decision.
The fix — make it server-authoritative
The server already persists a
devicesrow per device. Add aninitializedcolumn that is the single source of truth:initialized INTEGER NOT NULL DEFAULT 1, plus a guardedALTER TABLEso existing rows default to1. Every already-paired device is therefore instantly correct and can never be asked to re-bootstrap — this fixes the live regression on deploy with zero user action.initialized=1) only when no other device exists yet; a device joining an existing chain registersinitialized=0and must receive a transfer. An existing row keeps its earned state on reconnect.isNewDevice = !device.initialized && otherDevices.length > 0— computed from the DB, not a client hint.transfer-complete: the server marks the requesterinitialized=1, so it rejoins as a full member and never asks again.The client now simply trusts the server's
welcome.needsTransfer. All the client-side flag machinery (SYNC_INIT_ROOM_KEY, the migration,needsInitialTransfer, etc.) is deleted (net −42 lines). Defensive guards are kept/added: a device that still needs a transfer never shows an approval prompt, never approves its own request, and never requests from itself (client + server); approving now dismisses the prompt immediately so it can't hang.Files changed
server/src/db.ts—initializedcolumn + migration;registerDevice(…, initializedIfNew);markDeviceInitialized;DeviceRecord.initialized.server/src/index.ts— role from DB; mark initialized on transfer-complete.src/utils/sync.ts— trust serverneedsTransfer; delete client flag layer; dismiss prompt on approve.src/components/SettingsPanel.tsx— revert to plainenableSync.Scenario audit (20)
Legitimate:
initialized=1(column default) → no spurious prompts. ✓ (the reported regression)transfer-target-offline→ waits; source returns → auto-request + fix(sync): show device-approval prompt after reconnect (replay pending transfers) #8 replay → prompt. ✓initialized→ re-requests on reconnect. ✓initialized=1→ Connected, doesn't re-ask. ✓ (fixes "Connecting again")initialized→ Connected. ✓initialized=0→ requests. ✓Adversarial:
15. Join without the code → no valid HMAC join token → rejected. ✓
16. Forged/expired token → HMAC/expiry check fails → rejected. ✓
17. Flood generate/validate/join → rate-limited (per-IP windows + WS join cap). ✓
18. Client lies about its role on join → server ignores client hints entirely (authoritative DB). ✓ (hardened by this PR)
19. Request a transfer from yourself → server rejects + client guard. ✓ (hardened)
20. A code-holder reading notes / approving for themselves → the sync code is the documented sole auth and the E2E key; approval is a UX gate, not a security boundary, so no new exposure. ✓
Residual hardening (pre-existing, noted, not addressed here to keep this focused): no per-sender rate-limit on
request-transfer-from(prompt spam by an in-chain device), and a malicious in-chain client could ACK an inflated cursor to truncate ops early. Both require already knowing the sync code. Happy to follow up.Verification
notesRef→ false "needs transfer") is eliminated because the decision no longer touches the client at all.tsc/build/lint — sandbox has no installed toolchain for either workspace and installs are blocked (registry 403). Changes use only existing in-scope symbols; SQL column counts andrun()arities checked by hand.Manual test plan