From b65ff504954d89ef9ef764983d433054cee0e039 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Wed, 9 Sep 2026 12:33:30 -0700 Subject: [PATCH 01/11] feat(jobs): refuse to mutate alongside another operation's job container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 application 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. The check 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, and a deploy-phase gate container never appears in a job-run journal at all. One `docker ps` sees all three. Matching is on operation AND epoch. A sealed 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 matching the operation alone would let a second run of one plan exempt the container its own earlier run left behind. A container carrying 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, and after preflight so an unreachable daemon is reported by the check that exists for that, while still preceding every mutation. Both call sites are pinned by tests that drive the real entry points. The unit tests behind them passed with the calls deleted. Refs #179. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/deploy.go | 6 ++ internal/engine/deploy_test.go | 23 ++++++ internal/engine/job.go | 6 ++ internal/engine/job_reconcile.go | 103 ++++++++++++++++++++++++++ internal/engine/job_reconcile_test.go | 92 +++++++++++++++++++++++ internal/engine/job_test.go | 27 +++++++ internal/onebox/service_test.go | 5 ++ 7 files changed, 262 insertions(+) create mode 100644 internal/engine/job_reconcile.go create mode 100644 internal/engine/job_reconcile_test.go diff --git a/internal/engine/deploy.go b/internal/engine/deploy.go index 7274aa4e..3fa15621 100644 --- a/internal/engine/deploy.go +++ b/internal/engine/deploy.go @@ -105,6 +105,12 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri if err := e.requireServingApplicationManifest(ctx, prev); err != nil { return err } + // After preflight, so an unreachable daemon is reported by the check that + // exists for it rather than by a raw `docker ps` failure — and still before + // any workload is rolled or any gate job runs. + if err := e.refuseForeignJobContainers(ctx, releaseID, epoch); err != nil { + return err + } rollbackDebt := false if done == nil { rollbackDebt, err = e.rollbackEffectDebt(ctx, prev) diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index c911080f..6b45e714 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -563,3 +563,26 @@ func TestRollbackReplaysPreviousRelease(t *testing.T) { t.Fatalf("rollback must re-activate previous:\n%s", seq) } } + +// A deploy rolls workloads and runs its own gate jobs. An orphaned job +// container still changing data underneath it is the overlap the lock exists to +// prevent, and the lock does not catch it once its holder is gone. +func TestDeployRefusesWhileAForeignJobContainerRuns(t *testing.T) { + f := happyFake() + inner := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) + err := e.Deploy(context.Background(), "20260101-000000-aaa111", t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("deploy = %v, want a refusal naming the foreign operation", err) + } + // Refused before anything is rolled or any gate job runs. + if seq := strings.Join(f.Commands, "\n"); strings.Contains(seq, "--scale web=") { + t.Fatalf("the deploy rolled anyway:\n%s", seq) + } +} diff --git a/internal/engine/job.go b/internal/engine/job.go index 8ffd5dee..a22051b1 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -86,6 +86,12 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) return operationID, nil, errors.New("job plan is stale: current release runtime changed — re-plan") } + // After the staleness checks, so a stale plan is told it is stale rather + // than told about a container, and before this run creates one of its own. + if err := e.refuseForeignJobContainers(ctx, operationID, epoch); err != nil { + return operationID, nil, err + } + writer := &journal.Writer{ T: e.T, Names: e.names(), DeployID: operationID, Epoch: epoch, Operator: journal.DefaultOperator(), GitSHA: e.Opts.GitSHA, ConfigHash: e.Opts.ConfigHash, diff --git a/internal/engine/job_reconcile.go b/internal/engine/job_reconcile.go new file mode 100644 index 00000000..ca5cea3c --- /dev/null +++ b/internal/engine/job_reconcile.go @@ -0,0 +1,103 @@ +package engine + +import ( + "context" + "fmt" + "strconv" + "strings" +) + +// refuseForeignJobContainers refuses when a one-off job container from another +// operation is still running on the host. +// +// The question "is a job still running" is asked of Docker, not of the journal. +// A journal says what a client managed to write, and the whole failure mode +// here is a client that did not write. An interrupted run that DID record its +// interruption looks finished on paper while its container keeps changing data, +// and a plan re-run appends a second invocation to the same journal — both are +// invisible to any reduction over records, and both are one `docker ps` away. +func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperationID string, currentEpoch int) error { + containers, err := e.jobContainers(ctx) + if err != nil { + return err + } + currentEpochLabel := strconv.Itoa(currentEpoch) + for _, c := range containers { + // Operation AND epoch. A sealed job plan is re-runnable and carries one + // operation id for its whole life, and AcquireLock hands the lock + // straight back to a caller presenting the id already written in it. So + // a second run of one plan would reclaim the lock from a live first run + // and then exempt that run's container as its own — two concurrent + // data-changing containers, which is the single thing this prevents. + if c.operation == currentOperationID && c.epoch == currentEpochLabel { + continue + } + if c.operation == currentOperationID { + return fmt.Errorf( + "an earlier run of operation %s (epoch %s) left a job container running on this host (%.12s); "+ + "wait for it to finish, or establish what it did and stop it with `docker rm -f %.12s`", + c.operation, labelOrUnknown(c.epoch), c.id, c.id) + } + if c.operation == "" { + // The label is present but carries no value, so the container + // cannot be attributed. Refuse anyway: an unattributable job + // container is exactly as dangerous as an attributable one. + return fmt.Errorf( + "a job container is running on this host (%.12s) with an empty %s label, so the operation that "+ + "started it cannot be identified; establish what it did and stop it with `docker rm -f %.12s`", + c.id, JobOperationLabel, c.id) + } + 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) + } + return nil +} + +type jobContainer struct { + id string + operation string + epoch string +} + +func labelOrUnknown(value string) string { + if value == "" { + return "unknown" + } + return value +} + +// jobContainers lists every running one-off job container, whichever operation +// created it. The label is unvalued in the filter so this finds containers of +// operations this process knows nothing about, which is the point. +func (e *Engine) jobContainers(ctx context.Context) ([]jobContainer, error) { + res, err := e.T.Run(ctx, + "docker ps --filter label="+q(JobOperationLabel)+ + " --format "+q("{{.ID}} {{.Label \""+JobOperationLabel+"\"}} {{.Label \""+JobEpochLabel+"\"}}")) + if err != nil { + return nil, err + } + if res.ExitCode != 0 { + return nil, fmt.Errorf("list running job containers (exit %d): %s", res.ExitCode, strings.TrimSpace(res.Stderr)) + } + var out []jobContainer + for _, line := range strings.Split(res.Stdout, "\n") { + // Not TrimSpace before the cut: a container whose label carries no value + // prints " " with nothing after the separator, and trimming the line + // first removes the separator itself — the container would then be + // skipped as unparseable, which is precisely the one that most needs + // refusing. + id, rest, _ := strings.Cut(strings.TrimRight(line, "\r\n"), " ") + operation, epoch, _ := strings.Cut(rest, " ") + id, operation, epoch = strings.TrimSpace(id), strings.TrimSpace(operation), strings.TrimSpace(epoch) + if id == "" { + continue + } + if !validID.MatchString(id) { + return nil, fmt.Errorf("suspicious container id %q from docker ps — refusing to reuse in a command", id) + } + out = append(out, jobContainer{id: id, operation: operation, epoch: epoch}) + } + return out, nil +} diff --git a/internal/engine/job_reconcile_test.go b/internal/engine/job_reconcile_test.go new file mode 100644 index 00000000..ff4b44d4 --- /dev/null +++ b/internal/engine/job_reconcile_test.go @@ -0,0 +1,92 @@ +package engine + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/labstack/onebox/internal/transport" +) + +// reconcileFake answers the running-container probe. `running` is +// ` ` lines, exactly as the label probe formats them. +func reconcileFake(running []string) *transport.Fake { + return &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "label='ob.operation'"): + return transport.Result{Stdout: strings.Join(running, "\n") + "\n"}, true + } + return transport.Result{}, false + }} +} + +func reconcileEngine(t *testing.T, f *transport.Fake) *Engine { + t.Helper() + return New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) +} + +// closeAll reads the journals the way a caller does, then closes what it finds. +func TestRefuseWhileAnotherOperationsJobContainerRuns(t *testing.T) { + f := reconcileFake([]string{"abc123def456 J1 4"}) + err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + if err == nil { + t.Fatal("a live job container from another operation must refuse") + } + for _, want := range []string{"J1", "abc123def456", "still running"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal missing %q: %v", want, err) + } + } +} + +// This operation's own container is not a reason to refuse itself — a deploy +// runs gate jobs under its own id. +func TestRefuseAllowsThisOperationsOwnContainer(t *testing.T) { + f := reconcileFake([]string{"abc123def456 J1 4"}) + if err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4); err != nil { + t.Fatalf("own container refused: %v", err) + } +} + +func TestRefuseCatchesAContainerWithAnEmptyOperationLabel(t *testing.T) { + f := reconcileFake([]string{"abc123def456 "}) + err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + if err == nil { + t.Fatal("an unattributable job container must refuse") + } + if !strings.Contains(err.Error(), "empty") || !strings.Contains(err.Error(), "abc123def456") { + t.Fatalf("refusal did not name the problem: %v", err) + } +} + +// 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. Matching the operation alone would +// let a second run exempt the container its own earlier run left behind. +func TestRefuseCatchesAnEarlierRunOfTheSameOperation(t *testing.T) { + f := reconcileFake([]string{"abc123def456 J1 4"}) + err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 5) + if err == nil { + t.Fatal("an earlier invocation of the same plan must refuse") + } + for _, want := range []string{"earlier run", "J1", "epoch 4", "abc123def456"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal missing %q: %v", want, err) + } + } +} + +// A container carrying no epoch label cannot be shown to belong to this +// invocation, so it is not exempt from it either. +func TestRefuseDoesNotExemptAContainerWithNoEpoch(t *testing.T) { + f := reconcileFake([]string{"abc123def456 J1 "}) + err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4) + if err == nil || !strings.Contains(err.Error(), "epoch unknown") { + t.Fatalf("unlabelled epoch = %v, want a refusal naming it", err) + } +} + +// 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. diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index 88cfd4bc..fe05a70c 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -172,3 +172,30 @@ func TestInterruptedRunClassifiesTheRunNotTheClient(t *testing.T) { t.Fatal("a job that failed on its own terms is not interrupted") } } + +// The refusal is only worth having if it is actually called. Deleting the call +// site left the unit tests green, so this drives the whole run against a host +// reporting a foreign job container and requires it to stop before the job +// starts. +func TestRunJobRefusesWhileAForeignJobContainerRuns(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + engine := manualJobEngine(t, target) + _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("run job = %v, want a refusal naming the foreign operation", err) + } + if strings.Contains(strings.Join(target.Commands, "\n"), "ONEBOX_RESULT_FILE=") { + t.Fatalf("the job ran anyway:\n%s", strings.Join(target.Commands, "\n")) + } +} diff --git a/internal/onebox/service_test.go b/internal/onebox/service_test.go index 113c25b3..12aeecd9 100644 --- a/internal/onebox/service_test.go +++ b/internal/onebox/service_test.go @@ -81,6 +81,9 @@ func serviceFake() *transport.Fake { return transport.Result{Stdout: "demo\n"}, true case strings.Contains(cmd, "readlink"): return transport.Result{Stdout: "releases/R0\n"}, true + // Ahead of the project-container probe, which also uses --format. + case strings.Contains(cmd, "label='ob.operation'"): + return transport.Result{Stdout: "\n"}, true case strings.Contains(cmd, "docker ps") && strings.Contains(cmd, "--format"): return transport.Result{Stdout: "S1|web|R0|Up (healthy)\nPG1|database|R0|Up (healthy)\n"}, true case strings.Contains(cmd, "for f in"): @@ -267,6 +270,8 @@ deployment: {order: [web, worker]} return transport.Result{Stdout: project(false)}, true case strings.Contains(command, "cat ") && strings.Contains(command, "compose.yaml"): return transport.Result{Stdout: liveCompose}, true + case strings.Contains(command, "label='ob.operation'"): + return transport.Result{Stdout: "\n"}, true case strings.Contains(command, "docker ps") && strings.Contains(command, "--format"): return transport.Result{Stdout: "S1|web|R0|Up\nW1|worker|R0|Up\n"}, true default: From 44a15fc3f62114461aac7ea89b819da47424d994 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Wed, 9 Sep 2026 13:32:36 -0700 Subject: [PATCH 02/11] refactor(jobs): name the file for what it does, and paste a whole container id Splitting the reconciliation out left this file named for work it no longer contains, two helpers named after it, and an orphaned doc comment sitting above an unrelated test describing a function that is gone. Renamed to what it is: the running job containers of an operation. The refusals suggested `docker rm -f` with a twelve-character prefix. That is what docker prints and it is usually unique, which is the wrong property for a command an operator is expected to paste against a container they are about to destroy. The prose still abbreviates; the command carries the whole id. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- .../{job_reconcile.go => job_containers.go} | 6 ++-- ...concile_test.go => job_containers_test.go} | 31 ++++++++----------- 2 files changed, 16 insertions(+), 21 deletions(-) rename internal/engine/{job_reconcile.go => job_containers.go} (97%) rename internal/engine/{job_reconcile_test.go => job_containers_test.go} (67%) diff --git a/internal/engine/job_reconcile.go b/internal/engine/job_containers.go similarity index 97% rename from internal/engine/job_reconcile.go rename to internal/engine/job_containers.go index ca5cea3c..cc5b9df0 100644 --- a/internal/engine/job_reconcile.go +++ b/internal/engine/job_containers.go @@ -35,7 +35,7 @@ func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperatio if c.operation == currentOperationID { return fmt.Errorf( "an earlier run of operation %s (epoch %s) left a job container running on this host (%.12s); "+ - "wait for it to finish, or establish what it did and stop it with `docker rm -f %.12s`", + "wait for it to finish, or establish what it did and stop it with `docker rm -f %s`", c.operation, labelOrUnknown(c.epoch), c.id, c.id) } if c.operation == "" { @@ -44,12 +44,12 @@ func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperatio // container is exactly as dangerous as an attributable one. return fmt.Errorf( "a job container is running on this host (%.12s) with an empty %s label, so the operation that "+ - "started it cannot be identified; establish what it did and stop it with `docker rm -f %.12s`", + "started it cannot be identified; establish what it did and stop it with `docker rm -f %s`", c.id, JobOperationLabel, c.id) } 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`", + "wait for it to finish, or establish what it did and stop it with `docker rm -f %s`", c.operation, c.id, c.id) } return nil diff --git a/internal/engine/job_reconcile_test.go b/internal/engine/job_containers_test.go similarity index 67% rename from internal/engine/job_reconcile_test.go rename to internal/engine/job_containers_test.go index ff4b44d4..711b919f 100644 --- a/internal/engine/job_reconcile_test.go +++ b/internal/engine/job_containers_test.go @@ -9,9 +9,9 @@ import ( "github.com/labstack/onebox/internal/transport" ) -// reconcileFake answers the running-container probe. `running` is +// jobContainerFake answers the running-container probe. `running` is // ` ` lines, exactly as the label probe formats them. -func reconcileFake(running []string) *transport.Fake { +func jobContainerFake(running []string) *transport.Fake { return &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { switch { case strings.Contains(cmd, "label='ob.operation'"): @@ -21,15 +21,14 @@ func reconcileFake(running []string) *transport.Fake { }} } -func reconcileEngine(t *testing.T, f *transport.Fake) *Engine { +func jobContainerEngine(t *testing.T, f *transport.Fake) *Engine { t.Helper() return New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) } -// closeAll reads the journals the way a caller does, then closes what it finds. func TestRefuseWhileAnotherOperationsJobContainerRuns(t *testing.T) { - f := reconcileFake([]string{"abc123def456 J1 4"}) - err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + f := jobContainerFake([]string{"abc123def456 J1 4"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) if err == nil { t.Fatal("a live job container from another operation must refuse") } @@ -43,15 +42,15 @@ func TestRefuseWhileAnotherOperationsJobContainerRuns(t *testing.T) { // This operation's own container is not a reason to refuse itself — a deploy // runs gate jobs under its own id. func TestRefuseAllowsThisOperationsOwnContainer(t *testing.T) { - f := reconcileFake([]string{"abc123def456 J1 4"}) - if err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4); err != nil { + f := jobContainerFake([]string{"abc123def456 J1 4"}) + if err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4); err != nil { t.Fatalf("own container refused: %v", err) } } func TestRefuseCatchesAContainerWithAnEmptyOperationLabel(t *testing.T) { - f := reconcileFake([]string{"abc123def456 "}) - err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) + f := jobContainerFake([]string{"abc123def456 "}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J2", 4) if err == nil { t.Fatal("an unattributable job container must refuse") } @@ -65,8 +64,8 @@ func TestRefuseCatchesAContainerWithAnEmptyOperationLabel(t *testing.T) { // presenting the id already written in it. Matching the operation alone would // let a second run exempt the container its own earlier run left behind. func TestRefuseCatchesAnEarlierRunOfTheSameOperation(t *testing.T) { - f := reconcileFake([]string{"abc123def456 J1 4"}) - err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 5) + f := jobContainerFake([]string{"abc123def456 J1 4"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 5) if err == nil { t.Fatal("an earlier invocation of the same plan must refuse") } @@ -80,13 +79,9 @@ func TestRefuseCatchesAnEarlierRunOfTheSameOperation(t *testing.T) { // A container carrying no epoch label cannot be shown to belong to this // invocation, so it is not exempt from it either. func TestRefuseDoesNotExemptAContainerWithNoEpoch(t *testing.T) { - f := reconcileFake([]string{"abc123def456 J1 "}) - err := reconcileEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4) + f := jobContainerFake([]string{"abc123def456 J1 "}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4) if err == nil || !strings.Contains(err.Error(), "epoch unknown") { t.Fatalf("unlabelled epoch = %v, want a refusal naming it", err) } } - -// 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. From e3f4c6e450c06e43540e96ce213ab7ca9e8c4ad7 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:01:32 -0700 Subject: [PATCH 03/11] fix(jobs): do not assert what the refusal has not established MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The message said a running job container had "no process owning it". Nothing checks that. A container can be running while its operation is very much in progress — a lock reclaimed by the same operation id, or an operator watching it in another terminal — and telling them otherwise invites them to destroy work that is proceeding normally. It now states what is known and leaves the judgement where it belongs. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job_containers.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go index cc5b9df0..abe53967 100644 --- a/internal/engine/job_containers.go +++ b/internal/engine/job_containers.go @@ -48,8 +48,9 @@ func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperatio c.id, JobOperationLabel, c.id) } 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 %s`", + "a job container from operation %s is still running on this host (%.12s); "+ + "if that operation is still in progress, wait for it — otherwise establish what it did "+ + "and stop it with `docker rm -f %s`", c.operation, c.id, c.id) } return nil From f00a89a1e14749a915b3457614676b659cd35166 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:13:15 -0700 Subject: [PATCH 04/11] fix(jobs): say only what the labels establish about a container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same-operation refusal called the container an "earlier run" whichever epoch it carried, and "unknown" when it carried none. A differing epoch says the container belongs to some other invocation, not which one or when — epochs are not ordered against each other here — and a missing epoch says only that it cannot be placed at all. Both now say that, and the one with an epoch prints both epochs so an operator can tell the two invocations apart. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job_containers.go | 26 ++++++++++++++++---------- internal/engine/job_containers_test.go | 10 +++++----- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go index abe53967..8c47ff98 100644 --- a/internal/engine/job_containers.go +++ b/internal/engine/job_containers.go @@ -33,10 +33,23 @@ func (e *Engine) refuseForeignJobContainers(ctx context.Context, currentOperatio continue } if c.operation == currentOperationID { + // A differing epoch says the container belongs to some other + // invocation of this operation, not which one or when — epochs are + // not ordered against each other here — and a missing epoch says + // only that it cannot be placed at all. Neither supports calling it + // an earlier run. + if c.epoch == "" { + return fmt.Errorf( + "a job container of operation %s is running on this host (%.12s) carrying no %s label, "+ + "so it cannot be placed against this run; establish what it did and stop it with "+ + "`docker rm -f %s`", + c.operation, c.id, JobEpochLabel, c.id) + } return fmt.Errorf( - "an earlier run of operation %s (epoch %s) left a job container running on this host (%.12s); "+ - "wait for it to finish, or establish what it did and stop it with `docker rm -f %s`", - c.operation, labelOrUnknown(c.epoch), c.id, c.id) + "another invocation of operation %s (epoch %s, this run is epoch %s) left a job container "+ + "running on this host (%.12s); wait for it to finish, or establish what it did and stop "+ + "it with `docker rm -f %s`", + c.operation, c.epoch, currentEpochLabel, c.id, c.id) } if c.operation == "" { // The label is present but carries no value, so the container @@ -62,13 +75,6 @@ type jobContainer struct { epoch string } -func labelOrUnknown(value string) string { - if value == "" { - return "unknown" - } - return value -} - // jobContainers lists every running one-off job container, whichever operation // created it. The label is unvalued in the filter so this finds containers of // operations this process knows nothing about, which is the point. diff --git a/internal/engine/job_containers_test.go b/internal/engine/job_containers_test.go index 711b919f..99116feb 100644 --- a/internal/engine/job_containers_test.go +++ b/internal/engine/job_containers_test.go @@ -62,14 +62,14 @@ func TestRefuseCatchesAContainerWithAnEmptyOperationLabel(t *testing.T) { // 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. Matching the operation alone would -// let a second run exempt the container its own earlier run left behind. -func TestRefuseCatchesAnEarlierRunOfTheSameOperation(t *testing.T) { +// let one run exempt the container another invocation of it left behind. +func TestRefuseCatchesAnotherInvocationOfTheSameOperation(t *testing.T) { f := jobContainerFake([]string{"abc123def456 J1 4"}) err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 5) if err == nil { t.Fatal("an earlier invocation of the same plan must refuse") } - for _, want := range []string{"earlier run", "J1", "epoch 4", "abc123def456"} { + for _, want := range []string{"another invocation", "J1", "epoch 4", "this run is epoch 5", "abc123def456"} { if !strings.Contains(err.Error(), want) { t.Fatalf("refusal missing %q: %v", want, err) } @@ -81,7 +81,7 @@ func TestRefuseCatchesAnEarlierRunOfTheSameOperation(t *testing.T) { func TestRefuseDoesNotExemptAContainerWithNoEpoch(t *testing.T) { f := jobContainerFake([]string{"abc123def456 J1 "}) err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 4) - if err == nil || !strings.Contains(err.Error(), "epoch unknown") { - t.Fatalf("unlabelled epoch = %v, want a refusal naming it", err) + if err == nil || !strings.Contains(err.Error(), "carrying no ob.epoch label") { + t.Fatalf("unlabelled epoch = %v, want a refusal saying it cannot be placed", err) } } From 98e73fe220bf5579e2820bfd7281c3bcd338fe6e Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:28:25 -0700 Subject: [PATCH 05/11] fix(jobs): keep the lock when refusing over a live container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refusing released the application lock on the way out. On the job path the refusal returned before the flag that holds it could be set; on the deploy path the release was unconditional. So a run that found a live data-changing container told the operator about it and then handed the host to whatever came next — with the lock the interrupted run had been holding gone as well. That is worse than not refusing at all: without this check the second run would at least have kept a lock. Both paths now keep it, which is the state the lock exists for. Also corrects the comment above the docker ps parsing. It justified TrimRight by claiming a trimmed line would be unparseable, which is not true — the fields are trimmed individually either way. What TrimRight actually avoids is the inverse: leading whitespace cutting to an empty id and dropping the line in silence, which is the one failure this check cannot afford. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/deploy.go | 12 +++++++++++- internal/engine/job.go | 5 +++++ internal/engine/job_containers.go | 9 ++++----- internal/engine/job_test.go | 28 ++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/internal/engine/deploy.go b/internal/engine/deploy.go index 3fa15621..c954ed20 100644 --- a/internal/engine/deploy.go +++ b/internal/engine/deploy.go @@ -75,7 +75,16 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri if err != nil { return err } - defer e.ReleaseLock(ctx) + // Released unless a live job container is found below: handing the host to + // the next mutator over a container this deploy refused to run alongside + // would defeat the refusal. + holdLockForLiveContainer := false + defer func() { + if holdLockForLiveContainer { + return + } + e.ReleaseLock(ctx) + }() if err := e.WriteFence(ctx, releaseID, epoch); err != nil { return err } @@ -109,6 +118,7 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri // exists for it rather than by a raw `docker ps` failure — and still before // any workload is rolled or any gate job runs. if err := e.refuseForeignJobContainers(ctx, releaseID, epoch); err != nil { + holdLockForLiveContainer = true return err } rollbackDebt := false diff --git a/internal/engine/job.go b/internal/engine/job.go index a22051b1..58c56a50 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -89,6 +89,11 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // After the staleness checks, so a stale plan is told it is stale rather // than told about a container, and before this run creates one of its own. if err := e.refuseForeignJobContainers(ctx, operationID, epoch); err != nil { + // Keep the lock. Releasing it here would hand the host to the next + // mutator over a container this check has just established is alive — + // the opposite of what refusing is for, and worse than not refusing, + // because the lock reclaimed from the interrupted run would be gone too. + holdLockForLiveContainer = true return operationID, nil, err } diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go index 8c47ff98..981f9133 100644 --- a/internal/engine/job_containers.go +++ b/internal/engine/job_containers.go @@ -90,11 +90,10 @@ func (e *Engine) jobContainers(ctx context.Context) ([]jobContainer, error) { } var out []jobContainer for _, line := range strings.Split(res.Stdout, "\n") { - // Not TrimSpace before the cut: a container whose label carries no value - // prints " " with nothing after the separator, and trimming the line - // first removes the separator itself — the container would then be - // skipped as unparseable, which is precisely the one that most needs - // refusing. + // TrimRight, not TrimSpace: leading whitespace would cut to an empty id and + // drop the line silently, and a container nobody can see is the one failure + // this check cannot afford. Each field is trimmed on its own below, so a + // label carrying no value still parses. id, rest, _ := strings.Cut(strings.TrimRight(line, "\r\n"), " ") operation, epoch, _ := strings.Cut(rest, " ") id, operation, epoch = strings.TrimSpace(id), strings.TrimSpace(operation), strings.TrimSpace(epoch) diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index fe05a70c..5246d87f 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -199,3 +199,31 @@ func TestRunJobRefusesWhileAForeignJobContainerRuns(t *testing.T) { t.Fatalf("the job ran anyway:\n%s", strings.Join(target.Commands, "\n")) } } + +// Refusing must not hand the host to the next mutator. Releasing the lock here +// would leave no lock and a live data-changing container — worse than not +// refusing, because the lock this run reclaimed from the interrupted one would +// be gone with it. +func TestRunJobKeepsTheLockWhenItRefuses(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + engine := manualJobEngine(t, target) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("expected a refusal") + } + for _, c := range target.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released over a live container:\n%s", c) + } + } +} From ee374b99b74449cadeb08c107a95b4429f2a2dc4 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:51:26 -0700 Subject: [PATCH 06/11] fix(jobs): do not drop a container whose line arrives padded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's comment claimed TrimRight avoided leading whitespace cutting to an empty id. It does not — TrimRight strips the newline and nothing else, so a padded line still yielded an empty id and was skipped in silence. A container nobody can see is the one failure this check cannot afford, and the comment asserting otherwise made it harder to spot. Leading whitespace is now stripped and the line end only of its newline, which keeps the trailing space that makes an empty label parse as empty rather than absent. Both cases are pinned: a padded line still refuses, an empty label still refuses. The onebox fakes matched any command mentioning the label, which would also swallow a future query of a different shape. They now match the probe they mean to answer. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job_containers.go | 12 +++++++----- internal/engine/job_containers_test.go | 11 +++++++++++ internal/onebox/service_test.go | 4 ++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go index 981f9133..bfabbfcb 100644 --- a/internal/engine/job_containers.go +++ b/internal/engine/job_containers.go @@ -90,11 +90,13 @@ func (e *Engine) jobContainers(ctx context.Context) ([]jobContainer, error) { } var out []jobContainer for _, line := range strings.Split(res.Stdout, "\n") { - // TrimRight, not TrimSpace: leading whitespace would cut to an empty id and - // drop the line silently, and a container nobody can see is the one failure - // this check cannot afford. Each field is trimmed on its own below, so a - // label carrying no value still parses. - id, rest, _ := strings.Cut(strings.TrimRight(line, "\r\n"), " ") + // Leading whitespace is stripped so an id always parses, and the line end + // only of its newline — trailing space is the separator that makes an + // empty label parse as empty rather than as absent. A dropped line here + // is a container nobody can see, which is the one failure this cannot + // afford. + line = strings.TrimRight(strings.TrimLeft(line, " \t"), "\r\n") + id, rest, _ := strings.Cut(line, " ") operation, epoch, _ := strings.Cut(rest, " ") id, operation, epoch = strings.TrimSpace(id), strings.TrimSpace(operation), strings.TrimSpace(epoch) if id == "" { diff --git a/internal/engine/job_containers_test.go b/internal/engine/job_containers_test.go index 99116feb..ea5c196f 100644 --- a/internal/engine/job_containers_test.go +++ b/internal/engine/job_containers_test.go @@ -85,3 +85,14 @@ func TestRefuseDoesNotExemptAContainerWithNoEpoch(t *testing.T) { t.Fatalf("unlabelled epoch = %v, want a refusal saying it cannot be placed", err) } } + +// A line with leading whitespace must still yield its container. Cutting on the +// first space without stripping it yields an empty id, and the container is +// dropped in silence — invisible to the check that exists to see it. +func TestRefuseSeesAContainerOnAPaddedLine(t *testing.T) { + f := jobContainerFake([]string{" abc123def456 other-op 2"}) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "J1", 1) + if err == nil || !strings.Contains(err.Error(), "other-op") { + t.Fatalf("padded line = %v, want the container refused", err) + } +} diff --git a/internal/onebox/service_test.go b/internal/onebox/service_test.go index 12aeecd9..827873e7 100644 --- a/internal/onebox/service_test.go +++ b/internal/onebox/service_test.go @@ -82,7 +82,7 @@ func serviceFake() *transport.Fake { case strings.Contains(cmd, "readlink"): return transport.Result{Stdout: "releases/R0\n"}, true // Ahead of the project-container probe, which also uses --format. - case strings.Contains(cmd, "label='ob.operation'"): + case strings.Contains(cmd, "docker ps --filter label='ob.operation'"): return transport.Result{Stdout: "\n"}, true case strings.Contains(cmd, "docker ps") && strings.Contains(cmd, "--format"): return transport.Result{Stdout: "S1|web|R0|Up (healthy)\nPG1|database|R0|Up (healthy)\n"}, true @@ -270,7 +270,7 @@ deployment: {order: [web, worker]} return transport.Result{Stdout: project(false)}, true case strings.Contains(command, "cat ") && strings.Contains(command, "compose.yaml"): return transport.Result{Stdout: liveCompose}, true - case strings.Contains(command, "label='ob.operation'"): + case strings.Contains(command, "docker ps --filter label='ob.operation'"): return transport.Result{Stdout: "\n"}, true case strings.Contains(command, "docker ps") && strings.Contains(command, "--format"): return transport.Result{Stdout: "S1|web|R0|Up\nW1|worker|R0|Up\n"}, true From d0440b191584cddf6d67b8dd53537e7a2507cc01 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:55:27 -0700 Subject: [PATCH 07/11] test(jobs): state what docker ps can hand back, and simplify the parsing to match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This parsing has been wrong twice — once dropping a container whose label was empty, once dropping one whose line was padded — and each fix was checked against a single example, which is why the second bug survived the first fix. A table now states the whole shape: no output, blank lines, several containers, each label absent in turn, padding, a carriage return, and no trailing newline. Writing it exposed a third error, this time in my own reasoning rather than the code. The previous commit justified splitting TrimLeft from TrimRight by claiming the trailing separator was what let an empty label parse as empty. It is not: fields are cut and trimmed individually, and Cut yields empty for a separator that is not there, so a plain TrimSpace behaves identically. Verified by mutation — swapping them fails nothing. The parsing is now the simpler form and the comment says what is actually true. Also covers a daemon that cannot answer, and output that is not a container id; neither may read as an empty host. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job_containers.go | 13 +++--- internal/engine/job_containers_test.go | 59 ++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/internal/engine/job_containers.go b/internal/engine/job_containers.go index bfabbfcb..c45fd936 100644 --- a/internal/engine/job_containers.go +++ b/internal/engine/job_containers.go @@ -90,13 +90,12 @@ func (e *Engine) jobContainers(ctx context.Context) ([]jobContainer, error) { } var out []jobContainer for _, line := range strings.Split(res.Stdout, "\n") { - // Leading whitespace is stripped so an id always parses, and the line end - // only of its newline — trailing space is the separator that makes an - // empty label parse as empty rather than as absent. A dropped line here - // is a container nobody can see, which is the one failure this cannot - // afford. - line = strings.TrimRight(strings.TrimLeft(line, " \t"), "\r\n") - id, rest, _ := strings.Cut(line, " ") + // Each field is cut and trimmed on its own, and Cut yields empty for a + // separator that is not there — so a label docker could not resolve + // parses as empty whether or not its separator survives. What matters is + // that the id parses: a line that yields none is dropped below, and a + // dropped line is a container nobody can see. + id, rest, _ := strings.Cut(strings.TrimSpace(line), " ") operation, epoch, _ := strings.Cut(rest, " ") id, operation, epoch = strings.TrimSpace(id), strings.TrimSpace(operation), strings.TrimSpace(epoch) if id == "" { diff --git a/internal/engine/job_containers_test.go b/internal/engine/job_containers_test.go index ea5c196f..22d8ffab 100644 --- a/internal/engine/job_containers_test.go +++ b/internal/engine/job_containers_test.go @@ -96,3 +96,62 @@ func TestRefuseSeesAContainerOnAPaddedLine(t *testing.T) { t.Fatalf("padded line = %v, want the container refused", err) } } + +// The parsing has now been wrong twice in ways a single example did not catch — +// once dropping an empty label, once dropping a padded line. This states the +// whole shape of what `docker ps` can hand back, so the next mistake fails here +// rather than in the field, where a dropped line is a container nobody sees. +func TestJobContainerParsing(t *testing.T) { + for _, tc := range []struct { + name string + stdout string + want []jobContainer + }{ + {"nothing running", "", nil}, + {"blank output", "\n\n", nil}, + {"one container", "abc123def456 op-1 4\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"several", "abc123def456 op-1 4\nfed654cba321 op-2 9\n", + []jobContainer{{"abc123def456", "op-1", "4"}, {"fed654cba321", "op-2", "9"}}}, + // A label docker cannot resolve renders empty, and the separators stay. + {"no epoch label", "abc123def456 op-1 \n", []jobContainer{{"abc123def456", "op-1", ""}}}, + {"no operation label", "abc123def456 \n", []jobContainer{{"abc123def456", "", ""}}}, + {"no trailing separators", "abc123def456\n", []jobContainer{{"abc123def456", "", ""}}}, + {"padded line", " abc123def456 op-1 4\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"carriage return", "abc123def456 op-1 4\r\n", []jobContainer{{"abc123def456", "op-1", "4"}}}, + {"no trailing newline", "abc123def456 op-1 4", []jobContainer{{"abc123def456", "op-1", "4"}}}, + } { + t.Run(tc.name, func(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{Stdout: tc.stdout}, true + }} + got, err := jobContainerEngine(t, f).jobContainers(context.Background()) + if err != nil { + t.Fatalf("parse %q: %v", tc.stdout, err) + } + if len(got) != len(tc.want) { + t.Fatalf("parse %q = %+v, want %+v", tc.stdout, got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("parse %q [%d] = %+v, want %+v", tc.stdout, i, got[i], tc.want[i]) + } + } + }) + } +} + +// A daemon that cannot answer must stop the operation, not report an empty host. +func TestJobContainersRefusesAnUnusableAnswer(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{ExitCode: 1, Stderr: "Cannot connect to the Docker daemon"}, true + }} + if _, err := jobContainerEngine(t, f).jobContainers(context.Background()); err == nil { + t.Fatal("a failed docker ps must not read as no containers") + } + bad := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + return transport.Result{Stdout: "not-a-container-id op-1 4\n"}, true + }} + if _, err := jobContainerEngine(t, bad).jobContainers(context.Background()); err == nil { + t.Fatal("output that is not a container id must not be trusted") + } +} From ce08c79262987015f2ee46754657a05bb0799871 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 07:59:35 -0700 Subject: [PATCH 08/11] fix(jobs): say which of the two reasons kept the lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two different situations now hold the application lock on the way out: a run this process started was interrupted with its container still alive, and a run that never started because someone else's container is. They shared a boolean, so refusing printed the interrupted-run warning — telling an operator their run was interrupted when it had not begun, and pointing them at a container label that is not the one running. The flag is a reason now. Each path sets its own, the deferred release prints it, and an empty reason still means release. Both are pinned: removing either reason fails a test. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job.go | 25 ++++++++++++++++-------- internal/engine/job_test.go | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/internal/engine/job.go b/internal/engine/job.go index 58c56a50..75edad7f 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -49,13 +49,13 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // 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 + // A non-empty reason keeps the lock and says why. Two different situations + // hold it, and telling an operator the wrong one sends them looking for a + // run that never started. + holdLockReason := "" defer func() { - if holdLockForLiveContainer { - e.warnf("operation %s was interrupted while its container is still running; "+ - "keeping the application lock so nothing else mutates alongside it. "+ - "Inspect with `docker ps --filter label=%s=%s`; the lock expires on its own after %s", - operationID, JobOperationLabel, operationID, e.lockTTL()) + if holdLockReason != "" { + e.warnf("%s", holdLockReason) return } e.ReleaseLock(ctx) @@ -93,7 +93,10 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // mutator over a container this check has just established is alive — // the opposite of what refusing is for, and worse than not refusing, // because the lock reclaimed from the interrupted run would be gone too. - holdLockForLiveContainer = true + holdLockReason = fmt.Sprintf( + "nothing was run: %v. The application lock is being kept so nothing else "+ + "mutates alongside that container; it expires on its own after %s", + err, e.lockTTL()) return operationID, nil, err } @@ -186,7 +189,13 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) if interruptedRun(ctx, runErr) { // Cancelling the client kills at most the wrapper shell; the container // belongs to the daemon and keeps running. - holdLockForLiveContainer = e.jobContainerRunning(operationID) + if e.jobContainerRunning(operationID) { + holdLockReason = fmt.Sprintf( + "operation %s was interrupted while its container is still running; "+ + "keeping the application lock so nothing else mutates alongside it. "+ + "Inspect with `docker ps --filter label=%s=%s`; the lock expires on its own after %s", + operationID, JobOperationLabel, operationID, e.lockTTL()) + } } var result *journal.JobResultEvidence if evidence, ok := e.jobResults[job]; ok { diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index 5246d87f..819daad3 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -11,6 +11,13 @@ import ( ) func manualJobEngine(t *testing.T, target *transport.Fake) *Engine { + t.Helper() + return manualJobEngineTo(t, target, &bytes.Buffer{}) +} + +// manualJobEngineTo is the same engine with its narration captured, for the +// tests that assert what an operator is told. +func manualJobEngineTo(t *testing.T, target *transport.Fake, out *bytes.Buffer) *Engine { t.Helper() config := testConfig() job := config.Workloads["migrate"] @@ -18,7 +25,7 @@ func manualJobEngine(t *testing.T, target *transport.Fake) *Engine { job.DataEffect = "none" config.Workloads["migrate"] = job return New(config, testProject(t), target, Options{ - Out: &bytes.Buffer{}, Sleep: noSleep, + Out: out, Sleep: noSleep, ApprovalDigest: "approval-digest", ApprovalClass: "one_time", ApprovedBy: "operator@example.test", ApprovalSource: "local_cli", }) @@ -227,3 +234,32 @@ func TestRunJobKeepsTheLockWhenItRefuses(t *testing.T) { } } } + +// Both situations keep the lock, and each has to say which it is. Telling an +// operator their run was interrupted when it never started sends them looking +// for work that does not exist. +func TestRunJobExplainsWhyItKeptTheLock(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + var out bytes.Buffer + engine := manualJobEngineTo(t, target, &out) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("expected a refusal") + } + if s := out.String(); !strings.Contains(s, "nothing was run") { + t.Fatalf("refusal did not say the run never started:\n%s", s) + } + if s := out.String(); strings.Contains(s, "was interrupted while its container") { + t.Fatalf("refusal claimed this run was interrupted:\n%s", s) + } +} From 0efdc436d4678c63704ba22ef6e7b019e5a8ef98 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 08:14:15 -0700 Subject: [PATCH 09/11] fix(jobs): do not claim a container was found when the host could not be asked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refusal fires for two reasons — a job container is running, or `docker ps` could not be run or parsed — and the narration asserted the first for both. An operator whose daemon was unreachable was told something was mutating alongside a container that had never been observed. The lock is kept either way, and for the same reason: an unanswered question is not an answer of no. The message now says that instead of naming a finding it does not have. The error itself already carries which case it was. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job.go | 8 ++++++-- internal/engine/job_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/internal/engine/job.go b/internal/engine/job.go index 75edad7f..c5bfd64b 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -93,9 +93,13 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // mutator over a container this check has just established is alive — // the opposite of what refusing is for, and worse than not refusing, // because the lock reclaimed from the interrupted run would be gone too. + // Deliberately silent on what was found. This refuses both when a job + // container is running and when the host could not be asked, and the + // lock is kept for the same reason either way: an unanswered question + // is not an answer of no. holdLockReason = fmt.Sprintf( - "nothing was run: %v. The application lock is being kept so nothing else "+ - "mutates alongside that container; it expires on its own after %s", + "nothing was run: %v. The application lock is being kept until this is "+ + "resolved, so nothing else mutates meanwhile; it expires on its own after %s", err, e.lockTTL()) return operationID, nil, err } diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index 819daad3..67b2ba6f 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -263,3 +263,34 @@ func TestRunJobExplainsWhyItKeptTheLock(t *testing.T) { t.Fatalf("refusal claimed this run was interrupted:\n%s", s) } } + +// The refusal also fires when the host cannot be asked, and the lock is kept +// for the same reason: an unanswered question is not an answer of no. The +// narration must not claim a container was found in that case. +func TestRunJobKeepsTheLockWhenItCannotAskTheHost(t *testing.T) { + const runtime = "services:\n migrate:\n image: ghcr.io/x/app@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + target := currentJobFake(runtime) + inner := target.Dynamic + target.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{ExitCode: 1, Stderr: "Cannot connect to the Docker daemon"}, true + } + return inner(cmd) + } + var out bytes.Buffer + engine := manualJobEngineTo(t, target, &out) + if _, _, err := engine.RunJobWithJournalID(context.Background(), JobRunRequest{ + OperationID: "op-job-run", Job: "migrate", ExpectedRelease: engineTestPreviousReleaseID, + ExpectedRuntimeDigest: HashBytes([]byte(runtime)), ExpectedDataEffect: "none", + }); err == nil { + t.Fatal("an unanswerable host must refuse") + } + for _, c := range target.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released without an answer:\n%s", c) + } + } + if s := out.String(); strings.Contains(s, "alongside that container") { + t.Fatalf("narration claimed a container was found:\n%s", s) + } +} From 29f4bfab1a0cceb6a80035a67d777054e976608e Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 08:17:38 -0700 Subject: [PATCH 10/11] test(jobs): pin that exempting this run's container does not end the scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by mutating the loop rather than waiting to be told: replacing the `continue` with `return nil` passed the entire suite. A deploy runs its own gate jobs, so its own container is routinely the first one listed, and stopping there would step straight past a foreign container behind it — the exact overlap this check exists to catch, silently unguarded. The behaviour was already right; nothing held it there. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/job_containers_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/engine/job_containers_test.go b/internal/engine/job_containers_test.go index 22d8ffab..a37d8f25 100644 --- a/internal/engine/job_containers_test.go +++ b/internal/engine/job_containers_test.go @@ -155,3 +155,20 @@ func TestJobContainersRefusesAnUnusableAnswer(t *testing.T) { t.Fatal("output that is not a container id must not be trusted") } } + +// Exempting this run's own container must not end the scan. A deploy runs its +// own gate jobs, so its container is routinely listed first — and a foreign one +// behind it is exactly what this exists to catch. +func TestRefuseKeepsScanningPastItsOwnContainer(t *testing.T) { + f := jobContainerFake([]string{ + "aaa111bbb222 op-mine 3", + "ccc333ddd444 op-other 9", + }) + err := jobContainerEngine(t, f).refuseForeignJobContainers(context.Background(), "op-mine", 3) + if err == nil { + t.Fatal("a foreign container behind an exempt one must still refuse") + } + if !strings.Contains(err.Error(), "op-other") || !strings.Contains(err.Error(), "ccc333ddd444") { + t.Fatalf("refusal named the wrong container: %v", err) + } +} From 7fe1a3d7c524acb44191d3d8f022cbe66e69525a Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Thu, 10 Sep 2026 08:20:31 -0700 Subject: [PATCH 11/11] fix(deploy): tell the operator the lock is held, and describe both refusal cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deploy path kept the application lock on refusal and said nothing about it. An operator told only that the deploy stopped had no way to know the host was still held, or for how long. It now says so, in the same shape as the job path. Two comments corrected. The deploy one claimed the lock is kept when a live container is found — it is also kept when the host cannot be asked, which is the same inaccuracy fixed on the job path last commit and left here. The job one claimed the narration is silent about what was found, while the error it embeds may well name the container; what is true is that the sentence around it asserts nothing, and the finding travels in the error. Claude-Session: https://claude.ai/code/session_01JaxHfqFZk8GdrBNbtQZ6c2 --- internal/engine/deploy.go | 18 ++++++++++++------ internal/engine/deploy_test.go | 26 ++++++++++++++++++++++++++ internal/engine/job.go | 9 +++++---- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/internal/engine/deploy.go b/internal/engine/deploy.go index c954ed20..479fdc27 100644 --- a/internal/engine/deploy.go +++ b/internal/engine/deploy.go @@ -75,12 +75,15 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri if err != nil { return err } - // Released unless a live job container is found below: handing the host to - // the next mutator over a container this deploy refused to run alongside - // would defeat the refusal. - holdLockForLiveContainer := false + // A non-empty reason keeps the lock and says why. The check below refuses + // both when a job container is running and when the host could not be + // asked, and the lock is kept for the same reason either way: releasing it + // would hand the host to the next mutator over a state this deploy declined + // to proceed against. + holdLockReason := "" defer func() { - if holdLockForLiveContainer { + if holdLockReason != "" { + e.warnf("%s", holdLockReason) return } e.ReleaseLock(ctx) @@ -118,7 +121,10 @@ func (e *Engine) deployCore(ctx context.Context, releaseID, localStagingDir stri // exists for it rather than by a raw `docker ps` failure — and still before // any workload is rolled or any gate job runs. if err := e.refuseForeignJobContainers(ctx, releaseID, epoch); err != nil { - holdLockForLiveContainer = true + holdLockReason = fmt.Sprintf( + "nothing was deployed: %v. The application lock is being kept until this is "+ + "resolved, so nothing else mutates meanwhile; it expires on its own after %s", + err, e.lockTTL()) return err } rollbackDebt := false diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index 6b45e714..b12e5ef8 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -586,3 +586,29 @@ func TestDeployRefusesWhileAForeignJobContainerRuns(t *testing.T) { t.Fatalf("the deploy rolled anyway:\n%s", seq) } } + +// A refused deploy keeps the application lock, and has to say so: an operator +// who is told only that the deploy stopped will not know the host is still held. +func TestDeployKeepsAndExplainsTheLockWhenItRefuses(t *testing.T) { + f := happyFake() + inner := f.Dynamic + f.Dynamic = func(cmd string) (transport.Result, bool) { + if strings.Contains(cmd, "label='ob.operation'") { + return transport.Result{Stdout: "abc123def456 other-op 2\n"}, true + } + return inner(cmd) + } + var out bytes.Buffer + e := New(testConfig(), testProject(t), f, Options{Out: &out, Sleep: noSleep}) + if err := e.Deploy(context.Background(), "20260101-000000-aaa111", t.TempDir()); err == nil { + t.Fatal("expected a refusal") + } + for _, c := range f.Commands { + if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + t.Fatalf("the lock was released over a live container:\n%s", c) + } + } + if s := out.String(); !strings.Contains(s, "lock is being kept") { + t.Fatalf("the operator was not told the lock is held:\n%s", s) + } +} diff --git a/internal/engine/job.go b/internal/engine/job.go index c5bfd64b..c0e9240d 100644 --- a/internal/engine/job.go +++ b/internal/engine/job.go @@ -93,10 +93,11 @@ func (e *Engine) RunJobWithJournalID(ctx context.Context, request JobRunRequest) // mutator over a container this check has just established is alive — // the opposite of what refusing is for, and worse than not refusing, // because the lock reclaimed from the interrupted run would be gone too. - // Deliberately silent on what was found. This refuses both when a job - // container is running and when the host could not be asked, and the - // lock is kept for the same reason either way: an unanswered question - // is not an answer of no. + // The sentence around the error asserts nothing about what was found: + // this refuses both when a job container is running and when the host + // could not be asked, and the lock is kept for the same reason either + // way — an unanswered question is not an answer of no. What was + // actually determined travels in the error itself. holdLockReason = fmt.Sprintf( "nothing was run: %v. The application lock is being kept until this is "+ "resolved, so nothing else mutates meanwhile; it expires on its own after %s",