Skip to content

fix: backfill schemaDefined on replicas missing the flag #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ldt1996 wants to merge 2 commits into
base: main
Choose a base branch
from
+156 −14
Open

fix: backfill schemaDefined on replicas missing the flag #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
290d78b
fix: backfill schemaDefined on replicas missing the flag
ldt1996 May 11, 2026
e0f4e63
updates
ldt1996 May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions bin/copyDb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,10 @@ async function copyDbToRocks(sourceRootStore, sourceDatabase: string, targetPath
} of sourceDbi.getRange({ start, transaction, versions: true })) {
try {
start = key;
if (typeof key === 'symbol') {
skippedRecord++;
continue;
}
if (value == null) {
skippedRecord++;
continue;
Expand Down Expand Up @@ -497,6 +501,9 @@ async function copyDbToRocks(sourceRootStore, sourceDatabase: string, targetPath
for (const { key, value } of sourceDbi.getRange({ start, transaction })) {
try {
start = key;
if (typeof key === 'symbol') {
continue;
}
written = targetDbi.put(key, value);
recordsCopied++;
if (transaction.openTimer) transaction.openTimer = 0;
Expand Down
156 changes: 144 additions & 12 deletions integrationTests/upgrade/4.x-upgrade.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,25 @@ import {
type ContextWithHarper,
killHarper,
} from '@harperfast/integration-testing';
import { ok } from 'node:assert';
import { ok, deepStrictEqual } from 'node:assert';
import { join } from 'node:path';
import { existsSync } from 'node:fs';
import { existsSync, readdirSync, statSync } from 'node:fs';

const WIDGET_COUNT = 60;
const buildWidgets = () =>
Array.from({ length: WIDGET_COUNT }, (_, i) => ({
id: `w-${i}`,
name: `widget-${i}`,
category: i % 3 === 0 ? 'A' : i % 3 === 1 ? 'B' : 'C',
price: Number((9.99 + i).toFixed(2)),
inStock: i % 2 === 0,
tags: [`tag${i % 5}`, `bucket${i % 4}`],
}));

const legacyPath = process.env.HARPER_LEGACY_VERSION_PATH;
suite('Start 4.x server and test upgrade', { skip: !legacyPath }, (ctx: ContextWithHarper) => {
const widgets = buildWidgets();

before(async () => {
await startHarper(ctx, {
config: {},
Expand Down Expand Up @@ -48,6 +61,23 @@ suite('Start 4.x server and test upgrade', { skip: !legacyPath }, (ctx: ContextW
records: [{ id: 'id-' + Math.floor(Math.random() * 10), name: 'test data ' + Math.random() }],
});
}

await sendOperation(ctx.harper, {
operation: 'create_table',
table: 'widgets',
primary_key: 'id',
attributes: [
{ name: 'id', type: 'ID' },
{ name: 'name', type: 'String' },
{ name: 'category', type: 'String' },
{ name: 'price', type: 'Float' },
{ name: 'inStock', type: 'Boolean' },
{ name: 'tags', type: 'Any' },
],
});
for (const widget of widgets) {
await sendOperation(ctx.harper, { operation: 'upsert', table: 'widgets', records: [widget] });
}
});

after(async () => {
Expand Down Expand Up @@ -97,23 +127,125 @@ suite('Start 4.x server and test upgrade', { skip: !legacyPath }, (ctx: ContextW

test('upgrade and migrate LMDB to RocksDB', async () => {
await killHarper(ctx);
// restart with migrateOnStart enabled

const walk = (dir: string, depth = 0, max = 4): string[] => {
if (depth > max || !existsSync(dir)) return [];
let entries: string[] = [];
try {
for (const name of readdirSync(dir)) {
const p = join(dir, name);
try {
const st = statSync(p);
entries.push(`${' '.repeat(depth)}${name}${st.isDirectory() ? '/' : ` (${st.size}b)`}`);
if (st.isDirectory()) entries = entries.concat(walk(p, depth + 1, max));
} catch {}
}
} catch {}
return entries;
};
console.log(`[precondition] dataRootDir=${ctx.harper.dataRootDir}`);
console.log(`[precondition] contents:\n${walk(ctx.harper.dataRootDir).join('\n')}`);
const mdbCandidates = walk(ctx.harper.dataRootDir)
.filter((line) => line.includes('.mdb') && !line.includes('lock'))
.map((line) => line.trim().split(' ')[0]);
console.log(`[precondition] .mdb-ish entries found:`, mdbCandidates);

const candidateLmdbPaths = [
join(ctx.harper.dataRootDir, 'database', 'data.mdb'),
join(ctx.harper.dataRootDir, 'schema', 'data.mdb'),
join(ctx.harper.dataRootDir, 'database', 'data.mdb', 'data.mdb'),
join(ctx.harper.dataRootDir, 'schema', 'data', 'data.mdb'),
];
const lmdbPath = candidateLmdbPaths.find((p) => existsSync(p));
if (lmdbPath) {
console.log(`[precondition] opening LMDB at ${lmdbPath}`);
const { open: openLmdb } = await import('lmdb');
const env = openLmdb({ path: lmdbPath, readOnly: true });
try {
console.log(
`[precondition] DBIs in env:`,
[...env.getKeys({ start: undefined, limit: 50 })]
);
const widgetsDbi = env.openDB({ name: 'widgets/', encoding: 'binary' });
const structuresBuffer = widgetsDbi.getBinary(Symbol.for('structures'));
console.log(
`[precondition] widgets shared structures buffer:`,
structuresBuffer ? `${structuresBuffer.length} bytes` : 'NULL'
);
ok(
structuresBuffer && structuresBuffer.length > 0,
`source LMDB widgets DBI must have populated shared structures before migration; ` +
`got ${structuresBuffer ? structuresBuffer.length : 0} bytes. Increase WIDGET_COUNT ` +
`or widen the record shape so msgpackr promotes the structure into the shared dict.`
);
} finally {
await env.close();
}
} else {
throw new Error(
`Could not locate the v4 LMDB file. Tried: ${candidateLmdbPaths.join(', ')}. ` +
`See the directory tree printed above and add the correct path to candidateLmdbPaths.`
);
}

await startHarper(ctx, {
config: {
storage: {
migrateOnStart: true,
},
},
config: { storage: { migrateOnStart: true } },
env: {},
});
// verify data is still accessible after migration
const response = await sendOperation(ctx.harper, {

const testTableResponse = await sendOperation(ctx.harper, {
operation: 'search_by_conditions',
table: 'test',
conditions: [{ attribute: 'id', comparator: 'greater_than', value: 'id-4' }],
});
ok(response.length > 4);
ok(testTableResponse.length > 4);
ok(existsSync(join(ctx.harper.dataRootDir, 'database', 'data', 'CURRENT')));
ok(existsSync(join(ctx.harper.dataRootDir, 'database', 'system', 'CURRENT'))); // marker for rocksdb
ok(existsSync(join(ctx.harper.dataRootDir, 'database', 'system', 'CURRENT')));

for (const expected of widgets) {
const rows = await sendOperation(ctx.harper, {
operation: 'search_by_conditions',
table: 'widgets',
conditions: [{ attribute: 'id', comparator: 'equals', value: expected.id }],
});
ok(rows.length === 1, `expected exactly 1 row for ${expected.id}, got ${rows.length}`);
const actual = rows[0];
deepStrictEqual(
{
id: actual.id,
name: actual.name,
category: actual.category,
price: actual.price,
inStock: actual.inStock,
tags: actual.tags,
},
expected,
`record ${expected.id} did not round-trip cleanly through migration`
);
}

const byName = await sendOperation(ctx.harper, {
operation: 'search_by_conditions',
table: 'widgets',
conditions: [{ attribute: 'name', comparator: 'equals', value: 'widget-7' }],
});
ok(byName.length === 1 && byName[0].id === 'w-7', 'index lookup on widgets.name should resolve to w-7 after migration');

await killHarper(ctx);
const { RocksDatabase } = await import('@harperfast/rocksdb-js');
const widgetsCF = RocksDatabase.open(join(ctx.harper.dataRootDir, 'database', 'data'), {
name: 'widgets/',
sharedStructuresKey: Symbol.for('structures'),
});
try {
const keys = [...widgetsCF.getKeys()];
const symbolKeys = keys.filter((k) => typeof k === 'symbol');
ok(
symbolKeys.length === 0,
`widgets/ primary CF must not contain symbol-keyed entries post-migration; found ${symbolKeys.length}: ${symbolKeys.map((s) => s.toString()).join(', ')}`
);
} finally {
widgetsCF.close();
}
});
});
7 changes: 5 additions & 2 deletions resources/databases.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ function initStores(
databaseName,
indices,
attributes,
schemaDefined: primaryAttribute.schemaDefined,
schemaDefined: primaryAttribute.schemaDefined ?? true,
dbisDB: attributesDbi,
})
);
Expand Down Expand Up @@ -925,6 +925,7 @@ export function table<TableResourceType>(tableDefinition: TableDefinition): Tabl
// it table already exists, get the split segments setting
if (splitSegments == undefined) splitSegments = Table.splitSegments;
Table.attributes.splice(0, Table.attributes.length, ...attributes);
if (Table.schemaDefined === undefined) Table.schemaDefined = schemaDefined;
} else {
const auditStore = rootStore.auditStore;
primaryKeyAttribute = attributes.find((attribute) => attribute.isPrimaryKey) || {};
Expand Down Expand Up @@ -1049,7 +1050,6 @@ export function table<TableResourceType>(tableDefinition: TableDefinition): Tabl
}
const attributesToIndex = [];
try {
// TODO: If we have attributes and the schemaDefined flag is not set, turn it on
// iterate through the attributes to ensure that we have all the dbis created and indexed
for (const attribute of attributes || []) {
if (attribute.relationship || attribute.computed) {
Expand All @@ -1061,8 +1061,10 @@ export function table<TableResourceType>(tableDefinition: TableDefinition): Tabl
let attributeDescriptor = attributesDbi.getSync(dbiKey);
if (attribute.isPrimaryKey) {
attributeDescriptor = attributeDescriptor || attributesDbi.getSync((dbiKey = tableName + '/')) || {};
const needsSchemaDefinedBackfill = attributeDescriptor.schemaDefined === undefined;
// primary key can't change indexing, but settings can change
if (
needsSchemaDefinedBackfill ||
(audit !== undefined && audit !== Table.audit) ||
(sealed !== undefined && sealed !== Table.sealed) ||
(replicate !== undefined && replicate !== Table.replicate) ||
Expand All @@ -1080,6 +1082,7 @@ export function table<TableResourceType>(tableDefinition: TableDefinition): Tabl
if (sealed !== undefined) updatedPrimaryAttribute.sealed = sealed;
if (replicate !== undefined) updatedPrimaryAttribute.replicate = replicate;
if (attribute.type) updatedPrimaryAttribute.type = attribute.type;
if (needsSchemaDefinedBackfill) updatedPrimaryAttribute.schemaDefined = schemaDefined;
Comment on lines 1064 to +1085
Copy link
Copy Markdown
Contributor

@claude claude Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test for the backfill path

What: The needsSchemaDefinedBackfill check introduces a two-leg runtime-shape branch (schemaDefined === undefined vs present) with no test covering the backfill leg. The normal leg (descriptor already has schemaDefined) is exercised by existing tests; the new leg is not.

Why it matters: A regression here — e.g. the write silently no-ops or persists the wrong value — would leave replica tables still missing the flag after a restart. The fix looks correct today, but without a test this is invisible to CI.

Suggested fix: Add a unit test (alongside the existing unitTests/resources/databases.test.js suite) that:

  1. Opens or creates a table (so schemaDefined=true is written to the descriptor).
  2. Manually overwrites the primary-key entry in attributesDbi with a copy that omits schemaDefined (simulating an old replica).
  3. Calls table() again with the same definition.
  4. Asserts that Table.schemaDefined === true and that the stored descriptor now contains schemaDefined: true.

hasChanges = true; // send out notification of the change
exclusiveLock();
attributesDbi.put(dbiKey, updatedPrimaryAttribute);
Expand Down