StartEviction makes the loop's context with context.WithCancel(context.Background()) and keeps its cancel func. That context replaces the stop channel.
StopEviction(ctx) cancels it, then waits until the loop exits or ctx ends. If ctx ends first, it returns an error wrapping ctx's error. Still safe when never started, or called twice.
The handlers' stop hook passes fx's stop context and returns that error. An eviction still running at fx's stop deadline then fails the stop (exit code 1), like unfinished image processing since #169.
The two directory walks of the reconciliation pass return the context's error at each entry once it is cancelled.
Once the context is cancelled, the loop starts no further pass, and an eviction candidate that fails ends the pass without a warning of its own. A stop logs at most one warning, from the pass it interrupted.
Not in the diff: an interrupted pass leaves the accounting consistent, because rows are still deleted in one transaction before files are removed.
Disclosures:
Deviation: the reconciliation's two row loops also stop once the context is cancelled. This has no test of its own.
Judgement call: the StopEviction error says cache eviction still running so fx's log names what timed out.
//nolint:contextcheck on the start hook stays because the linter still reports it; its comment is now one line.
The four existing tests calling StopEviction() now pass t.Context(), nothing else changed in them.
README.md describes neither shutdown nor the eviction loop, so it is unchanged.
Model: opus-5-5
Implements the plan on https://git.eeqj.de/sneak/pixa/issues/102#issuecomment-119757.
- `StartEviction` makes the loop's context with `context.WithCancel(context.Background())` and keeps its cancel func. That context replaces the stop channel.
- `StopEviction(ctx)` cancels it, then waits until the loop exits or `ctx` ends. If `ctx` ends first, it returns an error wrapping `ctx`'s error. Still safe when never started, or called twice.
- The handlers' stop hook passes fx's stop context and returns that error. An eviction still running at fx's stop deadline then fails the stop (exit code 1), like unfinished image processing since https://git.eeqj.de/sneak/pixa/pulls/169.
- The two directory walks of the reconciliation pass return the context's error at each entry once it is cancelled.
- Once the context is cancelled, the loop starts no further pass, and an eviction candidate that fails ends the pass without a warning of its own. A stop logs at most one warning, from the pass it interrupted.
Not in the diff: an interrupted pass leaves the accounting consistent, because rows are still deleted in one transaction before files are removed.
Disclosures:
- Deviation: the reconciliation's two row loops also stop once the context is cancelled. This has no test of its own.
- Judgement call: the `StopEviction` error says `cache eviction still running` so fx's log names what timed out.
- `//nolint:contextcheck` on the start hook stays because the linter still reports it; its comment is now one line.
- The four existing tests calling `StopEviction()` now pass `t.Context()`, nothing else changed in them.
- `README.md` describes neither shutdown nor the eviction loop, so it is unchanged.
Model: opus-5-5
The branch at 213117e does not rebase cleanly onto next at 3a274aa. Conflicting file: TODO.md. This branch and next each add a new entry at the top of "Completed Steps". Acceptable: rebase onto current next and keep both entries.
Not verified: the code change itself. It gets a full review once the branch is rebased.
Model: opus-5-5
**FAIL** (needs-rebase)
1. The branch at `213117e` does not rebase cleanly onto `next` at `3a274aa`. Conflicting file: `TODO.md`. This branch and `next` each add a new entry at the top of "Completed Steps". Acceptable: rebase onto current `next` and keep both entries.
Not verified: the code change itself. It gets a full review once the branch is rebased.
Model: opus-5-5
Reviewed 213117e rebased onto next at 5b17d1f. TODO.md was the only conflict; I resolved it locally by keeping both entries.
A stop can log more than one warning, because the eviction loop starts new work after it is cancelled. In evictionLoop (internal/imgcache/eviction.go), when StopEviction cancels a reconciliation pass (at startup or on the periodic tick), the loop still starts the eviction pass. That pass fails at once and logs a second warning, cache eviction pass failed, with failed to compute cache usage: context canceled. A pending write-pressure wakeup can start one more pass. In evictBatch, a candidate whose database call is cut off by the cancel logs failed to evict cache entry before the pass logs its own warning. This also contradicts the comment on evictionLoop, which says the loop returns when its context is cancelled. Acceptable: once its context is cancelled, the loop returns without starting another pass, and a candidate that fails because the context was cancelled ends the pass without a warning of its own. A stop then logs at most one warning, and a test counts the warnings of a stop during reconciliation.
The new check between eviction candidates in evictBatch has no test, and removing it leaves every test passing. Yet it changes what a stop does: without it, each remaining candidate fails and logs its own warning. The existing evictSourceBlobTestHook can show this: store several source blobs, cancel the context from the hook, then check that EvictToLimit returns context.Canceled, the other blobs are still on disk, and no warning is logged for any of them. Acceptable: a test like that, which fails when the code that stops between candidates is removed.
Judgement call: the same check in the two reconciliation row loops has no test either. I accept that, because its only effect is to skip file checks that would otherwise still run, and testing it would need a new test hook.
Model: opus-5-5
**FAIL** (needs-rework)
Reviewed `213117e` rebased onto `next` at `5b17d1f`. `TODO.md` was the only conflict; I resolved it locally by keeping both entries.
1. A stop can log more than one warning, because the eviction loop starts new work after it is cancelled. In `evictionLoop` (`internal/imgcache/eviction.go`), when `StopEviction` cancels a reconciliation pass (at startup or on the periodic tick), the loop still starts the eviction pass. That pass fails at once and logs a second warning, `cache eviction pass failed`, with `failed to compute cache usage: context canceled`. A pending write-pressure wakeup can start one more pass. In `evictBatch`, a candidate whose database call is cut off by the cancel logs `failed to evict cache entry` before the pass logs its own warning. This also contradicts the comment on `evictionLoop`, which says the loop returns when its context is cancelled. Acceptable: once its context is cancelled, the loop returns without starting another pass, and a candidate that fails because the context was cancelled ends the pass without a warning of its own. A stop then logs at most one warning, and a test counts the warnings of a stop during reconciliation.
2. The new check between eviction candidates in `evictBatch` has no test, and removing it leaves every test passing. Yet it changes what a stop does: without it, each remaining candidate fails and logs its own warning. The existing `evictSourceBlobTestHook` can show this: store several source blobs, cancel the context from the hook, then check that `EvictToLimit` returns `context.Canceled`, the other blobs are still on disk, and no warning is logged for any of them. Acceptable: a test like that, which fails when the code that stops between candidates is removed.
Judgement call: the same check in the two reconciliation row loops has no test either. I accept that, because its only effect is to skip file checks that would otherwise still run, and testing it would need a new test hook.
Model: opus-5-5
Both pass runners now do nothing once the loop's context is cancelled, so no pass starts after a cancelled reconciliation, a pending write-pressure wakeup or a pending tick; in evictBatch a candidate that fails after cancellation ends the pass with the context's error and no warning of its own. TestStopEvictionInterruptsPassInProgress now expects exactly one warning from the stop.
New TestEvictToLimitStopsAtNextCandidateOnceCancelled cancels from evictSourceBlobTestHook while the oldest of three source blobs is evicted and expects context.Canceled, the other two blobs still on disk, and no warning; it fails without the check in evictBatch. That check now runs where a candidate fails instead of before each candidate, because a separate check before each candidate changed nothing a test could observe.
Model: opus-5-5
1. Both pass runners now do nothing once the loop's context is cancelled, so no pass starts after a cancelled reconciliation, a pending write-pressure wakeup or a pending tick; in `evictBatch` a candidate that fails after cancellation ends the pass with the context's error and no warning of its own. `TestStopEvictionInterruptsPassInProgress` now expects exactly one warning from the stop.
2. New `TestEvictToLimitStopsAtNextCandidateOnceCancelled` cancels from `evictSourceBlobTestHook` while the oldest of three source blobs is evicted and expects `context.Canceled`, the other two blobs still on disk, and no warning; it fails without the check in `evictBatch`. That check now runs where a candidate fails instead of before each candidate, because a separate check before each candidate changed nothing a test could observe.
Model: opus-5-5
Reviewed 12b4415, which is already based on next at 5b17d1f.
The new check at the top of runReconciliationPass (internal/imgcache/eviction.go) has no test, and removing it leaves every test passing. Yet it is what keeps a stop to one warning when a periodic tick is due while the stop interrupts an eviction pass: without it, the loop can take that tick after the interrupted pass and log a second warning, cache accounting reconciliation failed with context canceled. Acceptable: a test that fails when that check is removed, for example one that calls runReconciliationPass with an already cancelled context and expects no warning.
Judgement call: in evictBatch, a candidate that fails for some other reason just as the stop cancels also ends the pass without a warning of its own. I accept that, because the next reconciliation pass repairs what it leaves behind.
Not verified: the order and timing of the server's and the handlers' stop hooks in a running process. I checked them by reading the code only.
Model: opus-5-5
**FAIL** (needs-rework)
Reviewed `12b4415`, which is already based on `next` at `5b17d1f`.
1. The new check at the top of `runReconciliationPass` (`internal/imgcache/eviction.go`) has no test, and removing it leaves every test passing. Yet it is what keeps a stop to one warning when a periodic tick is due while the stop interrupts an eviction pass: without it, the loop can take that tick after the interrupted pass and log a second warning, `cache accounting reconciliation failed` with `context canceled`. Acceptable: a test that fails when that check is removed, for example one that calls `runReconciliationPass` with an already cancelled context and expects no warning.
Judgement call: in `evictBatch`, a candidate that fails for some other reason just as the stop cancels also ends the pass without a warning of its own. I accept that, because the next reconciliation pass repairs what it leaves behind.
Not verified: the order and timing of the server's and the handlers' stop hooks in a running process. I checked them by reading the code only.
Model: opus-5-5
StartEviction runs the eviction goroutine with its own context, which
StopEviction cancels in place of the old stop channel, so a pass in
progress stops at its next database call, file, row or eviction
candidate instead of running to completion. StopEviction takes a
context: when it ends before the goroutine exits, StopEviction stops
waiting and returns an error wrapping it. The handlers' stop hook
passes fx's stop context, so an eviction still running at fx's stop
deadline fails the stop and the exit code is 1. The contextcheck
suppression on the start hook stays, with a one-line reason: the loop
outlives OnStart.
Model: opus-5-5
The stop-during-reconciliation test now also expects exactly one
warning; it fails while the loop still starts an eviction pass after a
cancelled reconciliation. A new test cancels the context while the
oldest of three source blobs is evicted and expects EvictToLimit to
return context.Canceled, leave the other two on disk and log no
warning for them.
Model: opus-5-5
Both pass runners do nothing once the loop's context is cancelled, so
a stop no longer starts an eviction pass after a cancelled
reconciliation or on a pending write-pressure wakeup or tick. In
evictBatch, a candidate that fails after cancellation ends the batch
with the context's error instead of logging its own warning; this
replaces the check before each candidate, since every candidate
started after cancellation fails at its first database call. A stop
now logs at most one warning.
Model: opus-5-5
A reconciliation pass run with an already cancelled context logs no
warning, so a periodic tick the eviction loop takes after a stop adds
no warning to the one from the pass the stop interrupted.
Model: opus-5-5
New TestReconciliationPassLogsNoWarningOnceCancelled runs runReconciliationPass with an already cancelled context and expects no warning; it fails without the check at the top of runReconciliationPass. The branch is also rebased onto current next, keeping both TODO.md entries.
Model: opus-5-5
1. New `TestReconciliationPassLogsNoWarningOnceCancelled` runs `runReconciliationPass` with an already cancelled context and expects no warning; it fails without the check at the top of `runReconciliationPass`. The branch is also rebased onto current `next`, keeping both `TODO.md` entries.
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.
Implements the plan on #102 (comment).
StartEvictionmakes the loop's context withcontext.WithCancel(context.Background())and keeps its cancel func. That context replaces the stop channel.StopEviction(ctx)cancels it, then waits until the loop exits orctxends. Ifctxends first, it returns an error wrappingctx's error. Still safe when never started, or called twice.Not in the diff: an interrupted pass leaves the accounting consistent, because rows are still deleted in one transaction before files are removed.
Disclosures:
StopEvictionerror sayscache eviction still runningso fx's log names what timed out.//nolint:contextcheckon the start hook stays because the linter still reports it; its comment is now one line.StopEviction()now passt.Context(), nothing else changed in them.README.mddescribes neither shutdown nor the eviction loop, so it is unchanged.Model: opus-5-5
FAIL (needs-rebase)
213117edoes not rebase cleanly ontonextat3a274aa. Conflicting file:TODO.md. This branch andnexteach add a new entry at the top of "Completed Steps". Acceptable: rebase onto currentnextand keep both entries.Not verified: the code change itself. It gets a full review once the branch is rebased.
Model: opus-5-5
FAIL (needs-rework)
Reviewed
213117erebased ontonextat5b17d1f.TODO.mdwas the only conflict; I resolved it locally by keeping both entries.A stop can log more than one warning, because the eviction loop starts new work after it is cancelled. In
evictionLoop(internal/imgcache/eviction.go), whenStopEvictioncancels a reconciliation pass (at startup or on the periodic tick), the loop still starts the eviction pass. That pass fails at once and logs a second warning,cache eviction pass failed, withfailed to compute cache usage: context canceled. A pending write-pressure wakeup can start one more pass. InevictBatch, a candidate whose database call is cut off by the cancel logsfailed to evict cache entrybefore the pass logs its own warning. This also contradicts the comment onevictionLoop, which says the loop returns when its context is cancelled. Acceptable: once its context is cancelled, the loop returns without starting another pass, and a candidate that fails because the context was cancelled ends the pass without a warning of its own. A stop then logs at most one warning, and a test counts the warnings of a stop during reconciliation.The new check between eviction candidates in
evictBatchhas no test, and removing it leaves every test passing. Yet it changes what a stop does: without it, each remaining candidate fails and logs its own warning. The existingevictSourceBlobTestHookcan show this: store several source blobs, cancel the context from the hook, then check thatEvictToLimitreturnscontext.Canceled, the other blobs are still on disk, and no warning is logged for any of them. Acceptable: a test like that, which fails when the code that stops between candidates is removed.Judgement call: the same check in the two reconciliation row loops has no test either. I accept that, because its only effect is to skip file checks that would otherwise still run, and testing it would need a new test hook.
Model: opus-5-5
213117e2eeto742b6fa4c1evictBatcha candidate that fails after cancellation ends the pass with the context's error and no warning of its own.TestStopEvictionInterruptsPassInProgressnow expects exactly one warning from the stop.TestEvictToLimitStopsAtNextCandidateOnceCancelledcancels fromevictSourceBlobTestHookwhile the oldest of three source blobs is evicted and expectscontext.Canceled, the other two blobs still on disk, and no warning; it fails without the check inevictBatch. That check now runs where a candidate fails instead of before each candidate, because a separate check before each candidate changed nothing a test could observe.Model: opus-5-5
FAIL (needs-rework)
Reviewed
12b4415, which is already based onnextat5b17d1f.runReconciliationPass(internal/imgcache/eviction.go) has no test, and removing it leaves every test passing. Yet it is what keeps a stop to one warning when a periodic tick is due while the stop interrupts an eviction pass: without it, the loop can take that tick after the interrupted pass and log a second warning,cache accounting reconciliation failedwithcontext canceled. Acceptable: a test that fails when that check is removed, for example one that callsrunReconciliationPasswith an already cancelled context and expects no warning.Judgement call: in
evictBatch, a candidate that fails for some other reason just as the stop cancels also ends the pass without a warning of its own. I accept that, because the next reconciliation pass repairs what it leaves behind.Not verified: the order and timing of the server's and the handlers' stop hooks in a running process. I checked them by reading the code only.
Model: opus-5-5
12b4415c57tofbe45af544TestReconciliationPassLogsNoWarningOnceCancelledrunsrunReconciliationPasswith an already cancelled context and expects no warning; it fails without the check at the top ofrunReconciliationPass. The branch is also rebased onto currentnext, keeping bothTODO.mdentries.Model: opus-5-5
PASS
fbe45af544506a46f99b7f37641ca0b991850a08, rebased ontonextat6830bdc5de40f60c2051568dbe6b1f2a08b449fd.Model: opus-5-5