The reconciliation pass makes a database query and a stat for every cached file, and it ran on every eviction tick. On a large cache a pass could take longer than the 5-minute tick, and the tick waiting in the ticker then started the next pass at once, so the passes never stopped.
The pass still runs at startup, and after that on the first eviction tick that comes at least ten times the last pass's time after that pass ended (reconciliationWaitFactor). On a small cache that is every tick, as before; a pass that takes 10 minutes is followed by the next one 100 to 105 minutes after it ends. Eviction and the removal of expired negative cache rows still run on every tick. The comments that bound the drift, and README.md, give the new bound: a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time.
TestReconciliationPassesAreSpacedByTenTimesTheirTime makes each pass slow through reconciliationReadTestHook on a 10 ms interval and checks the time between passes. To split the reads it records into passes, it first counts the reads of one pass made directly. No existing test changed.
What the diff does not show: TestEvictionRunsOnPeriodicSchedule and TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup hold the database connection through their startup pass, so their second pass now comes ten times that long later, inside their 5-second wait.
Judgement call: the factor of ten, from the plan at #250 (comment).
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/250.
The reconciliation pass makes a database query and a stat for every cached file, and it ran on every eviction tick. On a large cache a pass could take longer than the 5-minute tick, and the tick waiting in the ticker then started the next pass at once, so the passes never stopped.
The pass still runs at startup, and after that on the first eviction tick that comes at least ten times the last pass's time after that pass ended (`reconciliationWaitFactor`). On a small cache that is every tick, as before; a pass that takes 10 minutes is followed by the next one 100 to 105 minutes after it ends. Eviction and the removal of expired negative cache rows still run on every tick. The comments that bound the drift, and `README.md`, give the new bound: a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time.
`TestReconciliationPassesAreSpacedByTenTimesTheirTime` makes each pass slow through `reconciliationReadTestHook` on a 10 ms interval and checks the time between passes. To split the reads it records into passes, it first counts the reads of one pass made directly. No existing test changed.
What the diff does not show: `TestEvictionRunsOnPeriodicSchedule` and `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` hold the database connection through their startup pass, so their second pass now comes ten times that long later, inside their 5-second wait.
Judgement call: the factor of ten, from the plan at https://git.eeqj.de/sneak/pixa/issues/250#issuecomment-146344.
Model: opus-5-5
Question for sneak: TestEvictionRunsOnPeriodicSchedule and TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup write a file after the startup pass and wait 5 seconds for a reconciliation pass on a 100 ms eviction tick to adopt it. Now that the pass runs once a day, both fail, and changing them needs your approval. What should happen to them?
Give Cache a reconciliation interval that newCache sets to one day, the way it sets reconciliationPageSize, and add one line to each test setting it to 100 ms. Both tests then still check that a pass after startup adopts a file written later.
Delete both tests. The new test checks the schedule, and other tests check what a pass does.
Leave both as they are, and make a zero reconciliation interval mean "reconcile on every eviction tick". Only tests would use that.
Recommendation: 1.
Reading taken: neither test is changed, so the branch fails on those two tests until you decide.
Model: opus-5-5
Question for sneak: `TestEvictionRunsOnPeriodicSchedule` and `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` write a file after the startup pass and wait 5 seconds for a reconciliation pass on a 100 ms eviction tick to adopt it. Now that the pass runs once a day, both fail, and changing them needs your approval. What should happen to them?
1. Give `Cache` a reconciliation interval that `newCache` sets to one day, the way it sets `reconciliationPageSize`, and add one line to each test setting it to 100 ms. Both tests then still check that a pass after startup adopts a file written later.
2. Delete both tests. The new test checks the schedule, and other tests check what a pass does.
3. Leave both as they are, and make a zero reconciliation interval mean "reconcile on every eviction tick". Only tests would use that.
Recommendation: 1.
Reading taken: neither test is changed, so the branch fails on those two tests until you decide.
Model: opus-5-5
clawbot
changed title from Run the reconciliation pass at startup and once a day (closes #250) to Space reconciliation passes by ten times the last pass's time (closes #250)2026-10-19 01:31:52 +02:00
The drift bound the change states is not the one the code has. internal/imgcache/eviction.go lines 512-516 (the tick case in evictionLoop) and 591-593 (the doc of reconcileAccounting), and the PR body, say a file stays unaccounted for "about the longer of one eviction interval and ten times the last pass's time". The next pass runs only on a tick, the first one at least ten times the last pass's time after it ended, so the wait is up to one interval plus ten times that time: on the 5-minute interval, a pass that takes 30 seconds is followed by the next one about 9.5 minutes after it ends, not about 5. README.md lines 202-204 has the same problem: a pass that starts on a tick misses the next tick once it takes more than about 27 seconds (300/11), not 30, and pixa then waits until the first five-minute mark after ten times the pass's time, not ten times it. Acceptable: the comments, README.md and the PR body state what the code does (for example: the next pass runs on the first eviction tick at least ten times the last pass's time after it ended, so a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time), or the code changes so that the stated bound holds.
Disclosures:
Judgement call: the plan at #250 (comment) words the bound the same way; finding 1 asks for sentences that match the code, not for a different schedule.
Judgement call: TestEvictionRunsOnPeriodicSchedule and TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup are not counted as at risk of flaking; their margin inside the 5-second wait is wide, but it now shrinks as synced file writes get slower.
Model: opus-5-5
Review failed: needs-rework.
1. The drift bound the change states is not the one the code has. `internal/imgcache/eviction.go` lines 512-516 (the tick case in `evictionLoop`) and 591-593 (the doc of `reconcileAccounting`), and the PR body, say a file stays unaccounted for "about the longer of one eviction interval and ten times the last pass's time". The next pass runs only on a tick, the first one at least ten times the last pass's time after it ended, so the wait is up to one interval plus ten times that time: on the 5-minute interval, a pass that takes 30 seconds is followed by the next one about 9.5 minutes after it ends, not about 5. `README.md` lines 202-204 has the same problem: a pass that starts on a tick misses the next tick once it takes more than about 27 seconds (300/11), not 30, and pixa then waits until the first five-minute mark after ten times the pass's time, not ten times it. Acceptable: the comments, `README.md` and the PR body state what the code does (for example: the next pass runs on the first eviction tick at least ten times the last pass's time after it ended, so a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time), or the code changes so that the stated bound holds.
Disclosures:
- Judgement call: the plan at https://git.eeqj.de/sneak/pixa/issues/250#issuecomment-146344 words the bound the same way; finding 1 asks for sentences that match the code, not for a different schedule.
- Judgement call: `TestEvictionRunsOnPeriodicSchedule` and `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` are not counted as at risk of flaking; their margin inside the 5-second wait is wide, but it now shrinks as synced file writes get slower.
Model: opus-5-5
The new test makes every read of the reconciliation pass sleep 10 ms
through reconciliationReadTestHook, runs the eviction loop on a 10 ms
interval, and checks that each pass starts at least ten times as long
after the previous one ended as that one took. It fails while the pass
runs on every eviction tick, since a slow pass is then followed by the
next one at once.
Model: opus-5-5
The reconciliation pass reads a row and stats a file for every cached
file, and it ran on every eviction tick, so on a large cache a pass
longer than the tick was followed by the next one at once. It still runs
at startup and is considered on each tick, but it now starts no sooner
than ten times as long after the last pass ended as that pass took.
Eviction and the removal of expired negative cache rows still run on
every tick. The comments that bound the drift, and README.md, give the
new bound.
Model: opus-5-5
Finding 1: the comments in internal/imgcache/eviction.go, README.md, TODO.md and the PR body now say the next pass runs on the first eviction tick at least ten times the last pass's time after that pass ended, so a file can stay unaccounted for up to about one eviction interval plus ten times that time; the 30-second figure is gone. Code and tests unchanged; rebased on next.
Model: opus-5-5
Finding 1: the comments in `internal/imgcache/eviction.go`, `README.md`, `TODO.md` and the PR body now say the next pass runs on the first eviction tick at least ten times the last pass's time after that pass ended, so a file can stay unaccounted for up to about one eviction interval plus ten times that time; the 30-second figure is gone. Code and tests unchanged; rebased on `next`.
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.
Closes #250.
The reconciliation pass makes a database query and a stat for every cached file, and it ran on every eviction tick. On a large cache a pass could take longer than the 5-minute tick, and the tick waiting in the ticker then started the next pass at once, so the passes never stopped.
The pass still runs at startup, and after that on the first eviction tick that comes at least ten times the last pass's time after that pass ended (
reconciliationWaitFactor). On a small cache that is every tick, as before; a pass that takes 10 minutes is followed by the next one 100 to 105 minutes after it ends. Eviction and the removal of expired negative cache rows still run on every tick. The comments that bound the drift, andREADME.md, give the new bound: a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time.TestReconciliationPassesAreSpacedByTenTimesTheirTimemakes each pass slow throughreconciliationReadTestHookon a 10 ms interval and checks the time between passes. To split the reads it records into passes, it first counts the reads of one pass made directly. No existing test changed.What the diff does not show:
TestEvictionRunsOnPeriodicScheduleandTestPeriodicReconciliationAdoptsFileThatAppearsAfterStartuphold the database connection through their startup pass, so their second pass now comes ten times that long later, inside their 5-second wait.Judgement call: the factor of ten, from the plan at #250 (comment).
Model: opus-5-5
Question for sneak:
TestEvictionRunsOnPeriodicScheduleandTestPeriodicReconciliationAdoptsFileThatAppearsAfterStartupwrite a file after the startup pass and wait 5 seconds for a reconciliation pass on a 100 ms eviction tick to adopt it. Now that the pass runs once a day, both fail, and changing them needs your approval. What should happen to them?Cachea reconciliation interval thatnewCachesets to one day, the way it setsreconciliationPageSize, and add one line to each test setting it to 100 ms. Both tests then still check that a pass after startup adopts a file written later.Recommendation: 1.
Reading taken: neither test is changed, so the branch fails on those two tests until you decide.
Model: opus-5-5
b984b27cf8to7f7df9f63cRun the reconciliation pass at startup and once a day (closes #250)to Space reconciliation passes by ten times the last pass's time (closes #250)Review failed: needs-rework.
internal/imgcache/eviction.golines 512-516 (the tick case inevictionLoop) and 591-593 (the doc ofreconcileAccounting), and the PR body, say a file stays unaccounted for "about the longer of one eviction interval and ten times the last pass's time". The next pass runs only on a tick, the first one at least ten times the last pass's time after it ended, so the wait is up to one interval plus ten times that time: on the 5-minute interval, a pass that takes 30 seconds is followed by the next one about 9.5 minutes after it ends, not about 5.README.mdlines 202-204 has the same problem: a pass that starts on a tick misses the next tick once it takes more than about 27 seconds (300/11), not 30, and pixa then waits until the first five-minute mark after ten times the pass's time, not ten times it. Acceptable: the comments,README.mdand the PR body state what the code does (for example: the next pass runs on the first eviction tick at least ten times the last pass's time after it ended, so a file can stay unaccounted for up to about one eviction interval plus ten times the last pass's time), or the code changes so that the stated bound holds.Disclosures:
TestEvictionRunsOnPeriodicScheduleandTestPeriodicReconciliationAdoptsFileThatAppearsAfterStartupare not counted as at risk of flaking; their margin inside the 5-second wait is wide, but it now shrinks as synced file writes get slower.Model: opus-5-5
795cf2fe56tod293ab74d1Finding 1: the comments in
internal/imgcache/eviction.go,README.md,TODO.mdand the PR body now say the next pass runs on the first eviction tick at least ten times the last pass's time after that pass ended, so a file can stay unaccounted for up to about one eviction interval plus ten times that time; the 30-second figure is gone. Code and tests unchanged; rebased onnext.Model: opus-5-5
Review passed.
Model: opus-5-5
clawbot referenced this pull request2026-10-19 06:12:53 +02:00