The failure notice's own 10 second bound has no test, so removing it would leave a wedged send unbounded #627

New issue

Closed

opened 2026-08-13 17:29:49 +00:00 by coilyco-ops · 1 comment

coilyco-ops commented

2026-08-13 17:29:49 +00:00

Member

Filed by Angie (ENG, claude seat) after building a duplicate of #623 and checking what, if anything, mine covered that the landed one does not. This is the only thing. Small, and it guards the most common failure this service has.

The property

notifyFailure does two independent detachments, and its comment names only the second:

// notifyFailure sends a notice on a context detached from the turn deadline. A
// turn that failed by expiring has no budget left to say so otherwise.
func (a *Agent) notifyFailure(ctx context.Context, turn turnIO, notice string) error {
	noticeCtx, cancel := context.WithTimeout(
		context.WithoutCancel(ctx),
		failureNoticeTimeout,
	)

WithoutCancel — the notice survives a turn that has already been cancelled
WithTimeout — the notice gets its own 10 second bound rather than none

noticethreading_test.go from 623 covers the threading mark thoroughly, including a control and a row pinning that WithoutCancel carries values. It does not assert either property above, because they were not what that issue was about.

Why it matters more than its size

The timeout turn is the case this protects, and it is the common one. A turn that fails by expiring reaches failTurn with an already-cancelled context. Without WithoutCancel, sendReply is handed a dead context and the member gets silence — on the single most frequent failure mode this service has. #178 calls dead air the worst outcome, and this one line is what prevents it for every timeout.

And WithoutCancel reads like a redundancy. A reasonable person simplifying this would see WithTimeout immediately below and conclude the inner call is doing nothing. The suite would stay green while every timed-out turn went quiet.

The WithTimeout half has the same exposure in the other direction: drop it and a wedged send holds a turn open with no bound at all.

Same shape as a defect already recorded here

This is the pattern from #604 — turnReference had no row, so the mechanism deciding one thing was untested from the day it landed. Both are load-bearing lines sitting under a comment that explains them, with nothing that fails if they go.

Acceptance

A notice derived from an already-cancelled turn context is not itself cancelled, and carries a deadline no later than failureNoticeTimeout. Both rows fail if the corresponding call is removed.

Note on how this was found

I built the whole of 623 independently before discovering it had merged, which is a fresh instance of #552. Recorded there separately. This issue is what survived the comparison rather than a consolation prize for it — I checked their tests row by row before claiming anything was missing.

**Filed by Angie (ENG, claude seat)** after building a duplicate of https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/623 and checking what, if anything, mine covered that the landed one does not. **This is the only thing.** Small, and it guards the most common failure this service has. ## The property `notifyFailure` does two independent detachments, and its comment names only the second: ```go // notifyFailure sends a notice on a context detached from the turn deadline. A // turn that failed by expiring has no budget left to say so otherwise. func (a *Agent) notifyFailure(ctx context.Context, turn turnIO, notice string) error { noticeCtx, cancel := context.WithTimeout( context.WithoutCancel(ctx), failureNoticeTimeout, ) ``` 1. **`WithoutCancel`** — the notice survives a turn that has already been cancelled 2. **`WithTimeout`** — the notice gets its own 10 second bound rather than none `noticethreading_test.go` from 623 covers the threading mark thoroughly, including a control and a row pinning that `WithoutCancel` carries values. **It does not assert either property above**, because they were not what that issue was about. ## Why it matters more than its size **The timeout turn is the case this protects, and it is the common one.** A turn that fails by expiring reaches `failTurn` with an already-cancelled context. Without `WithoutCancel`, `sendReply` is handed a dead context and the member gets silence — on the single most frequent failure mode this service has. https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/178 calls dead air the worst outcome, and this one line is what prevents it for every timeout. **And `WithoutCancel` reads like a redundancy.** A reasonable person simplifying this would see `WithTimeout` immediately below and conclude the inner call is doing nothing. The suite would stay green while every timed-out turn went quiet. The `WithTimeout` half has the same exposure in the other direction: drop it and a wedged send holds a turn open with no bound at all. ## Same shape as a defect already recorded here This is the pattern from https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/604 — `turnReference` had no row, so the mechanism deciding one thing was untested from the day it landed. Both are load-bearing lines sitting under a comment that explains them, with nothing that fails if they go. ## Acceptance A notice derived from an already-cancelled turn context is not itself cancelled, and carries a deadline no later than `failureNoticeTimeout`. Both rows fail if the corresponding call is removed. ## Note on how this was found I built the whole of 623 independently before discovering it had merged, which is a fresh instance of https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/552. Recorded there separately. This issue is what survived the comparison rather than a consolation prize for it — I checked their tests row by row before claiming anything was missing.

coilyco-ops changed title from ~~The failure notice's deadline detachment has no test, so the property its comment promises can be deleted silently~~ to The failure notice's own 10 second bound has no test, so removing it would leave a wedged send unbounded

2026-08-13 17:34:11 +00:00

coilyco-ops commented

2026-08-13 17:34:28 +00:00

Author

Member

Correction to my own filing, before anyone acts on it. Half of what I claimed was untested is tested. Angie (ENG, claude seat).

I wrote that notifyFailure's two detachments both had nothing reaching them. The WithoutCancel half is covered. failure_test.go:116:

func TestFailureNoticeSurvivesAnExpiredTurnContext(t *testing.T) {
	agent := failingAgent(context.DeadlineExceeded)
	expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second))
	...
	if turn.reply != noticeTimedOut {

I searched the notice* files and stopped there, having just read three of them. The property is asserted a file away, indirectly but properly — it checks the notice text actually arrived, which cannot happen on a cancelled context. Title corrected.

What I ran, rather than what I assumed the second time

Two mutations of notifyFailure, whole suite each time:

drop context.WithoutCancel   -> TestFailureNoticeSurvivesAnExpiredTurnContext  (existing)
                                TestANoticeSurvivesAnExpiredTurn               (mine, redundant)

drop context.WithTimeout     -> TestANoticeCarriesItsOwnDeadline               (mine, alone)

Nothing in the suite catches the second mutation. That is the real gap and it is one row, not two.

So I dropped my WithoutCancel row rather than ship a second assertion of a covered property, and dropped its premise test with it. What remains is one test whose failure message names the consequence: a send that never returns holds the turn open with no bound, because the turn's own bound was deliberately discarded one line earlier.

The general point, which survives the correction

The two calls are adjacent, they look like one idiom, and only one of them was pinned. A partial guard on a compound line is harder to see than no guard, because the file's presence in the suite reads as coverage of the whole construct.

Revised acceptance: the notice's deadline is no later than failureNoticeTimeout, and the row fails when WithTimeout is removed. The WithoutCancel half needs nothing.

**Correction to my own filing, before anyone acts on it. Half of what I claimed was untested is tested. Angie (ENG, claude seat).** I wrote that `notifyFailure`'s two detachments both had nothing reaching them. **The `WithoutCancel` half is covered.** `failure_test.go:116`: ```go func TestFailureNoticeSurvivesAnExpiredTurnContext(t *testing.T) { agent := failingAgent(context.DeadlineExceeded) expired, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) ... if turn.reply != noticeTimedOut { ``` I searched the `notice*` files and stopped there, having just read three of them. The property is asserted a file away, indirectly but properly — it checks the notice text actually arrived, which cannot happen on a cancelled context. Title corrected. ## What I ran, rather than what I assumed the second time Two mutations of `notifyFailure`, whole suite each time: ``` drop context.WithoutCancel -> TestFailureNoticeSurvivesAnExpiredTurnContext (existing) TestANoticeSurvivesAnExpiredTurn (mine, redundant) drop context.WithTimeout -> TestANoticeCarriesItsOwnDeadline (mine, alone) ``` **Nothing in the suite catches the second mutation.** That is the real gap and it is one row, not two. So I dropped my `WithoutCancel` row rather than ship a second assertion of a covered property, and dropped its premise test with it. What remains is one test whose failure message names the consequence: a send that never returns holds the turn open with no bound, because the turn's own bound was deliberately discarded one line earlier. ## The general point, which survives the correction The two calls are adjacent, they look like one idiom, and only one of them was pinned. **A partial guard on a compound line is harder to see than no guard**, because the file's presence in the suite reads as coverage of the whole construct. Revised acceptance: the notice's deadline is no later than `failureNoticeTimeout`, and the row fails when `WithTimeout` is removed. The `WithoutCancel` half needs nothing.

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-13 17:35:40 +00:00

test(notice): pin the bound the failure notice keeps for itself #632

coilyco-ops referenced this issue

2026-08-13 17:36:00 +00:00

The failure notice inherits the turn's long-reply state, so a 90 byte apology takes a thread and a model call to title it #625

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-13 17:36:02 +00:00

test(notice): pin both detachments notifyFailure performs #634

coilyco-ops referenced this issue

2026-08-13 17:36:30 +00:00

The failure notice is composed, then a 52-tool agent turn runs to deliver it and blows the 10s reply budget, so nothing is sent #619