feat(jobs): reconcile job runs whose client never recorded an outcome - #181
feat(jobs): reconcile job runs whose client never recorded an outcome#181vishr wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new orphan-closing logic can mis-record terminal outcomes (e.g., marking proven failures as “interrupted”) and can append finish records with an inconsistent epoch, which should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds host-side reconciliation to prevent sealed manual job runs from remaining permanently INCOMPLETE when the initiating client disappears, and to block new mutations when a still-running orphan container exists.
Changes:
- Reconcile orphaned
job_runjournals under the application lock at the start ofob job runandob deploy. - Introduce
internal/engine/job_reconcile.goto detect orphaned job runs, check for still-running labeled containers, and append a terminal record when the container is gone. - Add focused tests covering “refuse if container still running”, “close as interrupted if outcome unknown”, and “close as ok if success was journaled”.
File summaries
| File | Description |
|---|---|
| internal/engine/job.go | Runs orphan reconciliation under the lock before any job-run mutation. |
| internal/engine/job_reconcile.go | Implements orphan detection, container lookup, and journal closure/refusal logic. |
| internal/engine/job_reconcile_test.go | Adds unit tests for reconcile refusal/closure behavior. |
| internal/engine/deploy.go | Runs orphan reconciliation under the lock before any deploy mutation. |
Review details
Suppressed comments (2)
internal/engine/job_reconcile.go:108
- closeOrphanedJobRun creates a journal.Writer without setting Epoch, so even if the orphan capture includes it, the appended finish line will still be written with epoch 0. Set Writer.Epoch to the orphan's epoch so the terminal record matches the rest of that operation's journal.
writer := &journal.Writer{
T: e.T, Names: e.names(), DeployID: orphan.OperationID,
Operator: journal.DefaultOperator(),
}
internal/engine/job_reconcile.go:51
- Capture the lock epoch from the job start record so the subsequent terminal append can reuse it (instead of writing epoch 0).
started, orphan.Job = true, r.Service
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d827b9f to
6306946
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes deploy/job safety behavior and introduces reconciliation logic that can affect operational correctness under failure modes, warranting final human review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
6306946 to
8a59a37
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The docker ps output parsing can skip containers with an empty ob.operation value due to TrimSpace, undermining the safety refusal for unattributable live job containers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
8a59a37 to
57de9b9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
RunJobWithJournalID can reconcile against a snapshot that already includes the current invocation’s start record, allowing the new reconciliation step to prematurely close the job run before it executes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
898e79f to
42b3a6c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core deploy/job safety behavior based on Docker state and journal reconciliation, which has high operational impact and warrants final human validation despite tests.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
| // Under the lock, before anything mutates and before any host write: a job | ||
| // container from an earlier operation may still be running with no process | ||
| // owning it. Read-only, so it is safe on this side of the plan boundary. |
| return fmt.Errorf( | ||
| "a job container from operation %s is still running on this host (%.12s) with no process owning it; "+ | ||
| "wait for it to finish, or establish what it did and stop it with `docker rm -f %.12s`", | ||
| c.operation, c.id, c.id) |
A sealed job run that lost its client left a journal with a start and no terminal record. Nothing ever went back for it: `ob resume` and `ob abort` skip non-deploy journals, `ob status` reports the application in sync, and only `ob audit` showed the operation — as INCOMPLETE, permanently. Meanwhile the container it started could still be changing data, and once the lock's TTL expired the next operation took ownership on the strength of a dead heartbeat. Both mutating entry points now reconcile under the lock, before anything mutates. A still-running container refuses the operation outright and names it: the host is executing a data-changing job that no process owns, and the TTL says nothing about whether that is finished. A container that is gone gets the terminal record its client could not write. What it does not do is guess. `--rm` means nothing about the container survives its exit, so success is only recorded where the client itself journaled the job's result — the one window where the outcome was observed before the client died. Everything else is recorded interrupted, which keeps the rollback debt an unresolved data-changing job carries rather than erasing it with a fabricated success. Deploy journals and completed runs are not orphans, and cost no container lookup. Refs #179. Execution is still attached; a client killed outright still records nothing at the time, but is now closed honestly by the next operation instead of staying incomplete forever. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
…ld not Reconciliation derived "is a job still running" from a journal reduction. A journal says what a client managed to write, and the failure this exists for is a client that did not write — so the reduction was wrong in three ways at once. An interrupted run that DID record its interruption looked finished on paper while its container kept changing data, which is the Ctrl-C case the previous commit made more common. A plan re-run appends a second invocation to the same journal, and a finish in an earlier epoch was read as closing a later one. And a deploy-phase gate container carries the same labels but never appears in a job_run journal at all. All three are one `docker ps` away, so the refusal now asks Docker: any running one-off container whose operation is not this one stops the operation. The journal reduction stays only for the closing half, where being wrong is cheap. The closing half was also not doing what it claimed. It appended without an epoch, so the record landed in an invocation of its own and left the one it meant to close still open — `ob audit` went on reporting INCOMPLETE and grew a phantom row. It now groups a journal by invocation and writes into the one it closes. It also read the result record by an operation kind that record does not carry, so the branch that recognises a proven outcome could never fire and every reconciled run was recorded interrupted. Matched on its own shape now. A recorded failure is evidence exactly as much as a recorded success: only an absent result is an unknown outcome. The refusal is read-only and runs before the plan boundary; the append runs after it, so a plan that will not execute writes nothing. Refs #179. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
…utable container A deploy scanned every journal twice — once to close interrupted job runs and again for rollback debt — which on a high-latency host is the same bytes fetched and parsed for a second time. One read now serves both. A container carrying the operation label with no value cannot be attributed to an operation, and the refusal printed a blank id for it. It is refused on its own terms now: an unattributable job container is exactly as dangerous as an attributable one, and the message says which problem the operator has. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
Trimming the docker ps line before splitting it removed the separator on a container whose label carries no value, so the line parsed as unsplittable and the container was skipped — the refusal added for exactly that case could never fire. Split first, trim the fields after. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
The snapshot was taken after the start record was appended, so it contained a start with no finish yet — this very run — and the reconciler closed the job it was about to execute, writing a spurious interrupted record ahead of the real one. It now runs between the staleness checks and the start append: late enough that a plan which will not execute writes nothing, early enough that this run is not yet in the journal it is reading. The regression test drives the whole run against a journal listing that reflects what the run has appended so far, so the snapshot's contents depend on when it is taken — without that the fake returned nothing and the ordering could not be told apart. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
Now that a later operation closes a run the client could not, the registry comment can say so. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
42b3a6c to
a2668ef
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The refusal logic currently exempts containers that share the current operation id without considering ob.epoch, which can allow overlapping invocations of the same planned operation if an older invocation’s container is still running.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
internal/engine/job.go:72
- This comment says the foreign-container probe runs “before any host write”, but at this point the function has already written the fence (
WriteFence) and started the heartbeat (which touches the lock). That makes the comment misleading about ordering/side effects.
// Under the lock, before anything mutates and before any host write: a job
// container from an earlier operation may still be running with no process
// owning it. Read-only, so it is safe on this side of the plan boundary.
if err := e.refuseForeignJobContainers(ctx, operationID); err != nil {
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperationID string) error { | ||
| containers, err := e.jobContainers(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, c := range containers { | ||
| if c.operation == currentOperationID { | ||
| continue | ||
| } | ||
| if c.operation == "" { |
The refusal exempted any container sharing the current operation id. A sealed job plan carries one operation id for its whole life and is re-runnable, and AcquireLock hands the lock straight back to a caller presenting the id already written in it — so a second run of one plan reclaimed the lock from a live first run and then exempted that run's container as its own. Two concurrent data-changing containers, which is the single thing this exists to prevent. `ob resume` reaches the same hole, since it carries the interrupted attempt's id. Matching now requires the epoch as well, which is what the label added alongside the operation was for and which nothing had read. A container that carries no epoch cannot be shown to belong to this invocation and is not exempt from it. The terminal record no longer stamps the reconciling operator. Audit takes the last non-empty operator in an epoch group, so it rewrote the interrupted run's row to name whoever deployed next; the start record already carries who ran it. Adds the first test that pins a call site rather than the function behind it: every refusal call site could be deleted with the suite green. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2
|
Closing in favour of #182, which is the refusal half of this PR on its own. Five review rounds found seven defects here, and every one was in the closing half — the epoch grouping, a result branch that could never fire because it matched on an operation kind that record does not carry, a reconciler that closed the job it was about to run on every run, and a terminal record that attributed the interrupted run to whoever reconciled it. A mutation test then showed why none of that was caught: all four call sites could be deleted with the whole suite green. The tests exercised two pure functions and never checked that anything called them. The refusal half has held up under the same scrutiny and is worth having on its own — it is the safety property, and it is one question asked at two call sites. #182 carries it with both call sites pinned by tests that drive the real entry points. The reconciliation work returns to #179 as unstarted. It is worth building, but it wants designing once rather than repairing seven times, and it needs decisions this PR kept deferring: which operations reconcile, what an honest terminal record says when the evidence is gone, and how audit groups a journal whose invocations interleave. |
Stage 1 of #179. Stacked on #180 — base is
feat/label-and-reconcile-job-containers; review that first, retarget when it merges. Depends on its container labels.The gap
A sealed job run that lost its client left a journal with a start and no terminal record, and nothing ever went back for it:
ob resume/ob abortskip non-deploy journals (resume.go:37-39),ob statususes the same finder and reports the application in sync, and onlyob auditshowed it — asINCOMPLETE, permanently. Meanwhile the container could still be changing data, and once the TTL expired the next operation took ownership on the strength of a dead heartbeat.Ask Docker, not the journal
The first version of this PR derived "is a job still running" from a journal reduction. Review found that wrong three ways at once, because a journal records what a client managed to write and the whole failure mode here is a client that did not write:
finishrecord made the reduction call the run finished while its container kept going — so the two PRs together defeated the protection each provides alone.finishin an earlier epoch was read as closing a later one.job_runjournal, so they could never be reconciled.All three are one
docker psaway. The refusal now asks Docker: any running one-off job container whose operation is not this one stops the operation, and names it. The journal reduction survives only for the closing half, where being wrong is cheap.Closing an unfinished run
Two bugs here, both found in review, both meaning this half did not do what it said:
ob auditgroups by epoch, so the operation stayed INCOMPLETE and grew a phantom row with the reconcile timestamp as its start. It now groups a journal by invocation and writes into the one it closes.operation_kind == "job_run", which that record does not carry — it is written by the shared job phase. So the branch recognising a proven outcome could never fire, and every reconciled run came out interrupted. Matched on its own shape now.Only an absent result is an unknown outcome. A recorded failure is evidence exactly as much as a recorded success, and calling it interrupted would hide that the job ran and failed on its own terms.
The refusal is read-only and runs before the plan boundary; the append runs after it, so a plan that will not execute writes nothing.
Correction to an earlier claim
An earlier version of this description said fabricating success would erase the
rollbackDebtan unresolved data-changing job carries. That is not true — the debt comes from an unresolvedintent/resultpair inSummarize, not from the finish record, and a job journal can never setDeploySucceeded. The fail-closed rule stands on its own: do not record an outcome nobody observed.Coverage this does not have
Only
ob deployandob job runreconcile.ob rollback,ob service apply,ob exec,ob secrets push,ob schedule runand bootstrap take the same lock and mutate without it — rolling back while an orphaned migration container is mid-flight is a real hazard and is not covered here. Worth a follow-up that hoists the refusal to a shared point.Verification
Seven tests: a foreign live container refuses; this operation's own container does not refuse itself; a run that recorded its own interruption is still caught by the container check (the Ctrl-C case); an absent result closes as
interruptedwith the right epoch; a journaled success closes as success; a journaled failure stays a failure and is not called interrupted; a journal with a finished and an unfinished invocation closes only the second.go test ./...— all packages pass.Refs #179. Execution is still attached; Stage 2 (host-side supervision) is what makes the exit code survive the client.
https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2