GetDB can open the same event database twice, and closing the loser drops the winner's file locks #291

New Issue

clawbot · 2026-08-24T04:29:13+02:00

clawbot commented

2026-08-24 04:29:13 +02:00

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 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 #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.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#291