fix(storage,workspace): post-review hardening
Review findings applied across the group: - storage hub: stale disposers no longer remove a successor registration; the package now default-exports the Storage service class per the service-package export shape. - json backend: failed publishes roll back the authoritative memory state (a rejected write can no longer resurface via get() or ride the next publish); close() drains in-flight writes and blocks in-flight opens; double-open rejects as a plain caller error instead of malformed-medium. - sqlite backend: loadAll builds records on a null prototype (__proto__ keys round-trip instead of polluting), user_version is stamped only after the schema is fully created, and corrupt record JSON rejects as malformed-medium instead of a bare SyntaxError. - domain form: writes persist before mutating authoritative memory or emitting; DomainChanged is a put/deleted discriminated union. - workspace: attach/detach idempotence decided on the write chain (stale snapshots no longer short-circuit), create() requires a directory, and startup fails loud on duplicate stored paths. Eleven regression tests pin the fixed behaviors.
This commit is contained in:
@@ -81,10 +81,6 @@ function configureDatabase(db: DatabaseSync, path: string, journalMode: JournalM
|
||||
`storage database at "${path}" has schema version ${onDisk}, incompatible with this build (${STORAGE_SQLITE_SCHEMA_VERSION})`,
|
||||
)
|
||||
}
|
||||
if (onDisk === 0) {
|
||||
// Stamp fresh databases.
|
||||
db.exec(`PRAGMA user_version = ${STORAGE_SQLITE_SCHEMA_VERSION}`)
|
||||
}
|
||||
db.exec(`
|
||||
CREATE TABLE IF NOT EXISTS units (
|
||||
name TEXT PRIMARY KEY,
|
||||
@@ -97,6 +93,12 @@ function configureDatabase(db: DatabaseSync, path: string, journalMode: JournalM
|
||||
value TEXT NOT NULL
|
||||
) STRICT
|
||||
`)
|
||||
if (onDisk === 0) {
|
||||
// Stamp fresh databases LAST: the stamp asserts the layout is complete,
|
||||
// so a failure above must leave the medium unstamped (a re-open after
|
||||
// the obstruction is cleared retries materialization from scratch).
|
||||
db.exec(`PRAGMA user_version = ${STORAGE_SQLITE_SCHEMA_VERSION}`)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -66,20 +66,35 @@ export class SqliteKvUnit implements KvUnit {
|
||||
this.ensureOpen()
|
||||
const tables: Record<string, Record<string, unknown>> = {}
|
||||
for (const [name, statements] of this.tables) {
|
||||
const records: Record<string, unknown> = {}
|
||||
// Null prototype: record keys are arbitrary strings, so '__proto__'
|
||||
// must land as an own property instead of mutating the prototype.
|
||||
const records: Record<string, unknown> = Object.create(null) as Record<string, unknown>
|
||||
for (const row of statements.selectAll.all() as unknown as Array<{ key: string; value: string }>) {
|
||||
records[row.key] = JSON.parse(row.value)
|
||||
records[row.key] = this.parseValue(row.value, `table '${name}' key '${row.key}'`)
|
||||
}
|
||||
tables[name] = records
|
||||
}
|
||||
let global: unknown = null
|
||||
if (this.globalSelect !== undefined) {
|
||||
const row = this.globalSelect.get(this.descriptor.name) as { value: string } | undefined
|
||||
if (row !== undefined) global = JSON.parse(row.value)
|
||||
if (row !== undefined) global = this.parseValue(row.value, 'global slot')
|
||||
}
|
||||
return { tables, global }
|
||||
}
|
||||
|
||||
/** Parse one stored value column, mapping bad JSON to `malformed-medium`. */
|
||||
private parseValue(text: string, slot: string): unknown {
|
||||
try {
|
||||
return JSON.parse(text)
|
||||
} catch (error) {
|
||||
throw new StorageError(
|
||||
'malformed-medium',
|
||||
`kv unit '${this.descriptor.name}' holds unparsable JSON at ${slot}`,
|
||||
{ cause: error },
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
async putRecord(table: string, key: string, value: unknown): Promise<void> {
|
||||
this.ensureOpen()
|
||||
this.statementsFor(table).upsert.run(key, JSON.stringify(value))
|
||||
|
||||
@@ -106,4 +106,85 @@ describe('sqlite backend specifics', () => {
|
||||
await backend.close()
|
||||
await expect(backend.kv.open(DESCRIPTOR)).rejects.toMatchObject({ code: 'closed' })
|
||||
})
|
||||
|
||||
it('round-trips prototype-polluting keys as own properties', async () => {
|
||||
const backend = backendAt(':memory:')
|
||||
const unit = await backend.kv.open(DESCRIPTOR)
|
||||
await unit.putRecord('records', '__proto__', { evil: true })
|
||||
await unit.putRecord('records', 'constructor', { n: 1 })
|
||||
const { tables } = await unit.loadAll()
|
||||
const records = tables['records']!
|
||||
expect(Object.hasOwn(records, '__proto__')).toBe(true)
|
||||
expect(records['__proto__']).toEqual({ evil: true })
|
||||
expect(records['constructor']).toEqual({ n: 1 })
|
||||
expect(Object.getPrototypeOf({})).not.toHaveProperty('evil')
|
||||
await backend.close()
|
||||
})
|
||||
|
||||
it('leaves a failed materialization unstamped so a repaired medium reopens', async () => {
|
||||
const path = await freshDbPath()
|
||||
// Obstruct table creation: an index squatting on the unit_globals name
|
||||
// makes CREATE TABLE IF NOT EXISTS throw AFTER the units table exists.
|
||||
const setup = new DatabaseSync(path)
|
||||
setup.exec('CREATE TABLE squatter (x TEXT)')
|
||||
setup.exec('CREATE INDEX unit_globals ON squatter(x)')
|
||||
setup.close()
|
||||
|
||||
const broken = backendAt(path)
|
||||
await expect(broken.kv.open(DESCRIPTOR)).rejects.toThrow(/already an index/)
|
||||
await broken.close()
|
||||
|
||||
// Clear the obstruction; the medium must still be version 0, not a
|
||||
// half-materialized database stamped as current.
|
||||
const repair = new DatabaseSync(path)
|
||||
expect((repair.prepare('PRAGMA user_version').get() as { user_version: number }).user_version).toBe(0)
|
||||
repair.exec('DROP INDEX unit_globals')
|
||||
repair.close()
|
||||
|
||||
const backend = backendAt(path)
|
||||
const unit = await backend.kv.open(DESCRIPTOR)
|
||||
await unit.putRecord('records', 'k', { n: 1 })
|
||||
await backend.close()
|
||||
})
|
||||
|
||||
it('rejects unparsable stored JSON with malformed-medium', async () => {
|
||||
const path = await freshDbPath()
|
||||
const backend = backendAt(path)
|
||||
const unit = await backend.kv.open(DESCRIPTOR)
|
||||
await unit.putRecord('records', 'good', { n: 1 })
|
||||
await unit.setGlobal({ g: 1 })
|
||||
await backend.close()
|
||||
|
||||
const db = new DatabaseSync(path)
|
||||
db.prepare('UPDATE u_specimen_records SET value = ? WHERE key = ?').run('{not json', 'good')
|
||||
db.close()
|
||||
|
||||
const reopened = backendAt(path)
|
||||
const damaged = await reopened.kv.open(DESCRIPTOR)
|
||||
await expect(damaged.loadAll()).rejects.toMatchObject({
|
||||
name: 'StorageError',
|
||||
code: 'malformed-medium',
|
||||
})
|
||||
await reopened.close()
|
||||
})
|
||||
|
||||
it('rejects an unparsable global slot with malformed-medium', async () => {
|
||||
const path = await freshDbPath()
|
||||
const backend = backendAt(path)
|
||||
const unit = await backend.kv.open(DESCRIPTOR)
|
||||
await unit.setGlobal({ g: 1 })
|
||||
await backend.close()
|
||||
|
||||
const db = new DatabaseSync(path)
|
||||
db.prepare('UPDATE unit_globals SET value = ? WHERE unit = ?').run('][', 'specimen')
|
||||
db.close()
|
||||
|
||||
const reopened = backendAt(path)
|
||||
const damaged = await reopened.kv.open(DESCRIPTOR)
|
||||
await expect(damaged.loadAll()).rejects.toMatchObject({
|
||||
name: 'StorageError',
|
||||
code: 'malformed-medium',
|
||||
})
|
||||
await reopened.close()
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user