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

Merged

coilysiren merged 1 commit from aos/claude/hs68-queued-jobs-dropped into main

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

coilyco-ops commented

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

Member

#824 asked three questions about RecordEffect and EffectApplied. Answering them found that the issue's stated live consequence is stale in both halves, and that a different, real problem sits underneath it.

The premise, re-checked

Nothing in jobrunner.go records an effect. So job.Effects is empty for every job that has ever run

No longer true, and it was already fixed when the issue was filed. 11d5fe2 (2026-08-13) wired both functions into the content path. jobcontent.go:132 reads through EffectApplied and :149 writes through RecordEffect after every successful EmitJobContent. unwiredcapabilities_test.go dropped both rows in the same change. #824 was filed 2026-08-15.

a resumed job redoes every step it already completed

This cannot happen, because nothing resumes a job. Every path, checked:

queued at restart - left in queued. Start builds an empty channel, enqueue has exactly one caller (Submit), and JobStore has no query by state. Never picked up.
running or cancelling at restart - RecoverStrandedJobs, called from Agent.recoverJobs, moves them to failed or cancelled with interrupted by a restart. Settled, never re-run.
a redelivered submission - Submit dedupes on the idempotency key, returns the existing job, and queues nothing.

No job is executed twice, so the write half runs on every delivery and the read half has never returned true. The mechanism is correct, cheap, and dormant.

Answering the three questions

1. Is redo-on-resume harmful today? The question is moot rather than answered: there is no resume. And the honest version of the concern runs the other way - an accepted, durable, not-yet-started job is dropped silently on restart, with no notice to its requester. That is filed as #878.

2. The commit that introduced them. 1f8825f, 2026-08-12, the original jobs feature, alongside Attempts and the restart section of the doc. It shipped Start with this comment:

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

The body never did. That false comment is why this looked like an unwired mechanism rather than a missing one - it describes a resume path that would make Effects load-bearing, and reading the comment instead of the body gives you exactly #824's mental model.

3. Does Temporal change the answer? #430 is open and jobs were explicitly out of scope for August 19, so the mechanism stays for now. It does bear on #878: if jobs move onto Temporal, event history is the durable record and both the requeue and the effects guard are replaced rather than built. That is the reason #878 asks for a decision before code.

What this PR changes

The false comment on Start now says what the function does.
TestARestartDropsWhatWasQueued pins the observed behaviour, so the claim cannot drift back in as prose.
docs/sirens-echo-jobs.md says which mid-flight states recovery covers and which it does not, points at #878 for the gap, and corrects the effects paragraph: Effects is written per delivered message, and nothing reads a non-empty result yet because no path runs a job twice.

No behaviour change. Fixing the drop needs a store query by state across three implementations and a requeue-or-settle decision, which is #878's to make.

just gate passes.

closes #824

#824 asked three questions about `RecordEffect` and `EffectApplied`. Answering them found that the issue's stated live consequence is stale in both halves, and that a different, real problem sits underneath it. ## The premise, re-checked > **Nothing in `jobrunner.go` records an effect.** So `job.Effects` is empty for every job that has ever run **No longer true, and it was already fixed when the issue was filed.** `11d5fe2` (2026-08-13) wired both functions into the content path. `jobcontent.go:132` reads through `EffectApplied` and `:149` writes through `RecordEffect` after every successful `EmitJobContent`. `unwiredcapabilities_test.go` dropped both rows in the same change. #824 was filed 2026-08-15. > **a resumed job redoes every step it already completed** **This cannot happen, because nothing resumes a job.** Every path, checked: - `queued` at restart - left in `queued`. `Start` builds an empty channel, `enqueue` has exactly one caller (`Submit`), and `JobStore` has no query by state. Never picked up. - `running` or `cancelling` at restart - `RecoverStrandedJobs`, called from `Agent.recoverJobs`, moves them to `failed` or `cancelled` with `interrupted by a restart`. Settled, never re-run. - a redelivered submission - `Submit` dedupes on the idempotency key, returns the existing job, and queues nothing. No job is executed twice, so the write half runs on every delivery and the read half has never returned true. The mechanism is correct, cheap, and dormant. ## Answering the three questions **1. Is redo-on-resume harmful today?** The question is moot rather than answered: there is no resume. And the honest version of the concern runs the other way - an accepted, durable, not-yet-started job is **dropped silently on restart**, with no notice to its requester. That is filed as #878. **2. The commit that introduced them.** `1f8825f`, 2026-08-12, the original jobs feature, alongside `Attempts` and the restart section of the doc. It shipped `Start` with this comment: > Start brings up the workers and requeues anything a restart left queued. The body never did. **That false comment is why this looked like an unwired mechanism rather than a missing one** - it describes a resume path that would make `Effects` load-bearing, and reading the comment instead of the body gives you exactly #824's mental model. **3. Does Temporal change the answer?** #430 is open and jobs were explicitly out of scope for August 19, so the mechanism stays for now. It does bear on #878: if jobs move onto Temporal, event history is the durable record and both the requeue and the effects guard are replaced rather than built. That is the reason #878 asks for a decision before code. ## What this PR changes * **The false comment on `Start`** now says what the function does. * **`TestARestartDropsWhatWasQueued`** pins the observed behaviour, so the claim cannot drift back in as prose. * **`docs/sirens-echo-jobs.md`** says which mid-flight states recovery covers and which it does not, points at #878 for the gap, and corrects the effects paragraph: `Effects` is written per delivered message, and nothing reads a non-empty result yet because no path runs a job twice. **No behaviour change.** Fixing the drop needs a store query by state across three implementations and a requeue-or-settle decision, which is #878's to make. `just gate` passes. closes #824

coilyco-ops added 1 commit

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

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

ci / test (pull_request) Successful in 56s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

ci / image-build (pull_request) Successful in 20s

Details

2c2766e855

`Start`'s doc comment has claimed since 1f8825f that it "requeues anything a
restart left queued". It never has. It builds an empty channel and starts
workers on it, `enqueue` is called only by `Submit`, and `JobStore` exposes no
query by state, so nothing can find those jobs even in principle.

Found answering sirens-echo#824, which asked whether redo-on-resume is harmful.
It is not, because there is no resume. The exposure runs the other way: an
accepted, durable, not-yet-started job is dropped silently.

The comment is corrected, `TestARestartDropsWhatWasQueued` pins the behaviour
so the claim cannot drift back in, and the doc says which of the three
mid-flight states recovery covers and which it does not.

The behaviour is deliberately unchanged. Fixing it needs a store query by state
across three implementations and a requeue-or-settle decision, and it may be
moot if jobs move onto Temporal. Filed as sirens-echo#878.

The effects note is corrected in the same pass: `Effects` is not empty, because
the content path writes one per delivered message as of 11d5fe2. Nothing reads
a non-empty result yet, because no path executes a job twice.

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 requested review from coilysiren

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