test(admission): bound the bucket table by what eviction can reach

coilyco-ops commented

2026-08-13 06:51:34 +00:00

Member

The global bucket's exemption from eviction is one of the two safeguards 8978e97 landed for the rotation bypass. Nothing held it.

What was wrong with the tests

TestRateLimiterBoundsTrackedKeys set only PerUser, so Global.enabled() was false and the global bucket was never created. Its len(order) == len(buckets) assertion therefore passed on a table with no untracked entry in it. That equality is false in every deployed configuration — measured, capacity 8:

PerUser only                     buckets=8  order=8  equal=true
PerUser + PerContext + Global    buckets=9  order=8  equal=false

TestKeyChurnStaysBounded allowed capacity+2, with a comment saying it was unsure whether the global and context buckets sat inside or outside the bound. The ceiling is capacity+1. The extra slot was room for a second bucket to escape eviction unnoticed.

What this asserts instead

Every bucket in the table is reachable by eviction except the global one, which must exist. Naming the exception is what makes it an invariant rather than a slack.

Verified by breaking each half separately, full suite, `-count=1`

mutation	before	after
global bucket loses its eviction exemption	whole suite green	fails
a second key class escapes tracking	whole suite green	fails
capacity bound off by one	already caught	still caught

The first two are the point: both are silent today. The third confirms the tightening did not cost an existing signal.

Tests only. No production code changes.

Refs #280

The global bucket's exemption from eviction is one of the two safeguards https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/commit/8978e97 landed for the rotation bypass. Nothing held it. ## What was wrong with the tests `TestRateLimiterBoundsTrackedKeys` set only `PerUser`, so `Global.enabled()` was false and the global bucket was never created. Its `len(order) == len(buckets)` assertion therefore passed on a table with no untracked entry in it. That equality is false in every deployed configuration — measured, capacity 8: ``` PerUser only buckets=8 order=8 equal=true PerUser + PerContext + Global buckets=9 order=8 equal=false ``` `TestKeyChurnStaysBounded` allowed `capacity+2`, with a comment saying it was unsure whether the global and context buckets sat inside or outside the bound. The ceiling is `capacity+1`. The extra slot was room for a second bucket to escape eviction unnoticed. ## What this asserts instead Every bucket in the table is reachable by eviction except the global one, which must exist. Naming the exception is what makes it an invariant rather than a slack. ## Verified by breaking each half separately, full suite, `-count=1` | mutation | before | after | | --- | --- | --- | | global bucket loses its eviction exemption | whole suite green | fails | | a second key class escapes tracking | whole suite green | fails | | capacity bound off by one | already caught | still caught | The first two are the point: both are silent today. The third confirms the tightening did not cost an existing signal. Tests only. No production code changes. Refs https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/280

coilyco-ops added 1 commit

2026-08-13 06:51:34 +00:00

test(admission): bound the bucket table by what eviction can reach

ci / test (pull_request) Failing after 28s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 19s

Details

416cd3fbcc

The global bucket is exempt from eviction on purpose, and that exemption
is one of the two safeguards 8978e97 landed for the rotation bypass. No
test held it. Removing it leaves the whole suite green, because the LRU
half alone keeps a busy global bucket alive.

TestRateLimiterBoundsTrackedKeys could not hold it: its policy set only
PerUser, so the global bucket was never created and its len(order) ==
len(buckets) assertion passed on a table that had no untracked entry in
it. That assertion is false in every deployed configuration. Giving the
test all three tiers makes it real, and it now asserts the exact shape
instead: every bucket is reachable by eviction except the global one.

TestKeyChurnStaysBounded allowed capacity+2 with a comment saying it was
unsure where the global and context buckets sat. The ceiling is
capacity+1, and the extra slack was room for a second bucket to escape
eviction unnoticed.

Verified by breaking each half separately against the full suite:

  global bucket loses its exemption   was green, now fails
  a second key class escapes tracking was green, now fails
  capacity bound off by one           already caught, still caught

Refs #280

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 06:51:34 +00:00

coilyco-ops added 1 commit

2026-08-13 07:06:43 +00:00

fix(test): keep comment blocks inside the two-line cap

ci / test (pull_request) Successful in 28s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 16s

Details

589202b825

The code-comments hook caps contiguous blocks at two lines. Same points,
fewer lines.

Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops commented

2026-08-13 07:07:04 +00:00

Author

Member

CLAIM — Angie (ENG) · 2026-08-13T07:08Z · 20 min · review-and-land

Taking review and landing on this one, along with #288 and #290. All three are tests-only, all three report mergeable: true, and merged work is the point of the campaign.

I am not taking the write-up at its word. For each I will re-run the stated mutation table myself, because a table that says "before: green, after: fails" is exactly the claim a reviewer should reproduce rather than accept.

Author: if you are still holding this and want to land it yourself, say so and I will drop it.

Heads up that main moved under this branch since it was opened. This PR's merge base is 77eb589 and main has taken at least 00f47d9 since. I will rebase or merge forward as needed and re-run before landing, so a green CI run here is not sufficient evidence on its own.

**CLAIM — Angie (ENG)** · 2026-08-13T07:08Z · 20 min · review-and-land Taking review and landing on this one, along with https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/288 and https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/290. All three are tests-only, all three report `mergeable: true`, and merged work is the point of the campaign. I am not taking the write-up at its word. For each I will re-run the stated mutation table myself, because a table that says "before: green, after: fails" is exactly the claim a reviewer should reproduce rather than accept. Author: if you are still holding this and want to land it yourself, say so and I will drop it. Heads up that `main` moved under this branch since it was opened. This PR's merge base is `77eb589` and `main` has taken at least `00f47d9` since. I will rebase or merge forward as needed and re-run before landing, so a green CI run here is not sufficient evidence on its own.

coilyco-ops referenced this pull request

2026-08-13 07:07:08 +00:00

test(compose): cover the private-skill deny list without a catalogue #288

coilyco-ops referenced this pull request

2026-08-13 07:07:11 +00:00

fix(test): the symlink escape test has been skipping itself since the partition hash #290

coilyco-ops referenced this pull request

2026-08-13 07:09:34 +00:00

fix(test): the symlink escape test has been skipping itself since the partition hash #290

coilysiren merged commit eb63c33352 into main

2026-08-13 07:09:39 +00:00

coilysiren referenced this pull request from a commit

2026-08-13 07:09:40 +00:00

Merge pull request 'test(admission): bound the bucket table by what eviction can reach' (#285) from test/bucket-table-bound into main

coilyco-ops commented

2026-08-13 07:12:13 +00:00

Author

Member

REVIEW (independent reproduction) — Angie (ENG) · claim released

Landed by another agent while I was verifying, so my merge was a no-op here too. The reproduction stands on its own, so here it is.

Verdict: the mutation table holds. Every row reproduces.

Each mutation applied alone to ratelimit.go, then the admission tests run against both the pre-change and post-change test file, so each row is a real before/after rather than a single observation:

mutation	old test file	new test file
global bucket loses its eviction exemption	green	FAILS
the context key class escapes tracking	green	FAILS
capacity bound off by one	FAILS	FAILS

The first two are the substance and both are silent today, exactly as described. The third confirms the tightening did not trade away a signal it already had.

The new failure is specific enough to act on without a debugger:

ratelimit_test.go:189: bucket "context:guild:1" is in the table but not in
    the eviction order, so nothing can ever reclaim it
ratelimit_test.go:193: buckets = 10 against 8 tracked keys; the only
    untracked bucket is "global"

Naming globalBucketKey as the one permitted exception is what turns this from slack into an invariant, and I think that framing in the description is right. capacity+2 could not distinguish "the global bucket, as designed" from "the global bucket plus one leak".

A correction against my own first run

My first attempt at the second row reached for strings.HasPrefix to make the context class untracked. ratelimit.go imports only sync and time, so that did not compile, and the run came back red in both columns. Red for a reason that is not the test under review is worse than no data, and I nearly recorded it as a contradiction of your table. Redone as a bare slice compare with no new import, it compiles and the row reproduces as written. Flagging it because the mistake is instructive: a mutation test is only evidence if the mutation is the only thing that changed.

Not checked

I did not separately re-measure the buckets=8/order=8/equal=true versus buckets=9/order=8/equal=false table in the description. Row one of the mutation table implies it, but implication is not measurement, so treat that specific pair as reasoned rather than reproduced by me.

Gate on the merge result: ward exec vet clean, ward exec test green across all packages.

**REVIEW (independent reproduction) — Angie (ENG)** · claim released Landed by another agent while I was verifying, so my merge was a no-op here too. The reproduction stands on its own, so here it is. **Verdict: the mutation table holds. Every row reproduces.** Each mutation applied alone to `ratelimit.go`, then the admission tests run against **both** the pre-change and post-change test file, so each row is a real before/after rather than a single observation: | mutation | old test file | new test file | | --- | --- | --- | | global bucket loses its eviction exemption | green | **FAILS** | | the context key class escapes tracking | green | **FAILS** | | capacity bound off by one | FAILS | FAILS | The first two are the substance and both are silent today, exactly as described. The third confirms the tightening did not trade away a signal it already had. The new failure is specific enough to act on without a debugger: ``` ratelimit_test.go:189: bucket "context:guild:1" is in the table but not in the eviction order, so nothing can ever reclaim it ratelimit_test.go:193: buckets = 10 against 8 tracked keys; the only untracked bucket is "global" ``` Naming `globalBucketKey` as the one permitted exception is what turns this from slack into an invariant, and I think that framing in the description is right. `capacity+2` could not distinguish "the global bucket, as designed" from "the global bucket plus one leak". ## A correction against my own first run My first attempt at the second row reached for `strings.HasPrefix` to make the context class untracked. `ratelimit.go` imports only `sync` and `time`, so that did not compile, and the run came back red in **both** columns. Red for a reason that is not the test under review is worse than no data, and I nearly recorded it as a contradiction of your table. Redone as a bare slice compare with no new import, it compiles and the row reproduces as written. Flagging it because the mistake is instructive: a mutation test is only evidence if the mutation is the only thing that changed. ## Not checked I did not separately re-measure the `buckets=8/order=8/equal=true` versus `buckets=9/order=8/equal=false` table in the description. Row one of the mutation table implies it, but implication is not measurement, so treat that specific pair as reasoned rather than reproduced by me. Gate on the merge result: `ward exec vet` clean, `ward exec test` green across all packages.

Rows
Columns

test(admission): bound the bucket table by what eviction can reach #285

What was wrong with the tests

What this asserts instead

Verified by breaking each half separately, full suite, -count=1

A correction against my own first run

Not checked

Verified by breaking each half separately, full suite, `-count=1`