Skip to content

Commit 06e4fe4

Browse files
kriszypclaude
andcommitted
fix(databases): sync non-indexed attributes in initStores() on resetDatabases() (RE-7)
`initStores()` only merged indexed attributes back into `table.attributes` when updating an existing table (e.g. after a hot-reload–triggered `resetDatabases()`). Non-indexed, non-primary-key fields (e.g. `name: String`, `breed: String`) were written to `attributesDbi` by `table()` in the restarted worker and signalled via ITC, but the main thread's `resetDatabases()` → `initStores()` path skipped them — leaving `describe_database` returning only the old attribute list until a full kill+restart. Fix: extend the existing-table update loop in `initStores()` to add/replace non-indexed attributes as well as indexed ones, and remove them when they are no longer present in the persisted schema. Adds two regression tests in schemaMigrationFragility.test.js (RE-7 suite). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 88c94e6 commit 06e4fe4

2 files changed

Lines changed: 90 additions & 0 deletions

File tree

resources/databases.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,19 @@ function initStores(
620620
if (existingAttribute) existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1, attribute);
621621
else existingAttributes.push(attribute);
622622
attributesUpdated = true;
623+
} else if (!attribute.isPrimaryKey) {
624+
// Non-indexed, non-primary-key attributes (e.g. plain schema fields like `name: String`)
625+
// must also be kept in sync so that describe_database reflects schema changes after a
626+
// hot-reload / worker restart. Without this, resetDatabases() re-reads these attributes
627+
// from attributesDbi but never merges them back into table.attributes — causing stale
628+
// schema metadata until a full kill+restart. (RE-7)
629+
const existingIdx = existingAttributes.findIndex((ea) => ea.name === attribute.name);
630+
if (existingIdx >= 0) {
631+
existingAttributes.splice(existingIdx, 1, attribute);
632+
} else {
633+
existingAttributes.push(attribute);
634+
attributesUpdated = true;
635+
}
623636
}
624637
} catch (error) {
625638
logger.error(`Error trying to update attribute`, attribute, existingAttributes, indices, error);
@@ -647,6 +660,10 @@ function initStores(
647660
// we only remove attributes if they were indexed, in order to support dropAttribute that removes dynamic indexed attributes
648661
existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1);
649662
attributesUpdated = true;
663+
} else if (!existingAttribute.isPrimaryKey) {
664+
// Remove non-indexed attributes that are no longer present in the persisted schema.
665+
existingAttributes.splice(existingAttributes.indexOf(existingAttribute), 1);
666+
attributesUpdated = true;
650667
}
651668
}
652669
}

unitTests/resources/schemaMigrationFragility.test.js

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,79 @@ describe('schema-migration fragility: stale `changed` reused after re-fetch unde
382382
});
383383
});
384384

385+
describe('schema-migration fragility: non-indexed attributes missing from table.attributes after resetDatabases() (RE-7)', () => {
386+
if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return;
387+
388+
const DB = 're7NonIndexedAttrs';
389+
const TABLE = 'Pet';
390+
const testRoot = path.resolve(__dirname, '../envDir/re7NonIndexedAttrs');
391+
const dbDir = path.join(testRoot, terms.DATABASES_DIR_NAME);
392+
393+
before(async () => {
394+
setMainIsWorker(true);
395+
await fs.remove(testRoot);
396+
await fs.mkdirp(dbDir);
397+
env.setProperty(terms.HDB_SETTINGS_NAMES.HDB_ROOT_KEY, testRoot);
398+
env.setProperty(terms.CONFIG_PARAMS.ROOTPATH, testRoot);
399+
env.setProperty(terms.CONFIG_PARAMS.STORAGE_PATH, dbDir);
400+
env.setProperty(terms.CONFIG_PARAMS.DATABASES, {});
401+
402+
resetDatabases();
403+
// Initial schema: only the primary key
404+
table({
405+
table: TABLE,
406+
database: DB,
407+
attributes: [{ name: 'id', isPrimaryKey: true }],
408+
});
409+
// Simulate hot-reload: call table() again with an expanded schema (non-indexed fields).
410+
// This is what the graphqlSchema plugin does in a restarted worker.
411+
table({
412+
table: TABLE,
413+
database: DB,
414+
attributes: [
415+
{ name: 'id', isPrimaryKey: true },
416+
{ name: 'name' },
417+
{ name: 'breed' },
418+
{ name: 'age' },
419+
],
420+
});
421+
// Simulate the ITC schema-change handler calling resetDatabases() in the main thread
422+
// (the path that describe_database goes through).
423+
resetDatabases();
424+
});
425+
426+
after(async () => {
427+
await fs.remove(testRoot);
428+
});
429+
430+
it('table.attributes includes all non-indexed fields after resetDatabases()', () => {
431+
const tbl = getDatabases()[DB]?.[TABLE];
432+
assert.ok(tbl, `${DB}.${TABLE} should be registered after resetDatabases()`);
433+
const attrNames = tbl.attributes.map((a) => a.name);
434+
assert.ok(attrNames.includes('name'), `expected "name" in attributes, got: ${attrNames}`);
435+
assert.ok(attrNames.includes('breed'), `expected "breed" in attributes, got: ${attrNames}`);
436+
assert.ok(attrNames.includes('age'), `expected "age" in attributes, got: ${attrNames}`);
437+
});
438+
439+
it('removes non-indexed attributes dropped from the schema after resetDatabases()', () => {
440+
// Simulate schema shrinking (field removal), then another resetDatabases().
441+
table({
442+
table: TABLE,
443+
database: DB,
444+
attributes: [
445+
{ name: 'id', isPrimaryKey: true },
446+
{ name: 'name' },
447+
],
448+
});
449+
resetDatabases();
450+
const tbl = getDatabases()[DB]?.[TABLE];
451+
const attrNames = tbl.attributes.map((a) => a.name);
452+
assert.ok(attrNames.includes('name'), `expected "name" in attributes`);
453+
assert.ok(!attrNames.includes('breed'), `"breed" should be removed, got: ${attrNames}`);
454+
assert.ok(!attrNames.includes('age'), `"age" should be removed, got: ${attrNames}`);
455+
});
456+
});
457+
385458
describe('schema-migration fragility: stale store reused after LMDB to RocksDB engine migration (F4)', () => {
386459
if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return;
387460

0 commit comments

Comments
 (0)