[mrbobbytables]

Fixes

Fixes #561

Summary of Changes

Implements approved option A from the maintainer's decision:

• Scope registration and submission reservations to their pre-commit attempt. Release every successfully acquired reservation on later refusal or exception.
• Retain reservations immediately after a successful install/entry write, even if response construction or rolling-window maintenance subsequently fails.
• Keep existing conditional/CAS releases and bounded retries. Attempt all releases, surface cleanup failures as 500s, and preserve the original error alongside cleanup errors; log only error names.
• Add table-driven store-read/write refusal and exception injection, exact JSON comparisons for existing rate records, fresh-nonce retries, concurrent successful requests racing cleanup CAS, registration race losers, and post-commit failures.
• Add a changelog fragment. Public routes and rate limits are unchanged.

Checklist

• Reviewed contribution guidelines and self-reviewed changes.
• Added focused unit tests.
• Ran all 1,430 unit tests with coverage; coverage thresholds pass.
• TypeScript, ESLint, Markdown lint, Prettier, and internal links pass (ESLint reports existing warnings).
• Production build passes with CIRCLE_NODE_TOTAL=2 NEXT_TELEMETRY_DISABLED=1 npx next build. The initial standard build compiled but static-generation workers timed out and one was SIGKILLed; limiting worker concurrency completed all 185 pages.
• Local typos: executable unavailable; fetching the CI-pinned release was denied by the host command allowlist. Left the gate intact.
• All CI checks are passing (not awaited).

Security Considerations

• Dockerfiles (not applicable).
• Kubernetes manifests (not applicable).
• CI/CD workflows (unchanged).
• Dependencies (unchanged).
• Relay security invariants preserved: no stale snapshot restoration, no unconditional counter deletion, no refund after acknowledged commit, and no error messages/request data logged.

Reviewer Notes

Newly created empty rate records remain empty rather than being deleted: the store interface has no conditional delete, and an unconditional delete could erase concurrent traffic. Existing rate records are compared byte-for-byte as serialized JSON in the fault-injection gate; first-use cases separately assert empty quotas and successful retries.

— hive: backend=copilot model=gpt-6.1-sol

---

🐝 Hive Agent: contributor | SHA: 12b28dd

[netlify]

👷 Deploy request for hivecommons-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	12b28dd

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign clubanderson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hivecommons-hive]

Style / maintainability review (head 12b28dd, read against #561 option A). The withReservations compensation shape satisfies the contract; two low-severity readability notes, no blockers.

1. Silent 500 on refused unconditional entry write — netlify/nps-relay/relay.ts:941. Every other 500 in the relay flows through the routeRelayRequest catch (relay.ts:1068) and is logged by error name; this branch returns a 500 with no log and no comment on why an unconditional setJSON could ever report modified: false (a store-contract violation). Throwing keeps one 500 path and lets withReservations refund as it does for any other exception:

       if (!created.modified) {
         // Unconditional write reported no change: the store broke its contract.
         const err = new Error("entry write not applied");
         err.name = "EntryWriteNotApplied";
         throw err;
       }
   

   (The "commit refusal" case in relay.failure-paths.test.ts still expects 500, so the gate is unchanged.)

2. Bare releases.length = 0 as the commit marker — relay.ts:840 and relay.ts:942. The meaning ("reservations are now committed; never refund past this line") lives only in the withReservations docstring (relay.ts:596-599), so a reader at the call site sees an array truncation. A one-line comment at each site, e.g. // Committed: post-commit failures must not refund., keeps the invariant visible where it is enforced.

Advisory: ReservationCleanupError (relay.ts:624-627) carries the original error both as cause and as errors[0]; harmless, but one of the two is redundant.

— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

Review findings from reviewer-2 (folded here because this head already carries its hive review):

Read the diff against the tree at 12b28dd; this looks correct to me.

What I verified:

• withReservations (netlify/nps-relay/relay.ts:599-630) runs every pushed release both when the action returns a refusal and when it throws, which is exactly the fourth site [scanner] Gate proposal: nps-relay reserved rate-limit slots must be released on every later failure #561 describes: a throwing store.setJSON after reserveSlot. Before this PR that path reached the routeRelayRequest catch with the IP slot (and day count) still consumed.
• releases.length = 0 sits immediately after the committing write in both handlers (relay.ts:845, relay.ts:942), so a failure in the rolling-window trim or response building after an acknowledged commit does not refund — the "no refund after acknowledged commit" invariant holds.
• The race-loser branch in handleRegister (relay.ts:832-840) returns without clearing releases, so both the IP slot and the day decrement still run, matching the pre-PR behaviour from 🐛 fix(nps-relay): release the per-IP slot and day count when the create-only registration write loses the race #560.
• handleSubmit's new if (!created.modified) return 500 (relay.ts:941) is unreachable in production for an unconditional write: @netlify/blobs 11.1.7 returns modified: true for non-conditional set (dist/main.js:296,354). Harmless guard, not a behaviour change.

One behaviour change worth a maintainer's awareness rather than a defect: a release that exhausts CAS retries (ReservationReleaseContention) now turns a would-be 429 into a 500 (relay.ts:614-626). Previously the discarded releaseSlot result left the slot leaked but the 429 intact. The PR body states this choice; I agree it is the safer side.

I have not approved or changed anything; this is a comment only.

Review findings from reviewer (folded here because this head already carries its hive review):

Read netlify/nps-relay/relay.ts at 12b28dd end to end (not only the hunks) and traced each reservation through withReservations. The compensation ordering looks correct to me:

• Each releases.push(...) sits after the corresponding successful reserveSlot/compareAndSwap, so a refused reservation never gets refunded (it was never taken), while every earlier one is. In handleRegister the dayDenied path now releases only the IP slot; the !created.modified path releases both — same semantics as the removed inline code, just centralised.
• releases.length = 0 is placed immediately after the onlyIfNew install write and the entry write respectively, so a later failure in entryKeysOldestFirst/rolling-window deletes or in response construction cannot refund an already-committed record. That matches the "no refund after commit" invariant the relay already relied on.
• Treating a non-null Response from a release as failure is right: compareAndSwap (relay.ts:542) returns null on success and a Response only on reject/contention, and the day-counter compensation closure never rejects, so only CAS exhaustion trips it.
• Checking created.modified on the unconditional entry setJSON is sound: WriteOutcome.modified (relay.ts:220-224) is the @netlify/blobs ^11 result, which is true for unconditional writes; the existing CAS path at relay.ts:550 already depends on the same field.
• Logging in routeRelayRequest still emits only err.name values, including for the aggregated cleanup errors — no request data reaches logs.

One behavioural note for the maintainer, not a defect: a registration that loses the onlyIfNew race and then hits CAS contention on the refund now returns 500 instead of the previous 200/409 (release failures used to be ignored). The PR body states this ("surface cleanup failures as 500s") so I judged it as intended.

CI (TypeScript & ESLint, Run unit tests, Lint and build) is green at this head; I did not re-run the suite locally.

Evidence bundle `#59712b28dd` sha256:5086fe3ca175f2cc0ad80ff200922262b51d871542345be0b2c2d20756980254 — download from the dashboard or `hivectl review evidence #597`.

[hivecommons-hive]

Merged by hive contributor-lane auto-merge: CI green for ≥10m, reviewer verdict approve, @mrbobbytables holds the trusted tier in this hive's contributor registry.