[myzie]

Merge after #94.

What changes for the user

An operator can pause a run, look, fix the cause, and resume it, on every host. The new methods are Store.Pause and Store.Resume on postgres and sqlite, and Run.Pause and Run.Resume on local. A paused run has the status paused. Steps in flight finish, and their results are recorded. No new step or retry starts until Resume.

Issue

No issue. This removes "Pausing and resuming a run by hand" from README's "Not yet" list. The PRD is #94.

See it

ExampleRun_Pause in local/example_test.go. The first charge attempt fails while the run is paused, so its retry waits for Resume:

run, _ := runner.Start(ctx, "order-123", def, nil) // charge: retry max_attempts 2, backoff 10ms
<-started                                           // charge#1 is calling the payment API
_ = run.Pause("payment API down")
close(release)                                      // charge#1 fails while the run is paused
<-held                                              // charge#2 is dispatched, and held
time.Sleep(50 * time.Millisecond)                   // well past the backoff
fmt.Println("while paused:", called)

_ = run.Resume()
st, _ := run.Wait()
fmt.Println("after resume:", called, st.Status)

Output:

while paused: [main/charge#1]
after resume: [main/charge#1 main/charge#2] completed

Why

Today Cancel is the only way to stop a run that goes wrong, and Cancel ends the run for good. An operator who sees bad input, or a downstream API that is down, needs to stop new work on that one run now and continue it later. They must not lose the work the run already did.

Decisions to check

• The host holds the work, and the engine records the status. Apply advances a paused run as it does a running one, and it still emits dispatches. postgres and sqlite skip step jobs of a paused run in Claim, ClaimKey and QueueStats. local waits before it calls the handler. Rejected: hold dispatches in the engine. That misses retries and queued steps that were dispatched before the Pause.
• A timeout that fires during a pause is re-armed at Resume. The timeout does not fail the attempt. Resume emits a ScheduleTimer with a full timeout from the Resume time. Timer jobs are still claimed, so the hosts need no timer change. Rejected: count timeouts from handler start. That changes what timeout means for every run.
• A decided outcome ends the pause. A step that the host started before the Pause can fail for good while the run is paused. Then on_failure and finally start with no Resume. Rejected: hold cleanup until Resume. That needs a paused form of each finishing status.
• Resume's Result.Jobs() lists the re-armed timeout timers, then every running step of the run (held, or claimed before the Pause). A host that pushes one request per job (Cloud Tasks) sends them again, so a held step starts at Resume, not at the queue's next retry. The list comes from Outstanding, with no new query. A job pushed twice is claimed once. Rejected: rely on the queue's retry, whose backoff can be long.
• local keeps the name Run.Resume, next to the existing Runner.Resume. It is the same word on every host. The doc comment of each method names the other.

How to verify

make test
WORKFLOW_LONG=1 go test -count=1 .
cd sqlite && GOWORK=off go test ./...
cd postgres && WORKFLOW_PG_DSN=... GOWORK=off go test ./...
cd internal/parity && GOWORK=off go test ./...

Start reading at apply.go (the EventPause and EventResume cases, rearmTimeouts). Then read notPaused in postgres/backend.go and sqlite/backend.go, and hold and waitHeld in local/local.go. New tests:

• Engine (pause_test.go): pauses change nothing on random graphs; a timeout during a pause, also with two pauses; a sleep during a pause; each outcome while paused.
• Suite (internal/hosttest/pause.go, both backends): PauseHoldsStepsUntilResume, PauseStopsTheTimeoutClock, ResumeListsTheHeldSteps and CancelAPausedRun.
• local (local/pause_test.go): includes a slow Save.
• postgres claim_test.go: the claim keeps its index plan with a paused run.

Risk and rollout

• Upgrade every worker before you use Pause. An older worker does not know that a paused run's jobs are held, and it claims them. There is no migration: the claim filter reads runs.status.
• A short pause does not extend a timeout. A timer that has not fired by the Resume keeps its deadline. A 60s timeout dispatched at t=0 gives 1s after a Resume at 59s, but a full 60s after a Resume at 61s. Also, a handler that hangs during a pause times out later.
• A claim made just before the Pause commits still starts its step. The hosts.md contract says so.
• Each claim reads past held jobs that are due. A few paused runs cost nothing visible. Thousands of held jobs in one queue would slow claims of that queue.
• Code that tests Status == StatusRunning to mean "not finished" now misses paused runs. Terminal() is the documented test. The postgres supersede example now selects status IN ('running', 'paused').

Not in this PR

• Pause one path or one step, a pause step kind, and auto-resume. These are out of scope in the PRD.
• Pause many runs at once. The host app can list runs (run-listing track) and pause each one.

Reviews

• PRD review (prd-reviewer): revise. All findings fixed. The hold moved into the host. See docs(prd): pause and resume a run #94.
• PR docs(prd): pause and resume a run #94 (mobiusops): approved. B1 was fixed by the timeout re-arm. S1, S2, S3, N1 and N2 were fixed. Q1 was answered.
• Implementation review (implementation-reviewer): fix first. Each finding:
  1. local: a handler could start after Pause returned, when Save is slow. Fixed in da5a33b: the hold starts before the reply. A slow-Save test fails without the fix.
  2. No test covered a sleep that ends during a pause. Fixed in 0383236.
  3. The postgres supersede example ignored paused runs. Fixed in db4a558.
  4. The suite did not call the public Pause and Resume wrappers. Fixed in 57fe1ff: the suite now calls them on each backend.
• Also: 537ae2f fixes a timing flake in the new suite tests. A 10ms backoff could end inside the first RunOnce.
• PR review round 1 (mobiusops): S1 was fixed in 3c2814a: Resume lists the held step jobs for push hosts. N1 was fixed in the spec on docs(prd): pause and resume a run #94.
• PR review round 2 (mobiusops): approved. N1 (the spec named d.setHeld) was fixed in 27bee86 on docs(prd): pause and resume a run #94. N2 (the llms.txt line) and N3 (Resume lists every running step, not only held ones) were fixed in fa1f2e0.
• Acceptance: no acceptance tester. The implementation reviewer ran the build and the tests on every module.

PRD: #94

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[mobiusops]

Comment: One item must land before merge: documentation/hosts.md says a held step may start without a poll, and Execute hosts don't get that.

This adds Pause and Resume across the engine, local, postgres and sqlite, as PRD #94 describes. I traced the paused paths (pause, timeout re-arm, a decided outcome, cancel, abort), the claim filters in both backends, and the local hold. I found no wrong behavior. Settling a timer deletes its job in the same transaction as the delivery, so Resume's re-arm can't collide with an old row.

Should fix

S1. Held steps on Execute hosts wait for the queue's next retry (documentation/hosts.md:141)
Line 141 says a held step can start "without waiting for a poll" once the run leaves paused. Line 475 says an Execute host returns ErrNotClaimable for a held job until the queue retries it, and the scheduled RunOnce picks it up later. Resume wakes pull workers through JobsWritten, but Result.Jobs() returns only re-armed timers, so no task is sent again. After a Resume, an operator can wait as long as the queue's retry backoff, and the doc gives no bound. Fix: in hosts.md, say that on Execute hosts a held step starts at the queue's next retry, and that the queue's max backoff sets the delay. Or have Resume return the held step jobs for push hosts. Needed before merge, because hosts build against this doc.

Nits

• N1. planning/specs/009-pause-resume.md:211 names d.setHeld, but the code's helper is d.hold in local/local.go. The StatusRunning grep rule at line 94 is also out of date. The code wins, so a one-line note is enough.

Prompt for your AI tool: fix S1

Fix this review finding in deepnoodle-ai/workflow, PR #104.

1. S1, documentation/hosts.md, the Pause section near line 141 and the
   Execute section near line 475: the text says a held step may start
   "without waiting for a poll", but on Execute hosts a held job's task gets
   ErrNotClaimable until the queue retries it, and the scheduled RunOnce
   picks it up later. Wanted: say that on Execute hosts a held step starts at
   the queue's next retry, and that the queue's max backoff sets the delay.
   Keep the pull-host wording, which is correct: Resume wakes the workers
   through JobsWritten. Test: none; this is a doc change. Check that the two
   sections agree.

Run the existing tests after the changes.

[myzie]

One item must land before merge: documentation/hosts.md says a held step may start without a poll, and Execute hosts don't get that.

• S1 (held steps on Execute hosts wait for the queue's next retry): Fixed in 48bafc0, in code. On a Resume, Result.Jobs() lists the re-armed timeout timers and then the run's held step jobs, so a push host sends them again at once. The list comes from Outstanding on the resumed state, so there is no new query. Those commands are not persisted again: the jobs are in the outbox already, and a job pushed twice is claimed once. The new suite test ResumeListsTheHeldSteps pauses a run, gets ErrNotClaimable from Execute, resumes, and runs the listed job with Execute. hosts.md (Cancel, pause and Abort; postgres; the jobs table), llms.txt and the Resume doc comments now agree. The PRD decision is revised in fda6e0a on docs(prd): pause and resume a run #94.
• N1 (d.setHeld and the grep rule in the spec): Fixed in fda6e0a on docs(prd): pause and resume a run #94. The spec now notes the helper as built (d.hold, which starts before the reply to Pause) and the progressing() grep rule.

The branch is rebased on the PRD branch, so the earlier commit hashes in the description changed. The description is updated.

[mobiusops]

Approve: Updated for 48bafc0. S1 is fixed, and the new commit has no Blocking or Should-fix items.

The branch was force-pushed, so the old head 249060… is gone. Its commit message now sits at f2e97f5, and I took that as the prior head. The new commit makes Resume return the held step jobs, so a host that pushes one request per job sends them again at Resume.

Fixed: S1

Nits

• N1. The spec still names d.setHeld at line 223, but the code uses d.hold. The note at line 220 says so, so this is only a stale name. (planning/specs/009-pause-resume.md:223)
• N2. The llms.txt line reads "Resume's lists the timers…" and is missing Result.Jobs(). (llms.txt:129)
• N3. Outstanding returns every running step unit, including steps a worker is still running, not just held ones. Pushing a running step is harmless because the second task finds it leased. The docs still call these "held step jobs." (documentation/hosts.md:479)

[myzie]

Updated for 48bafc0. S1 is fixed, and the new commit has no Blocking or Should-fix items.

• N1 (the spec named d.setHeld): Fixed in cc4a8d6 on docs(prd): pause and resume a run #94. The spec now says loop calls d.hold(true).
• N2 (the llms.txt line was missing Result.Jobs()): Fixed in 20ef13b. It now reads "Resume's Result.Jobs() lists the timers it re-armed, then every running step of the run".
• N3 (Outstanding lists running steps too, not only held ones): Fixed in 20ef13b. hosts.md (the pause contract, the postgres section, the jobs table), llms.txt, the Resume doc comments and the DeliverLocked comment now say that Resume lists every running step unit: the held ones, and any a worker is already running because its claim was made before the Pause. A push of a running one finds it leased, which is harmless. The spec says the same (cc4a8d6 on docs(prd): pause and resume a run #94).

The branch was rebased on the PRD branch again, so the earlier commit hashes changed. The description lists the new ones; the S1 commit is now 49079d6.

[mobiusops]

Approve: Updated for 20ef13b. Resume now lists every running step, and a push of one that a worker still holds gets ErrNotClaimable, so it does not run twice.

Fixed: N1, N2, N3

Should fix

S2. The running-step case has no test (internal/hosttest/pause.go:177)
ResumeListsTheHeldSteps covers only a step that was held. Nothing tests a step a worker claimed before the Pause, though the docs now rely on it: Resume lists it, and a push of it is refused while its lease is live. The code does that (postgres/backend.go claimKeySQL checks the lease, and internal/hostcore/once.go:262 returns ErrNotClaimable), so this is a test gap, not a bug. Add a case that pauses while a handler runs, then checks that Resume lists the step and Execute returns ErrNotClaimable until the handler settles. This can land after merge.

Nits

• N4. The PR body still says Resume lists "the run's held step jobs". The code now lists every running step, so update the body. (PR description)
• N5. documentation/hosts.md:482 calls a push of a running step "harmless". It is harmless, but the push gets a 5xx, so the queue retries it. Say that. (documentation/hosts.md:482)

Prompt for your AI tool: fix S2

Fix this review finding in deepnoodle-ai/workflow, PR #104.

1. S2, internal/hosttest/pause.go, func testResumeListsTheHeldSteps:
   The test covers only a step that was held during the pause. Wanted: add
   a case where a worker claims a step before Pause and the handler is still
   running. Pause, then Resume. Test: Resume's Result.Jobs() lists that
   step; Execute on its JobRef returns ErrNotClaimable while the handler
   runs; after the handler settles, the run completes.

[mobiusops]

Approve: Updated for 5b2233e. The one new commit changes documentation/hosts.md only, and the new Cancel and decided-outcome text matches the code.

Fixed: N4 (the PR body now says Resume lists "held, or claimed before the Pause").

I checked the new text against abort.go, apply.go (failRun and cancelAll for a decided failure, EventCancel for a cancel), and internal/hostcore/store.go. CancelStep deletes the held job, so a held step never starts after a Cancel or a failure. Only on_failure and finally steps start while the run is paused, because the run status moves to failing or finishing.

Still open

• S2. ResumeListsTheHeldSteps covers only held jobs, not a step claimed before the Pause. Fine as a follow-up.
• N5. hosts.md calls a push of a running step "harmless." The task gets ErrNotClaimable, which is a 5xx for a task queue, and it retries. The wording should say so.

Prompt for your AI tool: fix S2

Fix this review finding in deepnoodle-ai/workflow, PR #104.

1. S2, internal/hosttest/pause.go, the ResumeListsTheHeldSteps suite test:
   it covers only a step held by the Pause. Wanted: add a case where a step
   is claimed before the Pause and its handler is still running. Resume's
   Result.Jobs() must list that step's unit, and a push of its job must
   return ErrNotClaimable while the job is leased. After the step settles,
   the job must be gone. Test: extend the suite test so both postgres and
   sqlite run it.

[mobiusops]

Reviewed at a92f5f3. The branch was rewritten since the last review (5b2233e is no longer in the clone), so I read the full PR diff against base 46d72fd, not just the new commits.

Blocking: none.

What I checked

• apply.go: a timeout that fires during a pause is held as zero and re-armed in rearmTimeouts at Resume. A duplicate held timer event is dropped early. dropSettledTimeouts clears entries for attempts that settled.
• outstanding.go and internal/hostcore/store.go: on a Resume, Result.Jobs() lists the re-armed timers, then every running step. Result.Commands is limited to the DispatchSteps, and they are added after persist, so nothing is written to the outbox twice.
• local/local.go: Resume emits no DispatchStep for running steps, so held steps wait on the held channel and a step is not called twice.
• documentation/hosts.md: the held-step, cancel and outcome rules match abort.go and apply.go. A claim made before the Pause runs on, and a Cancel or a decided outcome ends the pause with CancelStep for held attempts.

Nonblocking, still open

• N5: hosts.md says a push of a running step is "harmless". Its task gets ErrNotClaimable and answers 5xx, so the queue retries it. Nothing runs twice, but "harmless" overstates it. Suggest "a 5xx and a retry, with no second run".
• S2: ResumeListsTheHeldSteps covers only a held step. No test covers a step claimed before the Pause that Resume lists. Worth a follow-up test.

No tests, builds or linters were run. This is a review of the code and docs as written.

[mobiusops]

Base moved from docs/pause-resume-prd to main (now 45526da, which contains #94). The head is still a92f5f3, so the approved code review stands. The Go code in the PR is the same 30-file set as before.

The diff against main is larger (33 files, +2117). The extra files are planning docs that are not about pause/resume: planning/prds/011-run-listing.md, 012-decision-guard.md, 013-decision-edges.md, planning/specs/011-run-listing.md, 013-decision-edges.md, and planning/README.md.

Q1. Are those planning files meant to land with this PR? If not, they should be removed from the branch before merge. I did not review their content.

[myzie]

Merged main into this branch (wait step #100 and decision edges #103 landed first). Two things to check:

• Merge: conflicts in docs, internal/hosttest/list.go, local/local.go (Resolve from feat: wait for a delivery with a built-in wait step #100, then the pause hold) and outstanding.go (a wait has no dispatch; a held timeout is skipped or re-armed). PRD 009 and planning/README.md take main's copies: docs(prd): pause and resume a run #94 had three later commits.
• Fix in the follow-up commit: a paused run holds its decision jobs, but a decision's one-minute timeout still fired and used up an attempt. A pause of more than about three minutes then failed the decision's step. The pause now holds that timeout like a step's: Apply records it, Resume re-arms it, and Outstanding leaves it out until then. TestPauseStopsADecisionsTimeoutClock covers it.

🤖 Generated with Claude Code

[mobiusops]

Approve: Updated for d513115. The new commit holds a decision's one-minute timeout during a pause, as a step's timeout is held, and the change is sound.

I reviewed only d513115 (apply.go, outstanding.go, pause_test.go, hosts.md). The merge 13d5e16 brings in main's #97, #100 and #103 changes, which are outside this PR's diff.

Still open

• S2. No test covers Resume listing a step claimed before Pause. (fine as a follow-up)
• N5. documentation/hosts.md:669 calls a push of a running step "harmless". Execute returns ErrNotClaimable for a leased job, and the docs say errors get a 5xx and a retry. (fine as a follow-up)

New

• N6. The new sentence in documentation/hosts.md:185 makes that paragraph's line 185 about 132 characters. Rewrap the paragraph like its neighbours.
• N7. The Outstanding doc comment at outstanding.go:9-11 has a line about 100 characters long. Rewrap it.

Prompt for your AI tool: fix S2

Fix this review finding in deepnoodle-ai/workflow, PR #104.

1. S2, pause_test.go and internal/hosttest/pause.go: no test covers
   Resume listing a step that was claimed before Pause. Wanted: a test
   where a step is claimed, then Pause, then Resume, and Result.Jobs()
   includes that step's running job. Add it to the hosttest suite so
   both backends run it.

Run the existing tests after the changes.

[mobiusops]

Checked the merge of main (13d5e16) and the decision-timeout fix (d513115). Head is still d513115, the commit I approved. Nothing new was pushed since.

Merge conflicts, as you asked:

• outstanding.go: the wait case is right. Outstanding skips the DispatchStep for KindWait and keeps the timeout timer. The decision block emits its dispatch, and its timer only when the timeout is not held, or when Resume has re-armed it.
• local/local.go: Resolve runs before Apply, and the pause hold still runs before the reply. A slow Save cannot let a handler start after Pause returns.
• internal/hosttest/list.go: both lists keep their entries. The wait suite test sits after the pause tests, which is fine.

Not covered: main's #97, #100 and #103 changes outside these conflicted files. I did not review them as part of this PR. I did not run tests, builds or linters.

Still open, none blocking: S2 (a test for Resume listing a step claimed before Pause), N5 ("harmless" wording at documentation/hosts.md:669), and N6/N7 (long lines in hosts.md and the Outstanding doc comment).

The approval at d513115 stands.

[mobiusops]

Approve: Updated for 1d2413f. The only new commit rewraps the pause-timeout paragraph in documentation/hosts.md. The wording is unchanged, so the approval at d513115 still holds.

Fixed: N6 (the long line at hosts.md:185 is now wrapped).

Still open, not blocking:

• S2. No test covers Resume listing a step claimed before the Pause. (fine as a follow-up)
• N5. "harmless" wording at documentation/hosts.md:670. (nit)
• N7. outstanding.go:32 is 83 characters. (nit)