coilyco-gaming/sirens-echo

Watch

A queued job is dropped without notice on restart, because nothing requeues it and the store cannot list by state #878

New issue

Closed

opened 2026-08-17 02:37:57 +00:00 by coilyco-ops · 3 comments

coilyco-ops commented

2026-08-17 02:37:57 +00:00

Member

Found while answering #824, which asked whether redo-on-resume is harmful. It is not, because there is no resume. The exposure is the opposite one, and nothing had recorded it.

What happens

A job that is accepted, durable, and queued when the process stops is never picked up again. Not on the next start, not ever, and its requester is told nothing.

Three facts, each checkable in one grep:

JobRunner.Start builds r.queue as a fresh empty channel and starts workers on it. It reads nothing from the store.
enqueue has exactly one caller in the tree, JobRunner.Submit. Nothing else can put an id on the queue.
JobStore exposes Get, ListByPrincipal, and ListByThread. There is no query by state, so nothing could find the queued jobs even if a caller wanted to.

RecoverStrandedJobs covers the other two mid-flight states and is called from Agent.recoverJobs, so running and cancelling reach a terminal state with interrupted by a restart. queued is excluded by an explicit state check, and docs/sirens-echo-jobs.md described that as recovery correctly leaving an accurate record alone. That reading is right about the record and silent about the fact that nothing ever acts on it again.

Why it was invisible

Start's own doc comment claimed the opposite since the feature landed in 1f8825f:

Start brings up the workers and requeues anything a restart left queued.

The body never did. #824's PR corrects that comment, adds TestARestartDropsWhatWasQueued pinning the observed behaviour, and says so in the doc. This issue is the behaviour, which that PR deliberately does not change.

Why it is not obviously urgent

Agent.recoverJobs only engages a store implementing All() []Job, so the deployment's durability choice decides how much survives to be dropped.
The window is narrow: Submit enqueues immediately after the store write, so a job is queued for as long as it takes a worker to accept it, plus any time spent queue-full.
A restart during that window is a real event, and it lands during a deploy, which is when a member is most likely to have just asked for something.

What a fix needs

A decision before code, which is why this is filed rather than fixed:

Does JobStore grow a query by state? All three implementations would have to answer it, including PostgresJobStore. That is the mechanical cost.
Requeue, or settle? Re-enqueueing at boot resumes the work, and it is exactly the path that makes RecordEffect and EffectApplied load-bearing rather than dormant. Settling to failed with dropped by a restart is much cheaper and at least tells the requester.
Does #430 make this moot? If jobs move onto Temporal, its event history is the durable record and this is replaced rather than built. #430 is open and jobs were explicitly out of scope for August 19, so the question is when rather than whether.

Option 2 is the small honest one and does not block option 1 later. Option 1 is the one that gives the effects mechanism a reason to exist.

Related - #824 (where this was found), #430 (Temporal), #621 (the unwired-capability sweep).

Found while answering #824, which asked whether redo-on-resume is harmful. It is not, because there is no resume. The exposure is the opposite one, and nothing had recorded it. ## What happens A job that is accepted, durable, and `queued` when the process stops is never picked up again. Not on the next start, not ever, and its requester is told nothing. Three facts, each checkable in one grep: * `JobRunner.Start` builds `r.queue` as a fresh empty channel and starts workers on it. It reads nothing from the store. * `enqueue` has exactly one caller in the tree, `JobRunner.Submit`. Nothing else can put an id on the queue. * `JobStore` exposes `Get`, `ListByPrincipal`, and `ListByThread`. **There is no query by state**, so nothing could find the queued jobs even if a caller wanted to. `RecoverStrandedJobs` covers the other two mid-flight states and is called from `Agent.recoverJobs`, so `running` and `cancelling` reach a terminal state with `interrupted by a restart`. `queued` is excluded by an explicit state check, and `docs/sirens-echo-jobs.md` described that as recovery correctly leaving an accurate record alone. That reading is right about the record and silent about the fact that nothing ever acts on it again. ## Why it was invisible `Start`'s own doc comment claimed the opposite since the feature landed in `1f8825f`: > Start brings up the workers and requeues anything a restart left queued. The body never did. #824's PR corrects that comment, adds `TestARestartDropsWhatWasQueued` pinning the observed behaviour, and says so in the doc. **This issue is the behaviour, which that PR deliberately does not change.** ## Why it is not obviously urgent * `Agent.recoverJobs` only engages a store implementing `All() []Job`, so the deployment's durability choice decides how much survives to be dropped. * The window is narrow: `Submit` enqueues immediately after the store write, so a job is `queued` for as long as it takes a worker to accept it, plus any time spent queue-full. * A restart during that window is a real event, and it lands during a deploy, which is when a member is most likely to have just asked for something. ## What a fix needs A decision before code, which is why this is filed rather than fixed: 1. **Does `JobStore` grow a query by state?** All three implementations would have to answer it, including `PostgresJobStore`. That is the mechanical cost. 2. **Requeue, or settle?** Re-enqueueing at boot resumes the work, and it is exactly the path that makes `RecordEffect` and `EffectApplied` load-bearing rather than dormant. Settling to `failed` with `dropped by a restart` is much cheaper and at least tells the requester. 3. **Does #430 make this moot?** If jobs move onto Temporal, its event history is the durable record and this is replaced rather than built. #430 is open and jobs were explicitly out of scope for August 19, so the question is when rather than whether. Option 2 is the small honest one and does not block option 1 later. Option 1 is the one that gives the effects mechanism a reason to exist. **Related** - #824 (where this was found), #430 (Temporal), #621 (the unwired-capability sweep).

coilyco-ops referenced this issue

2026-08-17 02:38:49 +00:00

fix(jobs): say that a restart drops what was queued, because it does #879

coilyco-ops added the

labels

2026-08-17 16:33:20 +00:00

coilyco-ops commented

2026-08-17 17:34:26 +00:00

Author

Member

Claiming this. Angie (ENG, claude seat), working the headless engineer queue.

Taking option 2 from the issue, plus the notification half, and deliberately not option 1.

No JobStore query by state. Both durable stores already expose All() []Job, which is what Agent.recoverJobs asserts for today. Adding a fourth interface method that PostgresJobStore would have to answer buys nothing the existing assertion does not already give, and option 1 stays open behind it exactly as the issue says.
Settle rather than requeue. queued settles to failed with dropped by a restart, so the record stops claiming work that will never be picked up.
The requester gets told. Settling alone only fixes the record, and a Discord requester never reads one. Restart recovery now notifies, and that covers the running and cancelling jobs RecoverStrandedJobs already settled silently. Those were the same defect and only half of it was filed.

TestARestartDropsWhatWasQueued from #824 stays and stays passing: it pins Start requeueing nothing, and the change lives in recoverJobs rather than in Start.

Claiming this. Angie (ENG, `claude` seat), working the headless engineer queue. Taking **option 2** from the issue, plus the notification half, and deliberately not option 1. * **No `JobStore` query by state.** Both durable stores already expose `All() []Job`, which is what `Agent.recoverJobs` asserts for today. Adding a fourth interface method that `PostgresJobStore` would have to answer buys nothing the existing assertion does not already give, and option 1 stays open behind it exactly as the issue says. * **Settle rather than requeue.** `queued` settles to `failed` with `dropped by a restart`, so the record stops claiming work that will never be picked up. * **The requester gets told.** Settling alone only fixes the record, and a Discord requester never reads one. Restart recovery now notifies, and that covers the `running` and `cancelling` jobs `RecoverStrandedJobs` already settled silently. Those were the same defect and only half of it was filed. `TestARestartDropsWhatWasQueued` from #824 stays and stays passing: it pins `Start` requeueing nothing, and the change lives in `recoverJobs` rather than in `Start`.

coilyco-ops referenced this issue from a commit

2026-08-17 17:39:43 +00:00

fix: settle and announce the jobs a restart drops

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

2026-08-17 17:39:56 +00:00

fix: settle and announce the jobs a restart drops #911

coilyco-ops commented

2026-08-17 17:40:12 +00:00

Author

Member

Built and in review at #911. Angie (ENG, claude seat). Releasing the claim.

just gate PASS. Mutation checked: removing the dropped pass fails TestARestartSettlesAndAnnouncesWhatWasQueued rather than passing quietly.

One thing the issue did not ask for and the PR carries anyway. Settling only corrects the record, and a Discord requester never reads one, so recovery now announces every job it settles. That covers the running and cancelling jobs RecoverStrandedJobs has been settling silently since it landed. Same defect one state over, and leaving half the requesters uninformed to keep the diff narrow was the worse trade.

Question 3 from the issue is untouched. If #430 moves jobs onto Temporal this is replaced rather than extended, and settling does not make that harder.

Built and in review at https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/911. Angie (ENG, `claude` seat). Releasing the claim. `just gate` PASS. Mutation checked: removing the dropped pass fails `TestARestartSettlesAndAnnouncesWhatWasQueued` rather than passing quietly. One thing the issue did not ask for and the PR carries anyway. Settling only corrects the record, and a Discord requester never reads one, so recovery now announces every job it settles. That covers the `running` and `cancelling` jobs `RecoverStrandedJobs` has been settling silently since it landed. Same defect one state over, and leaving half the requesters uninformed to keep the diff narrow was the worse trade. Question 3 from the issue is untouched. If #430 moves jobs onto Temporal this is replaced rather than extended, and settling does not make that harder.

coilysiren closed this issue

2026-08-17 17:53:33 +00:00

coilyco-ops commented

2026-08-17 18:08:58 +00:00

Author

Member

Correction: the store is Postgres, not memory. This is worse, and it stays `priority/P0`.

Darren (director seat), 2026-08-17. Kai confirmed the tier.

I tiered this priority/P0 believing queued jobs evaporated with the process. They do not. sirens-echo-values.yaml wires SIRENS_ECHO_JOB_STORE_DSN to the sirens-echo-job-store secret, Postgres-backed since deploy#464, and the values file notes the harness refuses to fall back to memory if the database is unreachable.

That makes the failure harder to defend rather than easier. The three facts in the body are unchanged: JobRunner.Start builds a fresh empty channel and reads nothing from the store, enqueue has exactly one caller in Submit, and JobStore exposes no query by state. What changes is what survives. The row is still there. state = 'queued', requester attributed, idempotency key intact, sitting in a database that outlived the pod that abandoned it.

So the system holds durable, queryable proof that it accepted work it will never do, and tells the requester nothing. Losing a job to a memory wipe is a gap. Keeping the receipt and never acting on it is a broken promise with an audit trail.

What does not change

The fix shape is the same. Requeue queued rows at Start, and add the query by state that JobStore lacks. RecoverStrandedJobs already does the equivalent for running and cancelling.

What I raised and Kai did not take

I offered a variant that adds a one-off backfill pass over already-stranded rows, so people whose jobs were dropped before the fix either get them run or get told. Not chosen. Recording it because the rows will still be sitting there when this lands, and someone will have to decide what to do with them at that point rather than now.

Found during a capability audit mapping Go stdlib surface areas against what Echo can actually reach.

## Correction: the store is Postgres, not memory. This is worse, and it stays `priority/P0`. Darren (director seat), 2026-08-17. Kai confirmed the tier. I tiered this `priority/P0` believing queued jobs evaporated with the process. They do not. `sirens-echo-values.yaml` wires `SIRENS_ECHO_JOB_STORE_DSN` to the `sirens-echo-job-store` secret, Postgres-backed since deploy#464, and the values file notes the harness refuses to fall back to memory if the database is unreachable. **That makes the failure harder to defend rather than easier.** The three facts in the body are unchanged: `JobRunner.Start` builds a fresh empty channel and reads nothing from the store, `enqueue` has exactly one caller in `Submit`, and `JobStore` exposes no query by state. What changes is what survives. The row is still there. `state = 'queued'`, requester attributed, idempotency key intact, sitting in a database that outlived the pod that abandoned it. So the system holds durable, queryable proof that it accepted work it will never do, and tells the requester nothing. Losing a job to a memory wipe is a gap. Keeping the receipt and never acting on it is a broken promise with an audit trail. ### What does not change The fix shape is the same. Requeue `queued` rows at `Start`, and add the query by state that `JobStore` lacks. `RecoverStrandedJobs` already does the equivalent for `running` and `cancelling`. ### What I raised and Kai did not take I offered a variant that adds a one-off backfill pass over already-stranded rows, so people whose jobs were dropped before the fix either get them run or get told. Not chosen. Recording it because the rows will still be sitting there when this lands, and someone will have to decide what to do with them at that point rather than now. Found during a capability audit mapping Go stdlib surface areas against what Echo can actually reach.

coilyco-ops referenced this issue

2026-08-17 18:21:15 +00:00

ENG is out of actionable work: what unblocks it, by owner #751

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

No description provided.

Rows
Columns

A queued job is dropped without notice on restart, because nothing requeues it and the store cannot list by state #878

What happens

Why it was invisible

Why it is not obviously urgent

What a fix needs

Correction: the store is Postgres, not memory. This is worse, and it stays priority/P0.

What does not change

What I raised and Kai did not take

Correction: the store is Postgres, not memory. This is worse, and it stays `priority/P0`.