Fix copy-db producing a silently corrupt, non-restorable database copy

bin/harper.ts

@@ -119,7 +119,7 @@ async function harper() {
 		case SERVICE_ACTIONS_ENUM.COPYDB: {
 			let sourceDb = process.argv[3];
 			let targetDbPath = process.argv[4];
-			return require('./copyDb').copyDb(sourceDb, targetDbPath);
+			return require('./copyDb').copyDb(sourceDb, targetDbPath, { blobs: 'copy' });
 		}
 		case OPERATIONS_ENUM.CREATE_BACKUP:
 		case OPERATIONS_ENUM.LIST_BACKUPS:

bin/help.ts

@@ -86,7 +86,10 @@ const SECTIONS: Section[] = [
 					['upgrade', 'Upgrade harperdb'],
 					['register', 'Register harperdb'],
 					['renew-certs', 'Generate a new set of self-signed certificates'],
-					['copy-db <source> <target>', 'Copies a database from source path to target path'],
+					[
+						'copy-db <source> <target>',
+						'Copies the database named <source> to the <target> environment path. File-backed blobs are copied to <target>-blobs; see its README before restoring.',
+					],
 					['version', 'Print the version'],
 					['help', 'Display this output'],
 				],

dataLayer/blobBackup.ts

@@ -168,14 +168,13 @@ async function copyTree(
 }
 
 /**
- * Snapshot a database's blob roots into a backup's blob directory. Writes to a temporary sibling
- * and atomically renames into place so a create_backup that fails mid-copy never leaves a partial
- * `blobs/<backupId>/` that a later restore would trust. Overwrites any pre-existing snapshot for the
- * same id (create_backup always produces a fresh id, so this only matters on a retried offline run).
+ * Copy every blob root into `destDir` as `<rootIndex>/<relpath>`, hard-linking where possible.
+ * Writes to a temporary sibling and atomically renames into place so a run that fails mid-copy never
+ * leaves a partial directory a later restore would trust, and replaces any pre-existing `destDir`.
+ * Shared by managed-backup snapshots and `copy-db`'s standalone blob copy (harper#2048).
  */
-export async function snapshotBlobs(backupDir: string, backupId: number, blobRoots: string[]): Promise<void> {
-	const finalDir = blobSnapshotDir(backupDir, backupId);
-	const tempDir = join(blobsRootDir(backupDir), `.tmp-${backupId}`);
+export async function copyBlobRootsByIndex(destDir: string, blobRoots: string[]): Promise<void> {
+	const tempDir = destDir + '.tmp';
 	await rm(tempDir, { recursive: true, force: true });
 	await mkdir(tempDir, { recursive: true });
 	try {
@@ -188,45 +187,67 @@ export async function snapshotBlobs(backupDir: string, backupId: number, blobRoo
 		}
 		if (substituted > 0) {
 			logger.warn(
-				`Blob snapshot for backup ${backupId} substituted ${substituted} of ${substituted + captured} blob ` +
+				`Blob copy into ${destDir} substituted ${substituted} of ${substituted + captured} blob ` +
 					`file(s) with PENDING or ERROR markers because they were not capturable whole.`
 			);
 		}
-		await rm(finalDir, { recursive: true, force: true });
-		await rename(tempDir, finalDir);
+		await rm(destDir, { recursive: true, force: true });
+		await rename(tempDir, destDir);
 	} catch (error) {
 		await rm(tempDir, { recursive: true, force: true }).catch(() => {});
 		throw error;
 	}
+}
+
+/**
+ * Snapshot a database's blob roots into a backup's blob directory. Overwrites any pre-existing
+ * snapshot for the same id (create_backup always produces a fresh id, so this only matters on a
+ * retried offline run).
+ */
+export async function snapshotBlobs(backupDir: string, backupId: number, blobRoots: string[]): Promise<void> {
+	await copyBlobRootsByIndex(blobSnapshotDir(backupDir, backupId), blobRoots);
 	await writeBlobsReadme(backupDir, blobRoots);
 }
 
 /**
  * Build the `blobs/README.md` documenting the blob snapshot layout, so an operator inspecting or
- * hand-recovering a backup can decode the numeric directories. Two variants:
- * - managed (default): a create_backup repository, where snapshots are keyed by backup id
+ * hand-recovering a backup can decode the numeric directories. Three variants:
+ * - `managed` (default): a create_backup repository, where snapshots are keyed by backup id
  *   (`<backupId>/<rootIndex>/…`) and restore is automatic via `restore_backup`.
- * - archive (`archive: true`): a downloaded `get_backup` tar, which holds a single snapshot with no
- *   backup-id level (`<rootIndex>/…`) and is restored by extracting the files back into the roots.
+ * - `archive`: a downloaded `get_backup` tar, which holds a single snapshot with no backup-id level
+ *   (`<rootIndex>/…`) and is restored by extracting the files back into the roots.
+ * - `copy`: the companion directory `copy-db` writes beside a database copy, restored by hand.
  */
-export function blobsReadmeContent(blobRoots: string[], { archive = false }: { archive?: boolean } = {}): string {
+export function blobsReadmeContent(
+	blobRoots: string[],
+	{ variant = 'managed' }: { variant?: 'managed' | 'archive' | 'copy' } = {}
+): string {
 	const rootMapping =
 		blobRoots.length > 0 ? blobRoots.map((root, index) => `      ${index} -> ${root}`).join('\n') : '      (none)';
-	const layout = archive
-		? '<rootIndex>/<shard1>/<shard2>/<fileId>'
-		: '<backupId>/<rootIndex>/<shard1>/<shard2>/<fileId>';
-	const intro = archive
-		? `This directory holds this database's file-backed blobs within a downloaded \`get_backup\` archive.
+	const layout =
+		variant === 'managed'
+			? '<backupId>/<rootIndex>/<shard1>/<shard2>/<fileId>'
+			: '<rootIndex>/<shard1>/<shard2>/<fileId>';
+	const intro =
+		variant === 'copy'
+			? `This directory holds the file-backed blobs of a \`copy-db\` database copy; the database file itself
+is the sibling \`.mdb\` this directory is named after. Blobs are addressed by database NAME and the
+configured blob roots — never by the database file's path — so a copy is only restorable with these
+files: put each \`<rootIndex>/\` tree into the matching blob root of whatever database name you
+restore the copy as (mapping below).`
+			: variant === 'archive'
+				? `This directory holds this database's file-backed blobs within a downloaded \`get_backup\` archive.
 To restore them, extract each \`<rootIndex>/\` tree back into the matching blob root (see the mapping
 below and ../README.md).`
-		: `This directory holds point-in-time snapshots of this database's file-backed blobs, captured
+				: `This directory holds point-in-time snapshots of this database's file-backed blobs, captured
 alongside each RocksDB managed backup. You do not restore these by hand — \`restore_backup\` puts
 them back automatically (see ../README.md); this file just documents the layout.`;
-	const backupIdBullet = archive
-		? ''
-		: `- **<backupId>** matches the RocksDB backup id (\`harper list_backups\`). Each id is a full,
+	const backupIdBullet =
+		variant === 'managed'
+			? `- **<backupId>** matches the RocksDB backup id (\`harper list_backups\`). Each id is a full,
   independent snapshot (not incremental).
-`;
+`
+			: '';
 	return `# Harper blob snapshots
 
 ${intro}
@@ -237,7 +258,7 @@ ${intro}
 
 ${backupIdBullet}- **<rootIndex>** is which of the database's blob roots the file came from — the index into
   \`storage.blobPaths[n]\`. When \`storage.blobPaths\` is not configured there is a single default root
-  (\`<rootPath>/blobs/<db>\`) at index 0. Current mapping for this backup:
+  (\`<rootPath>/blobs/<db>\`) at index 0. Current mapping:
 
 ${rootMapping}
 
@@ -246,7 +267,7 @@ ${rootMapping}
   4096 entries per directory). E.g. a blob with id \`0x12345678\` lives at \`12/345/678\`; a short id
   like \`0xc1a\` lives at \`0/0/c1a\`.
 
-Complete blobs are hard links to the live blobs when the backup is on the same filesystem, and
+Complete blobs are hard links to the live blobs when the destination is on the same filesystem, and
 copies otherwise. A blob that was not capturable whole is stored as a marker instead: header type
 \`0xfe\` is retryable (PENDING), while \`0xff\` is terminal (ERROR). The marker preserves the file id
 but does not contain the original blob bytes.

dataLayer/rocksdbBackup.ts

@@ -754,7 +754,7 @@ async function streamBackupWithBlobs(
 		await appendBlobEntries(pack, blobRoots);
 		// generate the same self-documenting READMEs a managed backup writes to disk, on the fly
 		await addTextEntry(pack, 'README.md', streamedBackupReadme(databaseName));
-		await addTextEntry(pack, 'blobs/README.md', blobsReadmeContent(blobRoots, { archive: true }));
+		await addTextEntry(pack, 'blobs/README.md', blobsReadmeContent(blobRoots, { variant: 'archive' }));
 		pack.finalize();
 		await packed;
 		await consumed;

unitTests/bin/copyDB.test.js

@@ -3,7 +3,7 @@ const assert = require('assert');
 const path = require('path');
 const sinon = require('sinon');
 const env_mgr = require('#src/utility/environment/environmentManager');
-const { table } = require('#src/resources/databases');
+const { table, getDatabases } = require('#src/resources/databases');
 const { setMainIsWorker } = require('#js/server/threads/manageThreads');
 const config_utils = require('#src/config/configUtils');
 const copyDB = require('#src/bin/copyDb');
@@ -21,7 +21,9 @@ describe('Test database copy and compact', () => {
 	let update_config_stub;
 	let test_db_path;
 	let test_db_backup_path;
-	if (envGet(CONFIG_PARAMS.STORAGE_ENGINE) !== 'lmdb') return;
+	// HARPER_STORAGE_ENGINE is how `test:unit:lmdb` selects the engine; gating on the config value
+	// alone (unset under mocha) skipped this whole suite in every run.
+	if ((process.env.HARPER_STORAGE_ENGINE || envGet(CONFIG_PARAMS.STORAGE_ENGINE)) !== 'lmdb') return;
 	before(async function () {
 		console_error_spy = sandbox.spy(console, 'error');
 		sandbox.spy(console, 'log');
@@ -82,7 +84,7 @@ describe('Test database copy and compact', () => {
 
 	it('Test copyDB copies and compacts a DB', async () => {
 		const compacted_db = path.join(storage_path, 'db-copy.mdb');
-		await copyDB.copyDb('copy-test', compacted_db);
+		await copyDB.copyDb('copy-test', compacted_db, { blobs: 'copy' });
 		await TestTable.put(105, {
 			// should not be written
 			id: 105,
@@ -91,8 +93,13 @@ describe('Test database copy and compact', () => {
 			notIndexed: 'I am a non-indexed value',
 		});
 		const stat_after = await fs.stat(compacted_db);
-		const compaction = 100 - (stat_after.size / stat_before_compact.size) * 100;
-		assert(compaction >= 85, `Compaction should be at least 85% but was ${compaction}%`);
+		// The copy carries the audit log now that it goes to the target environment instead of back
+		// into the source (harper#2048), so it is about the size of the source rather than a fraction
+		// of it — the size drop this used to assert was the audit log being silently dropped.
+		assert(
+			stat_after.size <= stat_before_compact.size,
+			`Compacted copy (${stat_after.size}) should not exceed the source (${stat_before_compact.size})`
+		);
 		assert(!(await TestTable.get(105)));
 		let matches = [];
 		for await (let entry of TestTable.search([{ name: 'about', value: 'about' }])) matches.push(entry);
@@ -104,13 +111,15 @@ describe('Test database copy and compact', () => {
 	it('Test compactOnStart compacts and overwrites DB', async () => {
 		await copyDB.compactOnStart();
 		const stat_after = await fs.stat(path.join(storage_path, 'copy-test.mdb'));
-		const compaction = 100 - (stat_after.size / stat_before_compact.size) * 100;
 		assert(update_config_stub.called, 'updateConfigValue should be called');
 		assert(!console_error_spy.called, 'console.error should not be called');
 		assert(
-			compaction >= 85,
-			'after size ' + stat_after.size + ' should be' + ' much less than before size ' + stat_before_compact.size
+			stat_after.size <= stat_before_compact.size,
+			'after size ' + stat_after.size + ' should not exceed before size ' + stat_before_compact.size
 		);
+		let readable = 0;
+		for (let i = 0; i < 100; i++) if ((await TestTable.get(i))?.notIndexed) readable++;
+		assert.equal(readable, 100, 'every record should still be readable after compaction');
 	});
 
 	it('Test compactOnStart compacts and overwrites DB and keeps backups', async () => {
@@ -119,7 +128,45 @@ describe('Test database copy and compact', () => {
 		const stat_after = await fs.stat(path.join(storage_path, 'copy-test.mdb'));
 		assert(update_config_stub.called);
 		assert(!console_error_spy.called);
-		assert(stat_after.size < 2000000); // 2MB
+		assert(stat_after.size <= stat_before_compact.size);
 		assert(await fs.exists(path.join(storage_path, 'backup', 'copy-test.mdb')));
 	});
+
+	it('does not delete a database whose name matches another database compaction target', async () => {
+		const root_path_before = env_mgr.get(CONFIG_PARAMS.ROOTPATH);
+		const collision_root = path.join(storage_path, 'collision-root');
+		const collision_storage_path = path.join(collision_root, 'database');
+		const collision_path = path.join(collision_storage_path, 'collision-source-copy.mdb');
+		env_mgr.setProperty(CONFIG_PARAMS.ROOTPATH, collision_root);
+		env_mgr.setProperty(CONFIG_PARAMS.STORAGE_PATH, collision_storage_path);
+		resetDatabases();
+		try {
+			const CollisionSourceTable = table({
+				table: 'SourceTable',
+				database: 'collision-source',
+				attributes: [{ name: 'id', isPrimaryKey: true }],
+			});
+			const CollisionTable = table({
+				table: 'CollisionTable',
+				database: 'collision-source-copy',
+				attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'value' }],
+			});
+			await CollisionSourceTable.put({ id: 'source' });
+			await CollisionTable.put({ id: 'preserved', value: 'must survive compaction' });
+			assert.ok(getDatabases()['collision-source-copy'], 'the colliding database should be registered by name');
+			const bytes_before = await fs.readFile(collision_path);
+
+			await copyDB.compactOnStart();
+
+			assert.ok(await fs.exists(collision_path), 'the registered copy-target database should remain on disk');
+			assert.ok(
+				(await fs.readFile(collision_path)).equals(bytes_before),
+				'the registered copy-target database should remain byte-identical'
+			);
+		} finally {
+			env_mgr.setProperty(CONFIG_PARAMS.ROOTPATH, root_path_before);
+			env_mgr.setProperty(CONFIG_PARAMS.STORAGE_PATH, storage_path);
+			resetDatabases();
+		}
+	});
 });