pixa's SQLite database now has one open connection (SetMaxOpenConns(1) in internal/database), the fix recommended on #223. pixa's own reads and writes run on it one at a time instead of competing for SQLite's lock. With several connections, a write that kept losing could wait past the five-second busy timeout and fail with database is locked, as TestConcurrentWritesAllSucceed showed under load.
The busy timeout stays for another program, such as the sqlite3 shell, writing to the same file.
With one connection, a query run while rows or a transaction are still open on the same *sql.DB would wait forever. Only internal/database and internal/imgcache query it; each reads its rows to the end and closes them before the next query, and the one transaction, in evictSourceBlob, runs only its own two statements. No test was added for this: the eviction and concurrent-miss tests in internal/imgcache and the image proxy test in internal/server already run that code on a one-connection database.
Requests now wait while eviction runs one of its queries, some of which read a whole table, so on a large cache a wait can be long. README.md says so; #227 makes those queries cheap.
TestService_Get_ReturnsByItsDeadline can still fail on a busy host for another reason: #224.
Judgement call: the comment above TestConcurrentWritesAllSucceed no longer says its writes run on separate connections; its code is unchanged, and the manager ruled a comment-only correction is not a change to the test under CLAUDE.md.
Model: opus-5-5
pixa's SQLite database now has one open connection (`SetMaxOpenConns(1)` in `internal/database`), the fix recommended on https://git.eeqj.de/sneak/pixa/issues/223. pixa's own reads and writes run on it one at a time instead of competing for SQLite's lock. With several connections, a write that kept losing could wait past the five-second busy timeout and fail with `database is locked`, as `TestConcurrentWritesAllSucceed` showed under load.
The busy timeout stays for another program, such as the `sqlite3` shell, writing to the same file.
With one connection, a query run while rows or a transaction are still open on the same `*sql.DB` would wait forever. Only `internal/database` and `internal/imgcache` query it; each reads its rows to the end and closes them before the next query, and the one transaction, in `evictSourceBlob`, runs only its own two statements. No test was added for this: the eviction and concurrent-miss tests in `internal/imgcache` and the image proxy test in `internal/server` already run that code on a one-connection database.
Requests now wait while eviction runs one of its queries, some of which read a whole table, so on a large cache a wait can be long. `README.md` says so; https://git.eeqj.de/sneak/pixa/issues/227 makes those queries cheap.
`TestService_Get_ReturnsByItsDeadline` can still fail on a busy host for another reason: https://git.eeqj.de/sneak/pixa/issues/224.
Judgement call: the comment above `TestConcurrentWritesAllSucceed` no longer says its writes run on separate connections; its code is unchanged, and the manager ruled a comment-only correction is not a change to the test under `CLAUDE.md`.
Model: opus-5-5
internal/database/database.go:268-271: with one connection, every query a request makes (the negative-cache check, the touch of the LRU timestamp and the hit or miss count, on a cache hit too) waits while the background eviction pass runs one of its queries. Some of those read whole tables: UsageBytes (internal/imgcache/eviction.go:56) sums both tables at the start of every pass, and every store wakes a pass; the reconciliation reads every variant key and every source hash in one query each (eviction.go:688, eviction.go:823). On a cache of a few hundred thousand transformed images, requests stall for as long as each of those queries runs. Before this change, in WAL mode, those reads ran beside requests. Nothing in the PR, README.md or the code says so. Acceptable: the eviction pass no longer holds up requests for the length of a whole-table read; or, if that wait is accepted, README.md says that requests wait while the eviction pass runs its queries, and the PR links an issue for making those queries cheap.
internal/database/concurrent_writes_internal_test.go:15-18: the comment says the goroutines' writes "run on separate connections, as one request's writes and the background eviction pass do". After this change they share pixa's one connection. Acceptable: the comment says what the test now checks, with its writers, requests and assertions unchanged.
internal/database/database.go:268 and TODO.md:34-35: "wait for it in turn" promises an order that does not exist. Go's database/sql gives a freed connection to a random waiting caller, not to the one that has waited longest. Acceptable: "one at a time", as README.md says.
Not verified: I could not make TestConcurrentWritesAllSucceed fail on the unchanged tree under load; the failure on record is the one in #223.
Model: opus-5-5
Review: FAIL
1. `internal/database/database.go:268-271`: with one connection, every query a request makes (the negative-cache check, the touch of the LRU timestamp and the hit or miss count, on a cache hit too) waits while the background eviction pass runs one of its queries. Some of those read whole tables: `UsageBytes` (`internal/imgcache/eviction.go:56`) sums both tables at the start of every pass, and every store wakes a pass; the reconciliation reads every variant key and every source hash in one query each (`eviction.go:688`, `eviction.go:823`). On a cache of a few hundred thousand transformed images, requests stall for as long as each of those queries runs. Before this change, in WAL mode, those reads ran beside requests. Nothing in the PR, `README.md` or the code says so. Acceptable: the eviction pass no longer holds up requests for the length of a whole-table read; or, if that wait is accepted, `README.md` says that requests wait while the eviction pass runs its queries, and the PR links an issue for making those queries cheap.
2. `internal/database/concurrent_writes_internal_test.go:15-18`: the comment says the goroutines' writes "run on separate connections, as one request's writes and the background eviction pass do". After this change they share pixa's one connection. Acceptable: the comment says what the test now checks, with its writers, requests and assertions unchanged.
3. `internal/database/database.go:268` and `TODO.md:34-35`: "wait for it in turn" promises an order that does not exist. Go's `database/sql` gives a freed connection to a random waiting caller, not to the one that has waited longest. Acceptable: "one at a time", as `README.md` says.
Not verified: I could not make `TestConcurrentWritesAllSucceed` fail on the unchanged tree under load; the failure on record is the one in https://git.eeqj.de/sneak/pixa/issues/223.
Model: opus-5-5
internal/database now opens the database with one connection. pixa's
own reads and writes run on it one at a time instead of competing for
SQLite's lock, where a write that kept losing could wait past the
five-second busy timeout and fail with "database is locked". The busy
timeout stays, for another program writing to the same file.
With one connection, a query run while rows or a transaction are still
open would wait forever. No code in internal/database or
internal/imgcache does that; the comment on DB() tells callers.
README.md says pixa uses one connection and that requests wait while
eviction runs one of its queries.
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.
pixa's SQLite database now has one open connection (
SetMaxOpenConns(1)ininternal/database), the fix recommended on #223. pixa's own reads and writes run on it one at a time instead of competing for SQLite's lock. With several connections, a write that kept losing could wait past the five-second busy timeout and fail withdatabase is locked, asTestConcurrentWritesAllSucceedshowed under load.The busy timeout stays for another program, such as the
sqlite3shell, writing to the same file.With one connection, a query run while rows or a transaction are still open on the same
*sql.DBwould wait forever. Onlyinternal/databaseandinternal/imgcachequery it; each reads its rows to the end and closes them before the next query, and the one transaction, inevictSourceBlob, runs only its own two statements. No test was added for this: the eviction and concurrent-miss tests ininternal/imgcacheand the image proxy test ininternal/serveralready run that code on a one-connection database.Requests now wait while eviction runs one of its queries, some of which read a whole table, so on a large cache a wait can be long.
README.mdsays so; #227 makes those queries cheap.TestService_Get_ReturnsByItsDeadlinecan still fail on a busy host for another reason: #224.Judgement call: the comment above
TestConcurrentWritesAllSucceedno longer says its writes run on separate connections; its code is unchanged, and the manager ruled a comment-only correction is not a change to the test underCLAUDE.md.Model: opus-5-5
Review: FAIL
internal/database/database.go:268-271: with one connection, every query a request makes (the negative-cache check, the touch of the LRU timestamp and the hit or miss count, on a cache hit too) waits while the background eviction pass runs one of its queries. Some of those read whole tables:UsageBytes(internal/imgcache/eviction.go:56) sums both tables at the start of every pass, and every store wakes a pass; the reconciliation reads every variant key and every source hash in one query each (eviction.go:688,eviction.go:823). On a cache of a few hundred thousand transformed images, requests stall for as long as each of those queries runs. Before this change, in WAL mode, those reads ran beside requests. Nothing in the PR,README.mdor the code says so. Acceptable: the eviction pass no longer holds up requests for the length of a whole-table read; or, if that wait is accepted,README.mdsays that requests wait while the eviction pass runs its queries, and the PR links an issue for making those queries cheap.internal/database/concurrent_writes_internal_test.go:15-18: the comment says the goroutines' writes "run on separate connections, as one request's writes and the background eviction pass do". After this change they share pixa's one connection. Acceptable: the comment says what the test now checks, with its writers, requests and assertions unchanged.internal/database/database.go:268andTODO.md:34-35: "wait for it in turn" promises an order that does not exist. Go'sdatabase/sqlgives a freed connection to a random waiting caller, not to the one that has waited longest. Acceptable: "one at a time", asREADME.mdsays.Not verified: I could not make
TestConcurrentWritesAllSucceedfail on the unchanged tree under load; the failure on record is the one in #223.Model: opus-5-5
9e64435e66tofc080147afReview passed.
Not reproduced:
TestConcurrentWritesAllSucceeddid not fail on the unchangednextunder the gate.Model: opus-5-5