coilyco-gaming/sirens-echo

Watch

gracefully end active turns on SIGTERM #597

New issue

Closed

opened 2026-08-13 16:47:44 +00:00 by coilysiren · 2 comments

coilysiren commented

2026-08-13 16:47:44 +00:00

Owner

probs with a nice user notice about server shutdown

coilyco-ops commented

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

Member

Claiming. Angie (ENG, claude seat claude-macos-…-ee99), 16:54Z, for 20 minutes.

Read the shutdown path before claiming, and the defect is larger than a missing notice. Recording it here so the acceptance is checkable rather than a matter of taste.

What SIGTERM does today

cmd/sirens-echo/main.go:54 builds a signal.NotifyContext and hands it to Agent.Run. On cancel, Run does this:

case <-ctx.Done():
    shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
    if err := httpServer.Shutdown(shutdownCtx); err != nil { ... }
    return nil

Then the deferred a.session.Close(), a.tools.Close(), and a.jobs.Stop() fire and the process exits.

A live Discord turn is not part of any of that. Every one of them is rooted at context.Background():

receiveCtx, receiveSpan := a.telemetry.StartSpan(
    context.Background(),      // agent.go:756
    "discord.receive",
    ...
)

So the shutdown context reaches the HTTP listener and nothing else. Three consequences, in increasing order of how bad they are:

No notice. The member is left with an eyes mark and no reply, forever. Indistinguishable from the bot ignoring them.
No cancellation. The turn is not asked to stop. It is ended by process exit, mid-model-call.
The floor is removed first. session.Close() and tools.Close() run while turns are still executing, so a turn that lives a few more microseconds is talking to a closed gateway and a closed MCP. The drain order is inverted.

HTTP turns are fine, incidentally - they root at the request context, so httpServer.Shutdown already waits for them. This is Discord-only.

What I am building

A wait group over in-flight Discord turns, entered in admitMessage before the go a.handleMessage.
A drain state that refuses new summons once shutdown starts, marked the way an admission denial already is.
Discord turns rooted at an agent-held context so a drain can actually cancel them, rather than at context.Background().
On ctx.Done(): stop admitting, wait a bounded grace for turns to finish on their own, cancel what is left, then close the session and MCP - in that order rather than the current one.
The notice you asked for, distinguishable from the existing timeout one, because > \turn timed out, retry shortly`` would be a lie about what happened.

The one thing I will not choose alone

The grace period. Too short and this changes nothing; too long and a rolling deploy stalls on one slow turn. RequestTimeout is the natural ceiling and Kubernetes' default terminationGracePeriodSeconds is 30, so the drain has to fit inside whatever the deploy manifest sets. I will pick a value that fits the manifest, name it in the pull request, and make it a config field so Ops can move it without a rebuild.

Labelling headless: the acceptance above is checkable from code and a test, and the grace period is a bounded default rather than a decision.

**Claiming. Angie (ENG, claude seat `claude-macos-…-ee99`), 16:54Z, for 20 minutes.** Read the shutdown path before claiming, and the defect is larger than a missing notice. Recording it here so the acceptance is checkable rather than a matter of taste. ## What SIGTERM does today `cmd/sirens-echo/main.go:54` builds a `signal.NotifyContext` and hands it to `Agent.Run`. On cancel, `Run` does this: ```go case <-ctx.Done(): shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) if err := httpServer.Shutdown(shutdownCtx); err != nil { ... } return nil ``` Then the deferred `a.session.Close()`, `a.tools.Close()`, and `a.jobs.Stop()` fire and the process exits. **A live Discord turn is not part of any of that.** Every one of them is rooted at `context.Background()`: ```go receiveCtx, receiveSpan := a.telemetry.StartSpan( context.Background(), // agent.go:756 "discord.receive", ... ) ``` So the shutdown context reaches the HTTP listener and nothing else. Three consequences, in increasing order of how bad they are: 1. **No notice.** The member is left with an eyes mark and no reply, forever. Indistinguishable from the bot ignoring them. 2. **No cancellation.** The turn is not asked to stop. It is ended by process exit, mid-model-call. 3. **The floor is removed first.** `session.Close()` and `tools.Close()` run while turns are still executing, so a turn that lives a few more microseconds is talking to a closed gateway and a closed MCP. The drain order is inverted. HTTP turns are fine, incidentally - they root at the request context, so `httpServer.Shutdown` already waits for them. This is Discord-only. ## What I am building - A wait group over in-flight Discord turns, entered in `admitMessage` before the `go a.handleMessage`. - A drain state that refuses new summons once shutdown starts, marked the way an admission denial already is. - Discord turns rooted at an agent-held context so a drain can actually cancel them, rather than at `context.Background()`. - On `ctx.Done()`: stop admitting, wait a bounded grace for turns to finish on their own, cancel what is left, then close the session and MCP - in that order rather than the current one. - The notice you asked for, distinguishable from the existing timeout one, because `> \`turn timed out, retry shortly\`` would be a lie about what happened. ## The one thing I will not choose alone The grace period. Too short and this changes nothing; too long and a rolling deploy stalls on one slow turn. `RequestTimeout` is the natural ceiling and Kubernetes' default `terminationGracePeriodSeconds` is 30, so the drain has to fit inside whatever the deploy manifest sets. I will pick a value that fits the manifest, name it in the pull request, and make it a config field so Ops can move it without a rebuild. Labelling `headless`: the acceptance above is checkable from code and a test, and the grace period is a bounded default rather than a decision.

coilyco-ops added the

autonomy

headless

label

2026-08-13 16:54:35 +00:00

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

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

feat(shutdown): wait for the turns a restart would otherwise cut #612

coilyco-ops commented

2026-08-13 17:09:52 +00:00

Member

Built. #612, gate green.

Correcting my own claim comment first

I wrote that session.Close() and tools.Close() run while turns are still executing because the drain order is inverted. The order is not inverted. Those closes are deferred in Run, so they already fire after the select body returns.

The symptom is real and the mechanism I gave for it was wrong. The closes run while turns are executing because nothing waits for the turns, not because the closes are early. Fixing the wait fixes it, and no reordering was needed.

What shipped

Discord turns now descend from a root the service owns rather than context.Background(), and a counter tracks the ones in flight. A restart:

stops admitting, and marks a summon that arrives now with the refusal reaction and no reply, because the gateway it would answer through is closing
waits out the grace for the turns already running
cancels what is left, naming the restart as the cause
gives those turns a moment to send their notice while the gateway is still open
returns, at which point the deferred closes run

The notice

> `service restarting, retry shortly`

Chosen from context.Cause, not from the error. A cancelled turn sees context.Canceled, which is what a member deleting their message also produces, so the error alone cannot tell them apart. Without the cause it would have read > \model backend unavailable, retry shortly`` - the revert check confirms exactly that, which is a lie about a healthy backend.

The failure metric splits the same way, on shutdown rather than stage_failed, so a rollout does not read as an outage.

The grace period, and what decided it

15 seconds, under SIRENS_ECHO_SHUTDOWN_GRACE.

I said I would not choose this alone and would let the manifest decide. It did: no manifest in the deploy repo sets terminationGracePeriodSeconds, so Kubernetes' 30 second default is the hard ceiling. 15 plus a 3 second notice window leaves headroom under it.

It is deliberately shorter than RequestTimeout, which is 3 minutes. A turn permitted to run that long cannot be waited out by any value that fits a 30 second window, so the grace serves the common turn and cancels the rare one honestly rather than pretending to cover both.

Ops, if you would rather have a longer drain, the lever is terminationGracePeriodSeconds in the manifest first and SIRENS_ECHO_SHUTDOWN_GRACE second. Raising the second without the first buys nothing, because the kill arrives at 30 regardless.

One behaviour change to expect

The HTTP drain was a hardcoded 5 seconds and now shares the grace, so a slow in-flight HTTP turn gets longer than it used to. That is the same fix applied consistently rather than a separate decision, but it is a change and I would rather name it than have it noticed later.

Not covered

A turn that has already sent its reply and is mid-clearTurnMarks can still lose the tidy-up if the grace expires inside it. The member has their answer at that point, so the cost is a stale eyes mark rather than silence. Not worth a second mechanism, recording it so nobody re-discovers it as a defect.

**Built. https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/612, gate green.** ## Correcting my own claim comment first I wrote that `session.Close()` and `tools.Close()` run while turns are still executing **because the drain order is inverted**. The order is not inverted. Those closes are deferred in `Run`, so they already fire after the select body returns. The symptom is real and the mechanism I gave for it was wrong. The closes run while turns are executing because **nothing waits for the turns**, not because the closes are early. Fixing the wait fixes it, and no reordering was needed. ## What shipped Discord turns now descend from a root the service owns rather than `context.Background()`, and a counter tracks the ones in flight. A restart: 1. stops admitting, and marks a summon that arrives now with the refusal reaction and no reply, because the gateway it would answer through is closing 2. waits out the grace for the turns already running 3. cancels what is left, naming the restart as the cause 4. gives those turns a moment to send their notice while the gateway is still open 5. returns, at which point the deferred closes run ## The notice ``` > `service restarting, retry shortly` ``` Chosen from `context.Cause`, not from the error. A cancelled turn sees `context.Canceled`, which is what a member deleting their message also produces, so the error alone cannot tell them apart. Without the cause it would have read `> \`model backend unavailable, retry shortly\`` - the revert check confirms exactly that, which is a lie about a healthy backend. The failure metric splits the same way, on `shutdown` rather than `stage_failed`, so a rollout does not read as an outage. ## The grace period, and what decided it **15 seconds**, under `SIRENS_ECHO_SHUTDOWN_GRACE`. I said I would not choose this alone and would let the manifest decide. It did: **no manifest in the deploy repo sets `terminationGracePeriodSeconds`**, so Kubernetes' 30 second default is the hard ceiling. 15 plus a 3 second notice window leaves headroom under it. It is deliberately shorter than `RequestTimeout`, which is 3 minutes. A turn permitted to run that long cannot be waited out by any value that fits a 30 second window, so the grace serves the common turn and cancels the rare one honestly rather than pretending to cover both. **Ops**, if you would rather have a longer drain, the lever is `terminationGracePeriodSeconds` in the manifest first and `SIRENS_ECHO_SHUTDOWN_GRACE` second. Raising the second without the first buys nothing, because the kill arrives at 30 regardless. ## One behaviour change to expect The HTTP drain was a hardcoded 5 seconds and now shares the grace, so a slow in-flight HTTP turn gets longer than it used to. That is the same fix applied consistently rather than a separate decision, but it is a change and I would rather name it than have it noticed later. ## Not covered A turn that has already sent its reply and is mid-`clearTurnMarks` can still lose the tidy-up if the grace expires inside it. The member has their answer at that point, so the cost is a stale eyes mark rather than silence. Not worth a second mechanism, recording it so nobody re-discovers it as a defect.

coilyco-ops closed this issue

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

coilyco-ops referenced this issue from a commit

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

feat(shutdown): wait for the turns a restart would otherwise cut

No milestone

No project

No assignees

2 participants

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

No description provided.

Rows
Columns