feat(jobs): refuse to mutate alongside another operation's job container

[vishr]

Replaces #181, which I am closing. Refs #179 — does not close it.

Why this instead of #181

#181 did two things: refuse while a job container is live, and write the terminal record for runs whose client never did. Five review rounds found seven defects, every one of them in the second half — the epoch grouping, a result branch that could never fire, a reconciler that closed the job it was about to run, wrong operator attribution. A mutation test then showed the tests were largely inert: all four call sites could be deleted with the suite green.

So this is the first half only. One question, asked at two call sites, small enough to verify. The reconciliation work goes back to #179 as unstarted, to be built properly rather than repaired again.

What it does

A sealed job run executes attached over SSH. When the client goes away the container keeps running — the daemon owns it — while nothing on the workstation does. The lock is then either dropped outright (Ctrl-C releases it on its own background context) or expires on a dead heartbeat, and the next deploy or job run proceeds alongside a live data-changing container.

ob deploy and ob job run now refuse while another operation's job container is running, naming it and how to inspect it.

Two decisions worth reviewing

It asks Docker, not the journal. A journal records what a client managed to write, and the failure this exists for is a client that did not write. A run that recorded its own interruption looks finished on paper while its container keeps going; a re-run plan appends a second invocation to the same journal; a deploy-phase gate container never appears in a job-run journal at all. One docker ps sees all three. #181 learned this the expensive way.

Matching is on operation AND epoch. A sealed plan carries one operation id for its whole life and is re-runnable, and AcquireLock (lock.go:155) hands the lock straight back to a caller presenting the id already written in it. Matching the operation alone would let a second run of one plan reclaim the lock from a live first run and then exempt that run's container as its own — two concurrent migration containers, which is the single thing this prevents. ob resume reaches the same hole, since it carries the interrupted attempt's id. A container with no epoch cannot be shown to belong to this invocation and is not exempt from it.

Placement follows the diagnostics: after the staleness checks so a stale plan is told it is stale rather than told about a container, and after preflight so an unreachable daemon is reported by the check that exists for that — while still preceding every mutation.

Known gaps, not fixed here

• Only ob deploy and ob job run check. Twenty-one call sites take the application lock; two check. ob backup restore and ob destroy are the sharpest — restore's own contract says it must not interleave with a deploy or a recovery, and restoring a volume under a live migration container is exactly that. ob rollback, ob abort, ob service apply, ob exec, ob secrets push, ob schedule run and bootstrap are the rest. Tracked on Sealed manual job can complete after client disconnect but remain permanently INCOMPLETE #179.
• Scheduled-job containers carry no labels at all, so they are invisible to this check and run without it themselves. Also tracked on Sealed manual job can complete after client disconnect but remain permanently INCOMPLETE #179.
• Hoisting the check into AcquireLock is not a safe one-liner: bootstrap takes the lock before Docker exists, and rollback/secrets push/service apply pass release ids as the deploy id, which would make the rollback case self-exempt.
• Non-compose hook jobs get no labels, so they are invisible to this.
• The probe uses the caller's context, so a wedged daemon can hang a deploy under the lock.

Test gaps, named rather than implied

The call sites are pinned, but their arguments are not: passing epoch 0, or the wrong operation id, survives the suite. Placement is not pinned either — moving the deploy check after the managed-proxy convergence stays green. And the probe command itself is unpinned, because every fake keys on a substring and returns hand-written output: swapping docker ps for docker ps -a survives, as does dropping the epoch from the format string. Worth closing, but each needs a fake that models docker ps rather than matching it.

Verification

Six unit tests on the predicate — foreign operation, own container, an earlier invocation of the same operation, an empty operation label, a missing epoch — plus two integration tests that drive ob deploy and ob job run against a host reporting a foreign container. Both call sites verified pinned: deleting either makes a test fail. That is the check #181 did not have.

go test ./... — all packages pass.

https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2

[Copilot]

🟡 Changes recommended

The new refusal error messages currently suggest docker rm -f with truncated container IDs (can be ambiguous), and the new refusal test file includes misleading/unfinished comments that should be cleaned up.

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

Pull request overview

This PR adds a safety gate to prevent ob deploy and sealed manual ob job run from mutating the application while a one-off job container from another operation (or another epoch of the same operation) is still running on the host, using Docker labels (ob.operation, ob.epoch) as the source of truth.

Changes:

• Add a Docker-based probe (docker ps --filter label=ob.operation) and refusal predicate to detect foreign / prior-epoch job containers.
• Invoke this refusal in both deploy (deployCore) and manual job execution (RunJobWithJournalID) before any mutation.
• Add unit tests for the predicate and integration-style tests ensuring deploy/job-run stop before starting when a foreign container is present.

File summaries

File	Description
internal/onebox/service_test.go	Update fakes to handle the new docker ps label probe used by the refusal gate.
internal/engine/job.go	Call the refusal gate after plan staleness checks and before creating a job container.
internal/engine/job_test.go	Add an integration-style test ensuring RunJobWithJournalID refuses before starting the job.
internal/engine/job_reconcile.go	Implement the Docker-based job-container probe and refusal logic.
internal/engine/job_reconcile_test.go	Add unit tests for the refusal predicate and parsing edge cases.
internal/engine/deploy.go	Call the refusal gate after preflight and before any deploy mutations / gate jobs.
internal/engine/deploy_test.go	Add an integration-style test ensuring deploy refuses before rolling workloads when a foreign container exists.

Review details

Suppressed comments (1)

internal/engine/job_reconcile_test.go:92

• The trailing comment block is unfinished and refers to recording operators, which isn’t part of this PR’s refusal-only behavior. Leaving it here reads like a missing test or incomplete change; please remove it (or add the corresponding implementation/tests in the appropriate PR).

// The reconciling operator must not be recorded as the interrupted run's.
// Audit takes the last non-empty operator in an epoch group, so stamping it
// here rewrites the row to name whoever deployed next.

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

[Copilot]

🔵 Needs a closer look

The new refusal error message asserts “no process owning it” without evidence, which can mislead operators in realistic lock-reclaim scenarios.

Review details

Suppressed comments (1)

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

internal/engine/job_containers.go:53

• The refusal message claims the foreign container is running "with no process owning it", but this code only knows that the container exists and its labels; it does not (and cannot) prove whether some process is still attached/controlling it. This can mislead operators in cases where the workload is still actively driven but the lock was reclaimed.

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

[Copilot]

🔵 Needs a closer look

The refusal error message currently asserts “earlier run” even when the epoch label is missing/unknown, which can misstate what’s known about the running container.

Review details

Suppressed comments (1)

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

internal/engine/job_containers.go:40

• When a container has the same operation ID but no epoch label, the error message still states it was from “an earlier run”. With an unknown epoch, the code can’t reliably distinguish whether it was an earlier invocation or simply an unlabeled/legacy container, so the message should avoid asserting “earlier” in that case while still refusing.

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

[Copilot]

🟢 Approval recommended

The behavioral change is narrowly scoped, uses a Docker-grounded predicate with good unit/integration coverage, and no functional defects were found in the new refusal logic.

Review details

Suppressed comments (2)

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

internal/onebox/service_test.go:86

• This fake match is overly broad: it will also match commands like docker ps -q --filter label='ob.operation=<id>' (used by jobContainerRunning), returning an empty result and potentially masking behavior in this test. Make the match specific to the new docker ps --filter label='ob.operation' --format ... probe.

This issue also appears on line 273 of the same file.

internal/onebox/service_test.go:274

• This fake match is overly broad: it will also match commands like docker ps -q --filter label='ob.operation=<id>', returning an empty result and potentially hiding bugs. Match the specific docker ps --filter label='ob.operation' --format ... probe instead.

		case strings.Contains(command, "label='ob.operation'"):
			return transport.Result{Stdout: "\n"}, true

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

[Copilot]

🟡 Changes recommended

The new docker ps parsing can silently ignore a running container on unexpected leading whitespace, and the updated test fake matchers are overly broad and may mask future label-filter behaviors.

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

Review details

Suppressed comments (1)

internal/onebox/service_test.go:274

• This matcher is also too broad here: strings.Contains(command, "label='ob.operation'") will intercept both the key-only filter and key=value filters (e.g. label='ob.operation=<id>'). Make it specific to the key-only --filter label='ob.operation' probe so other label queries can still be simulated accurately if added later.

		case strings.Contains(command, "label='ob.operation'"):
			return transport.Result{Stdout: "\n"}, true

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

[Copilot]

🔵 Needs a closer look

A refusal path in RunJobWithJournalID now reuses the “interrupted run left its container alive” lock-hold flag, causing a misleading deferred warning that doesn’t match the refusal scenario.

Review details

Suppressed comments (1)

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

internal/engine/job.go:97

• When refuseForeignJobContainers fails, this sets holdLockForLiveContainer = true, which triggers the deferred warning "operation ... was interrupted while its container is still running". In this refusal path the operation wasn’t interrupted and the live container may be from a different operation, so the warning is misleading (and the suggested docker ps --filter label=ob.operation=<this operation> won’t help locate the foreign container you just refused on). Consider separating the concepts of “keep the lock” vs “interrupted run left this operation’s container alive”, or gating the warnf on the interrupted-run case only.

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

[Copilot]

🟡 Changes recommended

The new lock-hold warning message in RunJobWithJournalID can be misleading on non-container errors (e.g., inability to query Docker), and should be made accurate before merging.

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

Review details

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

[Copilot]

🟡 Changes recommended

Deploy refusal currently keeps the application lock without clearly informing the operator in the returned error (and one new comment is misleading), which can cause confusing operational behavior.

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

Review details

Suppressed comments (1)

internal/engine/deploy.go:123

• When refuseForeignJobContainers returns an error, the deploy keeps the application lock (by design) but the returned error doesn’t mention this, so an operator may not understand why subsequent mutating commands are blocked. Consider wrapping the error to explicitly state that the lock is being held and for how long (TTL).

	if err := e.refuseForeignJobContainers(ctx, releaseID, epoch); err != nil {
		holdLockForLiveContainer = true
		return err
	}

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

[Copilot]

🟢 Approval recommended

The refusal logic is integrated at the stated call sites, fails closed on unusable Docker answers, and is covered by targeted unit and integration-style tests that pin behavior and lock handling.

Review details

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