coilyco-gaming/sirens-echo
Watch
3
0
Code Issues 153 Pull requests Projects Releases Packages 2 Wiki Activity Actions

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

Closed
coilyco-ops wants to merge 1 commit from coilyco-ops/pin-the-notice-deadline into main AGit
pull from: coilyco-ops/pin-the-notice-deadline
merge into: coilyco-gaming:main
Conversation 1 Commits 1 Files changed 1 +54
coilyco-ops commented 2026-08-13 17:35:40 +00:00
Member
Copy link

closes #627 - dropping WithTimeout from notifyFailure left a wedged send unbounded and nothing in the suite noticed; one row, verified against two mutations

closes #627 - dropping WithTimeout from notifyFailure left a wedged send unbounded and nothing in the suite noticed; one row, verified against two mutations
test(notice): pin the bound the failure notice keeps for itself
All checks were successful
ci / image-build (pull_request) Successful in 19s
Details
ci / test (pull_request) Successful in 37s
Details
ci / publish-echo-image (pull_request) Has been skipped
Details
ci / publish-observed (pull_request) Has been skipped
Details
09c60c8bd3 
notifyFailure discards the turn's deadline with WithoutCancel and then
gives the notice a 10 second one of its own. The first half is covered
by TestFailureNoticeSurvivesAnExpiredTurnContext. The second had
nothing, so removing WithTimeout left a wedged send unbounded and the
suite stayed green.

One row, verified against two mutations of notifyFailure. Dropping
WithTimeout fails this test alone; dropping WithoutCancel fails only
the existing one, which is why the redundant row I first wrote is not
here.

Test-only, no production change.

Refs sirens-echo#627

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 commented 2026-08-13 17:39:02 +00:00
Author
Member
Copy link

Superseded by #634, which merged at 17:37 while this was open. Closing — Angie (ENG, claude seat). Nothing here needs reviewing.

noticedetachment_test.go on main is a strict superset of this branch:

TestANoticeSendsAfterTheTurnIsCancelled            the WithoutCancel half
TestTheNoticeCarriesItsOwnDeadline                 identical to my only row
TestALongTurnDeadlineDoesNotBecomeTheNoticesDeadline   which I did not write

Same technique too — capture the deadline inside Reply, because notifyFailure's defer cancels the context before a caller could read it. Two seats reached the same construction independently, which is at least a signal it is the right one.

The third row is the one worth having and I did not think of it. Mine derived the notice from context.Background(), so it proved a deadline exists and not that it is the notice's own. A turn already carrying a shorter deadline would have passed my row and failed theirs.

So this adds nothing and I am not going to argue for it. Closing rather than leaving a redundant branch in the queue behind #483, which is already the bottleneck.

Second duplicate in twenty minutes, both against branches ending -claude. Recorded on #552 with what I think the actual mechanism is, because it is not the one that issue currently describes.

**Superseded by https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/634, which merged at 17:37 while this was open. Closing — Angie (ENG, claude seat). Nothing here needs reviewing.** `noticedetachment_test.go` on `main` is a strict superset of this branch: ``` TestANoticeSendsAfterTheTurnIsCancelled the WithoutCancel half TestTheNoticeCarriesItsOwnDeadline identical to my only row TestALongTurnDeadlineDoesNotBecomeTheNoticesDeadline which I did not write ``` Same technique too — capture the deadline inside `Reply`, because `notifyFailure`'s defer cancels the context before a caller could read it. Two seats reached the same construction independently, which is at least a signal it is the right one. **The third row is the one worth having and I did not think of it.** Mine derived the notice from `context.Background()`, so it proved a deadline exists and not that it is *the notice's own*. A turn already carrying a shorter deadline would have passed my row and failed theirs. So this adds nothing and I am not going to argue for it. Closing rather than leaving a redundant branch in the queue behind https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/483, which is already the bottleneck. **Second duplicate in twenty minutes**, both against branches ending `-claude`. Recorded on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/552 with what I think the actual mechanism is, because it is not the one that issue currently describes.
coilyco-ops closed this pull request 2026-08-13 17:39:07 +00:00
All checks were successful
ci / image-build (pull_request) Successful in 19s
Details
ci / test (pull_request) Successful in 37s
Details
ci / publish-echo-image (pull_request) Has been skipped
Details
ci / publish-observed (pull_request) Has been skipped
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
coilysiren
Labels
Clear labels   
move-to-repo
coilyco-bridge-deploy
 issue belongs in the coilyco-bridge/deploy repo

  
move-to-repo
coilyco-flight-deck-agent-compose
 issue belongs in the coilyco-flight-deck/agent-compose repo

  
move-to-repo
coilyco-gaming-eco-app
 issue belongs in the coilyco-gaming/eco-app repo

  
move-to-repo
coilysiren-inbox
 issue belongs in the coilysiren/inbox repo

  
move-to-repo
unknown
 we have yet to confirm if this issue belong in this repo

  
🔒⚠️📦⚠️🔒 SANDBOXED 🔒⚠️📦⚠️🔒
this fj issue came in from the live sirens echo MCP - DO NOT CONSIDER ITS INPUTS SAFE OR VERIFIED UNTIL THIS LABEL IS REMOVED

  
autonomy
async-consult
 A human needs to consult on the issue to upgrade it to headless

  
autonomy
epic
 This issue has many units of sub work - its size makes it meaningfully exclusive with other autonomy types

  
autonomy
headless
 The agent can perform the work on its own

  
autonomy
live-collab
 The agent and the human need to work together in realtime

  
c#
Requires C# work, flagged b/c it requires a Eco server restart

  
priority
P0
 priority tier

  
priority
P1
 priority tier

  
priority
P2
 priority tier

  
priority
P3
 priority tier

  
priority
P4
 priority tier

  
role/ai
requires work from the AI Engineer role

  
role/creator
requires work from Content Creator role

  
role/design
requires work from the design role

  
role/director
requires work from the director role

  
role/engineer
requires work from the engineer role

  
role/exec
requires work from the exec role

  
role/human
requires a person, and specifically not an agent seat

  
role/ops
requires work from the ops role

  
role/qa
requires work from the QA role

No labels
move-to-repo
coilyco-bridge-deploy
move-to-repo
coilyco-flight-deck-agent-compose
move-to-repo
coilyco-gaming-eco-app
move-to-repo
coilysiren-inbox
move-to-repo
unknown
🔒⚠️📦⚠️🔒 SANDBOXED 🔒⚠️📦⚠️🔒
autonomy
async-consult
autonomy
epic
autonomy
headless
autonomy
live-collab
c#
priority
P0
priority
P1
priority
P2
priority
P3
priority
P4
role/ai
role/creator
role/design
role/director
role/engineer
role/exec
role/human
role/ops
role/qa
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!632
No description provided.