From 1cf5bf3aae45fe58af8dfbfc6795c283741bdc3e Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Mon, 24 Aug 2026 11:06:48 +0100 Subject: [PATCH 1/3] data-access: refuse to delete a dataset other datasets still reference Deleting a dataset removed the ConfigDB object but left every union list and session source that named it pointing at a UUID that no longer existed. On the fpd-ago cluster this had built up 18 union lists holding references to deleted datasets. Each dangling reference turned into a permanent permission failure, and a separate defect retried those failures about 90 times a minute. That traffic saturated the shared PostgreSQL database and took cluster sign-in down. The retry defect is being fixed separately. This change stops the dangling references being created. The delete now looks for referrers first and returns 409 with the list when it finds any. It refuses rather than editing the referrers, for three reasons: - The caller holds Delete dataset on this dataset only. Editing other datasets would change data they may have no permission to touch, and a caller who does not know the graph would not see it happen. - A union list can lose one entry and still mean something. A session is a time window over its source, so removing the source leaves a window over nothing. There is no sensible repair for that case. - Refusing writes nothing at all, so a refused delete cannot leave the graph half updated. Cleaning up would need one write per referrer with no way to roll back a failure in the middle. Callers delete from the top of the graph down: the union or session first, then its components. Referrers are read straight from the structure apps in ConfigDB rather than from the derived dataset map, so datasets that are currently invalid are checked too. An invalid dataset keeps its config document, so it can still hold a dangling reference. When a delete does go ahead it now also removes the subclass links the dataset owns in the other direction. A session is a subclass of its source, so the old sweep over direct subclasses never saw that link and left it behind. Adds immutable to the dependencies. lib/api-v1.js, lib/dataflow.js and lib/notify.js all import it but nothing declared it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A --- acs-data-access/lib/api-v1.js | 77 ++++++++++++++++++- acs-data-access/lib/base-structure-handler.js | 12 +++ acs-data-access/lib/session-limits-handler.js | 6 ++ .../lib/sparkplug-sources-handler.js | 6 ++ .../lib/unions-components-handler.js | 8 +- acs-data-access/package.json | 1 + 6 files changed, 106 insertions(+), 4 deletions(-) diff --git a/acs-data-access/lib/api-v1.js b/acs-data-access/lib/api-v1.js index 0e298c02e..49f7ed05a 100644 --- a/acs-data-access/lib/api-v1.js +++ b/acs-data-access/lib/api-v1.js @@ -79,23 +79,94 @@ export class APIv1 { } + /** Finds every dataset whose stored config points at the given dataset. + * + * Reads the structure apps straight from ConfigDB rather than using the + * derived dataset map, so datasets that are currently invalid are still + * checked. An invalid dataset keeps its config document, so it can still + * be left holding a dangling reference. + * + * @param dataset_uuid The dataset that is about to be deleted. + * @returns {Promise>} + */ + async find_referrers(dataset_uuid) { + const referrers = []; + + for (const [structure, handler] of Object.entries(this.handlers)) { + const configs = await rx.firstValueFrom(this.cdb.search_app(structure)); + if (!configs) continue; + + for (const [uuid, config] of configs) { + if (uuid === dataset_uuid) continue; + + if (handler.references(config, dataset_uuid)) + referrers.push({ dataset: uuid, structure }); + } + } + + return referrers; + } + + /** GET. Deletes a dataset. + * + * Refuses with 409 while another dataset still references this one, and + * returns the list of referrers so the caller can deal with them. See the + * comment below for why we refuse rather than rewrite the referrers. + */ async delete_dataset(req, res){ const dataset_uuid = req.params.uuid; if(!dataset_uuid) return fail(this.log, 422, `No req.params.uuid`); if(!valid_uuid(dataset_uuid)) return fail(this.log, 422, `Invalid uuid ${dataset_uuid}`); - + const ok = await this.auth.check_acl( req.auth, Constants.Perm.DeleteDataset, dataset_uuid, true, ); - + if (!ok) return fail(this.log, 403, `You don't have DELETE permissions for ${dataset_uuid}`); this.log(`Delete dataset called by ${req.auth} for ${dataset_uuid}`); - // remove all subclass relationships before deleting + /* Refuse the delete while anything still points here. + * + * The alternative is to edit the referrers. We do not, for three + * reasons. The caller holds DeleteDataset on this dataset only, so + * editing other datasets would change data they may have no permission + * to touch. A UnionComponents list can lose one entry and still mean + * something, but a SessionLimits dataset is a time window over its + * source, so removing the source leaves a window over nothing and there + * is no sensible repair. And refusing writes nothing at all, so a failed + * delete cannot leave the graph half updated. + * + * Callers delete from the top down: remove the union or session first, + * then its components. */ + const referrers = await this.find_referrers(dataset_uuid); + + if (referrers.length > 0) { + this.log(`Refusing to delete ${dataset_uuid}: referenced by %o`, referrers); + + return res.status(409).json({ + error: "dataset_in_use", + dataset: dataset_uuid, + message: `Dataset ${dataset_uuid} is still referenced by ${referrers.length} other dataset(s). Delete or update them first.`, + referrers, + }); + } + + /* Remove the links this dataset owns. The handler knows which way round + * its links point: a union is the superclass of its components, a + * session is a subclass of its source. */ + const datasets = await rx.firstValueFrom(this.data.datasets); + const dataset = datasets.get(dataset_uuid); + + if (dataset?.config && this.handlers[dataset.structure]) { + await this.handlers[dataset.structure] + .remove_subclass_relationships(dataset_uuid, dataset.config); + } + + // remove any remaining subclass relationships before deleting const subclasses = await this.cdb.class_direct_subclasses(dataset_uuid); if(subclasses){ for(let s of subclasses){ diff --git a/acs-data-access/lib/base-structure-handler.js b/acs-data-access/lib/base-structure-handler.js index 84f6b4224..10e82d797 100644 --- a/acs-data-access/lib/base-structure-handler.js +++ b/acs-data-access/lib/base-structure-handler.js @@ -11,4 +11,16 @@ export class BaseStructureHandler { resolve(ctx) {} create_subclass_relationships(datasetUuid, config) {} remove_subclass_relationships(datasetUuid, config) {} + + /** Does this config point at the given dataset? + * + * Used by the delete path to find datasets that would be left with a + * dangling reference. Structures that only point at non-dataset objects + * return false. + * + * @param config The stored config document of another dataset. + * @param target_uuid The dataset UUID we are about to delete. + * @returns {boolean} + */ + references(config, target_uuid) { return false; } } \ No newline at end of file diff --git a/acs-data-access/lib/session-limits-handler.js b/acs-data-access/lib/session-limits-handler.js index 382cf685f..3fdcaf49f 100644 --- a/acs-data-access/lib/session-limits-handler.js +++ b/acs-data-access/lib/session-limits-handler.js @@ -77,7 +77,13 @@ export class SessionLimitsHandler extends BaseStructureHandler { ); } + references(config, target_uuid) { + return config?.source === target_uuid; + } + async remove_subclass_relationships(dataset_uuid, config) { + if(!config?.source) return; + await this.cdb.class_remove_subclass(config.source, dataset_uuid); this.log(`Removed ${dataset_uuid} from ${config.source} subclasses.`) } diff --git a/acs-data-access/lib/sparkplug-sources-handler.js b/acs-data-access/lib/sparkplug-sources-handler.js index cdbed1308..12670f005 100644 --- a/acs-data-access/lib/sparkplug-sources-handler.js +++ b/acs-data-access/lib/sparkplug-sources-handler.js @@ -48,6 +48,12 @@ export class SparkplugSourcesHandler extends BaseStructureHandler { }]; } + /* A SparkplugSrc points at a device, not at another dataset, so it can + * never hold a dangling dataset reference. */ + references() { + return false; + } + async create_subclass_relationships() { return; } diff --git a/acs-data-access/lib/unions-components-handler.js b/acs-data-access/lib/unions-components-handler.js index c5328f100..2e74345e6 100644 --- a/acs-data-access/lib/unions-components-handler.js +++ b/acs-data-access/lib/unions-components-handler.js @@ -74,8 +74,14 @@ export class UnionComponentsHandler extends BaseStructureHandler { } } + references(config, target_uuid) { + if(!Array.isArray(config)) return false; + + return config.includes(target_uuid); + } + async remove_subclass_relationships(dataset_uuid, config) { - if(config.length <= 0) return; + if(!Array.isArray(config) || config.length <= 0) return; for (const src of config){ await this.cdb.class_remove_subclass(dataset_uuid, src); diff --git a/acs-data-access/package.json b/acs-data-access/package.json index 28e573c8f..b4c9da985 100644 --- a/acs-data-access/package.json +++ b/acs-data-access/package.json @@ -18,6 +18,7 @@ "@influxdata/influxdb-client": "^1.35.0", "deep-equal": "^2.2.3", "express": "^5.0.1", + "immutable": "^5.0.0", "p-limit": "^7.3.0", "rxjs": "^7.8.2", "stream": "^0.0.3", From 395d788d1fcea0a3ca59ec32a42b13ee89adad5f Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Mon, 24 Aug 2026 11:06:56 +0100 Subject: [PATCH 2/3] data-access: unit tests for the delete referential integrity check The existing tests under tests/http need a live cluster. npm test runs vitest against ./tests/current, which did not exist, so npm test found no files and exited 1. These tests run against fakes and need no cluster. They cover a dataset with no referrers, a dataset listed by a union, a dataset used as a session source, a dataset with both kinds of referrer, a referrer whose own dataset is invalid, a union that references itself, and the subclass links removed on a successful delete. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A --- .../current/delete_dataset_referrers.test.js | 281 ++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 acs-data-access/tests/current/delete_dataset_referrers.test.js diff --git a/acs-data-access/tests/current/delete_dataset_referrers.test.js b/acs-data-access/tests/current/delete_dataset_referrers.test.js new file mode 100644 index 000000000..6c532a624 --- /dev/null +++ b/acs-data-access/tests/current/delete_dataset_referrers.test.js @@ -0,0 +1,281 @@ +/* + * ACS Data Access Service + * Unit tests for the delete path's referential integrity check. + * + * These run against fakes, so they need no cluster. + */ + +import { Map as IMap } from "immutable"; +import * as rx from "rxjs"; +import { describe, expect, test } from "vitest"; + +import { APIv1 } from "../../lib/api-v1.js"; +import { DataAccess as Constants } from "../../lib/constants.js"; + +const SRC_A = "11111111-1111-1111-1111-111111111111"; +const SRC_B = "22222222-2222-2222-2222-222222222222"; +const UNION = "33333333-3333-3333-3333-333333333333"; +const SESSION = "44444444-4444-4444-4444-444444444444"; +const DEVICE = "55555555-5555-5555-5555-555555555555"; + +/* Builds an APIv1 wired to fakes. + * + * `datasets` maps a dataset UUID to { structure, config }, the same shape + * DataFlow produces. The fake ConfigDB derives its per-app search results + * from that map, so the tests only describe the graph once. + */ +function make_api(datasets, raw_configs) { + const calls = { + deleted: [], + removed_subclasses: [], + }; + + const cdb = { + search_app(app) { + /* raw_configs lets a test give ConfigDB a config document that the + * derived dataset map does not carry, which is what happens when a + * dataset is invalid. */ + if (raw_configs) + return rx.of(IMap(raw_configs[app] ?? {})); + + let configs = IMap(); + + for (const [uuid, def] of Object.entries(datasets)) { + if (def.structure === app && def.config != null) + configs = configs.set(uuid, def.config); + } + + return rx.of(configs); + }, + + async class_direct_subclasses() { return []; }, + + async class_remove_subclass(klass, sub) { + calls.removed_subclasses.push([klass, sub]); + }, + + async delete_object(uuid) { calls.deleted.push(uuid); }, + }; + + const api = new APIv1({ + data: { datasets: rx.of(IMap(datasets)) }, + auth: { async check_acl() { return true; } }, + cdb, + debug: { bound: () => () => {} }, + influxReader: {}, + }); + + return { api, calls }; +} + +/* Minimal express response double. */ +function make_res() { + return { + code: null, + body: null, + status(code) { this.code = code; return this; }, + json(body) { this.body = body; return this; }, + }; +} + +function make_req(uuid) { + return { params: { uuid }, auth: "tester" }; +} + +describe("delete_dataset referential integrity", () => { + test("deletes a dataset with no referrers", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(200); + expect(res.body).toBe(SRC_A); + expect(calls.deleted).toEqual([SRC_A]); + }); + + test("refuses to delete a dataset listed by a union", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A, SRC_B], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.error).toBe("dataset_in_use"); + expect(res.body.referrers).toEqual([ + { dataset: UNION, structure: Constants.App.UnionComponents }, + ]); + expect(calls.deleted).toEqual([]); + expect(calls.removed_subclasses).toEqual([]); + }); + + test("refuses to delete a dataset used as a session source", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers).toEqual([ + { dataset: SESSION, structure: Constants.App.SessionLimits }, + ]); + expect(calls.deleted).toEqual([]); + }); + + test("lists every referrer, not only the first", async () => { + const { api } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A], + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers.map(r => r.dataset).sort()) + .toEqual([SESSION, UNION].sort()); + }); + + test("deletes a union once nothing points at it", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [UNION]: { + structure: Constants.App.UnionComponents, + config: [SRC_A], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(UNION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([UNION]); + /* The union is the superclass of its components. */ + expect(calls.removed_subclasses).toEqual([[UNION, SRC_A]]); + }); + + test("drops the link a session owns on its source", async () => { + const { api, calls } = make_api({ + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + [SESSION]: { + structure: Constants.App.SessionLimits, + config: { + source: SRC_A, + from: "2025-01-01T00:00:00.000Z", + to: "2025-01-02T00:00:00.000Z", + }, + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(SESSION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([SESSION]); + /* A session is a subclass of its source, so the link points the other + * way and the old direct-subclasses sweep never saw it. */ + expect(calls.removed_subclasses).toEqual([[SRC_A, SESSION]]); + }); + + test("finds referrers whose own dataset is invalid", async () => { + /* A dataset with two structural definitions is invalid, but it keeps + * its config documents and can still hold a dangling reference. */ + const { api, calls } = make_api( + { + [SRC_A]: { + structure: Constants.App.SparkplugSrc, + config: { source: DEVICE }, + }, + /* DataFlow reports the union as invalid with a null config. */ + [UNION]: { + structure: Constants.Special.InvalidDataset, + config: null, + }, + }, + { + /* ConfigDB still holds the list. */ + [Constants.App.UnionComponents]: { [UNION]: [SRC_A] }, + }, + ); + + const res = make_res(); + await api.delete_dataset(make_req(SRC_A), res); + + expect(res.code).toBe(409); + expect(res.body.referrers).toEqual([ + { dataset: UNION, structure: Constants.App.UnionComponents }, + ]); + expect(calls.deleted).toEqual([]); + }); + + test("a dataset does not count as its own referrer", async () => { + const { api, calls } = make_api({ + [UNION]: { + structure: Constants.App.UnionComponents, + config: [UNION], + }, + }); + + const res = make_res(); + await api.delete_dataset(make_req(UNION), res); + + expect(res.code).toBe(200); + expect(calls.deleted).toEqual([UNION]); + }); + + test("rejects an invalid uuid before touching ConfigDB", async () => { + const { api, calls } = make_api({}); + + await expect(api.delete_dataset(make_req("xxx"), make_res())) + .rejects.toMatchObject({ status: 422 }); + + expect(calls.deleted).toEqual([]); + }); +}); From 5cc0eaa65fcb95012a988646e8aa1e415c2e89b1 Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Mon, 24 Aug 2026 11:06:56 +0100 Subject: [PATCH 3/3] docs: describe the new delete behaviour for data access The page described the old behaviour, including the dangling references it left behind, as if they were expected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A --- docs/services/data-access.md | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/docs/services/data-access.md b/docs/services/data-access.md index ac5ee7874..d4cd6059b 100644 --- a/docs/services/data-access.md +++ b/docs/services/data-access.md @@ -203,14 +203,33 @@ the structural app the way `POST` does. ### `GET v1/delete/:uuid` Deletes a dataset. Note this is a `GET`, not a `DELETE`, request. -Requires `Delete dataset` on `:uuid`. Before deleting the ConfigDB -object, it removes every direct subclass relationship in which this -dataset is the *superclass* (e.g. session/union children recorded -against it). It does **not** clean up relationships in which this -dataset is itself a subclass — e.g. a session's link to its source, or -a union's link to its members — so other structural configs can be left -referencing a UUID that no longer exists. Returns the deleted UUID as a -JSON string, or an empty string if the dataset didn't exist. +Requires `Delete dataset` on `:uuid`. + +The delete is refused with `409 Conflict` while any other dataset still +points at this one, that is while a `Union components` list contains it +or a `Session limits` config names it as its `source`. The response body +lists the referrers: + +```json +{ + "error": "dataset_in_use", + "dataset": "", + "message": "...", + "referrers": [ { "dataset": "", "structure": "" } ] +} +``` + +Nothing is written when the delete is refused. Delete from the top of +the graph down: remove the union or session first, then its components. +Invalid datasets are checked too, because they keep their config +documents. + +When there are no referrers, the service removes the subclass +relationships this dataset owns in both directions. That covers the +links where the dataset is the superclass (a union's members) and the +links where it is the subclass (a session's link to its source). It then +deletes the ConfigDB object. Returns the deleted UUID as a JSON string, +or an empty string if the dataset didn't exist. ## Permissions