sneak/pixa
1
0
Fork 0
Code Issues 20 Pull Requests 5 Actions 64 Packages Projects Releases Wiki Activity

Make next green: TestEvictionRunsOnPeriodicSchedule races the evictor's reconciliation and fails make test #183

New Issue
Closed
opened 2026-10-04 14:23:20 +02:00 by clawbot · 2 comments
clawbot commented 2026-10-04 14:23:20 +02:00
Collaborator
Copy Link
Copy Source

next is red. script/cibuild on a fresh clone of next (ba5a7162) fails in the make test step:

--- FAIL: TestEvictionRunsOnPeriodicSchedule (1.51s)
    eviction_internal_test.go:717: failed to insert variant accounting row: constraint failed: UNIQUE constraint failed: variant_content.cache_key (1555)

The test depends on timing. The commit that moved next (ba5a716) only adds tests under internal/handlers, and the previous head c7173c47 passed. The test starts the evictor first (StartEviction(100 * time.Millisecond)). Then, for each key, it stores the variant file and inserts that file's accounting row by hand. When the evictor's reconciliation runs between those two steps, it adopts the just-stored file into variant_content first (the log shows "adopted untracked variant file into size accounting cache_key=aabbccdd0001"), and the test's own INSERT then hits the unique key.

To reproduce: clone next fresh and run script/cibuild. It fails only when the reconciliation lands in that window, so it can take several runs.

Definition of done: the test sets up its over-limit state in a way the running evictor cannot interleave with, and still proves that only the periodic schedule triggers the eviction (no weakened assertion, no retry). script/cibuild on a fresh clone of next passes.

Top priority: this comes before all other pixa work. Labelled critical: a red next blocks every merge that has to keep it green.

Model: opus-5-5

`next` is red. `script/cibuild` on a fresh clone of `next` (`ba5a7162`) fails in the `make test` step: ``` --- FAIL: TestEvictionRunsOnPeriodicSchedule (1.51s) eviction_internal_test.go:717: failed to insert variant accounting row: constraint failed: UNIQUE constraint failed: variant_content.cache_key (1555) ``` The test depends on timing. The commit that moved `next` (`ba5a716`) only adds tests under `internal/handlers`, and the previous head `c7173c47` passed. The test starts the evictor first (`StartEviction(100 * time.Millisecond)`). Then, for each key, it stores the variant file and inserts that file's accounting row by hand. When the evictor's reconciliation runs between those two steps, it adopts the just-stored file into `variant_content` first (the log shows "adopted untracked variant file into size accounting cache_key=aabbccdd0001"), and the test's own `INSERT` then hits the unique key. To reproduce: clone `next` fresh and run `script/cibuild`. It fails only when the reconciliation lands in that window, so it can take several runs. Definition of done: the test sets up its over-limit state in a way the running evictor cannot interleave with, and still proves that only the periodic schedule triggers the eviction (no weakened assertion, no retry). `script/cibuild` on a fresh clone of `next` passes. Top priority: this comes before all other pixa work. Labelled critical: a red `next` blocks every merge that has to keep it green. Model: opus-5-5
clawbot added the critical label 2026-10-04 14:23:20 +02:00
clawbot commented 2026-10-04 15:09:28 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan for the pixa manager (it comes before all other pixa work).

Cause, on current next: evictionLoop in internal/imgcache/eviction.go runs runReconciliationPass at startup and on every tick, and reconcileVariantFiles adopts any variant file that has no accounting row. TestEvictionRunsOnPeriodicSchedule starts the evictor with a 100 ms interval and then, for each key, calls variants.Store and inserts the variant_content row by hand as a second step. A tick between the two steps adopts the file first, and the hand insert hits the unique key.

Requirements:

  • The test can no longer race the evictor: same result on every run, with no sleeps used as synchronisation and no retries.
  • It still proves what it claims: only the periodic ticker triggers the eviction (no store method, so no write-pressure notification), usage ends at or below the limit, and no dangling references remain. No assertion is weakened or removed.
  • Prefer a test-only change. One direction: let the periodic reconciliation adopt the stored files instead of inserting rows by hand, if that keeps the claim intact. Touch production code only if the test cannot be made deterministic otherwise, and keep that change minimal.
  • Check whether any other test in internal/imgcache does the same store-then-insert after StartEviction; fix those the same way.

Done when script/cibuild on a fresh clone of the branch rebased on current next passes, the fixed test has passed repeated runs through the repo's own targets, and an independent review passes.

Model: opus-5-5

Plan for the pixa manager (it comes before all other pixa work). Cause, on current `next`: `evictionLoop` in `internal/imgcache/eviction.go` runs `runReconciliationPass` at startup and on every tick, and `reconcileVariantFiles` adopts any variant file that has no accounting row. `TestEvictionRunsOnPeriodicSchedule` starts the evictor with a 100 ms interval and then, for each key, calls `variants.Store` and inserts the `variant_content` row by hand as a second step. A tick between the two steps adopts the file first, and the hand insert hits the unique key. Requirements: - The test can no longer race the evictor: same result on every run, with no sleeps used as synchronisation and no retries. - It still proves what it claims: only the periodic ticker triggers the eviction (no store method, so no write-pressure notification), usage ends at or below the limit, and no dangling references remain. No assertion is weakened or removed. - Prefer a test-only change. One direction: let the periodic reconciliation adopt the stored files instead of inserting rows by hand, if that keeps the claim intact. Touch production code only if the test cannot be made deterministic otherwise, and keep that change minimal. - Check whether any other test in `internal/imgcache` does the same store-then-insert after `StartEviction`; fix those the same way. Done when `script/cibuild` on a fresh clone of the branch rebased on current `next` passes, the fixed test has passed repeated runs through the repo's own targets, and an independent review passes. Model: opus-5-5
clawbot commented 2026-10-04 16:04:39 +02:00
Author
Collaborator
Copy Link
Copy Source

#187 changes only the test. It writes the variant files, with no accounting rows, while the evictor's startup pass waits on the test database's only connection. A periodic reconciliation pass then adopts the files, and the eviction pass that follows it evicts them.

Model: opus-5-5

https://git.eeqj.de/sneak/pixa/pulls/187 changes only the test. It writes the variant files, with no accounting rows, while the evictor's startup pass waits on the test database's only connection. A periodic reconciliation pass then adopts the files, and the eviction pass that follows it evicts them. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels critical
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#183