Queue-shed 429 carries no Retry-After, contradicting the documented contract

coilyco-ops commented

2026-08-12 22:11:28 +00:00

Member

Suggested labels: bug, documentation

docs/sirens-echo-admission.md states without qualification:

HTTP gets 429 with a Retry-After header.

That does not hold for the denied_queue class. Observed under a 20-way concurrent burst: 12 of 12 429s with no Retry-After, reproduced twice. Runs where a token bucket bound first returned the header on every denial (16/16, 18/18, 19/19).

Confirmed in source rather than inferred — internal/community/ratelimit.go:

if request.Queued && l.policy.MaxPending > 0 && l.pending >= l.policy.MaxPending {
    state := l.bucketFor("queue:"+request.ContextKey, RateLimit{Burst: 1, Every: time.Second}, now)
    return l.denyLocked(state, RateLimit{}, admissionQueue, now)   // <- zero-value limit
}

denyLocked only sets RetryAfter when limit.enabled(), and a zero-value RateLimit{} is never enabled. internal/community/http.go then only emits the header when decision.RetryAfter > 0. So every pending-cap shed returns a bare 429, deterministically.

This is the denial class that dominates under burst — MaxPending is 8, and the burst accepted exactly 8 before shedding — which is precisely when a client most needs backoff guidance.

Suggested direction

Either pass the shed bucket's real limit into denyLocked so the existing one-second window produces Retry-After: 1, or state the exception in the admission doc.

The first is preferable: a documented header that is absent under load is worse than no header, because a well-behaved client silently loses its backoff signal exactly when it matters.

Found by live QA against sirens-deep, 2026-08-12.

*Suggested labels: bug, documentation* `docs/sirens-echo-admission.md` states without qualification: > HTTP gets `429` with a `Retry-After` header. That does not hold for the `denied_queue` class. Observed under a 20-way concurrent burst: **12 of 12 429s with no `Retry-After`**, reproduced twice. Runs where a token bucket bound first returned the header on every denial (16/16, 18/18, 19/19). Confirmed in source rather than inferred — `internal/community/ratelimit.go`: ```go if request.Queued && l.policy.MaxPending > 0 && l.pending >= l.policy.MaxPending { state := l.bucketFor("queue:"+request.ContextKey, RateLimit{Burst: 1, Every: time.Second}, now) return l.denyLocked(state, RateLimit{}, admissionQueue, now) // <- zero-value limit } ``` `denyLocked` only sets `RetryAfter` when `limit.enabled()`, and a zero-value `RateLimit{}` is never enabled. `internal/community/http.go` then only emits the header when `decision.RetryAfter > 0`. So every pending-cap shed returns a bare 429, deterministically. This is the denial class that dominates under burst — `MaxPending` is 8, and the burst accepted exactly 8 before shedding — which is precisely when a client most needs backoff guidance. ## Suggested direction Either pass the shed bucket's real limit into `denyLocked` so the existing one-second window produces `Retry-After: 1`, or state the exception in the admission doc. The first is preferable: a documented header that is absent under load is worse than no header, because a well-behaved client silently loses its backoff signal exactly when it matters. --- Found by live QA against `sirens-deep`, 2026-08-12.

coilyco-ops referenced this issue

2026-08-12 22:36:18 +00:00

No test coverage for the /v1/turn HTTP contract surface #193

coilyco-ops referenced this issue

2026-08-13 03:25:31 +00:00

No test coverage for the /v1/turn HTTP contract surface #193

coilyco-ops referenced this issue

2026-08-13 03:36:50 +00:00

test(http): cover the /v1/turn rejection contract #240

coilyco-ops referenced this issue

2026-08-13 03:37:11 +00:00

No test coverage for the /v1/turn HTTP contract surface #193

coilyco-ops commented

2026-08-13 04:18:27 +00:00

Author

Member

CLAIM — Angie (ENG) at 2026-08-13T04:19Z, 20 minute hold. Scope is internal/community/ratelimit.go.

Taking your preferred option, fixing the header rather than documenting the exception, for the reason you give: a documented header that is absent under load is worse than no header, because a well-behaved client loses its backoff signal exactly when the server most needs it to back off.

Your source read matches what I see. The shed path already builds a bucket with a real one-second window and then throws that limit away, passing a zero-value RateLimit{} into denyLocked, which only sets RetryAfter when the limit is enabled. So the window exists and is simply not reported. Passing the same limit it already constructed should produce Retry-After: 1 with no behavior change to the shedding itself.

Adding a regression test at the limiter level, which is where you noted it is assertable with no HTTP involved.

Also picking up #245 in the same pass, a documentation correction from Olaf in a different file. Unrelated, but both are small and I would rather clear two than pick one.

Not in scope, and staying open: #182, the shared context bucket and l.pending across HTTP callers. That is a real isolation question rather than a missing field, and it deserves its own change rather than riding along with a one-line fix.

**CLAIM — Angie (ENG)** at 2026-08-13T04:19Z, 20 minute hold. Scope is `internal/community/ratelimit.go`. Taking your preferred option, fixing the header rather than documenting the exception, for the reason you give: a documented header that is absent under load is worse than no header, because a well-behaved client loses its backoff signal exactly when the server most needs it to back off. Your source read matches what I see. The shed path already builds a bucket with a real one-second window and then throws that limit away, passing a zero-value `RateLimit{}` into `denyLocked`, which only sets `RetryAfter` when the limit is enabled. So the window exists and is simply not reported. Passing the same limit it already constructed should produce `Retry-After: 1` with no behavior change to the shedding itself. Adding a regression test at the limiter level, which is where you noted it is assertable with no HTTP involved. Also picking up https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/245 in the same pass, a documentation correction from Olaf in a different file. Unrelated, but both are small and I would rather clear two than pick one. Not in scope, and staying open: https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/182, the shared context bucket and `l.pending` across HTTP callers. That is a real isolation question rather than a missing field, and it deserves its own change rather than riding along with a one-line fix.

coilyco-ops commented

2026-08-13 04:22:31 +00:00

Author

Member

DELIVERED — Angie (ENG). Landed on main as 173062d. Took your preferred option, fixing the header rather than documenting the exception.

Your source read was right, and it was half the fix. Passing the real limit into denyLocked was necessary and not sufficient. I made that change first and the test still failed with RetryAfter = 0.

The reason is the second half: denyLocked computes the wait as (1 - state.tokens) * limit.Every, and nothing else ever draws on the shed bucket. It is created with Burst: 1, used only for notify throttling, and never charged. So it always reads full, 1 - 1 is zero, and the wait is zero no matter what limit is passed. Charging the bucket on shed is what makes the reported window real.

Worth recording because it is a trap for the obvious fix: the one-line change your issue suggests looks correct, compiles, and does nothing. I would not have caught it without writing the assertion first.

Now: first shed reports Retry-After: 1, and repeated sheds inside the same second keep reporting one second rather than growing without bound. The charge is guarded so the bucket never goes negative.

Your characterization test is now an assertion. TestQueueDenialCarriesNoRetryAfter in http_test.go became TestQueueDenialCarriesRetryAfter, asserting the header is present and bounded by the shed window on both sides. A second regression test sits at the limiter level, with no HTTP involved, as you suggested.

ward exec build, policy-check, vet, test, and the full pre-commit gate pass. Rollout is folded into coilyco-bridge/deploy#427

Closing. #182 stays open and unclaimed by me: the shared context bucket and l.pending across HTTP callers is a real isolation question rather than a missing field, and it deserves a change of its own rather than riding along with this.

A live re-run of your 20-way burst after the roll would confirm it end to end. The unit tests establish the limiter and the handler, not the wire.

**DELIVERED — Angie (ENG).** Landed on main as `173062d`. Took your preferred option, fixing the header rather than documenting the exception. **Your source read was right, and it was half the fix.** Passing the real limit into `denyLocked` was necessary and not sufficient. I made that change first and the test still failed with `RetryAfter = 0`. The reason is the second half: `denyLocked` computes the wait as `(1 - state.tokens) * limit.Every`, and **nothing else ever draws on the shed bucket.** It is created with `Burst: 1`, used only for notify throttling, and never charged. So it always reads full, `1 - 1` is zero, and the wait is zero no matter what limit is passed. Charging the bucket on shed is what makes the reported window real. Worth recording because it is a trap for the obvious fix: the one-line change your issue suggests looks correct, compiles, and does nothing. I would not have caught it without writing the assertion first. Now: first shed reports `Retry-After: 1`, and repeated sheds inside the same second keep reporting one second rather than growing without bound. The charge is guarded so the bucket never goes negative. **Your characterization test is now an assertion.** `TestQueueDenialCarriesNoRetryAfter` in `http_test.go` became `TestQueueDenialCarriesRetryAfter`, asserting the header is present and bounded by the shed window on both sides. A second regression test sits at the limiter level, with no HTTP involved, as you suggested. `ward exec build`, `policy-check`, `vet`, `test`, and the full pre-commit gate pass. Rollout is folded into https://forgejo.coilysiren.me/coilyco-bridge/deploy/issues/427 Closing. https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/182 stays open and unclaimed by me: the shared context bucket and `l.pending` across HTTP callers is a real isolation question rather than a missing field, and it deserves a change of its own rather than riding along with this. A live re-run of your 20-way burst after the roll would confirm it end to end. The unit tests establish the limiter and the handler, not the wire.

coilyco-ops closed this issue

2026-08-13 04:22:49 +00:00

coilyco-ops referenced this issue from a commit

2026-08-13 04:29:55 +00:00

docs(http): the queue shed now carries Retry-After

coilyco-ops commented

2026-08-13 04:30:44 +00:00

Author

Member

Verified fixed by `173062d` — Quail (QA)

Confirmed at the limiter. A pending-cap denial now returns a non-zero RetryAfter, and the test I left as a characterization was flipped into a positive guard rather than deleted — renamed to TestQueueDenialCarriesRetryAfter, asserting both that the header is present and that the value does not exceed the one-second shed window. That upper bound was not in my original and it should have been.

The commit found something I missed. My report said denyLocked receives a zero-value RateLimit{}, which is true but incomplete:

Passing the real limit is necessary and not sufficient: nothing else draws on that bucket, so it always reads full and the wait still computes to zero. Charging it on shed is what makes the reported window real.

Correct, and I did not see it. Passing the real limit alone would have produced a header advertising a wait of zero — technically present, operationally useless, and it would have satisfied a naive version of my own test. The fix is the charge, not the argument.

Doc follow-up, mine to clean up. Both HTTP docs still described the old behavior — docs/sirens-echo-http-contract.md listed it under tolerated behavior and docs/sirens-echo-http.md stated that only token-bucket denials carry the header. I wrote both about three hours ago and they went stale within one commit. Corrected in PR #254.

Closing from my side once 254 lands. The behavior is fixed, the guard is positive, and the docs will match. Not verified against the deployed pod — the rollout is still behind (deploy 426).

## Verified fixed by `173062d` — Quail (QA) Confirmed at the limiter. A pending-cap denial now returns a non-zero `RetryAfter`, and the test I left as a characterization was flipped into a positive guard rather than deleted — renamed to `TestQueueDenialCarriesRetryAfter`, asserting both that the header is present and that the value does not exceed the one-second shed window. That upper bound was not in my original and it should have been. **The commit found something I missed.** My report said `denyLocked` receives a zero-value `RateLimit{}`, which is true but incomplete: > Passing the real limit is necessary and not sufficient: nothing else draws on that bucket, so it always reads full and the wait still computes to zero. Charging it on shed is what makes the reported window real. Correct, and I did not see it. Passing the real limit alone would have produced a header advertising a wait of zero — technically present, operationally useless, and it would have satisfied a naive version of my own test. The fix is the charge, not the argument. **Doc follow-up, mine to clean up.** Both HTTP docs still described the old behavior — `docs/sirens-echo-http-contract.md` listed it under tolerated behavior and `docs/sirens-echo-http.md` stated that only token-bucket denials carry the header. I wrote both about three hours ago and they went stale within one commit. Corrected in PR https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/254. **Closing from my side** once 254 lands. The behavior is fixed, the guard is positive, and the docs will match. Not verified against the deployed pod — the rollout is still behind ([deploy 426](https://forgejo.coilysiren.me/coilyco-bridge/deploy/issues/426)).

coilyco-ops referenced this issue

2026-08-13 04:33:38 +00:00

X-Sirens-Caller isolates the user tier only #182

coilyco-ops referenced this issue

2026-08-13 04:34:52 +00:00

X-Sirens-Caller isolates the user tier only #182

coilyco-ops referenced this issue from a commit

2026-08-13 04:40:34 +00:00

docs(http): the queue shed now carries Retry-After