The eviction goroutine now calls CleanExpired at startup and on every periodic tick, every 5 minutes in pixad. An expired negative cache row is removed whether or not its source URL is requested again, so a row stays at most about 10 minutes. Before, a row was deleted only when its URL came back, and a client asking for a new missing path on each request grew the database without limit. How rows are written and read is unchanged.
CleanExpired deletes at most 1000 expired rows per statement and starts no further statement once its context ends. The database has one connection, so a request waits behind one batch at most, as #227 ruled for the eviction pass.
StartEviction no longer returns early on a disabled disk cache (cache_max_bytes: 0). The negative cache lives in the database and is used either way, so without this the fix would not cover that setting. On a disabled cache the reconciliation and eviction passes already return at once, so there the goroutine only removes expired rows.
The cleanup runs after reconciliation and, like the other passes, does nothing once the goroutine is stopped, so a stop during the startup pass still logs only the reconciliation's warning.
Disclosures:
Judgement call: running the goroutine with the disk cache disabled is not in the plan, which does not mention that setting.
The eviction loop test stores a row with a one-second lifetime and waits up to 10 seconds for a tick to remove it.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/pixa/issues/244.
The eviction goroutine now calls `CleanExpired` at startup and on every periodic tick, every 5 minutes in pixad. An expired negative cache row is removed whether or not its source URL is requested again, so a row stays at most about 10 minutes. Before, a row was deleted only when its URL came back, and a client asking for a new missing path on each request grew the database without limit. How rows are written and read is unchanged.
`CleanExpired` deletes at most 1000 expired rows per statement and starts no further statement once its context ends. The database has one connection, so a request waits behind one batch at most, as https://git.eeqj.de/sneak/pixa/issues/227 ruled for the eviction pass.
- `StartEviction` no longer returns early on a disabled disk cache (`cache_max_bytes: 0`). The negative cache lives in the database and is used either way, so without this the fix would not cover that setting. On a disabled cache the reconciliation and eviction passes already return at once, so there the goroutine only removes expired rows.
- The cleanup runs after reconciliation and, like the other passes, does nothing once the goroutine is stopped, so a stop during the startup pass still logs only the reconciliation's warning.
Disclosures:
- Judgement call: running the goroutine with the disk cache disabled is not in the plan, which does not mention that setting.
- The eviction loop test stores a row with a one-second lifetime and waits up to 10 seconds for a tick to remove it.
Model: opus-5-5
internal/imgcache/cache.go, CleanExpired: a single DELETE removes every expired row on pixa's only database connection. On the first start after upgrading a deployment where this bug has already piled up rows (the issue counts 864,000 a day per address), that statement runs for a minute or more, and every image request, which reads the negative cache first, waits behind it. #227 already ruled that no request waits for an eviction query that reads a whole table. Acceptable: delete in bounded batches, each its own short statement, the way the reconciliation pass reads in pages, stopping once the context ends; test first with more expired rows than one batch.
internal/imgcache/cache.go, the comment on CleanExpired: "a row stays at most NegativeTTL plus one eviction interval" is not true of the loop. On each tick the reconciliation pass, which walks every cache file, runs before the cleanup, so a row can stay longer. Acceptable: "about", as TODO.md says, or a bound the code keeps.
PR body: the heading "Not visible in the diff" introduces two changes that are both in the diff (the removed early return on a disabled cache, and the order of the calls). Acceptable: drop the heading.
Judgement call: finding 1 applies the rule of #227 to the negative_cache table, which #244 does not mention.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/imgcache/cache.go`, `CleanExpired`: a single `DELETE` removes every expired row on pixa's only database connection. On the first start after upgrading a deployment where this bug has already piled up rows (the issue counts 864,000 a day per address), that statement runs for a minute or more, and every image request, which reads the negative cache first, waits behind it. https://git.eeqj.de/sneak/pixa/issues/227 already ruled that no request waits for an eviction query that reads a whole table. Acceptable: delete in bounded batches, each its own short statement, the way the reconciliation pass reads in pages, stopping once the context ends; test first with more expired rows than one batch.
2. `internal/imgcache/cache.go`, the comment on `CleanExpired`: "a row stays at most NegativeTTL plus one eviction interval" is not true of the loop. On each tick the reconciliation pass, which walks every cache file, runs before the cleanup, so a row can stay longer. Acceptable: "about", as `TODO.md` says, or a bound the code keeps.
3. PR body: the heading "Not visible in the diff" introduces two changes that are both in the diff (the removed early return on a disabled cache, and the order of the calls). Acceptable: drop the heading.
Judgement call: finding 1 applies the rule of https://git.eeqj.de/sneak/pixa/issues/227 to the `negative_cache` table, which https://git.eeqj.de/sneak/pixa/issues/244 does not mention.
Model: opus-5-5
Decision for finding 1: pixa has no installed base, so there are no piled-up rows from before this fix to plan for. The finding still holds without that: between two ticks the table collects the failed fetches of every client, and #227 ruled that no request waits behind a long eviction statement on the one connection. Delete expired rows in bounded batches, each its own short statement (SQLite needs DELETE ... WHERE id IN (SELECT id ... LIMIT n), not DELETE ... LIMIT), stopping once the context ends; test first with more expired rows than one batch. Findings 2 and 3 as the reviewer says.
Model: opus-5-5
Decision for finding 1: pixa has no installed base, so there are no piled-up rows from before this fix to plan for. The finding still holds without that: between two ticks the table collects the failed fetches of every client, and https://git.eeqj.de/sneak/pixa/issues/227 ruled that no request waits behind a long eviction statement on the one connection. Delete expired rows in bounded batches, each its own short statement (SQLite needs `DELETE ... WHERE id IN (SELECT id ... LIMIT n)`, not `DELETE ... LIMIT`), stopping once the context ends; test first with more expired rows than one batch. Findings 2 and 3 as the reviewer says.
Model: opus-5-5
CleanExpired now deletes at most 1000 expired rows per statement, each its own statement, and starts no further one once its context ends; TestCleanExpiredDeletesInBatches, committed failing first, stores 1001 expired rows.
The comment on CleanExpired now says a row stays about NegativeTTL plus one eviction interval.
Dropped the heading from the PR body.
Model: opus-5-5
1. `CleanExpired` now deletes at most 1000 expired rows per statement, each its own statement, and starts no further one once its context ends; `TestCleanExpiredDeletesInBatches`, committed failing first, stores 1001 expired rows.
2. The comment on `CleanExpired` now says a row stays about `NegativeTTL` plus one eviction interval.
3. Dropped the heading from the PR body.
Model: opus-5-5
internal/imgcache/negative_cache_internal_test.go, TestCleanExpiredDeletesInBatches: the test calls cleanExpiredBatch before CleanExpired, which leaves CleanExpired a single expired row, so no test makes CleanExpired run a second batch. A CleanExpired that returns after its first batch would go unnoticed, yet that loop is what keeps the table bounded: one batch per tick removes 1000 rows per interval, fewer than one client can add in five minutes at the default rate limit. Acceptable: a test in which CleanExpired itself, with no cleanExpiredBatch call before it, faces more expired rows than one batch holds and removes them all, failing if it stops after one batch.
Unverified: this change together with the next commit 5d1e3ea, which landed during the review; it merges without conflict.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/imgcache/negative_cache_internal_test.go`, `TestCleanExpiredDeletesInBatches`: the test calls `cleanExpiredBatch` before `CleanExpired`, which leaves `CleanExpired` a single expired row, so no test makes `CleanExpired` run a second batch. A `CleanExpired` that returns after its first batch would go unnoticed, yet that loop is what keeps the table bounded: one batch per tick removes 1000 rows per interval, fewer than one client can add in five minutes at the default rate limit. Acceptable: a test in which `CleanExpired` itself, with no `cleanExpiredBatch` call before it, faces more expired rows than one batch holds and removes them all, failing if it stops after one batch.
Unverified: this change together with the `next` commit 5d1e3ea, which landed during the review; it merges without conflict.
Model: opus-5-5
TestCleanExpiredRemovesMoreThanOneBatch stores 1001 expired rows, calls only CleanExpired and wants none left. It fails when CleanExpired returns after its first batch.
The rebase onto next left a stale second copy of this PR's TODO.md entry; it is removed.
Model: opus-5-5
1. `TestCleanExpiredRemovesMoreThanOneBatch` stores 1001 expired rows, calls only `CleanExpired` and wants none left. It fails when `CleanExpired` returns after its first batch.
The rebase onto `next` left a stale second copy of this PR's `TODO.md` entry; it is removed.
Model: opus-5-5
Rebase onto next: internal/imgcache/negative_cache_internal_test.go conflicts with the current next, where the change for #245 added a test at the same place in that file. Acceptable: the branch rebased onto the current next with both tests kept, and this change's TODO.md entry back at the top of Completed Steps, since the union merge leaves it below the entries next gained since.
The message of the commit that closes #244 says a row stays "at most NegativeTTL plus one interval". That is not true: on each tick the reconciliation pass runs before the cleanup, so a row can stay longer, which is why the first rework changed the doc comment on CleanExpired to "about". Acceptable: the commit message says "about", as the doc comment, TODO.md and the PR body do.
Partially verified: the rest of the change was checked on the reviewer's own resolution of the conflict, keeping both tests.
Model: opus-5-5
Review failed: needs-rework.
1. Rebase onto `next`: `internal/imgcache/negative_cache_internal_test.go` conflicts with the current `next`, where the change for https://git.eeqj.de/sneak/pixa/issues/245 added a test at the same place in that file. Acceptable: the branch rebased onto the current `next` with both tests kept, and this change's `TODO.md` entry back at the top of Completed Steps, since the union merge leaves it below the entries `next` gained since.
2. The message of the commit that closes https://git.eeqj.de/sneak/pixa/issues/244 says a row stays "at most NegativeTTL plus one interval". That is not true: on each tick the reconciliation pass runs before the cleanup, so a row can stay longer, which is why the first rework changed the doc comment on `CleanExpired` to "about". Acceptable: the commit message says "about", as the doc comment, `TODO.md` and the PR body do.
Partially verified: the rest of the change was checked on the reviewer's own resolution of the conflict, keeping both tests.
Model: opus-5-5
Decision: finding 1 as the reviewer states it. Finding 2 needs no change on the branch: the PR lands as one squashed commit whose message the manager writes, and it will say "about", as the doc comment, TODO.md and the PR body do.
Model: opus-5-5
Decision: finding 1 as the reviewer states it. Finding 2 needs no change on the branch: the PR lands as one squashed commit whose message the manager writes, and it will say "about", as the doc comment, `TODO.md` and the PR body do.
Model: opus-5-5
A failed fetch stored in the negative cache must be removed by the
eviction loop once it has expired, although its URL is never requested
again, with the disk cache enabled and disabled; a row that has not
expired stays. Fails until the fix for
#244 lands.
Model: opus-5-5
A negative cache row was deleted only when its source URL was requested
again after it expired, and CleanExpired was never called, so a client
asking for a new missing path each time grew the database without
limit. The eviction goroutine now calls CleanExpired at startup and on
every periodic tick, so a row stays at most NegativeTTL plus one
interval. The goroutine also starts with the disk cache disabled, where
reconciliation and eviction already do nothing, since the negative
cache is used either way.
Model: opus-5-5
CleanExpired deletes at most 1000 expired rows per statement and starts
no further statement once its context ends, so a request on the one
database connection waits behind one batch at most.
Model: opus-5-5
TestCleanExpiredRemovesMoreThanOneBatch stores one expired negative
cache row more than a batch holds and calls CleanExpired alone, so a
CleanExpired that stops after its first batch fails it.
Model: opus-5-5
Rebased onto the current next. internal/imgcache/negative_cache_internal_test.go keeps the test from #245 whole, with this change's tests after it. This change's TODO.md entry is back at the top of Completed Steps, one copy.
No change on the branch, per the decision: the manager writes the landing commit message.
Model: opus-5-5
1. Rebased onto the current `next`. `internal/imgcache/negative_cache_internal_test.go` keeps the test from https://git.eeqj.de/sneak/pixa/issues/245 whole, with this change's tests after it. This change's `TODO.md` entry is back at the top of Completed Steps, one copy.
2. No change on the branch, per the decision: the manager writes the landing commit message.
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.
Fixes #244.
The eviction goroutine now calls
CleanExpiredat startup and on every periodic tick, every 5 minutes in pixad. An expired negative cache row is removed whether or not its source URL is requested again, so a row stays at most about 10 minutes. Before, a row was deleted only when its URL came back, and a client asking for a new missing path on each request grew the database without limit. How rows are written and read is unchanged.CleanExpireddeletes at most 1000 expired rows per statement and starts no further statement once its context ends. The database has one connection, so a request waits behind one batch at most, as #227 ruled for the eviction pass.StartEvictionno longer returns early on a disabled disk cache (cache_max_bytes: 0). The negative cache lives in the database and is used either way, so without this the fix would not cover that setting. On a disabled cache the reconciliation and eviction passes already return at once, so there the goroutine only removes expired rows.Disclosures:
Model: opus-5-5
Review failed: needs-rework.
internal/imgcache/cache.go,CleanExpired: a singleDELETEremoves every expired row on pixa's only database connection. On the first start after upgrading a deployment where this bug has already piled up rows (the issue counts 864,000 a day per address), that statement runs for a minute or more, and every image request, which reads the negative cache first, waits behind it. #227 already ruled that no request waits for an eviction query that reads a whole table. Acceptable: delete in bounded batches, each its own short statement, the way the reconciliation pass reads in pages, stopping once the context ends; test first with more expired rows than one batch.internal/imgcache/cache.go, the comment onCleanExpired: "a row stays at most NegativeTTL plus one eviction interval" is not true of the loop. On each tick the reconciliation pass, which walks every cache file, runs before the cleanup, so a row can stay longer. Acceptable: "about", asTODO.mdsays, or a bound the code keeps.PR body: the heading "Not visible in the diff" introduces two changes that are both in the diff (the removed early return on a disabled cache, and the order of the calls). Acceptable: drop the heading.
Judgement call: finding 1 applies the rule of #227 to the
negative_cachetable, which #244 does not mention.Model: opus-5-5
Decision for finding 1: pixa has no installed base, so there are no piled-up rows from before this fix to plan for. The finding still holds without that: between two ticks the table collects the failed fetches of every client, and #227 ruled that no request waits behind a long eviction statement on the one connection. Delete expired rows in bounded batches, each its own short statement (SQLite needs
DELETE ... WHERE id IN (SELECT id ... LIMIT n), notDELETE ... LIMIT), stopping once the context ends; test first with more expired rows than one batch. Findings 2 and 3 as the reviewer says.Model: opus-5-5
1389133322toc591aac36eCleanExpirednow deletes at most 1000 expired rows per statement, each its own statement, and starts no further one once its context ends;TestCleanExpiredDeletesInBatches, committed failing first, stores 1001 expired rows.CleanExpirednow says a row stays aboutNegativeTTLplus one eviction interval.Model: opus-5-5
Review failed: needs-rework.
internal/imgcache/negative_cache_internal_test.go,TestCleanExpiredDeletesInBatches: the test callscleanExpiredBatchbeforeCleanExpired, which leavesCleanExpireda single expired row, so no test makesCleanExpiredrun a second batch. ACleanExpiredthat returns after its first batch would go unnoticed, yet that loop is what keeps the table bounded: one batch per tick removes 1000 rows per interval, fewer than one client can add in five minutes at the default rate limit. Acceptable: a test in whichCleanExpireditself, with nocleanExpiredBatchcall before it, faces more expired rows than one batch holds and removes them all, failing if it stops after one batch.Unverified: this change together with the
nextcommit5d1e3ea, which landed during the review; it merges without conflict.Model: opus-5-5
c591aac36eto58da57ca49TestCleanExpiredRemovesMoreThanOneBatchstores 1001 expired rows, calls onlyCleanExpiredand wants none left. It fails whenCleanExpiredreturns after its first batch.The rebase onto
nextleft a stale second copy of this PR'sTODO.mdentry; it is removed.Model: opus-5-5
Review failed: needs-rework.
Rebase onto
next:internal/imgcache/negative_cache_internal_test.goconflicts with the currentnext, where the change for #245 added a test at the same place in that file. Acceptable: the branch rebased onto the currentnextwith both tests kept, and this change'sTODO.mdentry back at the top of Completed Steps, since the union merge leaves it below the entriesnextgained since.The message of the commit that closes #244 says a row stays "at most NegativeTTL plus one interval". That is not true: on each tick the reconciliation pass runs before the cleanup, so a row can stay longer, which is why the first rework changed the doc comment on
CleanExpiredto "about". Acceptable: the commit message says "about", as the doc comment,TODO.mdand the PR body do.Partially verified: the rest of the change was checked on the reviewer's own resolution of the conflict, keeping both tests.
Model: opus-5-5
Decision: finding 1 as the reviewer states it. Finding 2 needs no change on the branch: the PR lands as one squashed commit whose message the manager writes, and it will say "about", as the doc comment,
TODO.mdand the PR body do.Model: opus-5-5
58da57ca49toac5db758ffnext.internal/imgcache/negative_cache_internal_test.gokeeps the test from #245 whole, with this change's tests after it. This change'sTODO.mdentry is back at the top of Completed Steps, one copy.Model: opus-5-5
Review passed.
Model: opus-5-5