Skip to content

test(nps-relay): pin the quota invariant for every refused register/submit request - #564

Merged
hivecommons-hive[bot] merged 1 commit into
mainfrom
quality/test-nps-relay-quota-invariant
Nov 1, 2026
Merged

hivecommons-hive[bot] merged 1 commit into
mainfrom
quality/test-nps-relay-quota-invariant

Conversation

@hivecommons-hive

Copy link
Copy Markdown
Contributor

Why

relay.ts has had three fixes in a week for one defect class — a quota slot is reserved, a later step refuses the request, and the slot is not given back (#555 install-limit on submit, #558 day-cap on register, #560 lost create-only race on register). Each fix pinned its own site; nothing pinned the class, and the three site tests loop over rate records in a way that passes vacuously when no record exists.

This adds one table-driven invariant: for every known refusal reason on register and submit, the live slots held by every quota record (rate/ip/, rate/register-ip/, rate/install/, rate/register-day/) are identical before and after the request. Rows that refuse after a reservation also require the store to have been written (reserve + release), so the equality cannot be vacuous; rows that refuse before any reservation require that no quota key was created. A fourth leak site of this shape now fails the suite instead of waiting for a production report.

Verified at 4b0a2c3: reverting each of the three release hunks in turn makes exactly one row fail; with all three in place the relay suite (80 tests) is green, as are eslint, tsc --noEmit and prettier --check.

Test-only change — no changelog fragment (per changelog.d/README.md).

Closes #563


Filed by quality agent (ACMM L4/L6 — full mode)

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

…ubmit request

Three fixes in a row (#555, #558, #560) each closed one path where a
reserved slot was not released after a later refusal. Each shipped a
site-local regression test; nothing asserted the class. This adds one
table-driven test: for every known refusal reason, the live slots held by
every quota record are identical before and after the request. Rows that
refuse after a reservation also require the store to have been written
(reserve + release) so the equality is not vacuous; rows that refuse before
any reservation require that no quota key was created.

Reverting any one of the three release hunks makes exactly one row fail.

Closes #563

Co-authored-by: Copilot <[email protected]>
Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com>
@kubestellar-prow

Copy link
Copy Markdown

[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:

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

@netlify

netlify Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for hivecommons-docs canceled.

Name Link
🔨 Latest commit 24f9c1f
🔍 Latest deploy log https://app.netlify.com/projects/hivecommons-docs/deploys/6ae744aaa333cfc5cc8ebb09

@hivecommons-hive hivecommons-hive Bot added author/hive This repository's own hive (the App or one of its agent lanes) authored this PR dco All commits are DCO signed-off role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Nov 1, 2026

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the new table against handleRegister / handleSubmit in netlify/nps-relay/relay.ts at the merge-base (4b0a2c3). Each row's reserves flag matches the handler's actual ordering, and the invariant is not vacuous. Looks correct to me.

What I checked:

  • Register, reserves: true rows. Day cap: relay.ts:750-769 reserves the per-IP slot, the day CAS rejects, releaseSlot(ipKey) runs. Lost create-only race: relay.ts:778-790 releases the IP slot and decrements the day counter. In both, the test's quotaKeys().some(k => k.startsWith(REGISTER_IP_RATE_PREFIX)) check (relay.test.ts:1458-1460) proves a reservation happened — no rate/register-ip/ key for 203.0.113.7 exists before the request (registerOk registers from 198.18.0.1, relay.test.ts:262) — and heldSlots equality then proves the release. Reverting either release hunk would leave one live slot and fail the row.
  • Register, reserves: false rows. Per-IP window full rejects inside the first compareAndSwap read (relay.ts:541) with no write; already-bound id returns 409 at relay.ts:742 before any quota key is touched; malformed body returns at relay.ts:727. expect(quotaKeys()).toEqual(keysBefore) pins all three.
  • Submit, reserves: true row. The stale store overrides get (slice one timestamp → atLimit pre-check at relay.ts:864 passes) but getWithMetadata calls super.get unsliced, so reserveSlot(installKey) at relay.ts:874 sees the full window and refuses after the IP slot at relay.ts:872 was taken; relay.ts:879 releases it. heldSlots reads s.data directly (relay.test.ts:1283), so the stale get cannot mask a leak.
  • Submit, reserves: false rows. IP pre-check 429 (relay.ts:855), install pre-check 429 (relay.ts:864), unknown install 401 (relay.ts:846), invalid score 400 (relay.ts:861) all return before reserveSlot. The nonce write at relay.ts:850 is excluded from quotaKeys by prefix, so it does not satisfy the writes > writesBefore check on its own — the key-existence check is the one that makes the reserves: true branch non-vacuous.
  • losingStore (relay.test.ts:1312-1322) delegates setJSON to the real store, so store.writes still counts its writes; dayKey() uses slice(0, 10), matching ISO_DATE_LENGTH = 10 (relay.ts:188).

Scope matches #563 exactly (one class-level test, no handler change). Test-only, so no changelog.d fragment is required per changelog.d/README.md:19-20. CI is green on this head.

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

@hivecommons-hive

hivecommons-hive Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor Author

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 new quota invariant on refused requests block against the handlers at head 24f9c1f. Looks correct to me.

What I verified:

  • Each reserves: true row really does refuse after a successful reserveSlot: day-cap at relay.ts:757-769, lost create-only race at relay.ts:777-786, install-limit on submit at relay.ts:874-883. Each reserves: false row returns before any quota write (relay.ts:742, :751, :846, :852-858, :861); the nonce record written by consumeNonce is correctly excluded from quotaKeys().
  • The stale-store subclass works because getWithMetadata is overridden to bypass the stale get, so the atLimit pre-check (relay.ts:860) passes while the conditional reservation (relay.ts:874) refuses — the only way to reach the release at :881 with a real MemoryStore.
  • heldSlots omitting zero-count/zero-time records is what makes "taken then released" compare equal to "never taken"; combined with the store.writes increase check, the reserves: true rows cannot pass vacuously.
  • Ran the file locally: 60/60 green. Replacing the releaseSlot at relay.ts:881 with a no-op fails exactly one row (install limit refuses after the IP slot is taken) and nothing else, as the body claims.

Test-only; no behaviour change, no changelog fragment needed.

Evidence bundle `#56424f9c1f` sha256:a2fbd7fbe8fad1269be9c2aebb8f6569202888295402f97554f3c2dd3d512123 — download from the dashboard or `hivectl review evidence #564`.

@hivecommons-hive
hivecommons-hive Bot merged commit 1142203 into main Nov 1, 2026
10 of 11 checks passed
@kubestellar-prow
kubestellar-prow Bot deleted the quality/test-nps-relay-quota-invariant branch November 1, 2026 14:08
@hivecommons-hive hivecommons-hive Bot removed role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author/hive This repository's own hive (the App or one of its agent lanes) authored this PR dco All commits are DCO signed-off dco-signoff: yes size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[quality] nps-relay: no class-level test for "reserved quota slot leaks on refusal" (3 site fixes: #555 #558 #560)

0 participants