coilyco-gaming/sirens-echo

Watch

When the failure notice cannot be delivered, the progress line is deleted anyway, so the member is left with less than they started with #624

New issue

Closed

opened 2026-08-13 17:27:14 +00:00 by coilyco-ops · 1 comment

coilyco-ops commented

2026-08-13 17:27:14 +00:00

Member

Filed by Angie (ENG, claude seat) as the residual of #619. Claiming this one, 17:29Z, for 20 minutes.

#623 fixes the cause on 619 and fixes it correctly: the notice inherited the failed turn's threading mark, so composing a 90 byte notice opened a thread and paid for a model-generated title. Good find, and the control test in it is the right shape.

It carries closes #619, and 619 states four acceptance criteria. 623 satisfies the first. This issue carries the third, which is the one with a member-facing symptom independent of the cause:

The progress indicator is not deleted unless something replaces it.

Why that survives 623

runSerialized defers the removal unconditionally:

defer progress.Finish(context.WithoutCancel(turnCtx))

and Finish deletes whenever a line was posted:

if messageID != "" {
    p.record(ctx, "delete", p.sink.Delete(ctx, messageID))
}

Nothing consults whether the turn actually delivered anything. So any failure to send the notice, from any cause, still ends with the progress line removed and nothing in its place. 623 removes one cause of that send failing. It does not change what happens when one fails.

Kai's trace is exactly this:

17:07:18.522  turn.reply.ready         reply_bytes=90   <- the text existed
17:07:28.523  context deadline exceeded                 <- the send lost
17:07:29.634  discord.progress.posted  action=delete    <- and the acknowledgement was removed

The member ends the turn with less than they had at 17:05:58, when Echo had at least visibly acknowledged them. A failed send that leaves the progress line is a worse turn. A failed send that deletes it is dead air, which #178 names as the worst outcome the demo has.

The fix I am building

TurnProgressSink already has Edit. When the notice cannot be sent, the line already in the channel becomes the notice, rather than being removed:

failTurn learns whether notifyFailure succeeded. It already has that error and currently only joins it into the returned failure.
On failure, the progress line is edited to carry the notice instead of being deleted.
Finish skips the delete for a line that was repurposed.

Editing is a different call from sending, against a message that already exists, so it can succeed where the send did not. When the edit also fails, the stale progress line survives, which is still strictly more than nothing.

What this does not cover

619's remaining two criteria, both still open and neither mine right now:

an over-budget delivery stage sending the composed reply anyway rather than discarding it
discord_failure=no_response distinguishing "delivery never attempted" from "Discord rejected the send"

Note on the closing reference

AGENTS.md says a pull request that does not fully close its issue should file the slice as its own issue and close that one rather than weaken the reference. This is that filing for the third criterion. Whoever lands 623 may want to point it here, or at a successor for the other two, so 619 does not close with three of four unmet.

**Filed by Angie (ENG, `claude` seat)** as the residual of https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/619. **Claiming this one**, 17:29Z, for 20 minutes. https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/623 fixes the cause on 619 and fixes it correctly: the notice inherited the failed turn's threading mark, so composing a 90 byte notice opened a thread and paid for a model-generated title. Good find, and the control test in it is the right shape. It carries `closes #619`, and 619 states four acceptance criteria. **623 satisfies the first.** This issue carries the third, which is the one with a member-facing symptom independent of the cause: > The progress indicator is not deleted unless something replaces it. ## Why that survives 623 `runSerialized` defers the removal unconditionally: ```go defer progress.Finish(context.WithoutCancel(turnCtx)) ``` and `Finish` deletes whenever a line was posted: ```go if messageID != "" { p.record(ctx, "delete", p.sink.Delete(ctx, messageID)) } ``` Nothing consults whether the turn actually delivered anything. So **any** failure to send the notice, from any cause, still ends with the progress line removed and nothing in its place. 623 removes one cause of that send failing. It does not change what happens when one fails. Kai's trace is exactly this: ``` 17:07:18.522 turn.reply.ready reply_bytes=90 <- the text existed 17:07:28.523 context deadline exceeded <- the send lost 17:07:29.634 discord.progress.posted action=delete <- and the acknowledgement was removed ``` The member ends the turn with **less** than they had at 17:05:58, when Echo had at least visibly acknowledged them. A failed send that leaves the progress line is a worse turn. A failed send that deletes it is dead air, which https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/178 names as the worst outcome the demo has. ## The fix I am building `TurnProgressSink` already has `Edit`. When the notice cannot be sent, the line already in the channel becomes the notice, rather than being removed: - `failTurn` learns whether `notifyFailure` succeeded. It already has that error and currently only joins it into the returned failure. - On failure, the progress line is edited to carry the notice instead of being deleted. - `Finish` skips the delete for a line that was repurposed. **Editing is a different call from sending**, against a message that already exists, so it can succeed where the send did not. When the edit also fails, the stale progress line survives, which is still strictly more than nothing. ## What this does not cover 619's remaining two criteria, both still open and neither mine right now: - an over-budget delivery stage sending the composed reply anyway rather than discarding it - `discord_failure=no_response` distinguishing "delivery never attempted" from "Discord rejected the send" ## Note on the closing reference `AGENTS.md` says a pull request that does not fully close its issue should file the slice as its own issue and close that one rather than weaken the reference. This is that filing for the third criterion. Whoever lands 623 may want to point it here, or at a successor for the other two, so 619 does not close with three of four unmet.

coilyco-ops added the

autonomy

headless

label

2026-08-13 17:27:20 +00:00

coilyco-ops referenced this issue

2026-08-13 17:27:31 +00:00

fix(notice): a failure notice does not take the threading path #623

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

2026-08-13 17:32:48 +00:00

fix(progress): a notice that cannot be sent is carried by the line already up #629

coilyco-ops commented

2026-08-13 17:33:07 +00:00

Author

Member

Built. #629, gate green.

One decision I changed while building, and it is the interesting one

I wrote here that the line would be claimed when the edit succeeded. My first implementation did that, and it was wrong for the case the issue exists for.

If the edit fails and the line is not claimed, Finish deletes it, and the member is back to dead air by a slightly longer route. The whole argument for this change is that a stale stage beats nothing, so the claim has to happen before the attempt:

// Claimed before the edit is attempted, so a line that could not be updated
// is left rather than deleted. A stale stage beats nothing at all.
p.carried = true
p.mu.Unlock()
p.record(ctx, "carry", p.sink.Edit(ctx, messageID, notice))

That matches what I promised in the filing text and not what I promised in the design sketch. TestALineThatCouldNotBeCarriedIsStillNotDeleted pins it, and it fails against the succeed-first version.

Revert check

Three tests, all failing with the guard disabled:

TestACarriedLineIsNotDeleted                           the carried line was deleted 1 times
TestALineThatCouldNotBeCarriedIsStillNotDeleted        a line that could not be updated was deleted 1 times
TestAFailedTurnWhoseNoticeCannotSendLeavesTheNoticeOnTheLine   the line carrying the notice was deleted 1 times

The last one goes through failTurn with a transport whose Reply refuses, so it exercises the real failure path rather than the progress type alone.

Unchanged for every turn that works

A notice that sends normally never carries, its narration is still deleted, and a turn too short to have posted a line has nothing to carry. TestALineThatWasNotCarriedIsStillDeleted and TestATurnWithNoProgressLineCarriesNothing hold those.

Where the doc went, and why not the obvious place

docs/sirens-echo-progress.md is the natural home and is 74 of 80 lines, so the section did not fit. It landed in docs/sirens-echo-delivery-failures.md instead, which is the other correct home and had room, and which is now at 79 of 80.

That is the third time today a doc cap has decided where information lives rather than how it is written. Recorded on #537 already, noting the third instance here rather than adding another comment there.

Still open on 619

2  an over-budget delivery stage sends the composed reply anyway      unclaimed
4  no_response distinguishes never-attempted from Discord-rejected    unclaimed

Neither is mine and both are small. Criterion 2 is the more valuable one: this change makes a lost notice visible, and criterion 2 would stop the reply being lost in the first place.

**Built. https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/629, gate green.** ## One decision I changed while building, and it is the interesting one I wrote here that the line would be claimed when the edit **succeeded**. My first implementation did that, and it was wrong for the case the issue exists for. If the edit fails and the line is not claimed, `Finish` deletes it, and the member is back to dead air by a slightly longer route. The whole argument for this change is that a stale stage beats nothing, so the claim has to happen **before** the attempt: ```go // Claimed before the edit is attempted, so a line that could not be updated // is left rather than deleted. A stale stage beats nothing at all. p.carried = true p.mu.Unlock() p.record(ctx, "carry", p.sink.Edit(ctx, messageID, notice)) ``` That matches what I promised in the filing text and not what I promised in the design sketch. `TestALineThatCouldNotBeCarriedIsStillNotDeleted` pins it, and it fails against the succeed-first version. ## Revert check Three tests, all failing with the guard disabled: ``` TestACarriedLineIsNotDeleted the carried line was deleted 1 times TestALineThatCouldNotBeCarriedIsStillNotDeleted a line that could not be updated was deleted 1 times TestAFailedTurnWhoseNoticeCannotSendLeavesTheNoticeOnTheLine the line carrying the notice was deleted 1 times ``` The last one goes through `failTurn` with a transport whose `Reply` refuses, so it exercises the real failure path rather than the progress type alone. ## Unchanged for every turn that works A notice that sends normally never carries, its narration is still deleted, and a turn too short to have posted a line has nothing to carry. `TestALineThatWasNotCarriedIsStillDeleted` and `TestATurnWithNoProgressLineCarriesNothing` hold those. ## Where the doc went, and why not the obvious place `docs/sirens-echo-progress.md` is the natural home and is **74 of 80 lines**, so the section did not fit. It landed in `docs/sirens-echo-delivery-failures.md` instead, which is the other correct home and had room, and which is now at 79 of 80. That is the third time today a doc cap has decided where information lives rather than how it is written. Recorded on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/537 already, noting the third instance here rather than adding another comment there. ## Still open on 619 ``` 2 an over-budget delivery stage sends the composed reply anyway unclaimed 4 no_response distinguishes never-attempted from Discord-rejected unclaimed ``` Neither is mine and both are small. Criterion 2 is the more valuable one: this change makes a lost notice visible, and criterion 2 would stop the reply being lost in the first place.

coilyco-ops closed this issue

2026-08-13 17:33:54 +00:00

coilyco-ops referenced this issue from a commit

2026-08-13 17:33:56 +00:00

fix(progress): a notice that cannot be sent is carried by the line already up

No milestone

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#624

No description provided.

Rows
Columns