From 445ef57ada44a762212dfc8b1d239d528d3c2dd1 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 04:13:56 +0000 Subject: [PATCH] Lock DATA_DIR against a second instance (closes #201) Nothing stopped two processes opening the same DATA_DIR. Both open the same per-webhook databases, both run delivery recovery over the same rows, and both deliver: every pending delivery reaches the destination twice, from nothing worse than an overlapping deploy. The entry point now takes an exclusive advisory flock(2) on {DATA_DIR}/webhooker.lock before anything opens a database, and holds it for the process lifetime. A second process pointed at the same directory prints a message naming that directory and exits 1. The lock is the kernel's, not the file's, so a process killed with SIGKILL leaves a lock file that blocks nothing -- which is what a pidfile would get wrong. The file is never unlinked: doing so would let the next process lock a fresh inode while a third still held the old one. Acquisition lives in internal/datadir rather than in the server's fx graph, so any entry point touching DATA_DIR takes it the same way, and ErrLocked lets a caller tell a live deployment from any other failure. config.DataDir() resolves DATA_DIR once, for both the lock and Config, so the two cannot disagree. Regression coverage: a real second process is refused, and a restart after kill -9 succeeds with the stale lock file in place. github.com/gofrs/flock carries the lock; its own module minimums pull testify to v1.11.1 and golang.org/x/sys to v0.37.0. --- README.md | 40 +++++- cmd/webhooker/main.go | 29 ++++ cmd/webhooker/main_test.go | 35 +++++ go.mod | 5 +- go.sum | 14 +- internal/config/config.go | 27 ++-- internal/config/config_test.go | 30 ++++ internal/datadir/lock.go | 126 +++++++++++++++++ internal/datadir/lock_test.go | 250 +++++++++++++++++++++++++++++++++ 9 files changed, 534 insertions(+), 22 deletions(-) create mode 100644 internal/datadir/lock.go create mode 100644 internal/datadir/lock_test.go diff --git a/README.md b/README.md index 2bb6e23..e2a5d8c 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,32 @@ TTY detection, and security headers are always applied. | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) | +#### Single-instance lock + +Exactly one webhooker process may use a `DATA_DIR` at a time. Two +processes sharing one open the same databases and each run delivery +recovery over the same rows, so every pending delivery goes out twice — +duplicate delivery to your endpoints, from nothing worse than an +overlapping deploy or a double start. + +At startup, before anything opens a database, the process takes an +exclusive advisory lock (`flock(2)`) on `{DATA_DIR}/webhooker.lock` and +holds it for its lifetime. A second process pointed at the same +directory prints a message naming it and exits non-zero: + +``` +webhooker: data directory is already in use by another instance: /var/lib/webhooker (/var/lib/webhooker/webhooker.lock). Only one webhooker may use a data directory: two both run delivery recovery over the same rows and both deliver +``` + +The lock is the kernel's, not the file's: it is released when the +process exits, including `kill -9`, so a leftover `webhooker.lock` +never blocks a restart and must not be deleted by hand. The file is +also left in place on a clean shutdown, deliberately — unlinking it +would let the next process lock a fresh inode while a third still held +the old one. + +To run two webhookers on one host, give each its own `DATA_DIR`. + #### Trusted proxies `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare @@ -283,10 +309,12 @@ is both the simplest and the only complete rule: `{webhook_uuid}` is the webhook's UUID primary key in its canonical 36-character hyphenated form, so a real filename looks like -`events-3f2a1c9e-....db`. Nothing else is written to `DATA_DIR`, and no -`-wal` or `-shm` files are produced (see below); a transient -`{name}.db-journal` may exist beside a database while a write is in -flight and is not part of the backup set. +`events-3f2a1c9e-....db`. The only other file is `webhooker.lock`, the +always-empty [single-instance lock](#single-instance-lock); it holds no +state and is not part of the backup set — a copied one is stale and +blocks nothing. No `-wal` or `-shm` files are produced (see below); a +transient `{name}.db-journal` may exist beside a database while a write +is in flight and is not part of the backup set either. Configuration is **not** in `DATA_DIR` — it comes from the environment and from a `.env` file read out of the process working directory. Back @@ -1709,7 +1737,7 @@ imports. The entry point is `cmd/webhooker/main.go`. ``` webhooker/ ├── cmd/webhooker/ -│ └── main.go # Entry point: sets globals, wires fx +│ └── main.go # Entry point: sets globals, locks DATA_DIR, wires fx ├── internal/ │ ├── config/ │ │ └── config.go # Configuration loading from environment variables @@ -1730,6 +1758,8 @@ webhooker/ │ │ ├── retention.go # Retention reaper (per-webhook event expiry) │ │ ├── testing.go # NewTestDatabase: wrapper for tests, no fx lifecycle │ │ └── webhook_db_manager.go # Per-webhook DB lifecycle manager +│ ├── datadir/ +│ │ └── lock.go # Exclusive advisory lock on DATA_DIR (one instance) │ ├── globals/ │ │ └── globals.go # Build-time variables (appname, version, arch) │ ├── gormlog/ diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 114d154..0bd5d32 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -2,11 +2,15 @@ package main import ( + "fmt" + "io" + "os" "time" "go.uber.org/fx" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/handlers" @@ -56,7 +60,32 @@ func main() { globals.Appname = appname globals.Version = version + os.Exit(run(os.Stderr)) +} + +// run takes the exclusive DATA_DIR lock, then runs the application +// under it, and returns the process exit status. +// +// The lock is taken here rather than inside the fx graph because it has +// to be held before anything opens a database, and because a refusal +// has to reach the operator as a plain line on standard error rather +// than as one entry in an fx failure dump. It is released by the defer +// on a clean shutdown, and by the kernel closing the descriptor on any +// other exit — including the one fx performs itself when a start or +// stop hook fails, which skips deferred calls. +func run(stderr io.Writer) int { + lock, err := datadir.Acquire(config.DataDir()) + if err != nil { + _, _ = fmt.Fprintf(stderr, "%s: %v\n", appname, err) + + return 1 + } + + defer func() { _ = lock.Release() }() + newApp().Run() + + return 0 } // newApp builds the application graph. It is separate from main so diff --git a/cmd/webhooker/main_test.go b/cmd/webhooker/main_test.go index fb22c18..15bb53b 100644 --- a/cmd/webhooker/main_test.go +++ b/cmd/webhooker/main_test.go @@ -1,10 +1,13 @@ package main import ( + "bytes" "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/server" ) @@ -33,6 +36,38 @@ func TestNewApp_StopTimeout(t *testing.T) { require.Less(t, got, dockerStopGrace) } +// TestRunRefusesLockedDataDir pins what an operator's second start +// does. The entry point must refuse before it builds the fx graph — +// nothing may open a database in a DATA_DIR another process holds — +// and must exit non-zero with a message naming the directory rather +// than starting a second delivery engine over the same rows. +// +// flock(2) locks descriptors independently, so holding the lock here +// is the same denial a separate process gets; internal/datadir pins +// that property and covers the real two-process case. +func TestRunRefusesLockedDataDir(t *testing.T) { + dir := t.TempDir() + t.Setenv("DATA_DIR", dir) + + lock, err := datadir.Acquire(dir) + require.NoError(t, err) + + defer func() { _ = lock.Release() }() + + var stderr bytes.Buffer + + code := run(&stderr) + + require.Equal( + t, 1, code, "a second instance must exit non-zero", + ) + assert.Contains( + t, stderr.String(), dir, + "the refusal must name the directory", + ) + assert.Contains(t, stderr.String(), "another instance") +} + // tailHeadroom is the slack the fx stop budget must keep beyond the // server stop hook. The hooks that run after the server — the // delivery engine, the healthcheck, the webhook DB manager and the diff --git a/go.mod b/go.mod index c51849e..e9363de 100644 --- a/go.mod +++ b/go.mod @@ -8,13 +8,14 @@ require ( github.com/go-chi/chi v1.5.5 github.com/go-chi/cors v1.2.1 github.com/go-chi/httprate v0.15.0 + github.com/gofrs/flock v0.13.0 github.com/google/uuid v1.6.0 github.com/gorilla/csrf v1.7.3 github.com/gorilla/sessions v1.4.0 github.com/joho/godotenv v1.5.1 github.com/prometheus/client_golang v1.18.0 github.com/slok/go-http-metrics v0.11.0 - github.com/stretchr/testify v1.8.4 + github.com/stretchr/testify v1.11.1 go.uber.org/fx v1.20.1 golang.org/x/crypto v0.38.0 gopkg.in/yaml.v3 v3.0.1 @@ -49,7 +50,7 @@ require ( go.uber.org/zap v1.23.0 // indirect golang.org/x/mod v0.17.0 // indirect golang.org/x/sync v0.14.0 // indirect - golang.org/x/sys v0.33.0 // indirect + golang.org/x/sys v0.37.0 // indirect golang.org/x/text v0.25.0 // indirect golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect google.golang.org/protobuf v1.31.0 // indirect diff --git a/go.sum b/go.sum index f7e0ba6..d2d615e 100644 --- a/go.sum +++ b/go.sum @@ -23,6 +23,8 @@ github.com/go-chi/httprate v0.15.0 h1:j54xcWV9KGmPf/X4H32/aTH+wBlrvxL7P+SdnRqxh5 github.com/go-chi/httprate v0.15.0/go.mod h1:rzGHhVrsBn3IMLYDOZQsSU4fJNWcjui4fWKJcCId1R4= github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA= github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og= +github.com/gofrs/flock v0.13.0 h1:95JolYOvGMqeH31+FC7D2+uULf6mG61mEZ/A8dRYMzw= +github.com/gofrs/flock v0.13.0/go.mod h1:jxeyy9R1auM5S6JYDBhDt+E2TCo7DkratH4Pgi8P+Z0= github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= @@ -81,11 +83,11 @@ github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncj github.com/slok/go-http-metrics v0.11.0 h1:ABJUpekCZSkQT1wQrFvS4kGbhea/w6ndFJaWJeh3zL0= github.com/slok/go-http-metrics v0.11.0/go.mod h1:ZGKeYG1ET6TEJpQx18BqAJAvxw9jBAZXCHU7bWQqqAc= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.5.1 h1:4VhoImhV/Bm0ToFkXFi8hXNXwpDRZ/ynw3amt82mzq0= -github.com/stretchr/objx v0.5.1/go.mod h1:/iHQpkQwBD6DLUmQ4pE+s1TXdob1mORJ4/UFdrifcy0= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= -github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= -github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ= github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0= github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0= @@ -109,8 +111,8 @@ golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/sync v0.14.0 h1:woo0S4Yywslg6hp4eUFjTVOyKt0RookbpAHG4c1HmhQ= golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.33.0 h1:q3i8TbbEz+JRD9ywIRlyRAQbM0qF7hu24q3teo2hbuw= -golang.org/x/sys v0.33.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ= +golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/text v0.25.0 h1:qVyWApTSYLk/drJRO5mDlNYskwQznZmkpV2c8q9zls4= golang.org/x/text v0.25.0/go.mod h1:WEdwpYrmk1qmdHvhkSTNPm3app7v4rsT8F2UD6+VHIA= golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d h1:vU5i/LfpvrRCpgM/VPfJLg5KjxD3E+hfT1SH+d9zLwg= diff --git a/internal/config/config.go b/internal/config/config.go index 0d5acb5..e8258bd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -26,6 +26,10 @@ const ( // EnvironmentProd represents production environment. EnvironmentProd = "prod" + // DefaultDataDir is where all SQLite databases live when DATA_DIR + // is unset. The same default applies in every environment. + DefaultDataDir = "/var/lib/webhooker" + // defaultPort is the default HTTP listen port. defaultPort = 8080 @@ -134,6 +138,19 @@ func envString(key string) string { return os.Getenv(key) } +// DataDir resolves DATA_DIR, applying DefaultDataDir when it is unset +// or empty. It is exported so that entry points which must act on the +// data directory before the fx graph exists — taking the exclusive +// directory lock, above all — resolve it exactly as Config does. +func DataDir() string { + dir := envString("DATA_DIR") + if dir == "" { + return DefaultDataDir + } + + return dir +} + // envBool returns the value of the named environment variable // parsed as a boolean. Returns defaultValue if not set. If the // variable is set but cannot be parsed, it returns a wrapped error @@ -407,7 +424,7 @@ func loadFromEnv() (*Config, error) { } return &Config{ - DataDir: envString("DATA_DIR"), + DataDir: DataDir(), Debug: debug, MaintenanceMode: maintenanceMode, Environment: environment, @@ -485,14 +502,6 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { s.log = log s.params = ¶ms - // Set default DataDir. All SQLite databases (main application - // DB and per-webhook event DBs) live here. The same default is - // used regardless of environment; override with DATA_DIR if - // needed. - if s.DataDir == "" { - s.DataDir = "/var/lib/webhooker" - } - if s.Debug { params.Logger.EnableDebugLogging() } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d7b3bc7..54075e2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -418,6 +418,36 @@ func TestDefaultDataDir(t *testing.T) { } } +// TestDataDirHelper pins the exported resolver against the value +// Config carries. The entry point takes the DATA_DIR lock through the +// helper before the fx graph exists, so the two disagreeing would mean +// locking one directory and writing to another. +func TestDataDirHelper(t *testing.T) { + for _, set := range []string{"", "/tmp/webhooker-datadir-helper"} { + name := "set" + if set == "" { + name = "unset" + } + + t.Run(name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if set == "" { + require.NoError(t, os.Unsetenv("DATA_DIR")) + } else { + t.Setenv("DATA_DIR", set) + } + + expected := set + if expected == "" { + expected = config.DefaultDataDir + } + + assert.Equal(t, expected, config.DataDir()) + }) + } +} + func TestReceiverRateLimit(t *testing.T) { tests := []struct { name string diff --git a/internal/datadir/lock.go b/internal/datadir/lock.go new file mode 100644 index 0000000..ab66f31 --- /dev/null +++ b/internal/datadir/lock.go @@ -0,0 +1,126 @@ +// Package datadir guards exclusive access to the directory holding +// every SQLite database webhooker writes. +// +// Two processes sharing a DATA_DIR each open the same per-webhook +// event databases and each run delivery recovery over the same rows, +// so every pending delivery goes out twice. SQLite's own locking does +// not prevent that: both writers are serialised correctly and both +// deliver. The only thing that prevents it is refusing to be the +// second process. +// +// The lock lives here rather than in the server's fx graph so that any +// entry point which touches DATA_DIR — the server, or a CLI +// subcommand that must not operate on a live deployment's data — takes +// it the same way. +package datadir + +import ( + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/gofrs/flock" +) + +// LockFileName is the advisory lock file created inside DATA_DIR. Its +// contents are never read: the lock is the flock(2) held on the open +// descriptor, not the file's existence, so a leftover file from a +// process that was killed with SIGKILL blocks nothing. +const LockFileName = "webhooker.lock" + +// dirPerm is the mode Acquire creates DATA_DIR with. It matches what +// internal/database uses, since whichever runs first creates it. +const dirPerm = 0o750 + +// ErrLocked reports that another live process holds the data +// directory. Callers that need to know whether a deployment is running +// — rather than merely failing to start — test for this with +// errors.Is. +var ErrLocked = errors.New( + "data directory is already in use by another instance", +) + +// ErrNoDir reports that Acquire was given an empty directory. +var ErrNoDir = errors.New("no data directory given") + +// Lock is a held exclusive advisory lock on a data directory. It is +// valid only while the process that took it lives: the kernel drops it +// when the descriptor closes, whether that is Release, a normal exit, +// or a SIGKILL. +type Lock struct { + dir string + file *flock.Flock +} + +// Acquire takes the exclusive advisory lock on dir, creating dir if it +// does not exist. It never waits: if another process holds the lock it +// returns an error wrapping ErrLocked and naming dir. +// +// The returned Lock must be held for as long as the caller intends to +// use dir. +func Acquire(dir string) (*Lock, error) { + if dir == "" { + return nil, ErrNoDir + } + + err := os.MkdirAll(dir, dirPerm) + if err != nil { + return nil, fmt.Errorf( + "creating data directory %s: %w", dir, err, + ) + } + + path := filepath.Join(dir, LockFileName) + fl := flock.New(path) + + held, err := fl.TryLock() + if err != nil { + return nil, fmt.Errorf( + "locking data directory %s: %w", dir, err, + ) + } + + if !held { + // TryLock leaves the descriptor open when it fails to take + // the lock, so it has to be closed explicitly. + _ = fl.Close() + + return nil, fmt.Errorf( + "%w: %s (%s). Only one webhooker may use a data "+ + "directory: two both run delivery recovery over the "+ + "same rows and both deliver", + ErrLocked, dir, path, + ) + } + + return &Lock{dir: dir, file: fl}, nil +} + +// Dir returns the locked directory. +func (l *Lock) Dir() string { + return l.dir +} + +// Path returns the lock file backing the lock. +func (l *Lock) Path() string { + return l.file.Path() +} + +// Release drops the lock and closes the descriptor. It is safe to call +// more than once. +// +// The lock file is deliberately left on disk. Unlinking it would let +// the next process create and lock a fresh inode while a third still +// holds the old one, which is the one outcome this package exists to +// prevent. +func (l *Lock) Release() error { + err := l.file.Unlock() + if err != nil { + return fmt.Errorf( + "releasing lock on data directory %s: %w", l.dir, err, + ) + } + + return nil +} diff --git a/internal/datadir/lock_test.go b/internal/datadir/lock_test.go new file mode 100644 index 0000000..13f4216 --- /dev/null +++ b/internal/datadir/lock_test.go @@ -0,0 +1,250 @@ +package datadir_test + +import ( + "bufio" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/datadir" +) + +// holderEnv names the directory the re-executed test binary should +// lock and hold. When it is unset the child test does nothing, so an +// ordinary run is unaffected. +const holderEnv = "WEBHOOKER_DATADIR_LOCK_HOLDER" + +// holderReadyPrefix labels the child's one-line report that it holds +// the lock, so the parent can find it among the testing package's own +// output on the same descriptor. +const holderReadyPrefix = "DATADIR-LOCK-HELD " + +// holderReadyTimeout bounds the wait for the child to take the lock. +// It only has to cover process start on a loaded shared host. +const holderReadyTimeout = 60 * time.Second + +// holderHold is how long the child keeps the lock if nothing kills it. +// A sleep rather than a bare block, so the runtime's deadlock detector +// has a pending timer and the child cannot outlive a killed test run +// by more than this. +const holderHold = 10 * time.Minute + +// TestLockHolder is the child half of the two-process tests below. It +// takes the lock on the directory named by holderEnv, reports the lock +// file on standard output, and then holds it until it is killed. +func TestLockHolder(t *testing.T) { + t.Parallel() + + dir := os.Getenv(holderEnv) + if dir == "" { + return + } + + lock, err := datadir.Acquire(dir) + require.NoError(t, err) + + // Written to the descriptor directly: the parent reads fd 1, not + // the testing package's buffered report. + _, err = fmt.Fprintf( + os.Stdout, "%s%s\n", holderReadyPrefix, lock.Path(), + ) + require.NoError(t, err) + + time.Sleep(holderHold) +} + +// startHolder re-executes this test binary as a separate process that +// takes and holds the lock on dir, and returns once that process +// actually holds it. The child is killed when the test ends. +func startHolder(t *testing.T, dir string) *exec.Cmd { + t.Helper() + + //nolint:gosec // Re-executing this test binary, with a fixed arg. + cmd := exec.CommandContext( + t.Context(), os.Args[0], "-test.run", "^TestLockHolder$", + ) + + cmd.Env = append(os.Environ(), holderEnv+"="+dir) + cmd.Stderr = os.Stderr + + stdout, err := cmd.StdoutPipe() + require.NoError(t, err) + require.NoError(t, cmd.Start()) + + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + ready := make(chan string, 1) + + go func() { + scanner := bufio.NewScanner(stdout) + for scanner.Scan() { + after, found := strings.CutPrefix( + scanner.Text(), holderReadyPrefix, + ) + if found { + ready <- after + + break + } + } + + close(ready) + + // Keep draining so the child never blocks on a full pipe. + _, _ = io.Copy(io.Discard, stdout) + }() + + select { + case path, ok := <-ready: + require.True( + t, ok, "holder exited without taking the lock", + ) + require.Equal(t, filepath.Join(dir, datadir.LockFileName), path) + case <-time.After(holderReadyTimeout): + t.Fatal("timed out waiting for the holder to take the lock") + } + + return cmd +} + +// TestSecondInstanceRefused is the regression test for the duplicate +// delivery this package exists to prevent: a real second process +// pointed at a data directory a live process already holds must be +// refused, with an error that names the directory. +func TestSecondInstanceRefused(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + startHolder(t, dir) + + lock, err := datadir.Acquire(dir) + + require.Error(t, err, "the second instance took the lock too") + require.Nil(t, lock) + require.ErrorIs( + t, err, datadir.ErrLocked, + "the refusal must be distinguishable from any other failure", + ) + assert.Contains( + t, err.Error(), dir, + "the refusal must name the directory it is about", + ) +} + +// TestRestartAfterHardKill is the other half of the regression: a +// process killed with SIGKILL runs no cleanup and leaves its lock file +// behind, and the next start must not be blocked by it. This is what a +// pidfile would get wrong; the kernel drops a flock when the +// descriptor closes, however the process died. +func TestRestartAfterHardKill(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + holder := startHolder(t, dir) + + require.NoError(t, holder.Process.Kill()) + + // Wait for the kill to have actually happened. Re-acquiring while + // the corpse still holds a descriptor would be a race, and would + // make this test pass or fail on scheduling. + _ = holder.Wait() + + require.FileExists( + t, filepath.Join(dir, datadir.LockFileName), + "the stale lock file is what must not block the restart", + ) + + lock, err := datadir.Acquire(dir) + require.NoError( + t, err, "a hard-killed instance must not block the next start", + ) + + require.NoError(t, lock.Release()) +} + +// TestSecondFdInSameProcessRefused pins the flock(2) property the +// tests in cmd/webhooker rely on: descriptors are locked +// independently, so a second acquisition is denied even when it comes +// from the process that already holds the lock. +func TestSecondFdInSameProcessRefused(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + first, err := datadir.Acquire(dir) + require.NoError(t, err) + + defer func() { _ = first.Release() }() + + _, err = datadir.Acquire(dir) + require.ErrorIs(t, err, datadir.ErrLocked) +} + +// TestReleaseAllowsReacquire covers the clean-shutdown path: the lock +// is released on exit, so a restart is not blocked by the previous +// run. +func TestReleaseAllowsReacquire(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + first, err := datadir.Acquire(dir) + require.NoError(t, err) + require.NoError(t, first.Release()) + + second, err := datadir.Acquire(dir) + require.NoError(t, err) + require.NoError(t, second.Release()) +} + +// TestAcquireCreatesDataDir covers a first start against a DATA_DIR +// that does not exist yet, which is the normal case for a fresh +// deployment: the lock is taken before anything else creates it. +func TestAcquireCreatesDataDir(t *testing.T) { + t.Parallel() + + dir := filepath.Join(t.TempDir(), "nested", "data") + + lock, err := datadir.Acquire(dir) + require.NoError(t, err) + + defer func() { _ = lock.Release() }() + + assert.Equal(t, dir, lock.Dir()) + assert.FileExists(t, filepath.Join(dir, datadir.LockFileName)) +} + +// TestAcquireEmptyDir rejects an empty directory rather than locking +// the process's working directory. +func TestAcquireEmptyDir(t *testing.T) { + t.Parallel() + + _, err := datadir.Acquire("") + require.ErrorIs(t, err, datadir.ErrNoDir) +} + +// TestAcquireUnusableDir reports an unusable DATA_DIR clearly, naming +// it, instead of failing later and deeper. +func TestAcquireUnusableDir(t *testing.T) { + t.Parallel() + + file := filepath.Join(t.TempDir(), "not-a-directory") + require.NoError(t, os.WriteFile(file, nil, 0o600)) + + _, err := datadir.Acquire(file) + + require.Error(t, err) + assert.Contains(t, err.Error(), file) +}