Stop data-access retrying ConfigDB 4xx forever - #735
Draft
AlexGodbehere wants to merge 1 commit into
Draft
Conversation
acs-data-access wrapped every ConfigDB subclass write in retryBackoff. That operator retried any error, with no attempt cap and no look at the error. A permanent 403 was therefore retried forever, at about 90 attempts a minute per dataset. Each attempt made ConfigDB run a permission check over a large class expansion, which loaded the shared database, which starved acs-auth, which made Keycloak time out. Sign-in went down cluster-wide. retryBackoff now takes an options argument with a retry count and a shouldRetry predicate. Existing callers pass neither and keep their current behaviour. acs-data-access routes its subclass writes through a new retry_cdb_write helper. It retries 5xx responses, connection failures and errors with no status. It does not retry 4xx, because ConfigDB has already given a settled answer and asking again cannot change it. The cap is four retries after the first attempt, so five attempts and 7.5 seconds of backoff in total. That is long enough to ride out a ConfigDB pod restart or a short database stall, and short enough that a client which keeps retrying cannot build up load on ConfigDB. Any cap at all removes the old failure mode; five attempts keeps the transient case working. On failure the helper logs the dataset UUID, the ConfigDB status and whether it gave up or refused to retry, then throws an APIError with the ConfigDB status for a 4xx and 503 otherwise. The dataset operation fails rather than reporting success with the relationship missing, which matches _update_dataset_config in api-v1.js: it does not catch errors, so a failure already aborts the request. Passing the status through also stops a 403 being reported to the client as 503. remove_subclass_relationships in both handlers now uses the same helper. It had no retry at all before. Adds tests/current with unit tests for the predicate, the cap, the fast-fail path and both handlers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A
AlexGodbehere
marked this pull request as draft
August 24, 2026 11:26
AlexGodbehere
added a commit
that referenced
this pull request
Aug 27, 2026
## 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.com/claude-code) https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review this first
lib/js-rx-util/lib/util.js. This PR changes a shared package, not only data-access. The change is additive and both new options default to today's behaviour, so the only other caller (acs-cluster-manager/lib/clusters.js) is unaffected. Confirm you are happy with that.What this does
Stops
acs-data-accessretrying ConfigDB writes forever. Two changes:Why
One bad reference in one service took out authentication for a whole cluster.
On fpd-ago, 18 union datasets referenced dataset objects that had been deleted. Each one produced a permanent 403 from ConfigDB. Both structure handlers wrapped that write in
retryBackoff, which had no attempt cap and never looked at the error:What followed, measured on the cluster:
acs-authstarvedChanges
lib/js-rx-util/lib/util.jsretryBackoff(delay, log, { count, shouldRetry }). Both new options optional.acs-data-access/lib/retry.jsretry_cdb_writeandis_transient_error.acs-data-access/lib/session-limits-handler.jsacs-data-access/lib/unions-components-handler.jsacs-data-access/tests/current/retry.test.jsRetried: 5xx, connection failures (the client reports status 0), errors with no status. Not retried: any 4xx.
Cap is 4 retries after the first attempt, so 5 attempts over 7.5s of backoff. That rides out a ConfigDB restart or a short database stall, and is small enough that a client hammering the endpoint cannot build load on ConfigDB. The number matters less than having one.
Testing
npm testinacs-data-access: 26 passed, 0 failed, 0.8s.Note:
npm testpointed at./tests/current, which did not exist, so the script had always exited 1. The new tests live there.tests/http/*still needs a live cluster and is not run bynpm test.Calls to check
remove_subclass_relationshipsnow retries. It had no retry before, so it used to fail immediately on a transient error. Confirm bounded retry is wanted there._update_dataset_configdoes not catch, so the request aborts. The ConfigDB status is mapped onto anAPIError: 4xx passes through, anything else becomes 503. Without that mapping a bareServiceErroris reported as 503, which tells the client a permanent 403 is worth retrying.package-lock.jsonis not committed. It was generated locally bynpm installand the service has never had one in the repo.To run the tests locally you also need
npm installinlib/js-service-api,lib/js-service-clientandlib/js-rx-util, because thefile:dependencies resolve from their own real paths.🤖 Generated with Claude Code
https://claude.ai/code/session_01MtATkkjtiFF8L6z98NFD8A