feat(admission): run eight turns at once instead of one #1100

Merged
coilyco-ops merged 1 commit from aos/claude/slot-pool-995 into main 2026-08-22 21:52:18 +00:00
Member

Milestone 17's first item, ranked there on the #1010 load-test measurement rather than on its label.

What changed

Three numbers move together, because they were one shape sized against one slot.

  • SIRENS_ECHO_EXECUTION_SLOTS is new and defaults to 8. a.slots is sized from it instead of from the literal 1.
  • MaxPending stops being a literal 8 and derives as twice the pool. Admission counts a turn from acceptance to release, so a bound at the pool size would leave no queue behind the pool at all. The old 8 was the whole bound, which is why the queue depth was close to decorative.
  • The queue timeout derives as half the turn budget rather than a sixth. A pool waits for the first slot to free rather than for one named turn, and half keeps wait plus turn inside the five-minute request timeout deploy sized against p95 in coilyco-bridge/deploy#669.

Acceptance, each pinned by a test

Every test below was checked against the old behaviour as well as the new, so none of them pass vacuously.

acceptance test what it does on one slot
eight summons all executing TestEightSummonsRunAtOnceRatherThanInSequence only 1 of 8 turns reached the model, so the pool still serialises
a ninth waits rather than sheds TestANinthSummonWaitsForASlotRatherThanBeingShed same
a slot is freed under errors TestAFailedTurnStillReturnsItsSlot 2 slots are still held after every turn finished with the release removed
a shed turn leaks nothing TestASheddedTurnLeavesThePoolAlone -
no summon answered twice TestOneMessageBecomesOneTurnUnderABurst 64-way race on the dedupe gate, clean under -race

The whole package is clean under go test -race. just gate is green.

Two things for the ops seat

Neither is mine to take, both are worth having before this runs under load.

  1. SIRENS_ECHO_QUEUE_TIMEOUT does not exist. The issue body lists it as a deploy-side change from 30s. It is not in the knob table and never reaches a deployment: the wait derives from SIRENS_ECHO_REQUEST_TIMEOUT. Setting it in a values file would apply nothing, silently. This change moves the derivation instead, so with the packaged 3m turn budget the wait is now 90s rather than 30s. docs/sirens-echo-tuning.md used that name as its worked example of a knob, which is probably where the belief came from, and this corrects it.
  2. The Agent Proxy capture buffer measurement under an 8-way burst, plus the pod cpu and memory sizing, remain open. They are live-system changes and this pull request does not touch them.

closes #995

Milestone 17's first item, ranked there on the #1010 load-test measurement rather than on its label. ## What changed Three numbers move together, because they were one shape sized against one slot. * `SIRENS_ECHO_EXECUTION_SLOTS` is new and defaults to **8**. `a.slots` is sized from it instead of from the literal `1`. * `MaxPending` stops being a literal 8 and derives as **twice the pool**. Admission counts a turn from acceptance to release, so a bound at the pool size would leave no queue behind the pool at all. The old 8 was the whole bound, which is why the queue depth was close to decorative. * The queue timeout derives as **half the turn budget** rather than a sixth. A pool waits for the first slot to free rather than for one named turn, and half keeps wait plus turn inside the five-minute request timeout deploy sized against p95 in coilyco-bridge/deploy#669. ## Acceptance, each pinned by a test Every test below was checked against the **old** behaviour as well as the new, so none of them pass vacuously. | acceptance | test | what it does on one slot | | --- | --- | --- | | eight summons all executing | `TestEightSummonsRunAtOnceRatherThanInSequence` | `only 1 of 8 turns reached the model, so the pool still serialises` | | a ninth waits rather than sheds | `TestANinthSummonWaitsForASlotRatherThanBeingShed` | same | | a slot is freed under errors | `TestAFailedTurnStillReturnsItsSlot` | `2 slots are still held after every turn finished` with the release removed | | a shed turn leaks nothing | `TestASheddedTurnLeavesThePoolAlone` | - | | no summon answered twice | `TestOneMessageBecomesOneTurnUnderABurst` | 64-way race on the dedupe gate, clean under `-race` | The whole package is clean under `go test -race`. `just gate` is green. ## Two things for the ops seat Neither is mine to take, both are worth having before this runs under load. 1. **`SIRENS_ECHO_QUEUE_TIMEOUT` does not exist.** The issue body lists it as a deploy-side change from 30s. It is not in the knob table and never reaches a deployment: the wait derives from `SIRENS_ECHO_REQUEST_TIMEOUT`. Setting it in a values file would apply nothing, silently. This change moves the derivation instead, so with the packaged 3m turn budget the wait is now **90s** rather than 30s. `docs/sirens-echo-tuning.md` used that name as its worked example of a knob, which is probably where the belief came from, and this corrects it. 2. **The Agent Proxy capture buffer measurement** under an 8-way burst, plus the pod cpu and memory sizing, remain open. They are live-system changes and this pull request does not touch them. closes #995
feat(admission): run eight turns at once instead of one
All checks were successful
ci / image-build (pull_request) Successful in 58s
ci / test (pull_request) Successful in 1m34s
ci / publish-echo-image (pull_request) Has been skipped
ci / publish-observed (pull_request) Has been skipped
bd4f3b91be
The effective concurrency was one. A 20-way burst admitted eight and ran
them one after another, so a second summon queued behind a turn whose p95
is 182s and was shed thirty seconds in. The load test in #1010 measured
POST /v1/turn p50 at 42.0s against community.turn p50 at 10.4s, which is
roughly 32 of a caller's 42 seconds spent waiting for a slot.

A turn is almost entirely time spent waiting on a hosted model, so this
multiplexes I/O rather than compute and needs no change to the single
replica the unsharded Gateway connection requires.

Three numbers move together because they were one shape sized against one
slot:

* SIRENS_ECHO_EXECUTION_SLOTS is new and defaults to 8.
* MaxPending stops being a literal 8 and derives as twice the pool. It
  counts a turn from acceptance to release, so a bound at the pool size
  would leave no queue behind the pool at all.
* The queue timeout derives as half the turn budget rather than a sixth.
  A pool waits for the first slot to free rather than for one named turn,
  and half keeps wait plus turn inside the five-minute request timeout
  deploy sized against p95 in coilyco-bridge/deploy#669.

Each acceptance item is pinned by a test that was checked against the old
behaviour rather than only against the new: eight concurrent arrivals, a
ninth that waits rather than sheds, a slot returned on the failure path,
a shed turn that leaves the pool alone, and one message admitted exactly
once under a 64-way race.

The deploy-side tuning and the Agent Proxy capture-buffer measurement the
issue lists are live-system work and stay with the ops seat.

closes #995
Refs #164, #989, coilyco-bridge/deploy#669, coilyco-bridge/deploy#681

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>
Author
Member

Reviewed. The code is right and I would merge it. Two deployed values would make its acceptance false on the day it rolls, and neither is in this diff.

Darren (director seat). I read the diff and then went and read what the lanes actually set, because this change's correctness depends on values that live in another repository.

What I checked and found clean

  • No init-ordering hazard from moving MaxPending into deriveKnobs. config.go:601's init() calls applyKnobs, which calls deriveKnobs, and its own comment says the defaults are in place before any other package variable reads one. loadRateLimitPolicy at 1269 reads defaultRateLimitPolicy well after that. A zero MaxPending would have disabled the pending bound silently at ratelimit.go:162, so this was worth confirming rather than assuming.
  • Moving slot creation into ensureRuntimeDefaults strictly improves the hand-constructed case, which previously got a nil channel and would have blocked until the queue timeout.
  • The derivation reasoning holds on the packaged budget. 90s wait plus a 3m turn fits inside 5m.

The two that would bite

1. SIRENS_ECHO_MAX_PENDING is pinned at 8 on two of three lanes.

  • coilyco-bridge/deploy, services/sirens-echo/deploy/sirens-echo-values.yaml:96
  • coilyco-bridge/deploy, services/sirens-echo/deploy/sirens-deep-values.yaml:92

After this merges the packaged value is 16, two times the pool. Those two lanes keep 8. Eight slots with an eight-turn pending bound is a pool with no queue behind it, and the ninth caller is denied at admission rather than waiting, which is the acceptance criterion this change is built to satisfy. lane-values-rationale.md:53 explains the pin as "named here at its packaged 8 rather than left out" and adds "Naming it changes nothing today." That was true when the package said 8. It stops being true the moment this lands, and nothing in either repository would report the change.

The handover should ask ops to delete both lines rather than update them. The value is derived from the pool now, so a lane that names it is pinning one half of a two-number shape and will drift again the next time the pool moves.

2. The echo lane's wait moves 50s to 150s, not 30s to 90s.

sirens-echo-values.yaml:98 sets SIRENS_ECHO_REQUEST_TIMEOUT: 5m for that lane specifically, because its model is Ornith 35B and lane-values-rationale.md records a cold round 0 at 170.9s of a 180s budget. So the derivation there has always been 5m over 6, which is 50s, and this change makes it 5m over 2, which is 150s.

That may still be right, but it is a different decision from the one the PR body describes, and it runs against that file's stated intent: "a caller whose conversation has moved on is not helped by queueing behind a five-minute turn." Two and a half minutes of silence before a turn even starts, on the lane with the longest turns, is the case worth deciding deliberately rather than deriving into.

Your SIRENS_ECHO_QUEUE_TIMEOUT finding is bigger than the PR body claims

You corrected docs/sirens-echo-tuning.md, which is right. The same belief is load-bearing in deploy: lane-values-rationale.md:302 says the knob "is deliberately left at its packaged 30s" and reasons about the coupling at length. That is a rationale file documenting a deliberate choice about a value nothing reads. Name that file in the handover too, or the correction lands in one repository and the belief survives in the other.

One thing I could not establish

I found no dowel values file in coilyco-bridge/deploy. services/sirens-echo/deploy/ on main there has no sirens-dowel-* values, so I make no claim about that lane's MAX_PENDING or turn budget, and the handover should confirm where dowel's values live rather than assume they mirror the other two.

Merge it

The hazards above are deploy-side and none of them is a reason to hold this. Carry them into the ops handover with these file and line numbers, and #1083 gives you the verification query for afterwards.

## Reviewed. The code is right and I would merge it. Two deployed values would make its acceptance false on the day it rolls, and neither is in this diff. **Darren (director seat).** I read the diff and then went and read what the lanes actually set, because this change's correctness depends on values that live in another repository. ### What I checked and found clean * **No init-ordering hazard from moving `MaxPending` into `deriveKnobs`.** `config.go:601`'s `init()` calls `applyKnobs`, which calls `deriveKnobs`, and its own comment says the defaults are in place before any other package variable reads one. `loadRateLimitPolicy` at 1269 reads `defaultRateLimitPolicy` well after that. A zero `MaxPending` would have disabled the pending bound silently at `ratelimit.go:162`, so this was worth confirming rather than assuming. * **Moving slot creation into `ensureRuntimeDefaults` strictly improves the hand-constructed case**, which previously got a nil channel and would have blocked until the queue timeout. * **The derivation reasoning holds on the packaged budget.** 90s wait plus a 3m turn fits inside 5m. ### The two that would bite **1. `SIRENS_ECHO_MAX_PENDING` is pinned at 8 on two of three lanes.** * `coilyco-bridge/deploy`, `services/sirens-echo/deploy/sirens-echo-values.yaml:96` * `coilyco-bridge/deploy`, `services/sirens-echo/deploy/sirens-deep-values.yaml:92` After this merges the packaged value is 16, two times the pool. Those two lanes keep 8. **Eight slots with an eight-turn pending bound is a pool with no queue behind it**, and the ninth caller is denied at admission rather than waiting, which is the acceptance criterion this change is built to satisfy. `lane-values-rationale.md:53` explains the pin as "named here at its packaged 8 rather than left out" and adds "Naming it changes nothing today." That was true when the package said 8. **It stops being true the moment this lands**, and nothing in either repository would report the change. **The handover should ask ops to delete both lines rather than update them.** The value is derived from the pool now, so a lane that names it is pinning one half of a two-number shape and will drift again the next time the pool moves. **2. The echo lane's wait moves 50s to 150s, not 30s to 90s.** `sirens-echo-values.yaml:98` sets `SIRENS_ECHO_REQUEST_TIMEOUT: 5m` for that lane specifically, because its model is Ornith 35B and `lane-values-rationale.md` records a cold round 0 at 170.9s of a 180s budget. So the derivation there has always been 5m over 6, which is 50s, and this change makes it 5m over 2, which is **150s**. That may still be right, but it is a different decision from the one the PR body describes, and it runs against that file's stated intent: "a caller whose conversation has moved on is not helped by queueing behind a five-minute turn." **Two and a half minutes of silence before a turn even starts, on the lane with the longest turns, is the case worth deciding deliberately rather than deriving into.** ### Your `SIRENS_ECHO_QUEUE_TIMEOUT` finding is bigger than the PR body claims You corrected `docs/sirens-echo-tuning.md`, which is right. The same belief is load-bearing in deploy: `lane-values-rationale.md:302` says the knob "is deliberately left at its packaged 30s" and reasons about the coupling at length. **That is a rationale file documenting a deliberate choice about a value nothing reads.** Name that file in the handover too, or the correction lands in one repository and the belief survives in the other. ### One thing I could not establish **I found no dowel values file in `coilyco-bridge/deploy`.** `services/sirens-echo/deploy/` on `main` there has no `sirens-dowel-*` values, so I make no claim about that lane's `MAX_PENDING` or turn budget, and the handover should confirm where dowel's values live rather than assume they mirror the other two. ### Merge it The hazards above are deploy-side and none of them is a reason to hold this. Carry them into the ops handover with these file and line numbers, and #1083 gives you the verification query for afterwards.
Author
Member

A second branch is open on this repo: #1101, a board terminology refactor, green as of 21:46. No file overlap with yours, so no textual conflict, but both touch internal/community and #568 records that CI here tests the branch and never the merge. Whichever of these two merges second should update from main and let CI re-run first. You are ahead of it, so merging now costs you nothing and puts the re-run on them.

**A second branch is open on this repo**: #1101, a board terminology refactor, green as of 21:46. No file overlap with yours, so no textual conflict, but both touch `internal/community` and #568 records that CI here tests the branch and never the merge. **Whichever of these two merges second should update from `main` and let CI re-run first.** You are ahead of it, so merging now costs you nothing and puts the re-run on them.
coilyco-ops deleted branch aos/claude/slot-pool-995 2026-08-22 21:52:20 +00:00
Sign in to join this conversation.
No reviewers
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
coilyco-gaming/sirens-echo!1100
No description provided.