fix(turns): a dropped turn always tells the member #987

Merged

coilysiren merged 1 commit from fix/dropped-turn-always-speaks into main

2026-08-18 22:09:05 +00:00

coilyco-ops commented

2026-08-18 22:07:09 +00:00

Member

Closes #939. Board item 1 on #981, priority/P0.

The wall is arithmetic, not a hang

Three turns died at 30.56s, 30.44s, 30.47s with no reply, no error, no model call.

overridable(&defaultRequestTimeout, "SIRENS_ECHO_REQUEST_TIMEOUT", 3*time.Minute)
defaultQueueTimeout = defaultRequestTimeout / 6

The lane ran the packaged 3 minutes, so the slot wait was exactly 30 seconds. A 120ms spread across three turns is a budget expiring, not a network.

They never entered runTurn, which is why the traces carry no turn.input.accepted and no children.

Why they were silent

replyQueueTimeout returned before Reply, because the notice was throttled Burst: 1, Every: time.Second, keyed on the channel. p95 community.turn was 182.2s against one execution slot, so several turns queued behind the same slow work and expired inside the same second. The first was told. The rest reported discord_failure: not_attempted, which is the runtime recording that it never asked Discord anything.

The throttle was deliberate, and it is wrong here

Its purpose is right for a denial: saturation must not become an amplifier. A denial says no.

A drop says nothing after admission already said yes. The member did nothing wrong and cannot tell that apart from being ignored. Kai's call on #939 is "never drop silently".

The amplifier concern survives without it. Only admitted turns reach the queue and MaxPending caps how many wait at once, so a saturation burst produces at most that many notices rather than unbounded ones.

A previous fix added a failure reaction ahead of the throttle so a suppressed member got something. #939 is the measurement that a mark alone is not enough: three members read it as being ignored.

Tests replaced, not deleted

TestQueueTimeoutNoticeIsThrottledForDiscordOnly and TestAThrottledQueueTimeoutStillMarksTheMessage pinned the old rule. Both are replaced by tests pinning the new one, across both transports and specifically for a second drop inside one window. Replacing rather than deleting keeps the inversion visible to whoever reads this next, since the old tests were correct for their time.

contextKey and notifyQueueTimeout are removed rather than left dead, so nothing invites re-wiring the throttle onto this path.

Correction on the issue thread

My first root-cause comment on #939 blamed MCP roster assembly. It was wrong, and the issue's own evidence ruled it out: that path runs inside runTurn, well after turn.input.accepted, which the traces do not have. Corrected in the thread rather than edited away.

Not in this PR

#943 is not the same defect, which #981 item 2 asked to be settled either way. mcp.tools.cached is derived as len(entries) > 0 && reached == 0, so it reads true both for a healthy warm turn and for one where every server is backing off. #943's 21 cached: true spans are consistent with normal warm turns, and its zero-tool rounds remain unexplained. The first move there is fixing the attribute so those two states stop looking alike.

#940 is independent. Result bounding after the round trip shares nothing with this.

Verification

go build, go vet, go test ./..., pre-commit all pass.

The acceptance on #939 is a repeat of the load burst showing zero silent terminations. That is a live measurement and belongs to whoever can run one, so I will note the before-and-after on the issue rather than claim it here.

Closes #939. Board item 1 on #981, `priority/P0`. ## The wall is arithmetic, not a hang Three turns died at **30.56s, 30.44s, 30.47s** with no reply, no error, no model call. ```go overridable(&defaultRequestTimeout, "SIRENS_ECHO_REQUEST_TIMEOUT", 3*time.Minute) defaultQueueTimeout = defaultRequestTimeout / 6 ``` The lane ran the packaged 3 minutes, so the slot wait was **exactly 30 seconds**. A 120ms spread across three turns is a budget expiring, not a network. They never entered `runTurn`, which is why the traces carry no `turn.input.accepted` and no children. ## Why they were silent `replyQueueTimeout` returned before `Reply`, because the notice was throttled `Burst: 1, Every: time.Second`, keyed on the **channel**. p95 `community.turn` was 182.2s against one execution slot, so several turns queued behind the same slow work and expired inside the same second. The first was told. The rest reported `discord_failure: not_attempted`, which is the runtime recording that it never asked Discord anything. ## The throttle was deliberate, and it is wrong here Its purpose is right for a **denial**: saturation must not become an amplifier. A denial says no. A **drop** says nothing after admission already said yes. The member did nothing wrong and cannot tell that apart from being ignored. Kai's call on #939 is "never drop silently". The amplifier concern survives without it. Only admitted turns reach the queue and `MaxPending` caps how many wait at once, so a saturation burst produces at most that many notices rather than unbounded ones. A previous fix added a failure reaction ahead of the throttle so a suppressed member got *something*. #939 is the measurement that a mark alone is not enough: three members read it as being ignored. ## Tests replaced, not deleted `TestQueueTimeoutNoticeIsThrottledForDiscordOnly` and `TestAThrottledQueueTimeoutStillMarksTheMessage` pinned the old rule. Both are replaced by tests pinning the new one, across both transports and specifically for a second drop inside one window. Replacing rather than deleting keeps the inversion visible to whoever reads this next, since the old tests were correct for their time. `contextKey` and `notifyQueueTimeout` are removed rather than left dead, so nothing invites re-wiring the throttle onto this path. ## Correction on the issue thread My first root-cause comment on #939 blamed MCP roster assembly. It was wrong, and the issue's own evidence ruled it out: that path runs inside `runTurn`, well after `turn.input.accepted`, which the traces do not have. Corrected in the thread rather than edited away. ## Not in this PR **#943 is not the same defect**, which #981 item 2 asked to be settled either way. `mcp.tools.cached` is derived as `len(entries) > 0 && reached == 0`, so it reads true both for a healthy warm turn and for one where every server is backing off. #943's 21 `cached: true` spans are consistent with normal warm turns, and its zero-tool rounds remain unexplained. The first move there is fixing the attribute so those two states stop looking alike. **#940 is independent.** Result bounding after the round trip shares nothing with this. ## Verification `go build`, `go vet`, `go test ./...`, `pre-commit` all pass. The acceptance on #939 is a repeat of the load burst showing zero silent terminations. That is a live measurement and belongs to whoever can run one, so I will note the before-and-after on the issue rather than claim it here.

coilyco-ops added 1 commit

2026-08-18 22:07:09 +00:00

fix(turns): a dropped turn always tells the member

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

Details

ci / test (pull_request) Successful in 1m3s

Details

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

Details

ci / publish-observed (pull_request) Has been skipped

Details

804579bf87

Closes #939.

Three of fourteen turns on owl.glass ended with no reply, no error, and no
model call, at 30.56s, 30.44s, and 30.47s.

That wall is arithmetic. defaultQueueTimeout is defaultRequestTimeout / 6, and
the lane ran the packaged 3 minutes, so the slot wait was exactly 30 seconds.
The turns never entered runTurn, which is why the traces carry no
turn.input.accepted and no children at all.

On expiry replyQueueTimeout ran and returned before Reply, because the notice
was throttled to Burst 1 / Every 1s keyed on the channel. p95 community.turn was
182.2s against one execution slot, so several turns queued behind the same slow
work and expired inside the same second. The first was told. The rest were not,
and reported discord_failure: not_attempted, which is the runtime saying it
never asked Discord anything.

THE THROTTLE WAS DELIBERATE AND IS NOW WRONG HERE. Its purpose, that saturation
must not become an amplifier, is right for a denial. A denial says no. A drop
says nothing after admission already said yes, and the member cannot tell that
apart from being ignored. Kai's call on #939 is "never drop silently".

The amplifier concern is bounded without it: only admitted turns reach the
queue, and MaxPending caps how many wait at once, so a saturation burst
produces at most that many notices rather than unbounded ones.

A previous fix added a failure reaction ahead of the throttle so a suppressed
member got something. #939 is the measurement that a mark alone is not enough.
The mark stays and the notice joins it.

contextKey and notifyQueueTimeout are removed rather than left dead, so nothing
invites re-wiring the throttle onto this path.

TESTS. TestQueueTimeoutNoticeIsThrottledForDiscordOnly and
TestAThrottledQueueTimeoutStillMarksTheMessage pinned the old rule and are
replaced by tests pinning the new one, across both transports and for a second
drop inside one window. Replacing them rather than deleting them keeps the
inversion visible to whoever reads this next.

NOTE FOR THE LANE. coilyco-bridge/deploy#669 raised this lane's request timeout
to 5m for its own good reasons, which moves the slot wait to 50s. That made a
dropped turn slower to arrive and no less silent. This is the half that fixes.

go build, go vet, go test ./..., and pre-commit all pass.

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>