From 0cd0e52a24b941e8e3ca1dc35e081bb10ce5d293 Mon Sep 17 00:00:00 2001 From: bircni Date: Fri, 31 Jul 2026 12:15:04 +0000 Subject: [PATCH] fix!: guard against two runner processes sharing one runner file (#1099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting two runner daemons with the same `.runner` file makes both present an identical UUID+token, so Gitea treats them as one runner and they cancel each other's jobs. This adds a non-blocking advisory lock on a sibling `.lock`: the daemon (and `register`) acquire it at startup, and a second process on the same host fails fast with a clear error instead of silently interfering. The OS releases the lock when the process exits — including a hard kill — so no stale lock is left behind. Legitimate multi-runner setups are unaffected since each already uses its own `runner.file`. Note: this covers the common single-host case; two hosts sharing a copied `.runner` (e.g. over NFS) would still need server-side detection in Gitea. --------- Co-authored-by: Zettat123 Reviewed-on: https://gitea.com/gitea/runner/pulls/1099 Reviewed-by: Zettat123 <39446+zettat123@noreply.gitea.com> --- .gitignore | 1 + internal/app/cmd/daemon.go | 20 ++++++++++++ internal/app/cmd/register.go | 14 ++++++++ internal/pkg/lock/lock.go | 35 ++++++++++++++++++++ internal/pkg/lock/lock_plan9.go | 11 +++++++ internal/pkg/lock/lock_test.go | 54 +++++++++++++++++++++++++++++++ internal/pkg/lock/lock_unix.go | 35 ++++++++++++++++++++ internal/pkg/lock/lock_windows.go | 36 +++++++++++++++++++++ 8 files changed, 206 insertions(+) create mode 100644 internal/pkg/lock/lock.go create mode 100644 internal/pkg/lock/lock_plan9.go create mode 100644 internal/pkg/lock/lock_test.go create mode 100644 internal/pkg/lock/lock_unix.go create mode 100644 internal/pkg/lock/lock_windows.go diff --git a/.gitignore b/.gitignore index 2e5bc78f..29a82272 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ .env !/act/runner/testdata/secrets/.env .runner +.runner.lock coverage.txt .tmp/ /config.yaml diff --git a/internal/app/cmd/daemon.go b/internal/app/cmd/daemon.go index 61624b3e..d1b3a69a 100644 --- a/internal/app/cmd/daemon.go +++ b/internal/app/cmd/daemon.go @@ -22,6 +22,7 @@ import ( "gitea.com/gitea/runner/internal/pkg/config" "gitea.com/gitea/runner/internal/pkg/envcheck" "gitea.com/gitea/runner/internal/pkg/labels" + "gitea.com/gitea/runner/internal/pkg/lock" "gitea.com/gitea/runner/internal/pkg/metrics" "gitea.com/gitea/runner/internal/pkg/ver" @@ -49,6 +50,25 @@ func runDaemon(ctx context.Context, daemArgs *daemonArgs, configFile *string) fu return fmt.Errorf("failed to load registration file: %w", err) } + // Guard against a second runner process sharing this runner file: two + // processes with the same identity are indistinguishable to Gitea and + // end up cancelling each other's jobs. + releaseLock, err := lock.TryLock(cfg.Runner.File) + if errors.Is(err, lock.ErrLocked) { + log.Errorf("another gitea-runner process is already using %q; each runner process needs its own runner file (runner.file)", cfg.Runner.File) + return err + } else if err != nil { + // Best-effort guard: if the lock file can't be created (e.g. a + // read-only runner-file mount), warn and start anyway rather than + // refusing to run. + log.Warnf("could not lock runner file %q, continuing without the single-process guard: %v", cfg.Runner.File, err) + } else { + // Held until shutdown finishes: the draining runner still owns this + // identity on the server, so releasing early would let a restart + // reintroduce the duplicate-identity job cancellations. + defer func() { _ = releaseLock() }() + } + lbls := resolveLabels(daemArgs.Labels, cfg.Runner.Labels, reg.Labels) ls := labels.Labels{} diff --git a/internal/app/cmd/register.go b/internal/app/cmd/register.go index 6859522d..e980cb15 100644 --- a/internal/app/cmd/register.go +++ b/internal/app/cmd/register.go @@ -18,6 +18,7 @@ import ( "gitea.com/gitea/runner/internal/pkg/client" "gitea.com/gitea/runner/internal/pkg/config" "gitea.com/gitea/runner/internal/pkg/labels" + "gitea.com/gitea/runner/internal/pkg/lock" "gitea.com/gitea/runner/internal/pkg/ver" "connectrpc.com/connect" @@ -346,6 +347,19 @@ func registerNoInteractive(ctx context.Context, configFile string, regArgs *regi } func doRegister(ctx context.Context, cfg *config.Config, inputs *registerInputs) error { + // Refuse to rewrite the runner file while another process is using it. + releaseLock, err := lock.TryLock(cfg.Runner.File) + if errors.Is(err, lock.ErrLocked) { + return fmt.Errorf("another process is already using %q; stop it before re-registering", cfg.Runner.File) + } else if err != nil { + // Best-effort guard: if the lock file can't be created, warn and + // register anyway; writing the runner file will surface any real + // permission problem with a clearer error. + log.Warnf("could not lock runner file %q, continuing without the single-process guard: %v", cfg.Runner.File, err) + } else { + defer func() { _ = releaseLock() }() + } + // initial http client cli := client.New( inputs.InstanceAddr, diff --git a/internal/pkg/lock/lock.go b/internal/pkg/lock/lock.go new file mode 100644 index 00000000..76544b88 --- /dev/null +++ b/internal/pkg/lock/lock.go @@ -0,0 +1,35 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +// Package lock provides a cross-platform, non-blocking advisory file lock used +// to ensure a single runner process owns a given runner file. +package lock + +import ( + "errors" + "fmt" +) + +// ErrLocked is returned by TryLock when another process already holds the lock. +var ErrLocked = errors.New("runner file is already locked by another process") + +// TryLock takes a non-blocking exclusive advisory lock tied to runnerFile. The +// lock is placed on a sibling ".lock" so it never interferes with +// in-place rewrites of the runner file itself. +// +// It returns a release function that drops the lock. The operating system also +// releases the lock automatically when the process exits, including on a hard +// kill, so a crashed runner never leaves a stale lock behind. +// +// If another process already holds the lock, it returns ErrLocked. +func TryLock(runnerFile string) (func() error, error) { + path := runnerFile + ".lock" + release, err := tryLock(path) + if errors.Is(err, ErrLocked) { + return nil, ErrLocked + } + if err != nil { + return nil, fmt.Errorf("lock %q: %w", path, err) + } + return release, nil +} diff --git a/internal/pkg/lock/lock_plan9.go b/internal/pkg/lock/lock_plan9.go new file mode 100644 index 00000000..8190464d --- /dev/null +++ b/internal/pkg/lock/lock_plan9.go @@ -0,0 +1,11 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build plan9 + +package lock + +// tryLock is a best-effort no-op on plan9, which lacks flock/LockFileEx. +func tryLock(_ string) (func() error, error) { + return func() error { return nil }, nil +} diff --git a/internal/pkg/lock/lock_test.go b/internal/pkg/lock/lock_test.go new file mode 100644 index 00000000..b7513611 --- /dev/null +++ b/internal/pkg/lock/lock_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !plan9 + +package lock + +import ( + "errors" + "path/filepath" + "testing" +) + +func TestTryLock(t *testing.T) { + runnerFile := filepath.Join(t.TempDir(), ".runner") + + release, err := TryLock(runnerFile) + if err != nil { + t.Fatalf("first TryLock failed: %v", err) + } + + // A second lock on the same file must be refused while the first is held. + if _, err := TryLock(runnerFile); !errors.Is(err, ErrLocked) { + t.Fatalf("second TryLock: want ErrLocked, got %v", err) + } + + if err := release(); err != nil { + t.Fatalf("release failed: %v", err) + } + + // After release the lock is available again. + release2, err := TryLock(runnerFile) + if err != nil { + t.Fatalf("TryLock after release failed: %v", err) + } + if err := release2(); err != nil { + t.Fatalf("second release failed: %v", err) + } +} + +// TestTryLockUncreatable ensures a lock file that cannot be created reports a +// non-ErrLocked error, so callers can tell "already locked by another process" +// apart from "couldn't lock" and degrade gracefully (e.g. a read-only mount). +func TestTryLockUncreatable(t *testing.T) { + runnerFile := filepath.Join(t.TempDir(), "missing-dir", ".runner") + + _, err := TryLock(runnerFile) + if err == nil { + t.Fatal("TryLock on an uncreatable lock file: want error, got nil") + } + if errors.Is(err, ErrLocked) { + t.Fatal("TryLock on an uncreatable lock file: want non-ErrLocked error, got ErrLocked") + } +} diff --git a/internal/pkg/lock/lock_unix.go b/internal/pkg/lock/lock_unix.go new file mode 100644 index 00000000..2d15011c --- /dev/null +++ b/internal/pkg/lock/lock_unix.go @@ -0,0 +1,35 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build !windows && !plan9 + +package lock + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +// tryLock opens (creating if needed) the lock file and takes a non-blocking +// exclusive flock on it. The returned release closes the file, which drops the +// lock; the kernel also drops it when the process exits. +func tryLock(path string) (func() error, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + if err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil { + _ = f.Close() + if errors.Is(err, unix.EWOULDBLOCK) || errors.Is(err, unix.EAGAIN) { + return nil, ErrLocked + } + return nil, err + } + return func() error { + // Best-effort unlock; closing the fd releases the lock regardless. + _ = unix.Flock(int(f.Fd()), unix.LOCK_UN) + return f.Close() + }, nil +} diff --git a/internal/pkg/lock/lock_windows.go b/internal/pkg/lock/lock_windows.go new file mode 100644 index 00000000..090c0dd9 --- /dev/null +++ b/internal/pkg/lock/lock_windows.go @@ -0,0 +1,36 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +//go:build windows + +package lock + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLock opens (creating if needed) the lock file and takes a non-blocking +// exclusive lock on it via LockFileEx. The returned release closes the file, +// which drops the lock; Windows also drops it when the process exits. +func tryLock(path string) (func() error, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + handle := windows.Handle(f.Fd()) + overlapped := new(windows.Overlapped) + if err := windows.LockFileEx(handle, windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, overlapped); err != nil { + _ = f.Close() + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return nil, ErrLocked + } + return nil, err + } + return func() error { + _ = windows.UnlockFileEx(handle, 0, 1, 0, overlapped) + return f.Close() + }, nil +}