Skip to content

Refuse dataset deletes that would leave dangling references - #736

Merged
AlexGodbehere merged 3 commits into
mainfrom
fix/dataset-delete-dangling-refs
Aug 27, 2026
Merged

Refuse dataset deletes that would leave dangling references#736
AlexGodbehere merged 3 commits into
mainfrom
fix/dataset-delete-dangling-refs

Conversation

@AlexGodbehere

@AlexGodbehere AlexGodbehere commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review this first

This changes behaviour on a public endpoint. Deletes that used to succeed now return 409. Callers must delete top-down. Nothing in the repo calls delete_dataset except the service client and the cluster tests, so no UI needed updating, but external callers are affected.

What this does

GET v1/delete/:uuid now refuses with 409 Conflict and lists the referrers when another dataset still points at the target.

Why

Deleting a dataset left every reference to it behind. On fpd-ago that produced 18 union lists pointing at datasets that no longer existed.

On its own that is untidy. Combined with the unbounded retry in #735, each dangling reference became a permanent 403 retried about 90 times a minute, which saturated PostgreSQL and took cluster sign-in down.

docs/services/data-access.md documented the dangling-reference behaviour as expected. That page is corrected here.

Why refuse rather than clean up

  1. The caller holds Delete dataset on that dataset only. Cleaning up would edit referrers they may have no permission to touch, invisibly.
  2. A SessionLimits dataset is a time window over its source. Remove the source and you have a window over nothing, with no list-item equivalent to remove. The options are cascade-delete (destroys data nobody asked to delete) or leave it broken (the bug).
  3. Refusal writes nothing, so a refused delete cannot leave the graph half updated. Cleanup needs N writes across N objects with no transaction and no rollback.

Changes

File Change
lib/api-v1.js find_referrers(), the 409 refusal, and remove_subclass_relationships on the success path.
lib/base-structure-handler.js New references(config, target_uuid) hook, default false.
lib/unions-components-handler.js references checks list membership. Guarded against a non-array config.
lib/session-limits-handler.js references checks config.source. Guarded against a missing source.
lib/sparkplug-sources-handler.js references returns false. Its source is a device, not a dataset.

Also: tests/current/delete_dataset_referrers.test.js (new), docs/services/data-access.md, package.json.

No sweep and no migration. Existing dangling references on live clusters were repaired by hand.

Testing

Node 24.13.0, vitest 4.1.11.

Run Result
npm test in acs-data-access 9 passed, 0 failed
Baseline before the change No test files found, exit 1
lib reverted, tests kept 6 failed, 3 passed

That last row is the point: the new tests do exercise the fix rather than passing regardless.

npm test had always exited 1 because it pointed at ./tests/current, which did not exist. tests/http/delete_dataset.test.js is unchanged and still deletes a referrer-free SparkplugSrc, so it should still pass on a cluster.

Calls to check

  1. immutable added to package.json. lib/api-v1.js, lib/dataflow.js and lib/notify.js all import it and nothing declared it. Without it the modules cannot load outside the container image. If it is meant to arrive through @amrc-factoryplus/rx-client, say so and it comes back out.
  2. Referrers are read from ConfigDB structure apps, not the derived dataset map, so invalid datasets are checked too. That is one search_app per structure app per delete, including SparkplugSrc, which always returns false. Skipping it is a one-line change if the extra read is unwelcome.
  3. Residual race. The check reads through watch streams that can lag. Stale data fails safe, giving a spurious retryable 409. The open window is a union or session created between the check and the delete. Closing it needs a cross-object transaction or a create-side existence check. The create-side check was left out because the streams visibly lag (the cluster tests sleep 4s after creating), so it would reject legitimate back-to-back creates.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A

AlexGodbehere and others added 3 commits August 27, 2026 09:25
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A
@AlexGodbehere
AlexGodbehere force-pushed the fix/dataset-delete-dangling-refs branch from 642a79c to 5cc0eaa Compare August 27, 2026 08:25
@AlexGodbehere
AlexGodbehere merged commit 80d637f into main Aug 27, 2026
1 check passed
@AlexGodbehere
AlexGodbehere deleted the fix/dataset-delete-dangling-refs branch August 27, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant