feat(jobs): tie a one-off container to its operation, and record an interruption by vishr · Pull Request #180 · labstack/onebox

vishr · 2026-09-09T16:57:54Z

Stage 0 of #179. Does not close it — see "What this does not do".

What #179 gets wrong, in our favour

The issue says ownership is lost when the heartbeat dies and the TTL expires. For kill -9 and SIGHUP that is right. For Ctrl-C it is worse: ReleaseLock runs on its own 5s background context (internal/engine/lock.go:188), so it succeeds — while the result and finish appends, which use the cancelled context, both fail. The lock is dropped immediately and silently, over a container still changing data, and nothing is recorded. A cleanup context for the lock and none for the journal — that asymmetry is the bug in one sentence.

Three changes

Containers carry their operation. compose run names nothing and inherits no identity (gate.go:131), so nothing on the host could match a running one-off container to a journal. Everything downstream keys on that, so it lands first. Generated runs and hook-supplied compose runs both get ob.operation and ob.epoch; a hook that is not a compose run has no container to label and is already reported as unresolvable. injectComposeJobResult's splice is now shared as injectComposeRunFlags.

An interruption is recorded. The terminal append used the caller's context — the one that is gone exactly when the record matters. ob exec already writes its own on a bounded background context (ops.go:425-431); the job path now does the same.

Two independent decisions, kept separate after review found them conflated: where to append follows the caller's context alone — once cancelled it cannot carry the write, whatever the run did — and what to record follows what the run did. A job that finished cleanly a moment before Ctrl-C records its success, rather than being written down as a failure or not written at all. Append redacts Detail on failure, so the reason rides on ErrorCode.

ob audit renders interrupted as its own outcome, ahead of the failure arm the same record also matches.

The lock is held over a live container. When an interrupted run leaves this operation's container running, the lock is kept and the operator is told what holds it and when it expires. An unreadable answer counts as running: keeping the lock over a container that has exited costs one --break-lock; releasing it over one that has not costs concurrent writers.

What this does not do

Execution is still attached and still not durable.

kill -9 / SIGHUP: no Go code runs, so nothing is recorded and no lock is held.
A dropped connection: Go code does run, but every append fails on the dead transport and ReleaseLock fails with it — so the lock survives by accident until TTL and nothing is recorded.

Both are covered by #181, which finds such a run unfinished and closes it. Together they still do not give a durable exit code without the client; that is Stage 2.

Today's orphan (20260909-053225-54af549-job_run-219d9e8c182c) predates the labels and is not matched by any of this.

Verification

TestJobContainerCarriesItsOperationIdentity asserts both labels on the generated run; TestInjectComposeJobLabelsOnlyTouchesAComposeRun covers a hook compose run, a non-compose hook, and an absent operation id. TestAuditDistinguishesAnInterruptedJobRun pins the outcome ordering — removing the interrupted arm makes it fail as failed.

Known test gaps, called out rather than implied: nothing covers the skipped ReleaseLock or the background append after cancellation, and nothing exercises a hook compose run through both splices. Those need a fake that can cancel mid-run; worth adding, not yet here.

Approval, plan staleness and the migration-backup path are untouched.

Refs #179.

https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

…nterruption Three defects that compose into one incident: a sealed manual job run whose client goes away leaves a container the host cannot match to anything, an operation that is INCOMPLETE in `ob audit` forever, and an application lock released out from under a container that is still changing data. `compose run` names nothing and inherits no operation identity, so the host had no way to tell which run a one-off container belonged to. Every refusal and every reconciliation has to key on that, so it comes first: generated and hook-supplied compose runs both now carry ob.operation and ob.epoch. The terminal record was appended on the caller's context, which is precisely the context that is gone when it matters. `ob exec` already writes its own on a bounded background context; the job path now does the same and marks the record `interrupted`, because Append redacts Detail on a failure and the reason has to ride on ErrorCode. `ob audit` reports that as its own outcome, ahead of the failure arm the same record matches: the client went away and the outcome is unknown, which is a different thing to tell an operator than "it failed". ReleaseLock runs on its own background context, so on Ctrl-C it succeeded while the journal append on the cancelled context did not — ownership dropped immediately and silently, over a live container, before the TTL could matter. The lock is now held when an interrupted run leaves this operation's container running, and the operator is told what holds it and how to look. An unreadable answer counts as running: keeping the lock over a container that has exited costs one `--break-lock`, releasing it over one that has not costs concurrent writers. This does not make execution durable. A job whose client is killed outright still records nothing, and reconciling what a container did after the fact is the next step. It makes every new run reconcilable, which nothing was before. Refs #179. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

The code reaches an operator through `ob audit` but was not in the registry, so the generated error reference could not document it. The enumeration test missed it because it matches the struct-literal form and the assignment was a tuple; written as separate statements it is now covered by that test too. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

Copilot

🟡 Changes recommended

The new interruption detection/journaling logic can misclassify outcomes and still miss the terminal finish record when the client context is cancelled after work completes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses issue #179 (stage 0) by making manual one-off job containers reconcilable after client interruption: it ties containers to their initiating operation and records an explicit “interrupted” terminal outcome so ob audit can distinguish “client went away” from “job failed”/“incomplete”.

Changes:

Add ob.operation and ob.epoch labels to docker compose run (including hook-supplied compose runs) so host-side containers can be matched back to the operation/journal.
Write a terminal interrupted journal record on client interruption and surface it as a distinct ob audit outcome (ordered ahead of generic failure).
Keep the application lock when an interrupted run’s labeled container is still running, to prevent concurrent mutators.

File summaries

File	Description
site/src/content/docs/reference/errors.mdx	Documents the new `interrupted` error code.
internal/onebox/operation_errors.go	Adds `interrupted` to operation failure definitions.
internal/engine/schedule_execution_test.go	Updates test callsites for the updated `runOneJob` signature.
internal/engine/job.go	Records interruption durably; optionally holds lock over a still-running labeled job container.
internal/engine/gate.go	Adds operation/epoch labels to compose-run job containers; shares flag injection logic for hooks.
internal/engine/gate_test.go	Adds tests asserting job container labels and label injection behavior.
internal/engine/audit.go	Introduces an `interrupted` outcome arm ahead of generic failure.
internal/engine/audit_test.go	Adds a test pinning `interrupted` outcome ordering and behavior.

Review details

Suppressed comments (1)

internal/engine/job.go:176

interruptedRun currently treats any cancelled ctx as an interruption, even if runErr is a real job failure (or nil). This can incorrectly rewrite a successful/failed run as interrupted (and can keep the lock) if the client disconnects after the job finishes. Prefer basing "interrupted" classification on cancellation-related errors, not ctx.Err() alone.

func interruptedRun(ctx context.Context, runErr error) bool {
	return ctx.Err() != nil ||
		errors.Is(runErr, context.Canceled) ||
		errors.Is(runErr, context.DeadlineExceeded)

Files reviewed: 8/8 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.

The terminal append switched to a background context only when the run itself was classified interrupted, which conflated two independent questions. WHERE to append is decided by the caller's context: once it is cancelled it cannot carry the write, whatever the run did. WHAT to record is decided by what the run did: a job that completed cleanly a moment before Ctrl-C produced its own outcome and is not interrupted. Conflating them meant a successful job, cancelled in the window between its result and its finish record, was written down as a failure — or, if the classification did not catch the cancellation, was not written down at all. Also corrects the registry comment for the code, which described reconciliation as its only writer. The interrupted run writes it itself whenever it still can. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

Copilot

🟡 Changes recommended

interruptedRun currently classifies any cancelled caller context as an interruption, which can incorrectly record successful job runs as interrupted if cancellation occurs before the terminal append.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 8/8 changed files
Comments generated: 1
Review effort level: Lite

The previous commit split where the terminal record is appended from what it says, but left the classification itself keyed on the caller's context. A job that finished cleanly and was then cancelled in the window before its record was still written down as interrupted — the very case that split was meant to fix. A run that produced no error finished, whatever became of the client afterwards. Only once the run failed does a gone context mean the client is why, and that still counts even when the transport reports something other than a cancelled context, which it usually does. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

Copilot

🟡 Changes recommended

Terminal journaling still has a cancellation race that can drop the finish record (leaving operations INCOMPLETE) unless the append is retried on a bounded background context when cancellation happens during the write.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 9/9 changed files
Comments generated: 2
Review effort level: Lite

 		record := journal.Record{Phase: "job", Event: "finish", Status: "ok", OperationKind: "job_run", Service: job}
 		if runErr != nil {
 			record.Status = "fail"
 			record.Detail = runErr.Error()
 		}


The append switched to a context of its own only when the caller's was already gone. Cancellation during the write was left with a failed append and no second attempt, which is the same permanently incomplete operation by a narrower path. One retry closes it. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

Copilot

🟡 Changes recommended

The interrupted error-code definition comment in internal/onebox/operation_errors.go currently claims behavior not implemented in this PR and should be corrected to avoid misleading future maintenance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/onebox/operation_errors.go:126

The comment implies this error code is also written by “the next operation once it finds the run unfinished”, but in this PR the only writer appears to be the interrupted job run itself (the only ErrorCode: "interrupted" assignment is in internal/engine/job.go). To avoid misleading future readers, adjust the comment to describe current behavior (or explicitly gate the forward-looking part behind a reference to the reconciliation stage/PR).

		// Not a failure of the job and not an incomplete operation: the client
		// went away, and what the job did is unknown unless it journaled a
		// result first. Written by the interrupted run itself when it still can,
		// and otherwise by the next operation once it finds the run unfinished.

Files reviewed: 9/9 changed files
Comments generated: 1
Review effort level: Lite

+	// Released unless an interrupted run left this operation's container alive.
+	// ReleaseLock runs on its own background context, so on Ctrl-C it succeeds
+	// while the terminal journal append — which uses the cancelled one — does
+	// not: ownership would be dropped, immediately and silently, over a
+	// container still changing data.
+	holdLockForLiveContainer := false


The registry comment named a reconciler that does not exist here, so a reader of this change alone would look for behaviour it does not contain. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

Copilot

🔵 Needs a closer look

The current journal-append retry logic can unnecessarily re-append terminal records on non-cancellation errors, and one newly added gate hook comment misstates the behavior for non-compose hooks.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

internal/engine/job.go:145

The retry on a background context currently triggers for any append error when the caller context was still active at the start (journalContext == ctx). Because Writer.Append is not idempotent, retrying on non-cancellation errors can append a second terminal record in cases like transient transport failures where the remote append may already have happened but the response didn’t make it back. Narrow the retry condition to cases where the caller context became cancelled/deadline-exceeded (the intended scenario described in the comment).
internal/engine/gate.go:155
This comment says a non-docker compose run hook "is already reported as unresolvable above", but there’s no earlier reporting/guard here—non-compose hooks are still allowed and can write ONEBOX_RESULT_FILE on the host (they just won’t get container labels injected). Reword to avoid implying behavior that doesn’t exist in this path.

Files reviewed: 9/9 changed files
Comments generated: 0 new
Review effort level: Lite

…rite The retry fired on any append failure. A write that failed for another reason — a full disk, a refused write — may still have landed on the host, and appending a second terminal record over it is worse than reporting the first failure. It now retries only when the caller's context died during the write, which is the case it was added for. Also corrects the label comment for a hook that is not a compose run: it gets no label because there is no container to put one on, and its result is unresolvable for the same reason rather than by a check above. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

vishr added 2 commits September 9, 2026 09:57

vishr force-pushed the feat/label-and-reconcile-job-containers branch from c7b46cb to 47a3f56 Compare September 9, 2026 17:03

vishr mentioned this pull request Sep 9, 2026

feat(jobs): reconcile job runs whose client never recorded an outcome #181

Closed

vishr requested a lite review from Copilot September 9, 2026 17:21

Copilot started reviewing on behalf of vishr September 9, 2026 17:21 View session