diff --git a/internal/engine/backup_postgres_ops.go b/internal/engine/backup_postgres_ops.go index c05e72a5..2170f814 100644 --- a/internal/engine/backup_postgres_ops.go +++ b/internal/engine/backup_postgres_ops.go @@ -496,6 +496,25 @@ func (e *Engine) hasFlock(ctx context.Context) bool { return e.flockPresent } +// hasScheduleFlock is deliberately stricter than hasFlock. Backup locking only +// needs the historical short options, while generated schedule units invoke +// /usr/bin/flock directly and need this complete util-linux long-option +// interface. Reject an incompatible host before installing units rather than +// discovering it when a timer fires. +func (e *Engine) hasScheduleFlock(ctx context.Context) bool { + if e.scheduleFlockProbed { + return e.scheduleFlockPresent + } + res, err := e.T.Run(ctx, scheduleFlockProbe("/usr/bin/flock")) + e.scheduleFlockProbed = true + e.scheduleFlockPresent = err == nil && strings.TrimSpace(res.Stdout) == "ok" + return e.scheduleFlockPresent +} + +func scheduleFlockProbe(path string) string { + return "command -v flock >/dev/null 2>&1 || exit; test -x " + q(path) + " || exit; help=$(" + q(path) + " --help 2>&1) || exit; for option in --conflict-exit-code --exclusive --nonblock --shared --timeout --unlock; do printf '%s\\n' \"$help\" | grep -q -- \"$option\" || exit; done; echo ok" +} + // walgLockPrefix is the flock every repository operation runs behind, as a // command prefix so callers that build their own docker exec can use it too. // Empty when the host has no flock — see hasFlock for why that is not a silent diff --git a/internal/engine/bootstrap_test.go b/internal/engine/bootstrap_test.go index afcebd4e..4aaddf62 100644 --- a/internal/engine/bootstrap_test.go +++ b/internal/engine/bootstrap_test.go @@ -40,7 +40,7 @@ func TestBootstrapSequence(t *testing.T) { seq := strings.Join(f.Commands, "\n") ordered := []string{ "mkdir -p", // dirs - "> '/var/lib/ob/sample/lock'", // application lock + `link "$tmp" '/var/lib/ob/sample/lock'`, // application lock "> '/var/lib/ob/sample/fence'", // mutation fence `"phase":"bootstrap","event":"start"`, // durable journal boundary "apt-get install -y something-host-specific", // bootstrap hook @@ -212,7 +212,7 @@ func TestBootstrapRefusesMissingRuntimeWithoutImplicitInstaller(t *testing.T) { if runtimeCheck < 0 { t.Fatalf("bootstrap did not check the runtime:\n%s", seq) } - for _, before := range []string{"> '/var/lib/ob/sample/lock'", "> '/var/lib/ob/sample/fence'", `"phase":"bootstrap","event":"start"`} { + for _, before := range []string{`link "$tmp" '/var/lib/ob/sample/lock'`, "> '/var/lib/ob/sample/fence'", `"phase":"bootstrap","event":"start"`} { if index := strings.Index(seq, before); index < 0 || index > runtimeCheck { t.Fatalf("%q did not precede the runtime check:\n%s", before, seq) } diff --git a/internal/engine/deploy_test.go b/internal/engine/deploy_test.go index b12e5ef8..d2e24cfd 100644 --- a/internal/engine/deploy_test.go +++ b/internal/engine/deploy_test.go @@ -604,7 +604,7 @@ func TestDeployKeepsAndExplainsTheLockWhenItRefuses(t *testing.T) { t.Fatal("expected a refusal") } for _, c := range f.Commands { - if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + if strings.Contains(c, "rm -f '/var/lib/ob/sample/lock'") { t.Fatalf("the lock was released over a live container:\n%s", c) } } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index b667aaae..6326aa71 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -88,8 +88,10 @@ type Engine struct { Spec *app.Resolved // flockProbed/flockPresent cache whether the target has flock, which every // wal-g invocation needs to know and which cannot change mid-operation. - flockProbed bool - flockPresent bool + flockProbed bool + flockPresent bool + scheduleFlockProbed bool + scheduleFlockPresent bool // triggerUnitProbed/triggerUnitPresent cache whether the host's systemd // tells a timer activation from a manual one (TRIGGER_UNIT, systemd 252). triggerUnitProbed bool diff --git a/internal/engine/epoch_test.go b/internal/engine/epoch_test.go index 764ac0de..1ee82e89 100644 --- a/internal/engine/epoch_test.go +++ b/internal/engine/epoch_test.go @@ -228,7 +228,8 @@ func assertEpochAcquisition(t *testing.T, fake *transport.Fake, got int, err err if err == nil { t.Fatal("invalid epoch was accepted") } - if strings.Contains(strings.Join(fake.Commands, "\n"), "set -C") { + commands := strings.Join(fake.Commands, "\n") + if strings.Contains(commands, "set -C") || strings.Contains(commands, "lock.candidate.XXXXXX") { t.Fatalf("lock was created after epoch validation failed:\n%s", strings.Join(fake.Commands, "\n")) } return diff --git a/internal/engine/job_test.go b/internal/engine/job_test.go index 67b2ba6f..51121816 100644 --- a/internal/engine/job_test.go +++ b/internal/engine/job_test.go @@ -85,7 +85,7 @@ func TestRunJobRejectsDeclarationDriftBeforeLock(t *testing.T) { if err == nil || !strings.Contains(err.Error(), test.want) { t.Fatalf("error = %v, want %q", err, test.want) } - if strings.Contains(strings.Join(target.Commands, "\n"), "set -C; echo") { + if strings.Contains(strings.Join(target.Commands, "\n"), "lock.candidate.XXXXXX") { t.Fatalf("declaration refusal acquired the app lock: %#v", target.Commands) } }) @@ -124,7 +124,7 @@ func TestRunJobRechecksReleaseAndRuntimeUnderLock(t *testing.T) { t.Fatalf("error = %v, want %q", err, test.want) } commands := strings.Join(target.Commands, "\n") - if !strings.Contains(commands, "set -C; echo") || !strings.Contains(commands, "/fence") { + if !strings.Contains(commands, "lock.candidate.XXXXXX") || !strings.Contains(commands, "/fence") { t.Fatalf("post-lock recheck was not lock/fence protected:\n%s", commands) } if strings.Contains(commands, "ONEBOX_RESULT_FILE") { @@ -229,7 +229,7 @@ func TestRunJobKeepsTheLockWhenItRefuses(t *testing.T) { t.Fatal("expected a refusal") } for _, c := range target.Commands { - if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + if strings.Contains(c, "rm -f '/var/lib/ob/sample/lock'") { t.Fatalf("the lock was released over a live container:\n%s", c) } } @@ -286,7 +286,7 @@ func TestRunJobKeepsTheLockWhenItCannotAskTheHost(t *testing.T) { t.Fatal("an unanswerable host must refuse") } for _, c := range target.Commands { - if strings.Contains(c, "rm -f") && strings.Contains(c, "/lock") { + if strings.Contains(c, "rm -f '/var/lib/ob/sample/lock'") { t.Fatalf("the lock was released without an answer:\n%s", c) } } diff --git a/internal/engine/lock.go b/internal/engine/lock.go index 088115bc..3055deac 100644 --- a/internal/engine/lock.go +++ b/internal/engine/lock.go @@ -36,6 +36,19 @@ type pinnedScheduleLeasePolicy struct { conflict string } +// scheduleRendezvousWaitSeconds lets a reader or writer already inside the +// short schedule/deploy handoff finish without turning ordinary concurrency +// into a missed firing or a refused operation. It does not wait for the +// durable application lock: that lock may cover a whole deploy or exec. +const scheduleRendezvousWaitSeconds = 10 + +// Commands run under flock normalize expected collisions so their exit status +// does not depend on shell or flock defaults. util-linux reserves 64–78 for its +// own errors; keep both sentinels above that range and distinct so +// infrastructure failures remain visible. +const applicationLockHeldExitCode = 79 +const flockConflictExitCode = 200 + func (e *Engine) base() string { return release.PathsFor(e.names()).Base } func (e *Engine) lockPath() string { return e.base() + "/lock" } func (e *Engine) epochPath() string { return e.base() + "/epoch" } @@ -76,15 +89,22 @@ func (e *Engine) acquireLock(ctx context.Context, deployID string, force bool, l TTLSeconds: int(e.lockTTL().Seconds()), AcquiredAt: time.Now().UTC().Format(time.RFC3339), } b, _ := json.Marshal(meta) - // noclobber: the remote shell refuses the redirect if the lock exists - create := "set -C; echo " + q(string(b)) + " > " + q(e.lockPath()) + " 2>/dev/null" + create := atomicApplicationLockCreateCmd(e.lockPath(), string(b)) jobs, scheduleErr := e.Spec.ScheduledJobs() if scheduleErr != nil { return 0, scheduleErr } - useScheduleLock := e.hasFlock(ctx) + useScheduleLock := e.hasScheduleFlock(ctx) + useLegacyScheduleLock := false + if !useScheduleLock { + // The current spec may have just removed its last schedule while an + // old unit is already starting. Preserve the pre-upgrade rendezvous + // with the short-option interface in that transition. Its ambiguous + // nonzero exits fail visibly below instead of being called contention. + useLegacyScheduleLock = e.hasFlock(ctx) + } if len(jobs) > 0 && !useScheduleLock { - return 0, errors.New("scheduled jobs require flock on the target so they cannot overlap deployments; install util-linux and deploy again") + return 0, errors.New("scheduled jobs require a compatible util-linux flock at /usr/bin/flock so lock contention can be distinguished from host failures; install util-linux or upgrade it and deploy again") } if useScheduleLock { // An exclusive scheduled job holds this kernel lock for its whole run; @@ -93,7 +113,10 @@ func (e *Engine) acquireLock(ctx context.Context, deployID string, force bool, l // pass its check before the other publishes ownership. Keep doing this // after the last schedule is removed: an old unit may already be // starting while that removal deploy begins. - create = "/usr/bin/flock --exclusive --nonblock --conflict-exit-code 76 " + + create = "/usr/bin/flock --exclusive --timeout " + strconv.Itoa(scheduleRendezvousWaitSeconds) + " --conflict-exit-code " + strconv.Itoa(flockConflictExitCode) + " " + + q(e.names().ScheduleRunLock()) + " /bin/sh -c " + q(create) + } else if useLegacyScheduleLock { + create = "/usr/bin/flock -x -w " + strconv.Itoa(scheduleRendezvousWaitSeconds) + " " + q(e.names().ScheduleRunLock()) + " /bin/sh -c " + q(create) } @@ -130,8 +153,15 @@ func (e *Engine) acquireLock(ctx context.Context, deployID string, force bool, l } return epoch, nil } - if res.ExitCode == 76 { - return 0, fmt.Errorf("deploy lock held by a scheduled job — wait for the job to finish") + if useScheduleLock && res.ExitCode == flockConflictExitCode { + return 0, fmt.Errorf("application scheduling rendezvous remained busy — wait for the current scheduled job or application operation to finish") + } + if res.ExitCode != applicationLockHeldExitCode { + detail := strings.TrimSpace(res.Stderr) + if detail == "" { + detail = "no diagnostic output" + } + return 0, fmt.Errorf("acquire application lock: lock creation or schedule rendezvous failed (exit %d): %s", res.ExitCode, detail) } // held — inspect holder + age hres, err := e.T.Run(ctx, "cat "+q(e.lockPath())+" 2>/dev/null || true") @@ -180,6 +210,22 @@ func (e *Engine) acquireLock(ctx context.Context, deployID string, force bool, l return 0, fmt.Errorf("could not acquire deploy lock") } +// atomicApplicationLockCreateCmd writes complete metadata before publishing +// the lock path. A noclobber redirect can create an empty lock before its write +// fails (for example on ENOSPC), which makes an infrastructure error look like +// contention. A same-directory hard link is an atomic no-replace claim, and +// removing the temporary name leaves the claimed inode at lockPath. +func atomicApplicationLockCreateCmd(lockPath, value string) string { + tmpPattern := lockPath + ".candidate.XXXXXX" + return "umask 077; tmp=$(mktemp " + q(tmpPattern) + ") || exit 80; " + + "cleanup() { rm -f \"$tmp\" || true; }; trap cleanup 0; trap 'exit 129' 1; trap 'exit 130' 2; trap 'exit 143' 15; " + + "printf '%s\\n' " + q(value) + " >\"$tmp\" || exit 80; " + + // Unlike ln, the POSIX link utility treats its second operand as the + // exact new path even when that path names a directory. + "if link \"$tmp\" " + q(lockPath) + "; then exit 0; fi; " + + "{ [ -e " + q(lockPath) + " ] || [ -L " + q(lockPath) + " ]; } && exit " + strconv.Itoa(applicationLockHeldExitCode) + "; exit 80" +} + func (e *Engine) ReleaseLock(ctx context.Context) { if e.lockVal == "" { return diff --git a/internal/engine/lock_test.go b/internal/engine/lock_test.go index 72f80e9a..1474c752 100644 --- a/internal/engine/lock_test.go +++ b/internal/engine/lock_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "os" + "path/filepath" "strconv" "strings" "testing" @@ -37,8 +38,8 @@ func TestAcquireLockHappyPath(t *testing.T) { t.Fatalf("epoch: %d", epoch) } seq := strings.Join(f.Commands, "\n") - if !strings.Contains(seq, "set -C") || !strings.Contains(seq, "/var/lib/ob/sample/lock") { - t.Fatalf("noclobber lock creation missing:\n%s", seq) + if !strings.Contains(seq, "lock.candidate.XXXXXX") || !strings.Contains(seq, `link "$tmp" '/var/lib/ob/sample/lock'`) { + t.Fatalf("atomic lock publication missing:\n%s", seq) } if !strings.Contains(seq, "mktemp '/var/lib/ob/sample/epoch.tmp.XXXXXX'") || !strings.Contains(seq, "printf '%s\\n' 7") || @@ -47,6 +48,92 @@ func TestAcquireLockHappyPath(t *testing.T) { } } +func TestAtomicApplicationLockCreatePublishesOnlyCompleteMetadata(t *testing.T) { + ctx := context.Background() + target := transport.NewLocal() + root := t.TempDir() + lock := filepath.Join(root, "lock") + + result, err := target.Run(ctx, atomicApplicationLockCreateCmd(lock, "complete metadata")) + if err != nil || result.ExitCode != 0 { + t.Fatalf("publish lock: result=%+v err=%v", result, err) + } + content, err := os.ReadFile(lock) + if err != nil || string(content) != "complete metadata\n" { + t.Fatalf("published lock = %q, %v", content, err) + } + if candidates, err := filepath.Glob(lock + ".candidate.*"); err != nil || len(candidates) != 0 { + t.Fatalf("temporary lock candidates leaked: %v, %v", candidates, err) + } + + result, err = target.Run(ctx, atomicApplicationLockCreateCmd(lock, "replacement")) + if err != nil || result.ExitCode != applicationLockHeldExitCode { + t.Fatalf("existing lock result=%+v err=%v, want exit %d", result, err, applicationLockHeldExitCode) + } + content, err = os.ReadFile(lock) + if err != nil || string(content) != "complete metadata\n" { + t.Fatalf("existing lock was replaced: %q, %v", content, err) + } + + failedLock := filepath.Join(root, "failed-lock") + unwritableCandidate := filepath.Join(root, "candidate-is-a-directory") + if err := os.Mkdir(unwritableCandidate, 0o700); err != nil { + t.Fatal(err) + } + // Override mktemp so the metadata redirect fails before the hard-link + // claim. The shared lock path must remain absent and the error must stay an + // infrastructure failure, not applicationLockHeldExitCode. + command := "mktemp() { printf '%s\\n' " + q(unwritableCandidate) + "; }; " + + atomicApplicationLockCreateCmd(failedLock, "never published") + result, err = target.Run(ctx, command) + if err != nil || result.ExitCode != 80 { + t.Fatalf("failed metadata write result=%+v err=%v, want exit 80", result, err) + } + if _, err := os.Lstat(failedLock); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("failed metadata write published a lock: %v", err) + } + + for _, tc := range []struct { + name string + symlink bool + }{ + {name: "directory"}, + {name: "directory symlink", symlink: true}, + } { + t.Run(tc.name, func(t *testing.T) { + targetDir := filepath.Join(root, strings.ReplaceAll(tc.name, " ", "-"), "target") + if err := os.MkdirAll(targetDir, 0o700); err != nil { + t.Fatal(err) + } + lockPath := targetDir + if tc.symlink { + lockPath = filepath.Join(root, "directory-link") + if err := os.Symlink(targetDir, lockPath); err != nil { + t.Fatal(err) + } + } + result, err := target.Run(ctx, atomicApplicationLockCreateCmd(lockPath, "not published")) + if err != nil || result.ExitCode != applicationLockHeldExitCode { + t.Fatalf("directory lock result=%+v err=%v, want exit %d", result, err, applicationLockHeldExitCode) + } + entries, err := os.ReadDir(targetDir) + if err != nil || len(entries) != 0 { + t.Fatalf("hard link was created inside directory lock: entries=%v err=%v", entries, err) + } + }) + } + + interruptedLock := filepath.Join(root, "interrupted-lock") + command = "link() { kill -HUP $$; return 0; }; " + atomicApplicationLockCreateCmd(interruptedLock, "not published") + result, err = target.Run(ctx, command) + if err != nil || result.ExitCode != 129 { + t.Fatalf("interrupted claim result=%+v err=%v, want exit 129", result, err) + } + if _, err := os.Lstat(interruptedLock); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("interrupted claim published a lock: %v", err) + } +} + func TestAcquireLockSerializesWithScheduledJobs(t *testing.T) { cfg := testConfig() cfg.Workloads["nightly"] = app.Workload{ @@ -58,21 +145,65 @@ func TestAcquireLockSerializesWithScheduledJobs(t *testing.T) { case strings.Contains(cmd, "command -v flock"): return transport.Result{Stdout: "ok\n"}, true case strings.Contains(cmd, "/usr/bin/flock"): - return transport.Result{ExitCode: 76}, true + return transport.Result{ExitCode: flockConflictExitCode}, true } return transport.Result{}, false }} e := New(cfg, testProject(t), f, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) _, err := e.AcquireLock(context.Background(), "R9", false) - if err == nil || !strings.Contains(err.Error(), "scheduled job") { - t.Fatalf("error = %v, want scheduled-job contention", err) + if err == nil || !strings.Contains(err.Error(), "scheduled job or application operation") { + t.Fatalf("error = %v, want schedule-rendezvous contention", err) } seq := strings.Join(f.Commands, "\n") - if !strings.Contains(seq, cfg.NamesFor("production").ScheduleRunLock()) || !strings.Contains(seq, "--conflict-exit-code 76") { + if !strings.Contains(seq, cfg.NamesFor("production").ScheduleRunLock()) || + !strings.Contains(seq, "--exclusive --timeout 10 --conflict-exit-code 200") { t.Fatalf("application lock was not created under the schedule mutex:\n%s", seq) } } +func TestAcquireLockReportsScheduleRendezvousFailure(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "command -v flock"): + return transport.Result{Stdout: "ok\n"}, true + case strings.Contains(cmd, "/usr/bin/flock"): + return transport.Result{ExitCode: 74, Stderr: "flock: I/O error\n"}, true + } + return transport.Result{}, false + }} + e := lockEngine(t, f) + _, err := e.AcquireLock(context.Background(), "R9", false) + if err == nil || !strings.Contains(err.Error(), "lock creation or schedule rendezvous failed (exit 74): flock: I/O error") { + t.Fatalf("error = %v, want preserved flock failure", err) + } + if strings.Contains(strings.Join(f.Commands, "\n"), "cat '/var/lib/ob/sample/lock'") { + t.Fatalf("infrastructure failure was treated as a held application lock:\n%s", strings.Join(f.Commands, "\n")) + } +} + +func TestAcquireLockKeepsLegacyRendezvousAfterLastScheduleIsRemoved(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + switch { + case strings.Contains(cmd, "command -v flock") && strings.Contains(cmd, "--conflict-exit-code"): + return transport.Result{}, true // flock exists, but lacks the strict schedule interface + case strings.Contains(cmd, "command -v flock"): + return transport.Result{Stdout: "ok\n"}, true + case strings.Contains(cmd, "/usr/bin/flock -x -w 10"): + return transport.Result{ExitCode: 1, Stderr: "legacy rendezvous unavailable\n"}, true + } + return transport.Result{}, false + }} + e := lockEngine(t, f) // no jobs in the current spec + _, err := e.AcquireLock(context.Background(), "R9", false) + if err == nil || !strings.Contains(err.Error(), "legacy rendezvous unavailable") { + t.Fatalf("legacy schedule rendezvous failure was not preserved: %v", err) + } + sequence := strings.Join(f.Commands, "\n") + if !strings.Contains(sequence, "/usr/bin/flock -x -w 10") || strings.Contains(sequence, "/usr/bin/flock --exclusive --timeout 10 --conflict-exit-code 200") { + t.Fatalf("last-schedule transition did not use the legacy-compatible rendezvous:\n%s", sequence) + } +} + func TestReleaseLockRemovesOnlyOwnedToken(t *testing.T) { f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { if strings.Contains(cmd, "cat '/var/lib/ob/sample/epoch'") { @@ -99,8 +230,8 @@ func TestReleaseLockRemovesOnlyOwnedToken(t *testing.T) { func TestAcquireLockHeldFreshRefuses(t *testing.T) { f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { - if strings.Contains(cmd, "set -C") { - return transport.Result{ExitCode: 1, Stderr: "cannot overwrite"}, true + if strings.Contains(cmd, "lock.candidate.XXXXXX") { + return transport.Result{ExitCode: applicationLockHeldExitCode, Stderr: "cannot overwrite"}, true } if strings.Contains(cmd, "cat '/var/lib/ob/sample/lock'") { return transport.Result{Stdout: `{"owner":"alice@laptop","deploy_id":"R8","epoch":6}`}, true @@ -121,10 +252,10 @@ func TestAcquireLockStaleTTLTakesOver(t *testing.T) { creates := 0 f := &transport.Fake{} f.Dynamic = func(cmd string) (transport.Result, bool) { - if strings.Contains(cmd, "set -C") { + if strings.Contains(cmd, "lock.candidate.XXXXXX") { creates++ if creates == 1 { - return transport.Result{ExitCode: 1}, true + return transport.Result{ExitCode: applicationLockHeldExitCode}, true } return transport.Result{}, true } @@ -149,10 +280,10 @@ func TestAcquireLockSameDeployReclaims(t *testing.T) { creates := 0 f := &transport.Fake{} f.Dynamic = func(cmd string) (transport.Result, bool) { - if strings.Contains(cmd, "set -C") { + if strings.Contains(cmd, "lock.candidate.XXXXXX") { creates++ if creates == 1 { - return transport.Result{ExitCode: 1}, true + return transport.Result{ExitCode: applicationLockHeldExitCode}, true } return transport.Result{}, true } @@ -191,10 +322,10 @@ func TestAcquireLockReReadsEpochAfterBreakingStaleLock(t *testing.T) { return transport.Result{Stdout: "5\n"}, true // stale holder's value } return transport.Result{Stdout: "6\n"}, true // advanced by a concurrent winner before our retry - case strings.Contains(cmd, "set -C"): + case strings.Contains(cmd, "lock.candidate.XXXXXX"): creates++ if creates == 1 { - return transport.Result{ExitCode: 1}, true // held → forces a break + retry + return transport.Result{ExitCode: applicationLockHeldExitCode}, true // held → forces a break + retry } return transport.Result{}, true // win on retry case strings.Contains(cmd, "cat '/var/lib/ob/sample/lock'"): @@ -368,10 +499,10 @@ func TestForceBreakPrintsHolderJournalTail(t *testing.T) { creates := 0 f := &transport.Fake{} f.Dynamic = func(cmd string) (transport.Result, bool) { - if strings.Contains(cmd, "set -C") { + if strings.Contains(cmd, "lock.candidate.XXXXXX") { creates++ if creates == 1 { - return transport.Result{ExitCode: 1}, true + return transport.Result{ExitCode: applicationLockHeldExitCode}, true } return transport.Result{}, true } diff --git a/internal/engine/schedule.go b/internal/engine/schedule.go index c6db5325..54e8523b 100644 --- a/internal/engine/schedule.go +++ b/internal/engine/schedule.go @@ -251,7 +251,7 @@ func scheduleRunnerScript(application string, job app.ScheduledJob, names app.Na "install -d -m 700 " + q(names.AppDir()+"/schedule"), } lines = append(lines, scheduleInputsLines(names.ScheduledJobRunInputs(job.Name))...) - lines = append(lines, scheduleLockLines(names, job.Name, applicationLock, lockTTL)...) + lines = append(lines, scheduleLockLines(names, job.Name, job.DeployLock, applicationLock, lockTTL, scheduleRendezvousWait(job.Timeout))...) lines = append(lines, // Best effort: the record names the release that ran, and an exclusive // job runs whatever `current` points at when it starts. @@ -287,7 +287,7 @@ func pinnedScheduleRunnerScript(application string, job app.ScheduledJob, names "install -d -m 700 " + q(scheduleDir), } lines = append(lines, scheduleInputsLines(names.ScheduledJobRunInputs(job.Name))...) - lines = append(lines, scheduleLockLines(names, job.Name, applicationLock, lockTTL)...) + lines = append(lines, scheduleLockLines(names, job.Name, job.DeployLock, applicationLock, lockTTL, scheduleRendezvousWait(job.Timeout))...) lines = append(lines, // These are misconfigurations, not timing: the run fails, loudly. "release_dir=$(readlink -f "+q(names.CurrentLink())+") || { echo 'onebox: current release cannot be resolved' >&2; exit 1; }", @@ -298,6 +298,10 @@ func pinnedScheduleRunnerScript(application string, job app.ScheduledJob, names "exec 7>>\"$release_dir/.ob-schedule.lease\"", "chmod 600 \"$release_dir/.ob-schedule.lease\"", "/usr/bin/flock --shared 7", + // The immutable release is leased, so the writer rendezvous is complete. + // Container cleanup and state bookkeeping are per-job work and must not + // keep an application operation waiting behind them. + "/usr/bin/flock --unlock 8", scheduleContainerCleanup(container), "cleanup() { "+scheduleContainerCleanup(container)+"; rm -f \"$tmp\"; }", "trap cleanup 0", @@ -306,9 +310,6 @@ func pinnedScheduleRunnerScript(application string, job app.ScheduledJob, names "trap 'exit 143' 15", ) lines = append(lines, scheduleRunPreamble(triggerUnit)...) - // The lease is held; the schedule mutex goes back before the first - // attempt so a compatible deploy is not blocked through the backoff. - lines = append(lines, "/usr/bin/flock --unlock 8") lines = append(lines, scheduleAttemptLoop(job, compose, container)...) lines = append(lines, "") return strings.Join(lines, "\n") @@ -327,11 +328,28 @@ func pinnedScheduleRunnerScript(application string, job app.ScheduledJob, names // takes it over, so the timer must not defer to it forever either. The age // comes from the same shell AcquireLock reads it with, in whole seconds, and // that shell fails closed: an unreadable lock reads as fresh. -func scheduleLockLines(names app.Names, job, applicationLock string, lockTTL time.Duration) []string { +func scheduleLockLines(names app.Names, job, deployLock, applicationLock string, lockTTL, rendezvousWait time.Duration) []string { ttlSeconds := int(math.Ceil(lockTTL.Seconds())) if ttlSeconds < 1 { ttlSeconds = 1 } + waitSeconds := strconv.FormatFloat(rendezvousWait.Seconds(), 'f', -1, 64) + waitMode := "--timeout " + waitSeconds + busyReason := "the application scheduling lock is busy" + if rendezvousWait > 0 { + busyReason = "the application scheduling lock remained busy for " + rendezvousWait.String() + } else { + // util-linux documents --timeout 0 as equivalent to --nonblock, but + // spelling the mode explicitly makes the zero-budget contract clear. + waitMode = "--nonblock" + } + rendezvousMode := "--exclusive" + if deployLock == "pinned" { + // Pinned jobs only need to exclude writers while they establish their + // immutable release leases. Different pinned jobs are readers of the + // same release state and may safely enter together. + rendezvousMode = "--shared" + } return []string{ "state=" + q(names.ScheduledJobRunState(job)), "tmp=\"$state.$$\"", @@ -353,18 +371,38 @@ func scheduleLockLines(names app.Names, job, applicationLock string, lockTTL tim "skip_marker=\"$state.skip.${INVOCATION_ID:-}\"", "stand_aside() { umask 077; printf 'skipped=%s\\noperation=%s\\ninputs=%s\\n' \"$1\" \"$operation\" \"$inputs_json\" >\"$skip_marker\"; echo \"onebox: skipped: $1\" >&2; exit 0; }", "exec 9>" + q(names.ScheduledJobRunLock(job)), - "/usr/bin/flock --exclusive --nonblock 9 || stand_aside 'another run of this job is still in progress'", + "lock_code=0; /usr/bin/flock --exclusive --nonblock --conflict-exit-code " + strconv.Itoa(flockConflictExitCode) + " 9 || lock_code=$?; case $lock_code in 0) ;; " + strconv.Itoa(flockConflictExitCode) + ") stand_aside 'another run of this job is still in progress' ;; *) echo 'onebox: cannot acquire the scheduled-job lock' >&2; exit \"$lock_code\" ;; esac", // Only the activation that wrote a note removes it, so one lost // between the runner exiting and ExecStopPost — a power cut, a killed // systemd — would sit here forever. Swept a day later, under the job // lock, which is long past any live note's few milliseconds. "find " + q(names.AppDir()+"/schedule") + " -maxdepth 1 -name " + q(job+".state.skip.*") + " -mtime +1 -delete 2>/dev/null || true", "exec 8>" + q(names.ScheduleRunLock()), - "/usr/bin/flock --exclusive --nonblock 8 || skip 'an application operation is taking its lock'", + "lock_code=0; /usr/bin/flock " + rendezvousMode + " " + waitMode + " --conflict-exit-code " + strconv.Itoa(flockConflictExitCode) + " 8 || lock_code=$?; case $lock_code in 0) ;; " + strconv.Itoa(flockConflictExitCode) + ") skip " + q(busyReason) + " ;; *) echo 'onebox: cannot acquire the application scheduling lock' >&2; exit \"$lock_code\" ;; esac", "if [ -e " + q(applicationLock) + " ] && [ \"$(" + lockAgeCmd(applicationLock) + ")\" -le " + strconv.Itoa(ttlSeconds) + " ]; then skip 'an application operation holds the deploy lock'; fi", } } +// scheduleRendezvousWait keeps the ordinary ten-second handoff without letting +// it consume a short job's entire systemd TimeoutStartSec. A second is reserved +// for the runner to record a contention skip and exit; sub-second jobs therefore +// use a non-blocking rendezvous rather than being killed while waiting. +func scheduleRendezvousWait(jobTimeout string) time.Duration { + wait := time.Duration(scheduleRendezvousWaitSeconds) * time.Second + timeout, ok := app.ParseDuration(jobTimeout) + if !ok || timeout <= 0 { + return wait + } + const exitReserve = time.Second + if timeout <= exitReserve { + return 0 + } + if available := timeout - exitReserve; available < wait { + return available + } + return wait +} + // requireScheduleHost is what a host needs before any scheduled job can be // installed on it. Preflight asks it so a deploy refuses before staging, and // SyncSchedules asks again so `ob schedule apply` cannot bypass it. @@ -377,8 +415,8 @@ func (e *Engine) requireScheduleHost(ctx context.Context, jobs []app.ScheduledJo if len(jobs) == 0 { return nil } - if !e.hasFlock(ctx) { - return errors.New("scheduled jobs require flock on the target so they cannot overlap deployments; install util-linux and deploy again") + if !e.hasScheduleFlock(ctx) { + return errors.New("scheduled jobs require a compatible util-linux flock at /usr/bin/flock so lock contention can be distinguished from host failures; install util-linux or upgrade it and deploy again") } for _, job := range jobs { if job.Execution == nil { @@ -814,6 +852,10 @@ func scheduleTimerUnit(application string, job app.ScheduledJob) string { // the host's zone, so a job declared for 02:00 Europe/Berlin runs at // 02:00 UTC and nothing anywhere says so. "OnCalendar=" + calendarExpr(job), + // systemd's one-minute default deliberately coalesces local timers. + // Five-field cron already chooses the minute; keep that staggering + // instead of bunching unrelated jobs at one host-wide wake-up. + "AccuracySec=1s", // A box that was off at 2am still runs the job when it comes back, // which is the behaviour anyone declaring a nightly job expects. fmt.Sprintf("Persistent=%t", job.CatchUp), diff --git a/internal/engine/schedule_execution.go b/internal/engine/schedule_execution.go index 07339493..f28c0737 100644 --- a/internal/engine/schedule_execution.go +++ b/internal/engine/schedule_execution.go @@ -120,7 +120,7 @@ func (e *Engine) durableScheduleRunner(job app.ScheduledJob, envFiles []app.EnvF helper := "/usr/bin/python3 " + q(durable.Helper(n.AppDir())) lines := []string{"#!/bin/sh", "# Written by Onebox. Durable execution protocol v1.", "set -eu", "install -d -m 700 " + q(n.AppDir()+"/schedule")} lines = append(lines, scheduleInputsLines(n.ScheduledJobRunInputs(job.Name))...) - lines = append(lines, scheduleLockLines(n, job.Name, e.lockPath(), e.lockTTL())...) + lines = append(lines, scheduleLockLines(n, job.Name, job.DeployLock, e.lockPath(), e.lockTTL(), scheduleRendezvousWait(job.Timeout))...) lines = append(lines, "release_dir=$(readlink -f "+q(n.CurrentLink())+")", "release=${release_dir##*/}", diff --git a/internal/engine/schedule_execution_test.go b/internal/engine/schedule_execution_test.go index 33bbc4cf..67fcbf15 100644 --- a/internal/engine/schedule_execution_test.go +++ b/internal/engine/schedule_execution_test.go @@ -105,6 +105,50 @@ func TestDurablePythonRequirementDoesNotAffectOrdinarySchedules(t *testing.T) { } } +func TestScheduleHostRejectsIncompatibleFlockBeforeInstall(t *testing.T) { + f := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "command -v flock") { + for _, option := range []string{"--conflict-exit-code", "--exclusive", "--nonblock", "--shared", "--timeout", "--unlock"} { + if !strings.Contains(command, option) { + t.Fatalf("flock compatibility probe does not require %s: %s", option, command) + } + } + return transport.Result{ExitCode: 1}, true + } + return transport.Result{}, false + }} + e := New(testConfig(), testProject(t), f, Options{Out: &bytes.Buffer{}}) + err := e.requireScheduleHost(context.Background(), []app.ScheduledJob{{Name: "refresh"}}) + if err == nil || !strings.Contains(err.Error(), "compatible util-linux flock") { + t.Fatalf("incompatible flock accepted: %v", err) + } +} + +func TestScheduleFlockProbeExecutesCapabilityChecks(t *testing.T) { + root := t.TempDir() + flock := filepath.Join(root, "flock") + writeHelp := func(options string) { + t.Helper() + if err := os.WriteFile(flock, []byte("#!/bin/sh\nprintf '%s\\n' "+q(options)+"\n"), 0o700); err != nil { + t.Fatal(err) + } + } + run := func() ([]byte, error) { + command := exec.CommandContext(t.Context(), "sh", "-c", scheduleFlockProbe(flock)) + command.Env = append(os.Environ(), "PATH="+root+":/usr/bin:/bin") + return command.CombinedOutput() + } + + writeHelp("--conflict-exit-code --exclusive --nonblock --shared --timeout --unlock") + if output, err := run(); err != nil || string(output) != "ok\n" { + t.Fatalf("compatible flock rejected: err=%v output=%q", err, output) + } + writeHelp("--conflict-exit-code --exclusive --nonblock --shared --timeout") + if output, err := run(); err == nil || strings.Contains(string(output), "ok") { + t.Fatalf("flock missing --unlock accepted: err=%v output=%q", err, output) + } +} + func TestDurableResumeRefusesLegacyRunnerBeforePublishingRequest(t *testing.T) { cfg := testConfig() cfg.Workloads["refresh"] = app.Workload{Role: app.RoleJob, When: "manual", DataEffect: app.DataEffectNone, diff --git a/internal/engine/schedule_test.go b/internal/engine/schedule_test.go index 3f0aeca1..da69f9e4 100644 --- a/internal/engine/schedule_test.go +++ b/internal/engine/schedule_test.go @@ -4,11 +4,13 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "os/exec" "path/filepath" "regexp" "runtime" + "strconv" "strings" "testing" "time" @@ -188,9 +190,9 @@ func TestScheduledJobUnitContract(t *testing.T) { for _, want := range []string{ "exec 9>'/var/lib/ob/sample/schedule/nightly.lock'", - "flock --exclusive --nonblock 9 || stand_aside", + "flock --exclusive --nonblock --conflict-exit-code 200 9", "exec 8>'/var/lib/ob/sample/schedule.lock'", - "flock --exclusive --nonblock 8 || skip", + "flock --exclusive --timeout 10 --conflict-exit-code 200 8", "/var/lib/ob/sample/lock", "application operation holds the deploy lock", "docker compose", @@ -222,6 +224,7 @@ func TestScheduledJobUnitContract(t *testing.T) { } for _, want := range []string{ "OnCalendar=*-*-* 02:00:00 UTC", + "AccuracySec=1s", "Persistent=false", "WantedBy=timers.target", } { @@ -236,6 +239,30 @@ func TestScheduledJobUnitContract(t *testing.T) { } } +func TestScheduleRendezvousWaitReservesShortJobTimeout(t *testing.T) { + for _, tc := range []struct { + timeout string + want time.Duration + }{ + {timeout: "", want: 10 * time.Second}, + {timeout: "30s", want: 10 * time.Second}, + {timeout: "5s", want: 4 * time.Second}, + {timeout: "1s", want: 0}, + {timeout: "500ms", want: 0}, + } { + if got := scheduleRendezvousWait(tc.timeout); got != tc.want { + t.Errorf("scheduleRendezvousWait(%q) = %s, want %s", tc.timeout, got, tc.want) + } + } + + names := app.Names{App: "sample", BasePath: "/var/lib/ob"} + runner := scheduleRunnerScript("sample", app.ScheduledJob{Name: "quick", Timeout: "1s", DeployLock: "pinned"}, names, "/var/lib/ob/sample/lock", nil, 10*time.Minute, true) + if !strings.Contains(runner, "flock --shared --nonblock --conflict-exit-code 200 8") || + !strings.Contains(runner, "skip 'the application scheduling lock is busy'") { + t.Fatalf("short-timeout runner can outlive its rendezvous budget:\n%s", runner) + } +} + func TestPinnedScheduledJobRunnerLeasesImmutableRelease(t *testing.T) { job := app.ScheduledJob{Name: "refresh", DeployLock: "pinned"} names := app.Names{App: "sample", BasePath: "/var/lib/ob"} @@ -246,8 +273,9 @@ func TestPinnedScheduledJobRunnerLeasesImmutableRelease(t *testing.T) { for _, want := range []string{ "exec 9>'/var/lib/ob/sample/schedule/refresh.lock'", - "flock --exclusive --nonblock 9 || stand_aside", + "flock --exclusive --nonblock --conflict-exit-code 200 9", "exec 8>'/var/lib/ob/sample/schedule.lock'", + "flock --shared --timeout 10 --conflict-exit-code 200 8", "release_dir=$(readlink -f '/var/lib/ob/sample/current')", "exec 7>>\"$release_dir/.ob-schedule.lease\"", "flock --shared 7", @@ -271,6 +299,11 @@ func TestPinnedScheduledJobRunnerLeasesImmutableRelease(t *testing.T) { if strings.Contains(runner, "secrets/runtime.env") { t.Fatalf("encrypted env file was passed as a Compose interpolation input:\n%s", runner) } + unlock := strings.Index(runner, "flock --unlock 8") + cleanup := strings.Index(runner, "docker rm -f") + if unlock < 0 || cleanup < 0 || unlock > cleanup { + t.Fatalf("pinned runner kept the application rendezvous through per-job cleanup:\n%s", runner) + } command := exec.CommandContext(context.Background(), "sh", "-n") command.Stdin = strings.NewReader(runner) if output, err := command.CombinedOutput(); err != nil { @@ -278,6 +311,234 @@ func TestPinnedScheduledJobRunnerLeasesImmutableRelease(t *testing.T) { } } +func TestPinnedScheduledJobsShareApplicationRendezvous(t *testing.T) { + requireUtilLinuxFlock(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + root := t.TempDir() + names := app.Names{App: "sample", BasePath: root} + if err := os.MkdirAll(filepath.Join(names.AppDir(), "schedule"), 0o700); err != nil { + t.Fatal(err) + } + gate := filepath.Join(root, "release-readers") + var commands []*exec.Cmd + for _, job := range []string{"first", "second"} { + ready := filepath.Join(root, job+"-ready") + lines := []string{"set -eu", "operation=''", "inputs_json=''", "INVOCATION_ID=" + job} + lines = append(lines, scheduleLockLines(names, job, "pinned", filepath.Join(names.AppDir(), "lock"), 10*time.Minute, 10*time.Second)...) + lines = append(lines, + "touch "+q(ready), + "while [ ! -e "+q(gate)+" ]; do sleep 0.01; done", + ) + command := exec.CommandContext(ctx, "sh") + command.Stdin = strings.NewReader(strings.Join(lines, "\n")) + if err := command.Start(); err != nil { + t.Fatal(err) + } + commands = append(commands, command) + } + defer func() { + _ = os.WriteFile(gate, nil, 0o600) + for _, command := range commands { + if command.ProcessState == nil { + _ = command.Process.Kill() + _ = command.Wait() + } + } + }() + + deadline := time.Now().Add(3 * time.Second) + for _, job := range []string{"first", "second"} { + ready := filepath.Join(root, job+"-ready") + for { + if _, err := os.Stat(ready); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("different pinned jobs did not enter the application rendezvous together") + } + time.Sleep(10 * time.Millisecond) + } + } + err := exec.CommandContext(ctx, "/usr/bin/flock", "--exclusive", "--nonblock", "--conflict-exit-code", strconv.Itoa(flockConflictExitCode), names.ScheduleRunLock(), "true").Run() + var conflict *exec.ExitError + if !errors.As(err, &conflict) || conflict.ExitCode() != flockConflictExitCode { + t.Fatalf("exclusive writer result while pinned readers held the rendezvous = %v, want exit %d", err, flockConflictExitCode) + } + if err := os.WriteFile(gate, nil, 0o600); err != nil { + t.Fatal(err) + } + for _, command := range commands { + if err := command.Wait(); err != nil { + t.Fatal(err) + } + } + if err := exec.CommandContext(ctx, "/usr/bin/flock", "--exclusive", "--nonblock", names.ScheduleRunLock(), "true").Run(); err != nil { + t.Fatalf("application rendezvous remained locked after pinned readers exited: %v", err) + } +} + +func TestScheduledJobApplicationRendezvousWaitsForShortWriter(t *testing.T) { + requireUtilLinuxFlock(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + root := t.TempDir() + names := app.Names{App: "sample", BasePath: root} + if err := os.MkdirAll(filepath.Join(names.AppDir(), "schedule"), 0o700); err != nil { + t.Fatal(err) + } + holder, gate := startExclusiveScheduleHolder(t, ctx, names.ScheduleRunLock(), root) + defer stopExclusiveScheduleHolder(holder, gate) + + entered := filepath.Join(root, "reader-entered") + lines := []string{"set -eu", "operation=''", "inputs_json=''", "INVOCATION_ID=wait-reader"} + lines = append(lines, scheduleLockLines(names, "reader", "pinned", filepath.Join(names.AppDir(), "lock"), 10*time.Minute, 10*time.Second)...) + lines = append(lines, "touch "+q(entered)) + command := exec.CommandContext(ctx, "sh") + command.Stdin = strings.NewReader(strings.Join(lines, "\n")) + var output bytes.Buffer + command.Stdout = &output + command.Stderr = &output + if err := command.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- command.Wait() }() + select { + case err := <-done: + t.Fatalf("pinned reader did not wait for the short writer: %v: %s", err, output.String()) + case <-time.After(100 * time.Millisecond): + } + if err := os.WriteFile(gate, nil, 0o600); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("pinned reader failed after the writer released: %v: %s", err, output.String()) + } + case <-ctx.Done(): + t.Fatal("pinned reader did not enter after the writer released") + } + if _, err := os.Stat(entered); err != nil { + t.Fatalf("pinned reader never entered the rendezvous: %v", err) + } +} + +func TestScheduledJobApplicationRendezvousTimeoutRecordsSkip(t *testing.T) { + requireUtilLinuxFlock(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + root := t.TempDir() + names := app.Names{App: "sample", BasePath: root} + if err := os.MkdirAll(filepath.Join(names.AppDir(), "schedule"), 0o700); err != nil { + t.Fatal(err) + } + holder, gate := startExclusiveScheduleHolder(t, ctx, names.ScheduleRunLock(), root) + defer stopExclusiveScheduleHolder(holder, gate) + + lines := []string{"set -eu", "operation=''", "inputs_json=''", "INVOCATION_ID=timeout-reader"} + lines = append(lines, scheduleLockLines(names, "reader", "pinned", filepath.Join(names.AppDir(), "lock"), 10*time.Minute, 10*time.Second)...) + for i := range lines { + lines[i] = strings.Replace(lines[i], "--timeout 10", "--timeout 0.1", 1) + } + command := exec.CommandContext(ctx, "sh") + command.Stdin = strings.NewReader(strings.Join(lines, "\n")) + output, err := command.CombinedOutput() + if err != nil { + t.Fatalf("timed-out reader did not exit as a clean skip: %v: %s", err, output) + } + state, err := os.ReadFile(names.ScheduledJobRunState("reader")) + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(state, []byte("skipped=the application scheduling lock remained busy for 10s")) { + t.Fatalf("timeout state = %q, want application-rendezvous skip", state) + } +} + +func TestScheduledJobRunnerDoesNotReportFlockErrorsAsContention(t *testing.T) { + root := t.TempDir() + names := app.Names{App: "sample", BasePath: root} + stub := filepath.Join(root, "broken-flock") + count := filepath.Join(root, "flock-count") + stubScript := "#!/bin/sh\ncount=0\n[ ! -f " + q(count) + " ] || count=$(cat " + q(count) + ")\ncount=$((count + 1))\nprintf '%s\\n' \"$count\" >" + q(count) + "\n[ \"$count\" -ne 1 ] || exit 0\nexit 74\n" + if err := os.WriteFile(stub, []byte(stubScript), 0o700); err != nil { + t.Fatal(err) + } + job := app.ScheduledJob{Name: "refresh", DeployLock: "pinned"} + runner := scheduleRunnerScript("sample", job, names, filepath.Join(names.AppDir(), "lock"), nil, 10*time.Minute, true) + runner = strings.ReplaceAll(runner, "/usr/bin/flock", q(stub)) + command := exec.CommandContext(context.Background(), "sh") + command.Stdin = strings.NewReader(runner) + output, err := command.CombinedOutput() + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 74 { + t.Fatalf("flock infrastructure error was not preserved: err=%v output=%s", err, output) + } + if strings.Contains(string(output), "onebox: skipped:") { + t.Fatalf("flock infrastructure error was reported as contention: %s", output) + } + if calls, err := os.ReadFile(count); err != nil || string(calls) != "2\n" { + t.Fatalf("flock calls = %q, %v; want application rendezvous to be the second call", calls, err) + } +} + +func requireUtilLinuxFlock(t *testing.T) { + t.Helper() + if runtime.GOOS != "linux" { + t.Skip("the installed runner targets Linux systemd hosts") + } + if _, err := os.Stat("/usr/bin/flock"); err != nil { + t.Skip("util-linux flock is unavailable") + } + help, err := exec.CommandContext(context.Background(), "/usr/bin/flock", "--help").CombinedOutput() + if err != nil || !bytes.Contains(help, []byte("--conflict-exit-code")) { + t.Skip("the installed flock does not provide the util-linux interface") + } +} + +func startExclusiveScheduleHolder(t *testing.T, ctx context.Context, lock, root string) (*exec.Cmd, string) { + t.Helper() + ready := filepath.Join(root, "writer-ready") + gate := filepath.Join(root, "release-writer") + lines := []string{ + "set -eu", + "exec 6>" + q(lock), + "/usr/bin/flock --exclusive 6", + "touch " + q(ready), + "while [ ! -e " + q(gate) + " ]; do sleep 0.01; done", + } + holder := exec.CommandContext(ctx, "sh") + holder.Stdin = strings.NewReader(strings.Join(lines, "\n")) + if err := holder.Start(); err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(3 * time.Second) + for { + if _, err := os.Stat(ready); err == nil { + return holder, gate + } + if time.Now().After(deadline) { + _ = holder.Process.Kill() + _ = holder.Wait() + t.Fatal("exclusive schedule-lock holder did not start") + } + time.Sleep(10 * time.Millisecond) + } +} + +func stopExclusiveScheduleHolder(holder *exec.Cmd, gate string) { + _ = os.WriteFile(gate, nil, 0o600) + if holder.ProcessState == nil { + _ = holder.Process.Kill() + _ = holder.Wait() + } +} + func TestPinnedScheduleDeployConflictClassifiesLifecycleEffects(t *testing.T) { for _, tc := range []struct { effect app.DataEffect @@ -374,7 +635,7 @@ func TestPinnedScheduledJobLockProtocol(t *testing.T) { assertLock := func(path string, available bool) { t.Helper() - err := exec.CommandContext(ctx, "/usr/bin/flock", "--exclusive", "--nonblock", "--conflict-exit-code", "75", path, "true").Run() + err := exec.CommandContext(ctx, "/usr/bin/flock", "--exclusive", "--nonblock", "--conflict-exit-code", strconv.Itoa(flockConflictExitCode), path, "true").Run() if available && err != nil { t.Fatalf("lock %s remained unavailable: %v", path, err) } @@ -1238,7 +1499,7 @@ func TestScheduledJobRunnerConsumesManualInputsWithoutShellInterpolation(t *test } // The consume block precedes the locks so a skipped manual run cannot // leave its inputs for the next timer firing. - if strings.Index(runner, "inputs_file=") > strings.Index(runner, "flock --exclusive --nonblock 9") { + if strings.Index(runner, "inputs_file=") > strings.Index(runner, "exec 9>") { t.Fatalf("inputs are consumed after the lock:\n%s", runner) } exclusive := scheduleRunnerScript("sample", app.ScheduledJob{Name: "sync", Timeout: "1h", DeployLock: "exclusive", RetryAttempts: 1}, names, "/var/lib/ob/sample/lock", nil, 10*time.Minute, true) @@ -1455,12 +1716,13 @@ func TestScheduledJobRunnerDoesNotClobberARunningJobsState(t *testing.T) { if strings.Contains(runner, `stand_aside`) && strings.Contains(runner, `>"$state"; echo "onebox: skipped`) { t.Fatalf("the lock-less skip writes the running job's state:\n%s", runner) } - if !strings.Contains(runner, "flock --exclusive --nonblock 9 || stand_aside 'another run of this job is still in progress'") { + if !strings.Contains(runner, "200) stand_aside 'another run of this job is still in progress'") { t.Fatalf("a job-lock conflict still writes state:\n%s", runner) } // The other two skips hold the job lock, so the state is theirs to write. for _, want := range []string{ - "flock --exclusive --nonblock 8 || skip 'an application operation is taking its lock'", + "flock --exclusive --timeout 10 --conflict-exit-code 200 8", + "200) skip 'the application scheduling lock remained busy for 10s'", "skip 'an application operation holds the deploy lock'", } { if !strings.Contains(runner, want) { diff --git a/internal/onebox/operation_errors.go b/internal/onebox/operation_errors.go index ce7ba7eb..2574d5ad 100644 --- a/internal/onebox/operation_errors.go +++ b/internal/onebox/operation_errors.go @@ -87,7 +87,7 @@ var operationFailureDefinitions = map[string]OperationFailure{ Command: "ob approve --plan --backup-report ", }, "divergence_detected": { - Message: "the live release does not match the recorded release state", + Message: "the live application state has diverged from its intended state", // Not `ob status`: this code is raised BY ob status, so publishing it // tells a caller to re-run the command that just failed. Command: "ob audit --output json", diff --git a/internal/release/schedule_lease.go b/internal/release/schedule_lease.go index d2d38c98..631e42d7 100644 --- a/internal/release/schedule_lease.go +++ b/internal/release/schedule_lease.go @@ -3,6 +3,7 @@ package release import ( "context" "fmt" + "strconv" "strings" "github.com/labstack/onebox/internal/app" @@ -10,6 +11,7 @@ import ( ) const scheduleLeaseFile = ".ob-schedule.lease" +const scheduleLeaseConflictExitCode = 200 // ActiveScheduleLeases returns release ids held by pinned scheduled jobs. The // runner takes a shared kernel lock; cleanup probes for an exclusive lock while @@ -19,8 +21,8 @@ func ActiveScheduleLeases(ctx context.Context, target transport.Transport, names command := "for lease in " + glob + "; do " + "[ -e \"$lease\" ] || continue; " + "[ -f \"$lease\" ] && [ ! -L \"$lease\" ] || exit 74; " + - "code=0; /usr/bin/flock --exclusive --nonblock --conflict-exit-code 75 \"$lease\" true || code=$?; " + - "case $code in 0) ;; 75) dir=${lease%/" + scheduleLeaseFile + "}; printf '%s\\n' \"${dir##*/}\" ;; *) exit $code ;; esac; " + + "code=0; /usr/bin/flock --exclusive --nonblock --conflict-exit-code " + strconv.Itoa(scheduleLeaseConflictExitCode) + " \"$lease\" true || code=$?; " + + "case $code in 0) ;; " + strconv.Itoa(scheduleLeaseConflictExitCode) + ") dir=${lease%/" + scheduleLeaseFile + "}; printf '%s\\n' \"${dir##*/}\" ;; *) exit $code ;; esac; " + "done" result, err := target.Run(ctx, command) if err != nil { diff --git a/internal/release/schedule_lease_test.go b/internal/release/schedule_lease_test.go new file mode 100644 index 00000000..39137895 --- /dev/null +++ b/internal/release/schedule_lease_test.go @@ -0,0 +1,61 @@ +package release + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/labstack/onebox/internal/app" + "github.com/labstack/onebox/internal/transport" +) + +func TestActiveScheduleLeasesDistinguishesContentionFromFlockFailure(t *testing.T) { + root := t.TempDir() + names := app.Names{App: "sample", BasePath: root} + releaseID := "20260911-120000-abcd" + releaseDir := filepath.Join(PathsFor(names).Releases, releaseID) + if err := os.MkdirAll(releaseDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(releaseDir, scheduleLeaseFile), nil, 0o600); err != nil { + t.Fatal(err) + } + + var command string + fake := &transport.Fake{Dynamic: func(candidate string) (transport.Result, bool) { + command = candidate + return transport.Result{}, true + }} + if _, err := ActiveScheduleLeases(context.Background(), fake, names); err != nil { + t.Fatal(err) + } + if !strings.Contains(command, "--conflict-exit-code 200") || !strings.Contains(command, "200) dir=") { + t.Fatalf("lease probe does not use its distinct conflict sentinel: %s", command) + } + + flock := filepath.Join(root, "flock") + run := func(exitCode string) ([]byte, error) { + t.Helper() + if err := os.WriteFile(flock, []byte("#!/bin/sh\nexit "+exitCode+"\n"), 0o700); err != nil { + t.Fatal(err) + } + probe := strings.ReplaceAll(command, "/usr/bin/flock", q(flock)) + return exec.CommandContext(t.Context(), "sh", "-c", probe).CombinedOutput() + } + + if output, err := run("74"); err == nil { + t.Fatalf("flock infrastructure failure was accepted: output=%q", output) + } else { + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 74 { + t.Fatalf("flock infrastructure failure = %v, want exit 74; output=%q", err, output) + } + } + if output, err := run("200"); err != nil || strings.TrimSpace(string(output)) != releaseID { + t.Fatalf("lease contention was not reported as active: err=%v output=%q", err, output) + } +} diff --git a/site/src/content/docs/guides/schedule-a-job.mdx b/site/src/content/docs/guides/schedule-a-job.mdx index 8a7362eb..47a41fab 100644 --- a/site/src/content/docs/guides/schedule-a-job.mdx +++ b/site/src/content/docs/guides/schedule-a-job.mdx @@ -76,13 +76,18 @@ release lease is held instead and every attempt runs the same release. ## Deployment coordination is exclusive by default -A host-fired job takes an application-wide kernel lock for its whole container -run. Every operation that establishes Onebox's fenced application lock takes -the same scheduling mutex while publishing its ownership. A timer that collides -does not modify Docker beside a deploy: it records a `skipped` run with the -reason and exits cleanly. An application lock older than its TTL is treated as -expired, exactly as a deploy treats it, so a runner that died mid-operation -cannot silence a timer forever. +An exclusive host-fired job takes an application-wide kernel lock for its whole +container run. An ordinary pinned job takes that rendezvous as a shared reader +until it establishes its immutable release lease; a durable pinned job retains +the reader through checkpoint preparation and publication. Different pinned +jobs may still start together. Every operation that establishes Onebox's fenced +application lock takes the rendezvous as an exclusive writer while publishing +its ownership. Readers and writers wait briefly for a handoff; the wait is +shortened for a job whose own timeout is near the ten-second default. A timer +that still collides does not modify Docker beside a deploy: it records a +`skipped` run with the reason and exits cleanly. An application lock older than +its TTL is treated as expired, exactly as a deploy treats it, so a runner that +died mid-operation cannot silence a timer forever. The target must provide `flock` (part of `util-linux` on supported Linux hosts). Onebox refuses to install or run schedules when that serialization primitive is @@ -152,12 +157,13 @@ ONEBOX_UNIT=ob-- -o cat` on the host is the raw history: `run` is systemd's invocation id, so the record and the run's own log share a key. `outcome` is one of `success`, `failure`, `timeout`, or `skipped`. A skip -is a firing that met a running instance of the same job or an application -operation holding the deploy lock. The runner writes the `reason` and exits -before any container starts, so the unit is not failed and the job's own exit -status is never mistaken for a skip. Skips are recorded because a job that is -silently never running looks exactly like one that works: one skip is timing, -and three in a row are reported by `ob status` as an issue. +is a firing that met a running instance of the same job, timed out waiting for +the application scheduling rendezvous, or found an application operation +holding the deploy lock. The runner writes the `reason` and exits before any +container starts, so the unit is not failed and the job's own exit status is +never mistaken for a skip. Skips are recorded because a job that is silently +never running looks exactly like one that works: one skip is timing, and three +in a row are reported by `ob status` as an issue. A skip is news about timing, not about the job, so it never clears a failure: `ob status` keeps reporting the newest run that actually happened, and says @@ -224,6 +230,8 @@ The last one declares a day-of-month **and** a day-of-week. Cron treats that as load rather than running on days nobody chose. `timezone` takes an IANA zone name and defaults to `UTC`. +Generated timers use one-second accuracy so systemd does not coalesce distinct +cron minutes into its default one-minute wake-up window. ## Jobs still declare a data effect diff --git a/site/src/content/docs/reference/errors.mdx b/site/src/content/docs/reference/errors.mdx index 03012bc2..b3f3b3aa 100644 --- a/site/src/content/docs/reference/errors.mdx +++ b/site/src/content/docs/reference/errors.mdx @@ -127,7 +127,7 @@ step to complete rather than a line to run verbatim. | `config_exists` | a project file already exists and init refuses to overwrite it | diagnostic | `ob validate --output json` | | `config_write_failed` | the project file could not be written | — | — | | `confirmation_failed` | the backup report for this local confirmation could not be loaded or does not bind to the plan | next | `ob approve --plan --backup-report ` | -| `divergence_detected` | the live release does not match the recorded release state | diagnostic | `ob audit --output json` | +| `divergence_detected` | the live application state has diverged from its intended state | diagnostic | `ob audit --output json` | | `doctor_failed` | a local readiness check failed | — | — | | `exec_failed` | the audited exec could not be completed | diagnostic | `ob status --output json` | | `execution_count_invalid` | execution count must be between 1 and 1000 | — | — |