From 2fea2da42c643ac0ec57ae08e43423a128da4847 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Fri, 16 Oct 2026 06:43:03 +0100 Subject: [PATCH 1/2] lib: one cascade graph for every store Which children a hard delete takes with it was decided separately in each backend. mysql gets each of them from a FOREIGN KEY. The file stores have no constraints and never implemented them, so destroying a group left its nameservers behind, a user its sessions, and a zone its records. references.js names those three edges once. The file stores call cascade() from destroy() before removing the row, mysql goes on getting it from the constraints, and references.test.js holds whichever store is configured to the same graph. Each case seeds a second parent whose child must survive, so a store that takes too much fails as loudly as one that takes too little. --- CHANGELOG.md | 1 + lib/group/store/file.js | 4 + lib/references.js | 35 ++++++++ lib/references.test.js | 181 ++++++++++++++++++++++++++++++++++++++++ lib/user/store/file.js | 4 + lib/zone/store/file.js | 4 + 6 files changed, 229 insertions(+) create mode 100644 lib/references.js create mode 100644 lib/references.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 41f0360..7abac6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - fix(sql): lowercase existing zone names, and the zone in record names - fix: one random, self describing session row per login, expired ones swept at login, and mysql throttles the activity touch - fix: a token is valid only while its session row exists, so logout revokes it, and any authenticated request counts as activity +- fix: destroying a group, user or zone on json or toml takes its nameservers, sessions or records with it, as mysql does ### [3.0.3] - 2026-07-27 diff --git a/lib/group/store/file.js b/lib/group/store/file.js index 76f244c..43961c9 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -3,6 +3,8 @@ import { idConflict } from '../../store/error.js' import GroupBase from './base.js' +import { cascade } from '../../references.js' + const defaultPermissions = { inherit: false, self_write: false, @@ -150,6 +152,8 @@ class GroupRepoFile extends GroupBase { } async destroy(args) { + if (!(await this._load()).some((g) => g.id === args.id)) return false + await cascade('group', args.id) return this.file.mutate('group', (groups) => removeRows(groups, (g) => g.id === args.id) > 0) } } diff --git a/lib/references.js b/lib/references.js new file mode 100644 index 0000000..2748d81 --- /dev/null +++ b/lib/references.js @@ -0,0 +1,35 @@ +// What a hard delete takes with it. mysql enforces each of these with a +// FOREIGN KEY; the file stores have none, so their destroy() calls cascade(). +const cascades = [ + { parent: 'group', child: 'nameserver', via: 'gid' }, + { parent: 'user', child: 'session', via: 'uid' }, + { parent: 'zone', child: 'zone_record', via: 'zid' }, +] + +// Imported when a cascade runs, so a child store loading its own parent does +// not close a circle at module load. +const childStores = { + nameserver: () => import('./nameserver/index.js'), + session: () => import('./session/index.js'), + zone_record: () => import('./zone_record/index.js'), +} + +export async function cascade(parent, id) { + for (const edge of cascades.filter((e) => e.parent === parent)) { + const store = (await childStores[edge.child]()).default + + // Session alone has no destroy(); its delete() is the hard one and clears + // every session for the parent in a single call. + if (typeof store.destroy !== 'function') { + await store.delete({ [edge.via]: id }) + continue + } + + for (const deleted of [false, true]) { + const found = await store.get({ [edge.via]: id, deleted }) + for (const child of [found].flat().filter(Boolean)) await store.destroy({ id: child.id }) + } + } +} + +export default cascades diff --git a/lib/references.test.js b/lib/references.test.js new file mode 100644 index 0000000..1d40262 --- /dev/null +++ b/lib/references.test.js @@ -0,0 +1,181 @@ +import assert from 'node:assert/strict' +import { after, before, describe, it } from 'node:test' + +import { storeType } from './config.js' +import cascades from './references.js' + +import Group from './group/index.js' +import Nameserver from './nameserver/index.js' +import Session from './session/index.js' +import User from './user/index.js' +import Zone from './zone/index.js' +import ZoneRecord from './zone_record/index.js' + +const GID = 47101 +const ids = { + group: [47101, 47111, 47121], + user: [47102, 47112, 47122], + zone: [47103, 47113, 47123], + nameserver: [47104, 47114, 47124], + session: [47105, 47115, 47125], + zone_record: [47106, 47116, 47126], +} + +const entities = { + group: { store: Group, body: (id) => ({ id, parent_gid: 0, name: `g${id}.refgraph.test` }) }, + user: { + store: User, + body: (id) => ({ + id, + gid: GID, + username: `refgraph${id}`, + email: `refgraph${id}@example.com`, + password: 'Wh@tA-Decent#P6ssw0rd', + first_name: 'Ref', + last_name: 'Graph', + is_admin: false, + }), + }, + zone: { + store: Zone, + body: (id) => ({ + id, + gid: GID, + zone: `z${id}.refgraph.test`, + mailaddr: `hostmaster.z${id}.refgraph.test.`, + serial: 1, + refresh: 1, + retry: 2, + expire: 3, + minimum: 4, + ttl: 3600, + }), + }, + nameserver: { + store: Nameserver, + body: (id) => ({ id, name: `ns${id}.refgraph.test.`, address: '203.0.113.10', type: 'nsd', ttl: 3600 }), + }, + session: { store: Session, body: (id) => ({ id, session: `refgraph-${id}`, last_access: 1700000000 }) }, + zone_record: { + store: ZoneRecord, + body: (id) => ({ id, owner: `r${id}.refgraph.test.`, type: 'A', address: '203.0.113.9', ttl: 3600 }), + }, +} + +const softDeletes = (name) => typeof entities[name].store.destroy === 'function' +const rows = (found) => [found].flat().filter(Boolean).length + +async function present(name, id) { + const { store } = entities[name] + if (!softDeletes(name)) return rows(await store.get({ id })) + let n = 0 + for (const deleted of [false, true]) n += rows(await store.get({ id, deleted })) + return n +} + +async function remove(name, id) { + const { store } = entities[name] + return softDeletes(name) ? store.destroy({ id }) : store.delete({ id }) +} + +// A store may allocate its own id, so seed() reports the one it used. +async function seed(name, body) { + const returned = await entities[name].store.create(body) + return Number.isInteger(returned) ? returned : body.id +} + +async function reset() { + for (const uid of ids.user) await Session.delete({ uid }) + for (const name of ['zone_record', 'session', 'zone', 'nameserver', 'user', 'group']) { + for (const id of ids[name]) await remove(name, id) + } +} + +before(reset) + +after(async () => { + await reset() + for (const { store } of Object.values(entities)) await store.disconnect?.() +}) + +describe('cascade graph', () => { + it('declares the edges the constraints enforce', () => { + assert.deepEqual( + cascades.map((e) => `${e.parent} -> ${e.child}`).sort(), + ['group -> nameserver', 'user -> session', 'zone -> zone_record'], + 'an edge was added or dropped; the file stores follow this list', + ) + for (const edge of cascades) { + assert.ok(entities[edge.parent], `no fixture for parent ${edge.parent}`) + assert.ok(entities[edge.child], `no fixture for child ${edge.child}`) + assert.ok(edge.via, `${edge.parent} -> ${edge.child} declares no via`) + } + }) + + for (const edge of cascades) { + it(`destroying a ${edge.parent} takes its ${edge.child} rows (${edge.via})`, async () => { + await reset() + await seed('group', entities.group.body(GID)) + + const [doomed, spared] = ids[edge.parent] + if (edge.parent === 'group') await seed('group', entities.group.body(spared)) + else for (const id of [doomed, spared]) await seed(edge.parent, entities[edge.parent].body(id)) + + const [first, other, extra] = ids[edge.child] + const child = await seed(edge.child, { ...entities[edge.child].body(first), [edge.via]: doomed }) + const sibling = await seed(edge.child, { ...entities[edge.child].body(extra), [edge.via]: doomed }) + const control = await seed(edge.child, { ...entities[edge.child].body(other), [edge.via]: spared }) + + if (softDeletes(edge.child)) await entities[edge.child].store.delete({ id: sibling }) + + for (const [id, what] of [ + [child, 'child'], + [sibling, 'sibling'], + [control, 'control'], + ]) { + assert.equal(await present(edge.child, id), 1, `the ${what} did not seed`) + } + + assert.ok(await remove(edge.parent, doomed), `destroying the ${edge.parent} reported no change`) + assert.equal(await present(edge.parent, doomed), 0, `the ${edge.parent} outlived its own destroy`) + + assert.equal(await present(edge.child, child), 0, `the ${edge.child} outlived its ${edge.parent}`) + assert.equal(await present(edge.child, sibling), 0, `a soft-deleted ${edge.child} was left behind`) + assert.equal(await present(edge.child, control), 1, `an unrelated ${edge.child} was taken too`) + + // mysql reads a session through its user, so a surviving row would read + // as absent. Deleting it again reports whether it is really gone. + for (const id of [child, sibling]) { + assert.equal(await remove(edge.child, id), false, `a ${edge.child} row was still there to delete`) + } + }) + } + + it('leaves the parent in place when a cascade fails', async () => { + await reset() + await seed('group', entities.group.body(GID)) + const zid = ids.zone[0] + await seed('zone', entities.zone.body(zid)) + const rec = await seed('zone_record', { ...entities.zone_record.body(ids.zone_record[0]), zid }) + + if (storeType() === 'mysql') { + assert.ok(await Zone.destroy({ id: zid }), 'the zone did not destroy') + assert.equal(await present('zone_record', rec), 0, 'the constraint left the record') + return + } + + const original = ZoneRecord.get.bind(ZoneRecord) + ZoneRecord.get = async () => { + throw new Error('cascade probe') + } + try { + await assert.rejects(() => Zone.destroy({ id: zid }), /cascade probe/) + } finally { + ZoneRecord.get = original + } + + assert.equal(await present('zone', zid), 1, 'the zone went while its records stayed') + assert.ok(await Zone.destroy({ id: zid }), 'the retry found no zone to destroy') + assert.equal(await present('zone_record', rec), 0, 'the record outlived the retry') + }) +}) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index f0b6318..18d4311 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -5,6 +5,8 @@ import Credentials from '../credentials.js' import UserBase from './base.js' import { splitQualifiedUsername } from '../qualified.js' +import { cascade } from '../../references.js' + const boolFields = ['is_admin', 'deleted'] // Read the group file directly (never via the Group module) to avoid circular imports. @@ -204,6 +206,8 @@ class UserRepoFile extends UserBase { } async destroy(args) { + if (!(await this._load()).some((u) => u.id === args.id)) return false + await cascade('user', args.id) return this.file.mutate('user', (users) => removeRows(users, (u) => u.id === args.id) > 0) } } diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index a697515..77dc493 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -3,6 +3,8 @@ import { idConflict } from '../../store/error.js' import ZoneBase, { canonicalZoneName, sortNameservers, zoneNameConflict } from './base.js' +import { cascade } from '../../references.js' + const zoneDefaults = { minimum: 3600, ttl: 3600, refresh: 86400, retry: 7200, expire: 1209600 } function assertZoneNameAvailable(zones, zone, excludeId) { @@ -174,6 +176,8 @@ class ZoneRepoFile extends ZoneBase { } async destroy(args) { + if (!(await this._load()).some((z) => z.id === args.id)) return false + await cascade('zone', args.id) return this.file.mutate('zone', (zones) => removeRows(zones, (z) => z.id === args.id) > 0) } From 9fcce9c5c2a54f52fad9d17c6c910193fd614750 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Fri, 16 Oct 2026 22:54:06 +0100 Subject: [PATCH 2/2] cascade: destroy a parent's children in one write --- lib/nameserver/store/file.js | 8 ++-- lib/nameserver/store/mysql.js | 10 +++-- lib/references.js | 15 ++----- lib/references.test.js | 72 ++++++++++++++++++++++++++++++++-- lib/store/file.js | 7 ++++ lib/zone_record/store/file.js | 5 ++- lib/zone_record/store/mysql.js | 10 +++-- 7 files changed, 100 insertions(+), 27 deletions(-) diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index 8598c41..0d689c8 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { claimId, removeRows } from '../../store/file.js' +import FileStore, { claimId, destroyMatch, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import NameserverBase from './base.js' @@ -85,10 +85,8 @@ class NameserverRepoFile extends NameserverBase { } async destroy(args) { - return this.file.mutate( - 'nameserver', - (nameservers) => removeRows(nameservers, (n) => n.id === args.id) > 0, - ) + const match = destroyMatch(args, ['id', 'gid']) + return this.file.mutate('nameserver', (nameservers) => removeRows(nameservers, match) > 0) } } diff --git a/lib/nameserver/store/mysql.js b/lib/nameserver/store/mysql.js index 3c04278..0be093b 100644 --- a/lib/nameserver/store/mysql.js +++ b/lib/nameserver/store/mysql.js @@ -95,9 +95,13 @@ class Nameserver extends NameserverBase { return r.changedRows === 1 } - async destroy(args) { - const r = await Mysql.execute(...Mysql.delete(`nt_nameserver`, { nt_nameserver_id: args.id })) - return r.affectedRows === 1 + async destroy({ id, gid }) { + if (id === undefined && gid === undefined) throw new Error('destroy() needs one of id, gid') + const where = {} + if (id !== undefined) where.nt_nameserver_id = id + if (gid !== undefined) where.nt_group_id = gid + const r = await Mysql.execute(...Mysql.delete(`nt_nameserver`, where)) + return r.affectedRows > 0 } disconnect() { diff --git a/lib/references.js b/lib/references.js index 2748d81..80eef54 100644 --- a/lib/references.js +++ b/lib/references.js @@ -18,17 +18,10 @@ export async function cascade(parent, id) { for (const edge of cascades.filter((e) => e.parent === parent)) { const store = (await childStores[edge.child]()).default - // Session alone has no destroy(); its delete() is the hard one and clears - // every session for the parent in a single call. - if (typeof store.destroy !== 'function') { - await store.delete({ [edge.via]: id }) - continue - } - - for (const deleted of [false, true]) { - const found = await store.get({ [edge.via]: id, deleted }) - for (const child of [found].flat().filter(Boolean)) await store.destroy({ id: child.id }) - } + // One call per edge, matched on the parent key at write time, so a child + // moved to another parent stays. Session's delete() is its hard delete. + const remove = typeof store.destroy === 'function' ? store.destroy : store.delete + await remove.call(store, { [edge.via]: id }) } } diff --git a/lib/references.test.js b/lib/references.test.js index 1d40262..cc9fc85 100644 --- a/lib/references.test.js +++ b/lib/references.test.js @@ -164,18 +164,84 @@ describe('cascade graph', () => { return } - const original = ZoneRecord.get.bind(ZoneRecord) - ZoneRecord.get = async () => { + ZoneRecord.destroy = async () => { throw new Error('cascade probe') } try { await assert.rejects(() => Zone.destroy({ id: zid }), /cascade probe/) } finally { - ZoneRecord.get = original + delete ZoneRecord.destroy } assert.equal(await present('zone', zid), 1, 'the zone went while its records stayed') assert.ok(await Zone.destroy({ id: zid }), 'the retry found no zone to destroy') assert.equal(await present('zone_record', rec), 0, 'the record outlived the retry') }) + + it('spares a record moved to another zone while the cascade runs', async () => { + await reset() + await seed('group', entities.group.body(GID)) + const [doomed, spared] = ids.zone + for (const id of [doomed, spared]) await seed('zone', entities.zone.body(id)) + const rec = await seed('zone_record', { ...entities.zone_record.body(ids.zone_record[0]), zid: doomed }) + + // mysql's constraint cascades inside the DELETE, so no move can land mid-way + if (storeType() === 'mysql') return + + // The move lands after any read and before any delete the cascade makes. + let moved = false + const move = async () => { + if (moved) return + moved = true + assert.ok(await ZoneRecord.put({ id: rec, zid: spared }), 'the record did not move') + } + const get = ZoneRecord.get.bind(ZoneRecord) + const destroy = ZoneRecord.destroy.bind(ZoneRecord) + ZoneRecord.get = async (...args) => { + const found = await get(...args) + await move() + return found + } + ZoneRecord.destroy = async (...args) => { + await move() + return destroy(...args) + } + try { + assert.ok(await Zone.destroy({ id: doomed }), 'the zone did not destroy') + } finally { + delete ZoneRecord.get + delete ZoneRecord.destroy + } + + assert.ok(moved, 'the cascade never reached the record store') + assert.equal(await present('zone_record', rec), 1, 'the moved record went with its old zone') + }) + + it("rewrites the record file once for all of a zone's records", async () => { + await reset() + await seed('group', entities.group.body(GID)) + const zid = ids.zone[0] + await seed('zone', entities.zone.body(zid)) + for (const id of ids.zone_record) await seed('zone_record', { ...entities.zone_record.body(id), zid }) + + if (storeType() === 'mysql') return + + let writes = 0 + const write = ZoneRecord.file._write.bind(ZoneRecord.file) + ZoneRecord.file._write = async (...args) => { + writes++ + return write(...args) + } + try { + assert.ok(await Zone.destroy({ id: zid }), 'the zone did not destroy') + } finally { + delete ZoneRecord.file._write + } + assert.equal(writes, 1, 'each record rewrote the file on its own') + }) + + it('refuses a destroy that names no row or parent', async () => { + await assert.rejects(() => Nameserver.destroy({}), /needs one of id, gid/) + await assert.rejects(() => ZoneRecord.destroy({}), /needs one of id, zid/) + }) }) diff --git a/lib/store/file.js b/lib/store/file.js index f0ab9bd..108e88b 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -143,6 +143,13 @@ export class FileStore { } } +// destroy() takes an id, a parent key, or both; never every row +export function destroyMatch(args, keys) { + const given = keys.filter((k) => args[k] !== undefined) + if (given.length === 0) throw new Error(`destroy() needs one of ${keys.join(', ')}`) + return (row) => given.every((k) => row[k] === args[k]) +} + // in place, so a mutate() callback can drop rows export function removeRows(rows, match) { let kept = 0 diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index 18cac93..0f2fd14 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { claimId, removeRows } from '../../store/file.js' +import FileStore, { claimId, destroyMatch, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneRecordBase, { checkPut, mergeChanges } from './base.js' @@ -115,7 +115,8 @@ class ZoneRecordRepoFile extends ZoneRecordBase { } async destroy(args) { - return this.file.mutate('zone_record', (records) => removeRows(records, (r) => r.id === args.id) > 0) + const match = destroyMatch(args, ['id', 'zid']) + return this.file.mutate('zone_record', (records) => removeRows(records, match) > 0) } } diff --git a/lib/zone_record/store/mysql.js b/lib/zone_record/store/mysql.js index 16f25e1..c35123a 100644 --- a/lib/zone_record/store/mysql.js +++ b/lib/zone_record/store/mysql.js @@ -214,9 +214,13 @@ class ZoneRecordMySQL extends ZoneRecordBase { return r.changedRows === 1 } - async destroy(args) { - const r = await Mysql.execute(...Mysql.delete(`nt_zone_record`, { nt_zone_record_id: args.id })) - return r.affectedRows === 1 + async destroy({ id, zid }) { + if (id === undefined && zid === undefined) throw new Error('destroy() needs one of id, zid') + const where = {} + if (id !== undefined) where.nt_zone_record_id = id + if (zid !== undefined) where.nt_zone_id = zid + const r = await Mysql.execute(...Mysql.delete(`nt_zone_record`, where)) + return r.affectedRows > 0 } disconnect() {