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 — duplicate delivery to the destination, triggered by 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.
internal/datadir — Acquire(dir) / (*Lock).Release(), non-blocking TryLock, exported ErrLocked so a caller can tell "a live instance holds this" from any other failure. It creates DATA_DIR if absent, since it now runs before internal/database would.
cmd/webhooker — run(io.Writer) int takes the lock, then runs the fx app under it. Acquisition is deliberately not in the fx graph: it has to happen before any database is opened, a refusal has to reach the operator as a plain line on stderr rather than as one entry in an fx failure dump, and the next entry point that touches DATA_DIR (a CLI subcommand, per #208) takes it the same way rather than standing up a server module.
config.DataDir() — resolves DATA_DIR (with its default) once, for both the lock and Config, so the two cannot lock one directory and write to another.
README — new #### Single-instance lock under Configuration; the backup section's "Nothing else is written to DATA_DIR" was made true again.
Why flock on a held fd, not a pidfile
The lock is the kernel's, not the file's. A process killed with kill -9 runs no cleanup and leaves webhooker.lock behind; the kernel drops the lock when the descriptor closes, so the next start is not blocked. A pidfile gets this wrong.
The file is never unlinked, on release either — doing so would let the next process create and lock a fresh inode while a third still held the old one, which is the exact outcome this prevents.
The guarantee is host-local: flock(2) is per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem. Documented in the README alongside the fail-closed behaviour on a filesystem that refuses flock.
Library
github.com/gofrs/flock v0.13.0. Stdlib exports no file-locking API; syscall.Flock is unix-only and would need hand-written build-tagged wrappers. flock is the widely used, maintained option (golangci-lint, Terraform, containerd). Its own module minimums force two transitive bumps, which are the whole of the go.mod diff besides the new line: stretchr/testify v1.8.4 → v1.11.1 and golang.org/x/sys v0.33.0 → v0.37.0.
Tests
internal/datadir/lock_test.go re-executes the test binary as a real second process:
TestSecondInstanceRefused — a live second process is denied, the error errors.Is → ErrLocked, and its text names the directory.
TestRestartAfterHardKill — holder is SIGKILLed, reaped, the stale lock file is asserted still present, and the next Acquire succeeds.
plus: second fd in the same process denied (the property the cmd/webhooker test rests on), release-then-reacquire, DATA_DIR created when absent, empty dir rejected, unusable dir named in the error.
cmd/webhooker: TestRunRefusesLockedDataDir pins the operator-facing contract — exit status 1, and stderr naming the directory.
Gate evidence
Rebased onto next at a13e5b7; the current gate evidence for the rebased tree is in #220 (comment) — make check exit 0, and a cache-defeated docker build --no-cache-filter=lint --no-cache-filter=builder with 0 issues. from the linter and all 16 test packages executed, none (cached).
Also driven end to end against the built binary, two real processes on one DATA_DIR:
=== second instance ===
second exit=1
--- stderr:
webhooker: data directory is already in use by another instance: /tmp/wh-201-probe-1787198842 (/tmp/wh-201-probe-1787198842/webhooker.lock). Only one webhooker may use a data directory: two both run delivery recovery over the same rows and both deliver
--- stdout:
=== kill -9 the first ===
-rw------- 1 user user 0 /tmp/wh-201-probe-1787198842/webhooker.lock
=== restart after kill -9 ===
third instance started OK after kill -9
Every image built was removed; no containers were started.
Closes https://git.eeqj.de/sneak/webhooker/issues/201
## What
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 — duplicate delivery to the destination, triggered by 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.
- `internal/datadir` — `Acquire(dir)` / `(*Lock).Release()`, non-blocking `TryLock`, exported `ErrLocked` so a caller can tell "a live instance holds this" from any other failure. It creates `DATA_DIR` if absent, since it now runs before `internal/database` would.
- `cmd/webhooker` — `run(io.Writer) int` takes the lock, then runs the fx app under it. Acquisition is deliberately **not** in the fx graph: it has to happen before any database is opened, a refusal has to reach the operator as a plain line on stderr rather than as one entry in an fx failure dump, and the next entry point that touches `DATA_DIR` (a CLI subcommand, per https://git.eeqj.de/sneak/webhooker/issues/208) takes it the same way rather than standing up a server module.
- `config.DataDir()` — resolves `DATA_DIR` (with its default) once, for both the lock and `Config`, so the two cannot lock one directory and write to another.
- README — new `#### Single-instance lock` under Configuration; the backup section's "Nothing else is written to `DATA_DIR`" was made true again.
## Why flock on a held fd, not a pidfile
The lock is the kernel's, not the file's. A process killed with `kill -9` runs no cleanup and leaves `webhooker.lock` behind; the kernel drops the lock when the descriptor closes, so the next start is not blocked. A pidfile gets this wrong.
The file is never unlinked, on release either — doing so would let the next process create and lock a fresh inode while a third still held the old one, which is the exact outcome this prevents.
The guarantee is host-local: `flock(2)` is per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem. Documented in the README alongside the fail-closed behaviour on a filesystem that refuses `flock`.
## Library
`github.com/gofrs/flock` v0.13.0. Stdlib exports no file-locking API; `syscall.Flock` is unix-only and would need hand-written build-tagged wrappers. flock is the widely used, maintained option (golangci-lint, Terraform, containerd). Its own module minimums force two transitive bumps, which are the whole of the `go.mod` diff besides the new line: `stretchr/testify` v1.8.4 → v1.11.1 and `golang.org/x/sys` v0.33.0 → v0.37.0.
## Tests
`internal/datadir/lock_test.go` re-executes the test binary as a real second process:
- `TestSecondInstanceRefused` — a live second process is denied, the error `errors.Is` → `ErrLocked`, and its text names the directory.
- `TestRestartAfterHardKill` — holder is `SIGKILL`ed, reaped, the stale lock file is asserted still present, and the next `Acquire` succeeds.
- plus: second fd in the same process denied (the property the `cmd/webhooker` test rests on), release-then-reacquire, `DATA_DIR` created when absent, empty dir rejected, unusable dir named in the error.
`cmd/webhooker`: `TestRunRefusesLockedDataDir` pins the operator-facing contract — exit status 1, and stderr naming the directory.
## Gate evidence
Rebased onto `next` at `a13e5b7`; the current gate evidence for the rebased tree is in https://git.eeqj.de/sneak/webhooker/pulls/220#issuecomment-66908 — `make check` exit 0, and a cache-defeated `docker build --no-cache-filter=lint --no-cache-filter=builder` with `0 issues.` from the linter and all 16 test packages executed, none `(cached)`.
Also driven end to end against the built binary, two real processes on one `DATA_DIR`:
```
=== second instance ===
second exit=1
--- stderr:
webhooker: data directory is already in use by another instance: /tmp/wh-201-probe-1787198842 (/tmp/wh-201-probe-1787198842/webhooker.lock). Only one webhooker may use a data directory: two both run delivery recovery over the same rows and both deliver
--- stdout:
=== kill -9 the first ===
-rw------- 1 user user 0 /tmp/wh-201-probe-1787198842/webhooker.lock
=== restart after kill -9 ===
third instance started OK after kill -9
```
Every image built was removed; no containers were started.
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.
FAIL — needs-rebase. The lock itself is correct and independently verified; the branch no longer merges into next.
BLOCKING — does not merge into current next
next has advanced to a13e5b7 since this branch was cut (#216 and #218 both landed). README.md conflicts: #216 inserted #### Metrics credentials and this PR inserts #### Single-instance lock at the same point, immediately after the environment-variable table (README.md around line 117).
git checkout -B t origin/next && git merge --no-ff origin/issue-201-datadir-lock
CONFLICT (content): Merge conflict in README.md
Gitea still shows mergeable: true because the PR record pins the stale base 10c8dd2. Resolution is trivial — keep both #### subsections — but the branch must be rebased onto a13e5b7 and re-pushed, and the gate re-run there. No other file conflicts.
Non-blocking findings
internal/datadir/lock.go, comment on the !held branch: "TryLock leaves the descriptor open when it fails to take the lock, so it has to be closed explicitly" is not true of gofrs/flock v0.13.0. (*Flock).try registers defer f.ensureFhState() whenever it opened the handle itself, and on the EWOULDBLOCK return that closes and nils fh; the following fl.Close() is a no-op, since Unlock short-circuits on f.fh == nil. Keep the call as version-robust defence; the comment asserting a false property of the dependency is what should change.
README.md, #### Single-instance lock states the guarantee unconditionally. flock(2) is per-inode and host-local: it does arbitrate between containers sharing a bind-mounted/volume DATA_DIR on one host (the documented case), but not between hosts on a network filesystem, and a DATA_DIR in a container's own overlay layer is not shared at all. One sentence closes it. Same place: when the filesystem refuses flock outright the process fails closed and refuses to start — right behaviour, currently undocumented.
For #208 — ErrLocked is %w-wrapped and errors.Is matches, Acquire needs no fx. But Acquire calls os.MkdirAll, so using it purely as a liveness probe creates DATA_DIR as a side effect.
Conclusions on the specific risks raised
Ordering: datadir.Acquire(config.DataDir()) runs before newApp() is called, so nothing opens a database or starts delivery recovery under a foreign lock. godotenv/autoload is a blank import of internal/config, so a .env-supplied DATA_DIR is in the environment at package-init time — lock and Config.DataDir cannot diverge.
Interaction with #218: no conflict.fx.App.Run calls os.Exit itself on the non-zero path, so the defer lock.Release() in run() IS skipped on the listen-failure exit — and it does not matter, because the kernel drops an advisory flock when the process's fds close. Verified on the real binary against next merged with this branch: instance B (own DATA_DIR, taken PORT) exits 1 on the listen-failure path, /proc/locks shows no surviving lock on that inode, instance C reuses the directory immediately.
kill -9: holder shows FLOCK ADVISORY WRITE <pid> in /proc/locks and /proc/<pid>/fd/4 -> .../webhooker.lock; after kill -9 the zero-byte lock file remains and the next instance starts. Release depends on nothing but the fd. Under GOGC=1 with traffic, lock and fd still held at 5/10/15/20/25/30s — nothing is finalized out from under the running process.
Refusal: real second process exits 1, stderr names the directory and lock file, stdout exactly 0 bytes. Clean SIGTERM exits 0 with all OnStop hooks run. Regression tests re-execute the test binary as a genuine second process, not two Acquire calls in one.
Also clean: base next; subject ends (closes #201); TODO.md untouched (#112); no attribution trailers; no scope creep; make fmt-check clean; terminology and naming fine.
Gate evidence (mine, not CI)
Run 1, head 445ef57, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — lintDONE 120.3s, 0 issues., zero (cached); make test failed on internal/handlers at 91.4s.
Run 2, same tree, builder re-executed — DONE 194.6s, zero (cached), internal/handlers PASSED at 91.548s against a 90s per-package budget.
Run 3, this branch merged onto a13e5b7 with the README conflict resolved by hand locally — lintDONE 124.8s, 0 issues., zero CACHED on lint/builder, zero (cached); 15 packages ok including internal/datadir 1.298s, cmd/webhooker 1.299s, internal/server 10.068s with TestListenFailure_ShutsDownTheApp PASS; internal/handlers timed out again at 91.858s.
Every internal/datadir and cmd/webhooker test passed in all three runs. The internal/handlers timeout is #225, not this change: the failing subtests are TestFailedLogin_LogLineDoesNotTrackUsernameSize/*, which touch nothing in this diff, and the package straddles the budget at 91.5s pass / 91.9s fail on the same tree. Host load average 172 (run 1) and 168 (run 3) across 48 cores. -race via script/test; linting Docker-only.
CI check / check (push) on 445ef57 is green (6m35s) — recorded, not relied on, per #119.
FAIL — `needs-rebase`. The lock itself is correct and independently verified; the branch no longer merges into `next`.
**BLOCKING — does not merge into current `next`**
`next` has advanced to `a13e5b7` since this branch was cut (https://git.eeqj.de/sneak/webhooker/pulls/216 and https://git.eeqj.de/sneak/webhooker/pulls/218 both landed). `README.md` conflicts: #216 inserted `#### Metrics credentials` and this PR inserts `#### Single-instance lock` at the same point, immediately after the environment-variable table (`README.md` around line 117).
```
git checkout -B t origin/next && git merge --no-ff origin/issue-201-datadir-lock
CONFLICT (content): Merge conflict in README.md
```
Gitea still shows `mergeable: true` because the PR record pins the stale base `10c8dd2`. Resolution is trivial — keep both `####` subsections — but the branch must be rebased onto `a13e5b7` and re-pushed, and the gate re-run there. No other file conflicts.
**Non-blocking findings**
1. `internal/datadir/lock.go`, comment on the `!held` branch: "TryLock leaves the descriptor open when it fails to take the lock, so it has to be closed explicitly" is not true of `gofrs/flock` v0.13.0. `(*Flock).try` registers `defer f.ensureFhState()` whenever it opened the handle itself, and on the `EWOULDBLOCK` return that closes and nils `fh`; the following `fl.Close()` is a no-op, since `Unlock` short-circuits on `f.fh == nil`. Keep the call as version-robust defence; the comment asserting a false property of the dependency is what should change.
2. `README.md`, `#### Single-instance lock` states the guarantee unconditionally. `flock(2)` is per-inode and host-local: it does arbitrate between containers sharing a bind-mounted/volume `DATA_DIR` on one host (the documented case), but not between hosts on a network filesystem, and a `DATA_DIR` in a container's own overlay layer is not shared at all. One sentence closes it. Same place: when the filesystem refuses `flock` outright the process fails closed and refuses to start — right behaviour, currently undocumented.
3. For https://git.eeqj.de/sneak/webhooker/issues/208 — `ErrLocked` is `%w`-wrapped and `errors.Is` matches, `Acquire` needs no fx. But `Acquire` calls `os.MkdirAll`, so using it purely as a liveness probe creates `DATA_DIR` as a side effect.
**Conclusions on the specific risks raised**
Ordering: `datadir.Acquire(config.DataDir())` runs before `newApp()` is called, so nothing opens a database or starts delivery recovery under a foreign lock. `godotenv/autoload` is a blank import of `internal/config`, so a `.env`-supplied `DATA_DIR` is in the environment at package-init time — lock and `Config.DataDir` cannot diverge.
Interaction with https://git.eeqj.de/sneak/webhooker/pulls/218: **no conflict.** `fx.App.Run` calls `os.Exit` itself on the non-zero path, so the `defer lock.Release()` in `run()` IS skipped on the listen-failure exit — and it does not matter, because the kernel drops an advisory `flock` when the process's fds close. Verified on the real binary against `next` merged with this branch: instance B (own `DATA_DIR`, taken `PORT`) exits 1 on the listen-failure path, `/proc/locks` shows no surviving lock on that inode, instance C reuses the directory immediately.
`kill -9`: holder shows `FLOCK ADVISORY WRITE <pid>` in `/proc/locks` and `/proc/<pid>/fd/4 -> .../webhooker.lock`; after `kill -9` the zero-byte lock file remains and the next instance starts. Release depends on nothing but the fd. Under `GOGC=1` with traffic, lock and fd still held at 5/10/15/20/25/30s — nothing is finalized out from under the running process.
Refusal: real second process exits 1, stderr names the directory and lock file, stdout exactly 0 bytes. Clean SIGTERM exits 0 with all `OnStop` hooks run. Regression tests re-execute the test binary as a genuine second process, not two `Acquire` calls in one.
Also clean: base `next`; subject ends `(closes #201)`; `TODO.md` untouched (https://git.eeqj.de/sneak/webhooker/issues/112); no attribution trailers; no scope creep; `make fmt-check` clean; terminology and naming fine.
**Gate evidence (mine, not CI)**
Run 1, head `445ef57`, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — `lint` `DONE 120.3s`, `0 issues.`, zero `(cached)`; `make test` failed on `internal/handlers` at 91.4s.
Run 2, same tree, builder re-executed — `DONE 194.6s`, zero `(cached)`, `internal/handlers` PASSED at 91.548s against a 90s per-package budget.
Run 3, this branch merged onto `a13e5b7` with the README conflict resolved by hand locally — `lint` `DONE 124.8s`, `0 issues.`, zero `CACHED` on `lint`/`builder`, zero `(cached)`; 15 packages ok including `internal/datadir` 1.298s, `cmd/webhooker` 1.299s, `internal/server` 10.068s with `TestListenFailure_ShutsDownTheApp` PASS; `internal/handlers` timed out again at 91.858s.
Every `internal/datadir` and `cmd/webhooker` test passed in all three runs. The `internal/handlers` timeout is https://git.eeqj.de/sneak/webhooker/issues/225, not this change: the failing subtests are `TestFailedLogin_LogLineDoesNotTrackUsernameSize/*`, which touch nothing in this diff, and the package straddles the budget at 91.5s pass / 91.9s fail on the same tree. Host load average 172 (run 1) and 168 (run 3) across 48 cores. `-race` via `script/test`; linting Docker-only.
CI `check / check (push)` on `445ef57` is green (6m35s) — recorded, not relied on, per https://git.eeqj.de/sneak/webhooker/issues/119.
Rebased onto next at a13e5b7, force-pushed as af88877. Still one commit; no implementation changes.
README conflict — resolved by keeping both subsections, in table order: #### Metrics credentials (from #216) then #### Single-instance lock, both between the env-var table and #### Trusted proxies. Neither was edited. No other file conflicted.
Non-blocking findings 1 and 2, fixed
internal/datadir/lock.go — the comment no longer asserts that TryLock leaves the descriptor open. The Close() call stays: "A no-op on flock v0.13.0, which closes its own descriptor on a failed TryLock; kept so no version can leak one."
README.md, #### Single-instance lock — one paragraph added: flock(2) is host-local and per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem, and a DATA_DIR in a container's own writable layer is not shared at all; on a filesystem that refuses flock, startup fails closed.
Finding 3 is a note for #208, not a change here. TODO.md untouched (#112).
Gate, re-run on the rebased tree
make check → exit 0, lint 0 issues. (DONE 68.4s).
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . → exit 0. Zero CACHED on lint/builder (the only CACHED steps are the two base-image FROMs and three runtime-stage steps), zero (cached) test lines anywhere, all 16 packages ran:
#20 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#20 74.80 0 issues.
#20 DONE 75.7s
#28 [builder 9/11] RUN make test
#28 67.09 ok sneak.berlin/go/webhooker/cmd/webhooker 1.118s
#28 73.40 ok sneak.berlin/go/webhooker/internal/datadir 1.103s
#28 101.4 ok sneak.berlin/go/webhooker/internal/handlers 34.295s
#28 101.4 ok sneak.berlin/go/webhooker/internal/server 5.280s
#28 DONE 102.0s
#29 [builder 10/11] RUN make build DONE 55.8s
No FAIL and no panic in the log. TestSecondInstanceRefused, TestRestartAfterHardKill, TestSecondFdInSameProcessRefused, TestRunRefusesLockedDataDir all PASS, as does TestListenFailure_ShutsDownTheApp from #218 on the merged tree. internal/handlers did not hit #225 this time — 34.3s against the 90s budget, host load average 70-93 across 48 cores during the run.
The build image was removed; no containers were started.
Rebased onto `next` at `a13e5b7`, force-pushed as `af88877`. Still one commit; no implementation changes.
**README conflict** — resolved by keeping both subsections, in table order: `#### Metrics credentials` (from https://git.eeqj.de/sneak/webhooker/pulls/216) then `#### Single-instance lock`, both between the env-var table and `#### Trusted proxies`. Neither was edited. No other file conflicted.
**Non-blocking findings 1 and 2, fixed**
1. `internal/datadir/lock.go` — the comment no longer asserts that `TryLock` leaves the descriptor open. The `Close()` call stays: "A no-op on flock v0.13.0, which closes its own descriptor on a failed TryLock; kept so no version can leak one."
2. `README.md`, `#### Single-instance lock` — one paragraph added: `flock(2)` is host-local and per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem, and a `DATA_DIR` in a container's own writable layer is not shared at all; on a filesystem that refuses `flock`, startup fails closed.
Finding 3 is a note for https://git.eeqj.de/sneak/webhooker/issues/208, not a change here. `TODO.md` untouched (https://git.eeqj.de/sneak/webhooker/issues/112).
**Gate, re-run on the rebased tree**
`make check` → exit 0, lint `0 issues.` (`DONE 68.4s`).
`docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` → exit 0. Zero `CACHED` on `lint`/`builder` (the only `CACHED` steps are the two base-image `FROM`s and three runtime-stage steps), zero `(cached)` test lines anywhere, all 16 packages ran:
```
#20 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#20 74.80 0 issues.
#20 DONE 75.7s
#28 [builder 9/11] RUN make test
#28 67.09 ok sneak.berlin/go/webhooker/cmd/webhooker 1.118s
#28 73.40 ok sneak.berlin/go/webhooker/internal/datadir 1.103s
#28 101.4 ok sneak.berlin/go/webhooker/internal/handlers 34.295s
#28 101.4 ok sneak.berlin/go/webhooker/internal/server 5.280s
#28 DONE 102.0s
#29 [builder 10/11] RUN make build DONE 55.8s
```
No `FAIL` and no `panic` in the log. `TestSecondInstanceRefused`, `TestRestartAfterHardKill`, `TestSecondFdInSameProcessRefused`, `TestRunRefusesLockedDataDir` all PASS, as does `TestListenFailure_ShutsDownTheApp` from https://git.eeqj.de/sneak/webhooker/pulls/218 on the merged tree. `internal/handlers` did not hit https://git.eeqj.de/sneak/webhooker/issues/225 this time — 34.3s against the 90s budget, host load average 70-93 across 48 cores during the run.
The build image was removed; no containers were started.
PASS — rebase is sound, nothing regressed. Branch is one commit af88877 on next at a13e5b7; test-merged into current next cleanly (merged tree byte-identical to the branch tree, so the gate below is also the gate on the merge result). Diffed the branch's own patch pre-rebase (445ef57^..445ef57, fetched by full SHA) against post-rebase (a13e5b7..af88877): identical apart from hunk offsets, the two-line lock.go comment, and the added README paragraph — main.go, main_test.go, go.mod, go.sum, lock_test.go blobs unchanged; lock.go differs only in that comment, with fl.Close() retained. The corrected comment is true against gofrs/flock v0.13.0 in the module cache (try defers ensureFhState, which resetFhs on the EWOULDBLOCK return, and Close/Unlock short-circuits on f.fh == nil, so the retained call is a genuine no-op, not an error). README kept both #### Metrics credentials and #### Single-instance lock unaltered in table order, no conflict markers; the fail-closed claim holds in code — a TryLock error propagates out of Acquire and run() returns 1. #216's metrics-auth logic survived the config.go replay intact. TODO.md untouched, no attribution trailers, subject ends (closes #201).
Disclosure: the CI status on af88877 is pending ("Waiting to run"), not green — not relied on either way, per #119; the gate below is my own.
Gate, my run on af88877: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . → exit 0. lint #20 DONE 75.5s with 0 issues. (that stage also runs make fmt-check, so the README formatting is covered); #28 [builder 9/11] RUN make test DONE 103.8s; #29 [builder 10/11] RUN make build DONE 58.4s. Zero CACHED on lint/builder — the only five are the two base-image FROMs (#4, #5) and three runtime stage-2 steps (#9, #10, #11); zero (cached) test lines, zero FAIL, zero panic: test timed out. All 16 packages ran, including internal/datadir 1.077s and cmd/webhooker 1.136s. TestSecondInstanceRefused, TestRestartAfterHardKill, TestSecondFdInSameProcessRefused, TestRunRefusesLockedDataDir and TestListenFailure_ShutsDownTheApp all PASS. internal/handlers at 31.611s did not hit #225. Host load average 87 at start, 42 at finish across 48 cores. Build image removed; no containers started.
PASS — rebase is sound, nothing regressed. Branch is one commit `af88877` on `next` at `a13e5b7`; test-merged into current `next` cleanly (merged tree byte-identical to the branch tree, so the gate below is also the gate on the merge result). Diffed the branch's own patch pre-rebase (`445ef57^..445ef57`, fetched by full SHA) against post-rebase (`a13e5b7..af88877`): identical apart from hunk offsets, the two-line `lock.go` comment, and the added README paragraph — `main.go`, `main_test.go`, `go.mod`, `go.sum`, `lock_test.go` blobs unchanged; `lock.go` differs only in that comment, with `fl.Close()` retained. The corrected comment is true against `gofrs/flock` v0.13.0 in the module cache (`try` defers `ensureFhState`, which `resetFh`s on the `EWOULDBLOCK` return, and `Close`/`Unlock` short-circuits on `f.fh == nil`, so the retained call is a genuine no-op, not an error). README kept both `#### Metrics credentials` and `#### Single-instance lock` unaltered in table order, no conflict markers; the fail-closed claim holds in code — a `TryLock` error propagates out of `Acquire` and `run()` returns 1. #216's metrics-auth logic survived the `config.go` replay intact. `TODO.md` untouched, no attribution trailers, subject ends `(closes #201)`.
Disclosure: the CI status on `af88877` is `pending` ("Waiting to run"), not green — not relied on either way, per https://git.eeqj.de/sneak/webhooker/issues/119; the gate below is my own.
Gate, my run on `af88877`: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` → exit 0. lint `#20 DONE 75.5s` with `0 issues.` (that stage also runs `make fmt-check`, so the README formatting is covered); `#28 [builder 9/11] RUN make test DONE 103.8s`; `#29 [builder 10/11] RUN make build DONE 58.4s`. Zero `CACHED` on `lint`/`builder` — the only five are the two base-image `FROM`s (`#4`, `#5`) and three runtime `stage-2` steps (`#9`, `#10`, `#11`); zero `(cached)` test lines, zero `FAIL`, zero `panic: test timed out`. All 16 packages ran, including `internal/datadir` 1.077s and `cmd/webhooker` 1.136s. `TestSecondInstanceRefused`, `TestRestartAfterHardKill`, `TestSecondFdInSameProcessRefused`, `TestRunRefusesLockedDataDir` and `TestListenFailure_ShutsDownTheApp` all PASS. `internal/handlers` at 31.611s did not hit https://git.eeqj.de/sneak/webhooker/issues/225. Host load average 87 at start, 42 at finish across 48 cores. Build image removed; no containers started.
clawbot
merged commit c6a9884f86 into next2026-08-20 07:23:01 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #201
What
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 — duplicate delivery to the destination, triggered by nothing worse than an overlapping deploy.The entry point now takes an exclusive advisory
flock(2)on{DATA_DIR}/webhooker.lockbefore anything opens a database, and holds it for the process lifetime.internal/datadir—Acquire(dir)/(*Lock).Release(), non-blockingTryLock, exportedErrLockedso a caller can tell "a live instance holds this" from any other failure. It createsDATA_DIRif absent, since it now runs beforeinternal/databasewould.cmd/webhooker—run(io.Writer) inttakes the lock, then runs the fx app under it. Acquisition is deliberately not in the fx graph: it has to happen before any database is opened, a refusal has to reach the operator as a plain line on stderr rather than as one entry in an fx failure dump, and the next entry point that touchesDATA_DIR(a CLI subcommand, per #208) takes it the same way rather than standing up a server module.config.DataDir()— resolvesDATA_DIR(with its default) once, for both the lock andConfig, so the two cannot lock one directory and write to another.#### Single-instance lockunder Configuration; the backup section's "Nothing else is written toDATA_DIR" was made true again.Why flock on a held fd, not a pidfile
The lock is the kernel's, not the file's. A process killed with
kill -9runs no cleanup and leaveswebhooker.lockbehind; the kernel drops the lock when the descriptor closes, so the next start is not blocked. A pidfile gets this wrong.The file is never unlinked, on release either — doing so would let the next process create and lock a fresh inode while a third still held the old one, which is the exact outcome this prevents.
The guarantee is host-local:
flock(2)is per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem. Documented in the README alongside the fail-closed behaviour on a filesystem that refusesflock.Library
github.com/gofrs/flockv0.13.0. Stdlib exports no file-locking API;syscall.Flockis unix-only and would need hand-written build-tagged wrappers. flock is the widely used, maintained option (golangci-lint, Terraform, containerd). Its own module minimums force two transitive bumps, which are the whole of thego.moddiff besides the new line:stretchr/testifyv1.8.4 → v1.11.1 andgolang.org/x/sysv0.33.0 → v0.37.0.Tests
internal/datadir/lock_test.gore-executes the test binary as a real second process:TestSecondInstanceRefused— a live second process is denied, the errorerrors.Is→ErrLocked, and its text names the directory.TestRestartAfterHardKill— holder isSIGKILLed, reaped, the stale lock file is asserted still present, and the nextAcquiresucceeds.cmd/webhookertest rests on), release-then-reacquire,DATA_DIRcreated when absent, empty dir rejected, unusable dir named in the error.cmd/webhooker:TestRunRefusesLockedDataDirpins the operator-facing contract — exit status 1, and stderr naming the directory.Gate evidence
Rebased onto
nextata13e5b7; the current gate evidence for the rebased tree is in #220 (comment) —make checkexit 0, and a cache-defeateddocker build --no-cache-filter=lint --no-cache-filter=builderwith0 issues.from the linter and all 16 test packages executed, none(cached).Also driven end to end against the built binary, two real processes on one
DATA_DIR:Every image built was removed; no containers were started.
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.FAIL —
needs-rebase. The lock itself is correct and independently verified; the branch no longer merges intonext.BLOCKING — does not merge into current
nextnexthas advanced toa13e5b7since this branch was cut (#216 and #218 both landed).README.mdconflicts: #216 inserted#### Metrics credentialsand this PR inserts#### Single-instance lockat the same point, immediately after the environment-variable table (README.mdaround line 117).Gitea still shows
mergeable: truebecause the PR record pins the stale base10c8dd2. Resolution is trivial — keep both####subsections — but the branch must be rebased ontoa13e5b7and re-pushed, and the gate re-run there. No other file conflicts.Non-blocking findings
internal/datadir/lock.go, comment on the!heldbranch: "TryLock leaves the descriptor open when it fails to take the lock, so it has to be closed explicitly" is not true ofgofrs/flockv0.13.0.(*Flock).tryregistersdefer f.ensureFhState()whenever it opened the handle itself, and on theEWOULDBLOCKreturn that closes and nilsfh; the followingfl.Close()is a no-op, sinceUnlockshort-circuits onf.fh == nil. Keep the call as version-robust defence; the comment asserting a false property of the dependency is what should change.README.md,#### Single-instance lockstates the guarantee unconditionally.flock(2)is per-inode and host-local: it does arbitrate between containers sharing a bind-mounted/volumeDATA_DIRon one host (the documented case), but not between hosts on a network filesystem, and aDATA_DIRin a container's own overlay layer is not shared at all. One sentence closes it. Same place: when the filesystem refusesflockoutright the process fails closed and refuses to start — right behaviour, currently undocumented.For #208 —
ErrLockedis%w-wrapped anderrors.Ismatches,Acquireneeds no fx. ButAcquirecallsos.MkdirAll, so using it purely as a liveness probe createsDATA_DIRas a side effect.Conclusions on the specific risks raised
Ordering:
datadir.Acquire(config.DataDir())runs beforenewApp()is called, so nothing opens a database or starts delivery recovery under a foreign lock.godotenv/autoloadis a blank import ofinternal/config, so a.env-suppliedDATA_DIRis in the environment at package-init time — lock andConfig.DataDircannot diverge.Interaction with #218: no conflict.
fx.App.Runcallsos.Exititself on the non-zero path, so thedefer lock.Release()inrun()IS skipped on the listen-failure exit — and it does not matter, because the kernel drops an advisoryflockwhen the process's fds close. Verified on the real binary againstnextmerged with this branch: instance B (ownDATA_DIR, takenPORT) exits 1 on the listen-failure path,/proc/locksshows no surviving lock on that inode, instance C reuses the directory immediately.kill -9: holder showsFLOCK ADVISORY WRITE <pid>in/proc/locksand/proc/<pid>/fd/4 -> .../webhooker.lock; afterkill -9the zero-byte lock file remains and the next instance starts. Release depends on nothing but the fd. UnderGOGC=1with traffic, lock and fd still held at 5/10/15/20/25/30s — nothing is finalized out from under the running process.Refusal: real second process exits 1, stderr names the directory and lock file, stdout exactly 0 bytes. Clean SIGTERM exits 0 with all
OnStophooks run. Regression tests re-execute the test binary as a genuine second process, not twoAcquirecalls in one.Also clean: base
next; subject ends(closes #201);TODO.mduntouched (#112); no attribution trailers; no scope creep;make fmt-checkclean; terminology and naming fine.Gate evidence (mine, not CI)
Run 1, head
445ef57,docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .—lintDONE 120.3s,0 issues., zero(cached);make testfailed oninternal/handlersat 91.4s.Run 2, same tree, builder re-executed —
DONE 194.6s, zero(cached),internal/handlersPASSED at 91.548s against a 90s per-package budget.Run 3, this branch merged onto
a13e5b7with the README conflict resolved by hand locally —lintDONE 124.8s,0 issues., zeroCACHEDonlint/builder, zero(cached); 15 packages ok includinginternal/datadir1.298s,cmd/webhooker1.299s,internal/server10.068s withTestListenFailure_ShutsDownTheAppPASS;internal/handlerstimed out again at 91.858s.Every
internal/datadirandcmd/webhookertest passed in all three runs. Theinternal/handlerstimeout is #225, not this change: the failing subtests areTestFailedLogin_LogLineDoesNotTrackUsernameSize/*, which touch nothing in this diff, and the package straddles the budget at 91.5s pass / 91.9s fail on the same tree. Host load average 172 (run 1) and 168 (run 3) across 48 cores.-raceviascript/test; linting Docker-only.CI
check / check (push)on445ef57is green (6m35s) — recorded, not relied on, per #119.445ef57adatoaf88877245Rebased onto
nextata13e5b7, force-pushed asaf88877. Still one commit; no implementation changes.README conflict — resolved by keeping both subsections, in table order:
#### Metrics credentials(from #216) then#### Single-instance lock, both between the env-var table and#### Trusted proxies. Neither was edited. No other file conflicted.Non-blocking findings 1 and 2, fixed
internal/datadir/lock.go— the comment no longer asserts thatTryLockleaves the descriptor open. TheClose()call stays: "A no-op on flock v0.13.0, which closes its own descriptor on a failed TryLock; kept so no version can leak one."README.md,#### Single-instance lock— one paragraph added:flock(2)is host-local and per-inode, so it arbitrates between processes and containers sharing a volume or bind mount on one machine, but not between hosts on a network filesystem, and aDATA_DIRin a container's own writable layer is not shared at all; on a filesystem that refusesflock, startup fails closed.Finding 3 is a note for #208, not a change here.
TODO.mduntouched (#112).Gate, re-run on the rebased tree
make check→ exit 0, lint0 issues.(DONE 68.4s).docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .→ exit 0. ZeroCACHEDonlint/builder(the onlyCACHEDsteps are the two base-imageFROMs and three runtime-stage steps), zero(cached)test lines anywhere, all 16 packages ran:No
FAILand nopanicin the log.TestSecondInstanceRefused,TestRestartAfterHardKill,TestSecondFdInSameProcessRefused,TestRunRefusesLockedDataDirall PASS, as doesTestListenFailure_ShutsDownTheAppfrom #218 on the merged tree.internal/handlersdid not hit #225 this time — 34.3s against the 90s budget, host load average 70-93 across 48 cores during the run.The build image was removed; no containers were started.
PASS — rebase is sound, nothing regressed. Branch is one commit
af88877onnextata13e5b7; test-merged into currentnextcleanly (merged tree byte-identical to the branch tree, so the gate below is also the gate on the merge result). Diffed the branch's own patch pre-rebase (445ef57^..445ef57, fetched by full SHA) against post-rebase (a13e5b7..af88877): identical apart from hunk offsets, the two-linelock.gocomment, and the added README paragraph —main.go,main_test.go,go.mod,go.sum,lock_test.goblobs unchanged;lock.godiffers only in that comment, withfl.Close()retained. The corrected comment is true againstgofrs/flockv0.13.0 in the module cache (trydefersensureFhState, whichresetFhs on theEWOULDBLOCKreturn, andClose/Unlockshort-circuits onf.fh == nil, so the retained call is a genuine no-op, not an error). README kept both#### Metrics credentialsand#### Single-instance lockunaltered in table order, no conflict markers; the fail-closed claim holds in code — aTryLockerror propagates out ofAcquireandrun()returns 1. #216's metrics-auth logic survived theconfig.goreplay intact.TODO.mduntouched, no attribution trailers, subject ends(closes #201).Disclosure: the CI status on
af88877ispending("Waiting to run"), not green — not relied on either way, per #119; the gate below is my own.Gate, my run on
af88877:docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .→ exit 0. lint#20 DONE 75.5swith0 issues.(that stage also runsmake fmt-check, so the README formatting is covered);#28 [builder 9/11] RUN make test DONE 103.8s;#29 [builder 10/11] RUN make build DONE 58.4s. ZeroCACHEDonlint/builder— the only five are the two base-imageFROMs (#4,#5) and three runtimestage-2steps (#9,#10,#11); zero(cached)test lines, zeroFAIL, zeropanic: test timed out. All 16 packages ran, includinginternal/datadir1.077s andcmd/webhooker1.136s.TestSecondInstanceRefused,TestRestartAfterHardKill,TestSecondFdInSameProcessRefused,TestRunRefusesLockedDataDirandTestListenFailure_ShutsDownTheAppall PASS.internal/handlersat 31.611s did not hit #225. Host load average 87 at start, 42 at finish across 48 cores. Build image removed; no containers started.