From 653e4aa16b706f2ed087d4bf31abe6e55c947d2b Mon Sep 17 00:00:00 2001 From: "Khokon M." <50947615+khokonm@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:41:55 +0600 Subject: [PATCH] fix(sync): make device 'initialized' state server-authoritative (fixes 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. --- server/src/db.ts | 93 ++++++++++++++++--------- server/src/index.ts | 29 ++++---- src/components/SettingsPanel.tsx | 7 +- src/utils/sync.ts | 115 ++++++------------------------- 4 files changed, 101 insertions(+), 143 deletions(-) diff --git a/server/src/db.ts b/server/src/db.ts index fe6faa8..40ff8fd 100644 --- a/server/src/db.ts +++ b/server/src/db.ts @@ -43,6 +43,13 @@ db.exec(` device_id TEXT NOT NULL, device_name TEXT NOT NULL DEFAULT 'Unknown Device', cursor INTEGER NOT NULL DEFAULT 0, + -- 1 once this device holds the chain's notes (it founded the chain or + -- finished an initial transfer); 0 while it still needs to be bootstrapped. + -- This is the AUTHORITATIVE answer to "does this device need a transfer?" — + -- the client used to decide it from a localStorage flag / note count, which + -- raced with note-loading and got wiped, causing already-synced devices to + -- ask each other to re-sync. + initialized INTEGER NOT NULL DEFAULT 1, last_seen_at INTEGER NOT NULL DEFAULT (unixepoch() * 1000), created_at INTEGER NOT NULL DEFAULT (unixepoch() * 1000), PRIMARY KEY (room_id, device_id) @@ -73,6 +80,16 @@ db.exec(` CREATE INDEX IF NOT EXISTS idx_transfers_room ON transfers(room_id, status); `) +// Migration for databases created before the `initialized` column existed. +// Existing rows are devices that were ALREADY paired and syncing, so they are +// established members — default them to initialized = 1 so they are never asked +// to re-bootstrap. (Idempotent: the duplicate-column error is ignored.) +try { + db.exec(`ALTER TABLE devices ADD COLUMN initialized INTEGER NOT NULL DEFAULT 1`) +} catch { + /* column already exists */ +} + // ── Sync code operations ───────────────────────────────────────────────────── const stmtRegisterSyncCode = db.prepare(` @@ -97,25 +114,33 @@ export interface DeviceRecord { deviceId: string deviceName: string cursor: number + initialized: boolean lastSeenAt: number createdAt: number } +// On first registration the `initialized` flag is set from the caller-supplied +// value. On reconnect (ON CONFLICT) it is intentionally NOT touched — a device's +// initialized state, once earned, is permanent for the life of its row. const stmtRegisterDevice = db.prepare(` - INSERT INTO devices (room_id, device_id, device_name, cursor, last_seen_at) - VALUES (?, ?, ?, 0, ?) + INSERT INTO devices (room_id, device_id, device_name, cursor, initialized, last_seen_at) + VALUES (?, ?, ?, 0, ?, ?) ON CONFLICT(room_id, device_id) DO UPDATE SET device_name = excluded.device_name, last_seen_at = excluded.last_seen_at `) +const stmtSetInitialized = db.prepare(` + UPDATE devices SET initialized = 1 WHERE room_id = ? AND device_id = ? +`) + const stmtGetDevice = db.prepare(` - SELECT device_id, device_name, cursor, last_seen_at, created_at + SELECT device_id, device_name, cursor, initialized, last_seen_at, created_at FROM devices WHERE room_id = ? AND device_id = ? `) const stmtGetDevices = db.prepare(` - SELECT device_id, device_name, cursor, last_seen_at, created_at + SELECT device_id, device_name, cursor, initialized, last_seen_at, created_at FROM devices WHERE room_id = ? `) @@ -139,46 +164,52 @@ const stmtGetMinCursor = db.prepare(` SELECT MIN(cursor) as min_cursor FROM devices WHERE room_id = ? `) -export function registerDevice(roomId: string, deviceId: string, deviceName: string): DeviceRecord { - const now = Date.now() - stmtRegisterDevice.run(roomId, deviceId, deviceName, now) - const row = stmtGetDevice.get(roomId, deviceId) as { - device_id: string; device_name: string; cursor: number; last_seen_at: number; created_at: number - } +type DeviceRow = { + device_id: string; device_name: string; cursor: number + initialized: number; last_seen_at: number; created_at: number +} + +function toDeviceRecord(row: DeviceRow): DeviceRecord { return { deviceId: row.device_id, deviceName: row.device_name, cursor: row.cursor, + initialized: !!row.initialized, lastSeenAt: row.last_seen_at, createdAt: row.created_at, } } +/** + * Register (or touch) a device. `initializedIfNew` sets the initialized flag + * ONLY when the row is first created — the founder of a chain (no other devices + * yet) is initialized; a device that joins an existing chain is not, until it + * finishes a transfer. Reconnects never change an existing row's flag. + */ +export function registerDevice( + roomId: string, + deviceId: string, + deviceName: string, + initializedIfNew: boolean, +): DeviceRecord { + const now = Date.now() + stmtRegisterDevice.run(roomId, deviceId, deviceName, initializedIfNew ? 1 : 0, now) + return toDeviceRecord(stmtGetDevice.get(roomId, deviceId) as DeviceRow) +} + +/** Mark a device as having completed its initial bootstrap (received a transfer). */ +export function markDeviceInitialized(roomId: string, deviceId: string): void { + stmtSetInitialized.run(roomId, deviceId) +} + export function getDevice(roomId: string, deviceId: string): DeviceRecord | null { - const row = stmtGetDevice.get(roomId, deviceId) as { - device_id: string; device_name: string; cursor: number; last_seen_at: number; created_at: number - } | undefined - if (!row) return null - return { - deviceId: row.device_id, - deviceName: row.device_name, - cursor: row.cursor, - lastSeenAt: row.last_seen_at, - createdAt: row.created_at, - } + const row = stmtGetDevice.get(roomId, deviceId) as DeviceRow | undefined + return row ? toDeviceRecord(row) : null } export function getDevices(roomId: string): DeviceRecord[] { - const rows = stmtGetDevices.all(roomId) as { - device_id: string; device_name: string; cursor: number; last_seen_at: number; created_at: number - }[] - return rows.map(r => ({ - deviceId: r.device_id, - deviceName: r.device_name, - cursor: r.cursor, - lastSeenAt: r.last_seen_at, - createdAt: r.created_at, - })) + const rows = stmtGetDevices.all(roomId) as DeviceRow[] + return rows.map(toDeviceRecord) } export function updateDeviceCursor(roomId: string, deviceId: string, cursor: number): void { diff --git a/server/src/index.ts b/server/src/index.ts index 58805f2..107464c 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -16,7 +16,7 @@ * * WebSocket protocol (JSON messages): * Client → Server: - * { type: 'join', roomId, token, deviceId, deviceName, needsTransfer } + * { type: 'join', roomId, token, deviceId, deviceName } * { type: 'push-op', payload (base64) } * { type: 'ack', cursor } * { type: 'request-transfer' } @@ -50,6 +50,7 @@ import { registerSyncCode, syncCodeExists, registerDevice, + markDeviceInitialized, getDevice, getDevices, updateDeviceCursor, @@ -632,8 +633,16 @@ function handleMessage(client: Client, msg: Record, ip: string) // Leave previous room if any removeClient(client) + // Determine the device's role from AUTHORITATIVE server state, not a client + // hint. A brand-new registration is a "founder" (initialized) only when no + // other device exists yet; a device joining an existing chain starts + // un-initialized and must receive a transfer before it's a full member. + // An existing row keeps whatever initialized state it already earned. + const othersBefore = getDevices(roomId).filter(d => d.deviceId !== deviceId) + const initializedIfNew = othersBefore.length === 0 + // Register/update device in DB - const device = registerDevice(roomId, deviceId, deviceName) + const device = registerDevice(roomId, deviceId, deviceName, initializedIfNew) client.roomId = roomId client.deviceId = deviceId @@ -641,17 +650,11 @@ function handleMessage(client: Client, msg: Record, ip: string) const room = getRoom(roomId) room.add(client) - // Whether this device needs an initial transfer is the CLIENT's own - // declaration (`needsTransfer`), driven by persisted intent: it generated - // the code (origin → false) or entered an existing code and hasn't yet - // pulled the notes (→ true). The server can't infer this — the data is - // E2E-encrypted, note count is unreliable (fresh devices can have a seed - // note), and `cursor === 0` is true for the origin too. We only honor the - // request when there's actually another device to copy from. - const clientNeedsTransfer = msg.needsTransfer === true const allDevices = getDevices(roomId) const otherDevices = allDevices.filter(d => d.deviceId !== deviceId) - const isNewDevice = clientNeedsTransfer && otherDevices.length > 0 + // A device "needs a transfer" iff the server has it as un-initialized AND + // there is another device to copy from. + const isNewDevice = !device.initialized && otherDevices.length > 0 const maxSeq = getMaxSeq(roomId) const hasPendingOps = device.cursor < maxSeq @@ -1028,9 +1031,11 @@ function handleMessage(client: Client, msg: Record, ip: string) updateTransferStatus(transferId, 'completed') // Update the new device's cursor to current max so it doesn't replay ops - // that were part of the transfer + // that were part of the transfer, and mark it initialized so it is never + // asked to bootstrap again (and rejoins as a full member). const maxSeq = getMaxSeq(roomId) updateDeviceCursor(roomId, transfer.requesterId, maxSeq) + markDeviceInitialized(roomId, transfer.requesterId) // Notify both parties sendToDevice(roomId, transfer.requesterId, { type: 'transfer-complete', transferId }) diff --git a/src/components/SettingsPanel.tsx b/src/components/SettingsPanel.tsx index 16c65e7..6bd6e7c 100644 --- a/src/components/SettingsPanel.tsx +++ b/src/components/SettingsPanel.tsx @@ -1108,8 +1108,7 @@ function SyncPage({ syncState, onTriggerSync, onSyncEnabled, onSyncDisabled }: S try { const result = await generateSyncCode() setGeneratedCode(result.syncCode) - // We created this chain — we are the source of truth, no transfer needed. - enableSync(result.syncCode, result.roomId, result.token, { initialized: true }) + enableSync(result.syncCode, result.roomId, result.token) setEnabled(true) onSyncEnabled() } catch (e) { @@ -1125,9 +1124,7 @@ function SyncPage({ syncState, onTriggerSync, onSyncEnabled, onSyncDisabled }: S setError(null) try { const result = await validateSyncCode(syncCodeInput) - // We're joining an existing chain — we must pull the notes from an - // existing device before we're a full member. - enableSync(syncCodeInput, result.roomId, result.token, { initialized: false }) + enableSync(syncCodeInput, result.roomId, result.token) setEnabled(true) setMode(null) setSyncCodeInput('') diff --git a/src/utils/sync.ts b/src/utils/sync.ts index e098f91..ed7a559 100644 --- a/src/utils/sync.ts +++ b/src/utils/sync.ts @@ -44,16 +44,6 @@ const SYNC_CURSOR_KEY = 'notes-sync-cursor' const SYNC_QUEUE_KEY = 'notes-sync-queue' const SYNC_LAST_KEY = 'notes-sync-last' const SYNC_TOMBSTONES_KEY = 'notes-sync-tombstones' -// Records the roomId this device has completed its initial bootstrap for. A -// device is "initialized" for a chain when it either GENERATED the code (it is -// the origin / source of truth) or finished RECEIVING an initial transfer. Any -// other state means it still needs to pull the existing notes before it can be -// treated as a full member. This is the authoritative signal for "do I need a -// transfer?" — note count and the server-side cursor are both unreliable -// proxies (a fresh device can have a seed note; the origin's cursor is 0). -const SYNC_INIT_ROOM_KEY = 'notes-sync-initialized-room' -// One-time marker so the legacy backfill below runs at most once per device. -const SYNC_INIT_MIGRATED_KEY = 'notes-sync-init-migrated-v1' // How long to keep tombstones around. A device that's been offline longer // than this and then re-broadcasts an old note will be allowed to resurrect @@ -194,43 +184,11 @@ function setCursor(cursor: number): void { localStorage.setItem(SYNC_CURSOR_KEY, String(cursor)) } -// ── Initial-transfer bootstrap state ─────────────────────────────────────── - -function getRoomId(): string | null { - return localStorage.getItem(SYNC_ROOM_KEY) -} - -function isInitializedForRoom(roomId: string | null): boolean { - return !!roomId && localStorage.getItem(SYNC_INIT_ROOM_KEY) === roomId -} - -function markInitializedForRoom(roomId: string | null): void { - if (roomId) localStorage.setItem(SYNC_INIT_ROOM_KEY, roomId) -} - -/** True when this device still needs an initial transfer for the active chain. */ -function needsInitialTransfer(): boolean { - return !isInitializedForRoom(getRoomId()) -} - -/** - * One-time backfill for devices that were paired BEFORE the per-chain - * initialized flag existed (they have no flag yet). A device that already holds - * notes is the source of truth for its chain and must never demand a transfer — - * mark it initialized. A device with no notes is genuinely empty and is left to - * request a transfer as normal. Runs at most once (gated by a marker that is - * set unconditionally), so a device that legitimately needs a transfer is never - * grandfathered later just because the user typed a note before approving. - */ -function migrateInitFlagIfNeeded(): void { - if (localStorage.getItem(SYNC_INIT_MIGRATED_KEY)) return - localStorage.setItem(SYNC_INIT_MIGRATED_KEY, '1') - const roomId = getRoomId() - if (!roomId) return - if (isInitializedForRoom(roomId)) return - const hasNotes = (callbacks?.getNotes()?.length ?? 0) > 0 - if (hasNotes) markInitializedForRoom(roomId) -} +// Whether this device needs an initial transfer is decided AUTHORITATIVELY by +// the server (the devices.initialized column) and delivered in `welcome`. We +// keep it in `currentState.needsTransfer`; the client never tries to infer it +// from local note count or a localStorage flag (both were unreliable and +// caused already-synced devices to ask each other to re-sync). // ── Tombstones ──────────────────────────────────────────────────────────── // @@ -483,14 +441,9 @@ async function refreshJoinToken(): Promise<{ roomId: string; token: string } | n return getJoinToken() } -// Build the `join` payload. `needsTransfer` is the device's own declaration of -// whether it still needs an initial bootstrap for this chain — set from -// persisted intent (generated the code, or finished a transfer), NOT inferred -// from note count or the server cursor. Both of those were unreliable: a fresh -// joining device often has a seed/empty note (so "has data" was true and it was -// wrongly marked established → it never asked to sync), and the origin's cursor -// is 0 (so it was wrongly marked new). The server trusts this declaration to -// decide the device's role. +// Build the `join` payload. The device's role (does it need an initial +// transfer?) is decided server-side from authoritative per-device state, so the +// client sends only its identity. function buildJoinPayload(roomId: string, token: string): string { return JSON.stringify({ type: 'join', @@ -498,7 +451,6 @@ function buildJoinPayload(roomId: string, token: string): string { token, deviceId: getDeviceId(), deviceName: getDeviceName(), - needsTransfer: !isInitializedForRoom(roomId), }) } @@ -579,7 +531,7 @@ function scheduleReconnect(): void { * fires while a transfer is already pending/in progress. */ function maybeAutoRequestTransfer(): void { - if (!needsInitialTransfer()) return + if (!currentState.needsTransfer) return if (currentState.status === 'transferring') return if (currentState.pendingTransfer) return if (currentState.awaitingDeviceId) return @@ -607,12 +559,9 @@ async function handleServerMessage(msg: Record): Promise isSelf: d.deviceId === selfId, })) - // Authoritative "do we still need a transfer?" is our own persisted - // bootstrap state — not the server's hint (which only reflects whether - // other devices exist). We may still need a transfer even when the server - // can't tell, and we must NOT think we're done just because we happen to - // hold a stray local note. - const needsTransfer = needsInitialTransfer() + // The server is authoritative about whether we still need a transfer + // (it tracks per-device `initialized` state that can't race or get wiped). + const needsTransfer = msg.needsTransfer === true updateState({ status: needsTransfer ? 'awaiting-source' : 'idle', @@ -713,7 +662,7 @@ async function handleServerMessage(msg: Record): Promise // notes to give, so it must never be asked to approve one. And we never // approve our own request. Without these guards a brand-new device could // be shown an approve/deny prompt for its own join — nonsensical. - if (needsInitialTransfer() || requesterId === getDeviceId()) { + if (currentState.needsTransfer || requesterId === getDeviceId()) { break } @@ -772,12 +721,11 @@ async function handleServerMessage(msg: Record): Promise console.warn('[sync] Failed to process transfer chunk:', e) } - // If this was the last chunk, mark transfer complete + // If this was the last chunk, mark transfer complete. The server records + // our `initialized` state on receiving transfer-complete, so subsequent + // joins will report needsTransfer:false; locally we clear it now too. if (chunkIndex + 1 >= totalChunks) { ws?.send(JSON.stringify({ type: 'transfer-complete', transferId })) - // We now hold the chain's notes — record this so we never ask for - // another initial transfer (and join as a full member from now on). - markInitializedForRoom(getRoomId()) updateState({ status: 'idle', transferProgress: undefined, needsTransfer: false }) // Any local edits made during the transfer were already pushed // live via syncPushSingle (ws was open). Just flush anything that @@ -1175,33 +1123,12 @@ export async function loadSyncPassword(): Promise { return loadSyncCode() } -/** - * Enable sync for a chain. - * - * `opts.initialized` declares whether this device already holds the chain's - * notes and therefore does NOT need an initial transfer: - * - true → this device GENERATED the code (it is the origin / source). - * - false → this device ENTERED an existing code (it must pull the notes - * from an existing device before it's a full member). - */ -export function enableSync( - syncCode: string, - roomId?: string, - token?: string, - opts?: { initialized?: boolean }, -): void { +export function enableSync(syncCode: string, roomId?: string, token?: string): void { localStorage.setItem(SYNC_ENABLED_KEY, '1') cachedSyncCode = normalizeSyncCode(syncCode) secureSet(SYNC_CODE_KEY, cachedSyncCode).catch(() => {}) if (roomId) localStorage.setItem(SYNC_ROOM_KEY, roomId) if (token) localStorage.setItem(SYNC_TOKEN_KEY, token) - if (opts?.initialized && roomId) { - markInitializedForRoom(roomId) - } else if (opts?.initialized === false && roomId && !isInitializedForRoom(roomId)) { - // Joining a chain we haven't bootstrapped — make sure any stale flag from a - // different room doesn't suppress the initial transfer. - localStorage.removeItem(SYNC_INIT_ROOM_KEY) - } } export function disableSync(): void { @@ -1215,7 +1142,6 @@ export function disableSync(): void { localStorage.removeItem(SYNC_QUEUE_KEY) localStorage.removeItem(SYNC_LAST_KEY) localStorage.removeItem(SYNC_TOMBSTONES_KEY) - localStorage.removeItem(SYNC_INIT_ROOM_KEY) cachedSyncCode = null stopSync() } @@ -1231,10 +1157,6 @@ export function startSync(cbs: SyncCallbacks): void { stopSync() if (!isSyncEnabled()) return callbacks = cbs - // Grandfather pre-existing devices (paired before the initialized flag) so a - // device that already holds the notes isn't wrongly treated as needing a - // transfer (which made it auto-request and prompt the brand-new device). - migrateInitFlagIfNeeded() updateState({ enabled: true, status: 'connecting' }) loadSyncCode().then(() => connect()).catch(() => {}) } @@ -1301,6 +1223,9 @@ export async function triggerSync(): Promise { export async function approveTransfer(transferId: number): Promise { if (ws?.readyState !== WebSocket.OPEN) return ws.send(JSON.stringify({ type: 'approve-transfer', transferId })) + // Dismiss the prompt immediately — we're the sender and stay connected. The + // modal must never linger even if the requester is slow or drops off. + updateState({ pendingTransfer: undefined }) // Start sending chunks after a short delay (server will relay) setTimeout(() => sendTransferChunks(transferId, 0), 500) }