From 85139d383027147d1c05da276349a0b4aa01d42f Mon Sep 17 00:00:00 2001 From: oliver <54379826+ok@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:30:46 +0200 Subject: [PATCH] [fix] Lift a restore hold on blocks no holder has (#634) A restored catalog or profile stayed read-only until its copy had no gaps, but members hold sparse copies, so a block nobody fetched never arrived and the space stayed held for good. Missing blocks now keep it held only while a connected holder can supply one; the length match still guards against a fork. The flow test now waits for the catalog. (cherry picked from commit 9ace9c65f1fe5450a25d49427507b541841686c9) --- src/shared/core/restore-hold-rules.js | 16 +++++++++------- src/worker/restore-catch-up.js | 10 ++++++++++ test/flow/backup-restore.test.js | 3 ++- test/unit/restore-hold-rules.test.js | 9 ++++++++- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/shared/core/restore-hold-rules.js b/src/shared/core/restore-hold-rules.js index c890fb78..c4e2d39b 100644 --- a/src/shared/core/restore-hold-rules.js +++ b/src/shared/core/restore-hold-rules.js @@ -1,18 +1,20 @@ // When a restored core may be written again. A core restored from a key or a backup is at most as // long as the copies peers hold, and appending before catching up signs a second history at a length // they already have, which every peer that sees both refuses for good. So it stays read-only until -// every connected holder has been matched and the copy is complete, and for a dwell after the first -// holder answered, so a stale holder that happens to connect first does not decide alone. A profile -// this device already holds data of, with no co-member in any space it knows, has no other holder. A -// catalog (`emptyMayBeSolo`) is known with its space, so it has no other holder even when empty — a -// profile restored from a key alone starts empty and knows no spaces yet, so it must wait. +// every connected holder has been matched and holds no block the copy still lacks, and for a dwell +// after the first holder answered, so a stale holder that happens to connect first does not decide +// alone. A block no holder has does not hold the core: holders keep sparse copies, so it may never +// arrive, and appending needs only the matched length. A profile this device already holds data of, +// with no co-member in any space it knows, has no other holder. A catalog (`emptyMayBeSolo`) is known +// with its space, so it has no other holder even when empty — a profile restored from a key alone +// starts empty and knows no spaces yet, so it must wait. import { RESTORE_VERDICT } from '../contract/restore-verdict.js' -export function releaseVerdict({ localLength, contiguousLength, holderLengths, firstHolderAt, now, dwellMs, coMembers, emptyMayBeSolo = false }) { +export function releaseVerdict({ localLength, contiguousLength, holderLengths, firstHolderAt, now, dwellMs, coMembers, missingServable, emptyMayBeSolo = false }) { if (coMembers === 0 && (localLength > 0 || emptyMayBeSolo)) return RESTORE_VERDICT.CAUGHT_UP if (holderLengths.length === 0 || firstHolderAt === null) return RESTORE_VERDICT.NO_HOLDER if (localLength < Math.max(...holderLengths)) return RESTORE_VERDICT.BEHIND - if (contiguousLength < localLength) return RESTORE_VERDICT.DOWNLOADING + if (contiguousLength < localLength && missingServable) return RESTORE_VERDICT.DOWNLOADING if (now - firstHolderAt < dwellMs) return RESTORE_VERDICT.DWELL return RESTORE_VERDICT.CAUGHT_UP } diff --git a/src/worker/restore-catch-up.js b/src/worker/restore-catch-up.js index 1504bfd0..f5da24b7 100644 --- a/src/worker/restore-catch-up.js +++ b/src/worker/restore-catch-up.js @@ -26,6 +26,15 @@ function coMembers(spaces, self, spaceId) { return keys.size } +// Whether a connected holder has a block this copy still lacks; only such a block is worth waiting for. +// Peers are asked first, as their bitfields are in memory and the local check reads storage. +async function missingServable(core) { + for (let i = core.contiguousLength; i < core.length; i++) { + if (core.peers.some((peer) => peer.remoteBitfield?.get(i)) && !(await core.has(i))) return true + } + return false +} + export class RestoreCatchUp extends Subsystem { constructor(name, deps) { super(name, deps); this.require('ipc', 'profile') } @@ -112,6 +121,7 @@ export class RestoreCatchUp extends Subsystem { now: Date.now(), dwellMs: getRestoreReleaseDwellMs(), coMembers: members, + missingServable: await missingServable(core), emptyMayBeSolo: !isProfile, }) if (isProfile) this.report({ verdict, length: core.contiguousLength, target: Math.max(core.length, ...holderLengths), released: false }) diff --git a/test/flow/backup-restore.test.js b/test/flow/backup-restore.test.js index 262e4844..6ce2f0fb 100644 --- a/test/flow/backup-restore.test.js +++ b/test/flow/backup-restore.test.js @@ -67,7 +67,8 @@ test('a device restored from a backup catches up from a co-member, then writes w const status = await restoring.request('identity:status') t.ok(status.restore, 'the restored device starts in restore mode') t.is((await restoring.request('profile:get')).displayName, 'Alice', 'with the profile as it was backed up') - await restoring.until('identity:status', {}, (s) => s.restore?.profile?.released === true, { ms: 180000, every: 500 }) + // The profile and the space's catalog are released apart; a hold still on at stop carries into the next boot. + await restoring.until('identity:status', {}, (s) => s.restore?.profile?.released === true && s.restore.heldSpaceIds.length === 0, { ms: 180000, every: 500 }) await stop(restoring) const back = await launch() diff --git a/test/unit/restore-hold-rules.test.js b/test/unit/restore-hold-rules.test.js index 06129314..1a101492 100644 --- a/test/unit/restore-hold-rules.test.js +++ b/test/unit/restore-hold-rules.test.js @@ -2,7 +2,7 @@ import test from 'brittle' import { releaseVerdict } from '../../src/shared/core/restore-hold-rules.js' import { RESTORE_VERDICT } from '../../src/shared/contract/restore-verdict.js' -const state = (over = {}) => ({ localLength: 10, contiguousLength: 10, holderLengths: [10], firstHolderAt: 0, now: 30_000, dwellMs: 20_000, coMembers: 1, ...over }) +const state = (over = {}) => ({ localLength: 10, contiguousLength: 10, holderLengths: [10], firstHolderAt: 0, now: 30_000, dwellMs: 20_000, coMembers: 1, missingServable: true, ...over }) test('a core no connected peer holds stays held', (t) => { t.is(releaseVerdict(state({ holderLengths: [], firstHolderAt: null })), RESTORE_VERDICT.NO_HOLDER) @@ -23,6 +23,13 @@ test('a matched length with blocks still missing is not caught up', (t) => { t.is(releaseVerdict(state({ contiguousLength: 7 })), RESTORE_VERDICT.DOWNLOADING) }) +test('REGRESSION (FIX: restore hold on a sparse holder) — a missing block no holder has does not hold the core', (t) => { + t.is(releaseVerdict(state({ contiguousLength: 7, missingServable: false, now: 19_999 })), RESTORE_VERDICT.DWELL) + t.is(releaseVerdict(state({ contiguousLength: 7, missingServable: false })), RESTORE_VERDICT.CAUGHT_UP) + t.is(releaseVerdict(state({ contiguousLength: 7, holderLengths: [12], missingServable: false })), RESTORE_VERDICT.BEHIND, + 'a holder with a longer copy still keeps it held') +}) + test('a caught-up core waits out the dwell after the first holder answered', (t) => { t.is(releaseVerdict(state({ now: 19_999 })), RESTORE_VERDICT.DWELL) t.is(releaseVerdict(state({ now: 20_000 })), RESTORE_VERDICT.CAUGHT_UP, 'the boundary releases')