Found during the review of #287. Pre-existing; that PR does not introduce it.
WebhookDBManager.GetDB (internal/database/webhook_db_manager.go:85-105) uses an optimistic sync.Map pattern: Load, then openDB on a miss, then LoadOrStore. Two goroutines racing on the same webhook can therefore both open the SAME events-*.db, and the one that loses LoadOrStore then calls Close() on its handle.
That close is the problem. On POSIX, close() on ANY file descriptor referring to an inode drops everyfcntl lock this process holds on that inode — including the winner's. So the loser's cleanup can silently strip the surviving handle's locks.
Why it is latent rather than urgent:
Same-process only. The cross-process case that would make a lost WAL DMS lock genuinely dangerous is already blocked by the exclusive DATA_DIR lock from #201.
It needs two goroutines to miss the cache for the same webhook in the same instant, which requires concurrent first-touch of one webhook.
Why it is worth recording anyway: the consequence is not an error, it is silently weakened locking on a handle that keeps working. Nothing would surface it until something else depended on those locks. And #256 has just made the SQLite layer's locking behaviour load-bearing in a way it was not before.
Definition of done
One open per database, guaranteed. The usual shape is a per-key mutex or singleflight so the second caller waits rather than opening its own handle and discarding it. Do not solve it by leaking the loser's handle instead of closing it — that trades a lock bug for an fd leak.
A test that drives concurrent first-touch of one webhook and asserts exactly one open occurred.
While there, check whether any other cache in the tree uses the same optimistic open-then-discard shape. The archive writer cache is the obvious neighbour.
Not milestoned: no known failure, and the cross-process case is blocked by the startup lock.
Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/287. Pre-existing; that PR does not introduce it.
`WebhookDBManager.GetDB` (`internal/database/webhook_db_manager.go:85-105`) uses an optimistic `sync.Map` pattern: `Load`, then `openDB` on a miss, then `LoadOrStore`. Two goroutines racing on the same webhook can therefore both open the SAME `events-*.db`, and the one that loses `LoadOrStore` then calls `Close()` on its handle.
That close is the problem. On POSIX, `close()` on ANY file descriptor referring to an inode drops **every** `fcntl` lock this process holds on that inode — including the winner's. So the loser's cleanup can silently strip the surviving handle's locks.
Why it is latent rather than urgent:
- Same-process only. The cross-process case that would make a lost WAL DMS lock genuinely dangerous is already blocked by the exclusive `DATA_DIR` lock from https://git.eeqj.de/sneak/webhooker/issues/201.
- It needs two goroutines to miss the cache for the same webhook in the same instant, which requires concurrent first-touch of one webhook.
Why it is worth recording anyway: the consequence is not an error, it is silently weakened locking on a handle that keeps working. Nothing would surface it until something else depended on those locks. And https://git.eeqj.de/sneak/webhooker/issues/256 has just made the SQLite layer's locking behaviour load-bearing in a way it was not before.
## Definition of done
- One open per database, guaranteed. The usual shape is a per-key mutex or `singleflight` so the second caller waits rather than opening its own handle and discarding it. Do not solve it by leaking the loser's handle instead of closing it — that trades a lock bug for an fd leak.
- A test that drives concurrent first-touch of one webhook and asserts exactly one open occurred.
- While there, check whether any other cache in the tree uses the same optimistic open-then-discard shape. The archive writer cache is the obvious neighbour.
Not milestoned: no known failure, and the cross-process case is blocked by the startup lock.
Plan. The code the issue names is unchanged on next (6ebac4f): GetDB (internal/database/webhook_db_manager.go) opens on a cache miss and then calls LoadOrStore, and the loser closes its handle.
Fix: serialize the open path. One mutex taken on a cache miss: re-check the map under it, then open and store. A database is opened once per webhook per process, so the cost is nil. That is plainer than per-key locks or a new dependency. The cached fast path stays lock-free.
Neighbours:DeleteDB and CloseAll must not interleave with an open for the same webhook in a way that leaves a closed handle cached or reopens a deleted file. Check them, and take the same mutex where they need it.
Test: concurrent first touch of one webhook from many goroutines, asserting exactly one open. The test needs a way to count opens; keep it minimal and plain.
Other caches: check the tree for the same open-then-discard shape. The archive writer registry (internal/delivery/target_database*.go) is the neighbour the issue names; the circuit-breaker LoadOrStore in internal/delivery/target_http.go holds no file. Fix a real instance in this unit if it is small; otherwise file it as its own issue and say so on the PR.
Model: opus-5-5
Plan. The code the issue names is unchanged on `next` (`6ebac4f`): `GetDB` (`internal/database/webhook_db_manager.go`) opens on a cache miss and then calls `LoadOrStore`, and the loser closes its handle.
- **Fix:** serialize the open path. One mutex taken on a cache miss: re-check the map under it, then open and store. A database is opened once per webhook per process, so the cost is nil. That is plainer than per-key locks or a new dependency. The cached fast path stays lock-free.
- **Neighbours:** `DeleteDB` and `CloseAll` must not interleave with an open for the same webhook in a way that leaves a closed handle cached or reopens a deleted file. Check them, and take the same mutex where they need it.
- **Test:** concurrent first touch of one webhook from many goroutines, asserting exactly one open. The test needs a way to count opens; keep it minimal and plain.
- **Other caches:** check the tree for the same open-then-discard shape. The archive writer registry (`internal/delivery/target_database*.go`) is the neighbour the issue names; the circuit-breaker `LoadOrStore` in `internal/delivery/target_http.go` holds no file. Fix a real instance in this unit if it is small; otherwise file it as its own issue and say so on the PR.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 03:14:51 +02:00
Fixed in #327: one mutex now covers opening, deleting and closing event databases, so a webhook's file is opened once; a racing caller waits for the first caller's handle. The PR body covers the other caches and one doubt about the lock-loss consequence described above.
Model: opus-5-5
Fixed in https://git.eeqj.de/sneak/webhooker/pulls/327: one mutex now covers opening, deleting and closing event databases, so a webhook's file is opened once; a racing caller waits for the first caller's handle. The PR body covers the other caches and one doubt about the lock-loss consequence described above.
Model: opus-5-5
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.
Found during the review of #287. Pre-existing; that PR does not introduce it.
WebhookDBManager.GetDB(internal/database/webhook_db_manager.go:85-105) uses an optimisticsync.Mappattern:Load, thenopenDBon a miss, thenLoadOrStore. Two goroutines racing on the same webhook can therefore both open the SAMEevents-*.db, and the one that losesLoadOrStorethen callsClose()on its handle.That close is the problem. On POSIX,
close()on ANY file descriptor referring to an inode drops everyfcntllock this process holds on that inode — including the winner's. So the loser's cleanup can silently strip the surviving handle's locks.Why it is latent rather than urgent:
DATA_DIRlock from #201.Why it is worth recording anyway: the consequence is not an error, it is silently weakened locking on a handle that keeps working. Nothing would surface it until something else depended on those locks. And #256 has just made the SQLite layer's locking behaviour load-bearing in a way it was not before.
Definition of done
singleflightso the second caller waits rather than opening its own handle and discarding it. Do not solve it by leaking the loser's handle instead of closing it — that trades a lock bug for an fd leak.Not milestoned: no known failure, and the cross-process case is blocked by the startup lock.
Plan. The code the issue names is unchanged on
next(6ebac4f):GetDB(internal/database/webhook_db_manager.go) opens on a cache miss and then callsLoadOrStore, and the loser closes its handle.DeleteDBandCloseAllmust not interleave with an open for the same webhook in a way that leaves a closed handle cached or reopens a deleted file. Check them, and take the same mutex where they need it.internal/delivery/target_database*.go) is the neighbour the issue names; the circuit-breakerLoadOrStoreininternal/delivery/target_http.goholds no file. Fix a real instance in this unit if it is small; otherwise file it as its own issue and say so on the PR.Model: opus-5-5
Fixed in #327: one mutex now covers opening, deleting and closing event databases, so a webhook's file is opened once; a racing caller waits for the first caller's handle. The PR body covers the other caches and one doubt about the lock-loss consequence described above.
Model: opus-5-5