Publish the restoring marker by rename so a torn write cannot unblock a half-purged database

dataLayer/restoreMarker.ts

@@ -8,6 +8,7 @@ import {
 	openSync,
 	readdirSync,
 	readFileSync,
+	renameSync,
 	unlinkSync,
 	writeSync,
 } from 'node:fs';
@@ -39,18 +40,27 @@ import { tryFileLock, fileLockRelease } from '@harperfast/rocksdb-js';
  *   duration of a restore, and briefly by `dropDatabase` so the two serialize on the same primitive.
  *   Known limitation: the lock is owned by the process, so if the restore job's worker *thread*
  *   dies without the process exiting, the lock stays held (restores 409) until Harper restarts.
- * - `<meta-dir>/<key>.restoring` — the completion marker. Written (and fsynced) after the lock is
- *   acquired and before the destructive restore begins; deleted only after the restore completes
- *   successfully, while still holding the lock. Its *existence* means "a restore started and has
- *   not finished successfully". Its first line records the database directory name so the startup
- *   scan can map a marker back to the database it blocks without decoding the hashed key.
+ * - `<meta-dir>/<key>.restoring` — the completion marker. Published (temp → fsync → rename → parent
+ *   fsync) after the lock is acquired and before the destructive restore begins; deleted only after
+ *   the restore completes successfully, while still holding the lock. Its *existence* means "a
+ *   restore started and has not finished successfully". Its first line records the database
+ *   directory name as a fallback so the startup scan can map a marker whose database directory is
+ *   missing without decoding the hashed key.
+ *
+ *   The rename is what makes the marker trustworthy: the marker path only ever holds the previous
+ *   content or the complete new content, so a torn write can never replace a valid marker with one
+ *   the scan reads as empty (and therefore honors as no block). An intact marker is also never
+ *   rewritten at all — see `beginRestore`.
  */
 
 // The backtick makes this an illegal database name (schemaRegex rejects `/` and backtick only), so
 // it can never collide with a real database directory — see the module header.
 export const RESTORE_META_DIR = '`restore`';
 export const RESTORE_LOCK_SUFFIX = '.lock';
 export const RESTORING_MARKER_SUFFIX = '.restoring';
+// Deliberately not a `.restoring` suffix: `scanBlockedRestores` selects markers by that suffix, and
+// a half-written temp must never be mistaken for one.
+const MARKER_TEMP_SUFFIX = '.tmp';
 
 /**
  * Directory holding the restore metadata for a database — the reserved `` `restore` `` sibling of
@@ -180,17 +190,29 @@ export function releaseRestoreLock(lock: RestoreLock): void {
 }
 
 /**
- * Acquire the per-database restore lock and write the restoring marker. Call before any
- * destructive step. Returns the lock (with `preexisting` set when a marker was already present, so
- * a failed recovery attempt knows not to clear it). Throws (statusCode 409) if another restore
- * already holds the lock.
+ * Whether a marker on disk still blocks the database it belongs to. `scanBlockedRestores` maps a
+ * marker to a database by its first line, so the line has to be the whole name: a torn write that
+ * left a prefix of it ("ord" for "orders") is non-empty and reads as valid, but blocks a database
+ * that does not exist while the real one loads.
  */
-export function beginRestore(dbPath: string): RestoreLock {
-	const markerPath = restoringMarkerPath(dbPath);
-	const preexisting = existsSync(markerPath);
-	const lock = acquireRestoreLock(dbPath);
+function markerIsIntact(markerPath: string, dbPath: string): boolean {
+	try {
+		return readFileSync(markerPath, 'utf8').split('\n', 1)[0] === basename(dbPath);
+	} catch {
+		return false;
+	}
+}
+
+/**
+ * Publish the restoring marker atomically. The caller must already hold the restore lock, which is
+ * what makes the fixed temp name safe: only one writer per database can exist at a time, so a temp
+ * left by an earlier crash is this database's own debris and is simply overwritten.
+ */
+function publishRestoringMarker(dbPath: string): void {
+	const metaDir = restoreMetaDir(dbPath);
+	const tempPath = join(metaDir, restoreMetaKey(dbPath) + MARKER_TEMP_SUFFIX);
 	try {
-		const fd = openSync(markerPath, 'w');
+		const fd = openSync(tempPath, 'w');
 		try {
 			// first line is the database directory name so the startup scan can map this marker back to
 			// the database it blocks without reversing the hashed key
@@ -199,9 +221,44 @@ export function beginRestore(dbPath: string): RestoreLock {
 		} finally {
 			closeSync(fd);
 		}
-		// fsync the metadata directory so the marker's directory entry is durable — without this a
-		// power loss can lose the entry, and a half-purged database would load as healthy
-		fsyncDir(restoreMetaDir(dbPath));
+		renameSync(tempPath, restoringMarkerPath(dbPath));
+	} catch (error) {
+		try {
+			unlinkSync(tempPath);
+		} catch {
+			// debris either way; the write failure is the error worth reporting
+		}
+		throw error;
+	}
+	// fsync the metadata directory so the marker's directory entry is durable — without this a
+	// power loss can lose the entry, and a half-purged database would load as healthy
+	fsyncDir(metaDir);
+}
+
+/**
+ * Acquire the per-database restore lock and ensure the restoring marker is in place. Call before any
+ * destructive step. Returns the lock (with `preexisting` set when a marker was already present, so
+ * a failed recovery attempt knows not to clear it). Throws (statusCode 409) if another restore
+ * already holds the lock.
+ *
+ * An intact marker is left exactly as it is. A recovery attempt runs over a directory an earlier
+ * restore may have half-purged, so the marker it finds is the only thing keeping that directory
+ * from loading as healthy; rewriting it buys nothing (the content it would write is the content
+ * already there) and risks everything.
+ */
+export function beginRestore(dbPath: string): RestoreLock {
+	const markerPath = restoringMarkerPath(dbPath);
+	const lock = acquireRestoreLock(dbPath);
+	// Sampled while holding the lock, not before it: a restore that waited out an earlier one would
+	// otherwise carry the earlier run's "no marker" reading, and on its own pre-destruction failure
+	// clear the marker protecting a directory that run had already half-purged.
+	const preexisting = existsSync(markerPath);
+	try {
+		if (!preexisting || !markerIsIntact(markerPath, dbPath)) publishRestoringMarker(dbPath);
+		// An intact marker is kept, but its durability is not assumed: the publisher that wrote it may
+		// have been interrupted between the rename and this flush, which would leave the directory
+		// entry — the thing the startup scan reads — still only in the page cache.
+		else fsyncDir(restoreMetaDir(dbPath));
 	} catch (error) {
 		fileLockRelease(lock.token);
 		throw error;
@@ -251,24 +308,34 @@ export function clearRestoreMarker(lock: RestoreLock): void {
 }
 
 /**
- * Scan a databases root's reserved `` `restore` `` metadata directory and report every database currently blocked from
- * loading, mapping each surviving marker back to its database name via the marker's first line.
+ * Scan a databases root's reserved `` `restore` `` metadata directory and report every database
+ * currently blocked from loading. Existing database entries are mapped to markers by their hashed
+ * keys, so corrupt marker contents cannot unblock them; the first line remains the fallback for a
+ * marker whose database directory is missing.
  * Returns `[dbName, state]` pairs for markers whose state is `in-progress` or `incomplete`
  * (a `clear` result means the marker was removed concurrently and the database is loadable).
  */
 export function scanBlockedRestores(databasesRoot: string): Array<[string, RestoreState]> {
 	const metaDir = join(databasesRoot, RESTORE_META_DIR);
 	if (!existsSync(metaDir)) return [];
+	const databaseNamesByMarker = new Map(
+		readdirSync(databasesRoot, { withFileTypes: true })
+			.filter((entry) => entry.name !== RESTORE_META_DIR)
+			.map((entry) => [restoreMetaKey(join(databasesRoot, entry.name)) + RESTORING_MARKER_SUFFIX, entry.name])
+	);
 	const blocked: Array<[string, RestoreState]> = [];
 	for (const entry of readdirSync(metaDir, { withFileTypes: true })) {
 		if (!entry.isFile() || !entry.name.endsWith(RESTORING_MARKER_SUFFIX)) continue;
-		let dbName: string;
-		try {
-			dbName = readFileSync(join(metaDir, entry.name), 'utf8').split('\n', 1)[0];
-		} catch {
-			continue; // marker removed concurrently
+		const markerPath = join(metaDir, entry.name);
+		let dbName = databaseNamesByMarker.get(entry.name);
+		if (!dbName) {
+			try {
+				dbName = readFileSync(markerPath, 'utf8').split('\n', 1)[0];
+			} catch {
+				continue; // marker removed concurrently
+			}
+			if (!dbName || restoringMarkerPath(join(databasesRoot, dbName)) !== markerPath) continue;
 		}
-		if (!dbName) continue;
 		const state = checkRestoreState(join(databasesRoot, dbName));
 		if (state !== 'clear') blocked.push([dbName, state]);
 	}

unitTests/dataLayer/restoreMarker.test.js

@@ -1,7 +1,16 @@
 'use strict';
 
 const assert = require('node:assert');
-const { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } = require('node:fs');
+const {
+	appendFileSync,
+	existsSync,
+	mkdtempSync,
+	mkdirSync,
+	readdirSync,
+	readFileSync,
+	rmSync,
+	writeFileSync,
+} = require('node:fs');
 const { basename, dirname, join } = require('node:path');
 const { tmpdir } = require('node:os');
 const { tryFileLock, fileLockRelease } = require('@harperfast/rocksdb-js');
@@ -181,6 +190,89 @@ describe('restoreMarker', function () {
 			completeRestore(lock);
 			assert.strictEqual(checkRestoreState(dbPath), 'clear');
 		});
+
+		it('leaves an intact marker byte-identical on a recovery attempt', function () {
+			// The marker a recovery attempt finds is the only thing keeping a possibly half-purged
+			// directory from loading as healthy. Rewriting it opens a window where a crash leaves it
+			// empty — which scanBlockedRestores reads as "no block".
+			abandonRestore(beginRestore(dbPath));
+			appendFileSync(restoringMarkerPath(dbPath), 'written by the interrupted restore\n');
+			const before = readFileSync(restoringMarkerPath(dbPath), 'utf8');
+
+			const rerun = beginRestore(dbPath);
+			try {
+				assert.strictEqual(rerun.preexisting, true);
+				assert.strictEqual(
+					readFileSync(restoringMarkerPath(dbPath), 'utf8'),
+					before,
+					'a recovery attempt must not rewrite the marker that is blocking the database'
+				);
+			} finally {
+				abandonRestore(rerun);
+			}
+			assert.strictEqual(checkRestoreState(dbPath), 'incomplete');
+		});
+
+		it('repairs a marker an interrupted write left empty, so the database stays blocked', function () {
+			// what a torn write under the pre-rename implementation left behind
+			mkdirSync(dbPath, { recursive: true });
+			mkdirSync(restoreMetaDir(dbPath), { recursive: true });
+			writeFileSync(restoringMarkerPath(dbPath), '');
+			assert.deepStrictEqual(
+				scanBlockedRestores(tempDir),
+				[[basename(dbPath), 'incomplete']],
+				'the startup scan must block the database without trusting the empty marker'
+			);
+
+			abandonRestore(beginRestore(dbPath));
+
+			assert.strictEqual(readFileSync(restoringMarkerPath(dbPath), 'utf8').split('\n', 1)[0], basename(dbPath));
+			assert.deepStrictEqual(scanBlockedRestores(tempDir), [[basename(dbPath), 'incomplete']]);
+		});
+
+		it('repairs a marker whose database name was torn mid-write', function () {
+			// Non-empty but truncated. The scan resolves the name it reads back to a *different* metadata
+			// key, so a torn name blocks nothing at all while the real database loads.
+			mkdirSync(dbPath, { recursive: true });
+			mkdirSync(restoreMetaDir(dbPath), { recursive: true });
+			writeFileSync(restoringMarkerPath(dbPath), `${basename(dbPath).slice(0, 3)}`);
+			assert.deepStrictEqual(
+				scanBlockedRestores(tempDir),
+				[[basename(dbPath), 'incomplete']],
+				'the startup scan must block the database without trusting the torn name'
+			);
+
+			abandonRestore(beginRestore(dbPath));
+
+			assert.deepStrictEqual(scanBlockedRestores(tempDir), [[basename(dbPath), 'incomplete']]);
+		});
+
+		it('publishes the marker by rename, leaving no temp file behind', function () {
+			const lock = beginRestore(dbPath);
+			try {
+				assert.deepStrictEqual(
+					readdirSync(restoreMetaDir(dbPath)).sort(),
+					[basename(restoreLockPath(dbPath)), basename(restoringMarkerPath(dbPath))].sort()
+				);
+			} finally {
+				completeRestore(lock);
+			}
+		});
+
+		it('overwrites a temp file a crashed write left behind, and the scan never sees it', function () {
+			const tempPath = restoringMarkerPath(dbPath).replace(/\.restoring$/, '.tmp');
+			mkdirSync(restoreMetaDir(dbPath), { recursive: true });
+			writeFileSync(tempPath, 'someotherdb\npartial write\n');
+			assert.deepStrictEqual(scanBlockedRestores(tempDir), [], 'a temp file must never read as a marker');
+
+			const lock = beginRestore(dbPath);
+			try {
+				assert.deepStrictEqual(scanBlockedRestores(tempDir), [[basename(dbPath), 'in-progress']]);
+				assert.ok(!existsSync(tempPath), 'the temp file is consumed by the rename');
+			} finally {
+				completeRestore(lock);
+			}
+		});
 	});
 
 	describe('acquireRestoreLock (drop serialization primitive)', function () {

unitTests/resources/databases.test.js

@@ -6,7 +6,13 @@ const { dirname, join } = require('node:path');
 const { table, flushDatabases, dropDatabase, getDatabases, resetDatabases } = require('#src/resources/databases');
 const { setMainIsWorker } = require('#js/server/threads/manageThreads');
 const { RocksDatabase } = require('@harperfast/rocksdb-js');
-const { beginRestore, completeRestore, RESTORE_META_DIR } = require('#src/dataLayer/restoreMarker');
+const {
+	beginRestore,
+	completeRestore,
+	abandonRestore,
+	restoringMarkerPath,
+	RESTORE_META_DIR,
+} = require('#src/dataLayer/restoreMarker');
 
 describe('flushDatabases', () => {
 	before(async function () {
@@ -174,6 +180,30 @@ describe('dropDatabase restore serialization', () => {
 		assert.strictEqual(loaded[RESTORE_META_DIR], undefined, 'reserved dir must not be loaded as a database');
 		assert.ok(existsSync(reservedDir), 'the reserved dir itself is left in place (used for lifecycle metadata)');
 	});
+
+	it('never loads a database whose pre-atomic restoring marker is empty', function () {
+		const databaseName = 'corrupt-marker-startup-test';
+		const Table = table({
+			table: 'CorruptMarker',
+			database: databaseName,
+			attributes: [{ name: 'id', isPrimaryKey: true }],
+		});
+		const rootStore = Table.primaryStore.rootStore;
+		if (!(rootStore instanceof RocksDatabase)) return this.skip();
+
+		abandonRestore(beginRestore(rootStore.path));
+		writeFileSync(restoringMarkerPath(rootStore.path), '');
+		try {
+			resetDatabases();
+			assert.strictEqual(
+				getDatabases()[databaseName],
+				undefined,
+				'a database with a corrupt restoring marker must remain blocked during startup'
+			);
+		} finally {
+			completeRestore(beginRestore(rootStore.path));
+		}
+	});
 });
 
 describe('openBranchDatabase (scope-private graph, harper#643)', () => {