[simonchrz]

Eviction gets slower with every artifact it has ever cleared.

Eviction and retention clear an artifact's storage_path and keep the row. In idx_artifacts_last_used those rows sort first: they were the least recently used, and a cleared row that was never served has no date left at all. GetLeastRecentlyUsedCacheEntries reads that index in order and looks up each row to find 50 that are still in storage, so every batch walks past every cleared row. Eviction asks for a batch again and again until the cache fits.

The index is now partial, on storage_path IS NOT NULL. The eviction query already filters on that, so the query is unchanged. Migration 025 drops and recreates the index in one transaction.

Measured with 200,000 artifacts, 150,000 of them cleared, one batch of 50:

	full index	partial index
SQLite	39 ms	0.07 ms
Postgres	8.2 ms	0.9 ms

Both return the same 50 entries. The migration took 34 ms on SQLite and 12 ms on Postgres for that table. On Postgres, CREATE INDEX blocks writes to artifacts while it builds.

Tests cover the index definition in a fresh schema, the migration from the old index (run twice, and from no index), and that eviction still lists stored artifacts in order with cleared ones left out. They run on SQLite and Postgres.

#709 also adds migrations; whichever lands later needs renumbering.

Summary by CodeRabbit

• Bug Fixes
  • Cache cleanup now selects stored artifacts in least-recently-used order, excluding artifacts without a storage location. This prevents artifacts that cannot be used for storage from appearing in cleanup results.
  • Database index behavior is consistent across SQLite and PostgreSQL, including when upgrading existing databases. Existing databases are updated to use the same index behavior as new databases.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The SQLite and PostgreSQL schemas define idx_artifacts_last_used to include only artifacts with a non-NULL storage_path. Migration 025 replaces the index. Tests cover fresh schemas, migration behavior, and cache-eviction results.

Changes: Partial artifact index

Layer / File(s)	Summary
Index definition, migration, and validation internal/database/schema.go, internal/database/cache_eviction_index_test.go	The SQLite and PostgreSQL schema definitions use a non-NULL storage_path predicate. Migration 025 replaces the index in a transaction. Tests verify fresh schemas, repeated migration runs, index recreation, and eviction results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: pinguinfuss

Merge Risk: Minimal · up to ca14e

This change narrows the artifact last-used index to stored artifacts, so cache eviction reads skip cleared rows. No behavior change or concrete defect was identified. The migration is transactional and safe to rerun.

Security architecture risk: Low · up to ca14e

The index change preserves eviction eligibility and supports transactional recovery. During a PostgreSQL upgrade, however, rebuilding the index can temporarily block artifact reads and writes across instances sharing the database.

Retained concerns

• Low · reliability · inferred: On shared PostgreSQL deployments, startup migration 025 can block artifact reads and writes across already-serving instances. Ordinary DROP INDEX takes an ACCESS EXCLUSIVE table lock, held through the subsequent index build until commit. The schema advisory lock serializes migrations but does not drain serving traffic, and this migration supplies no local deadline. Transactional replacement protects consistency, not uninterrupted availability.

Security review details

Security Blast Radius

• inferred — The changed operation is scoped to the artifacts index in the connected database. Its availability effects can reach artifact-table users across instances sharing that PostgreSQL database, rather than only the instance performing the upgrade. No attacker-triggered migration path is established by the inspected evidence.

Trust Boundaries and Controls

• observed — Artifact eviction eligibility remains enforced in the SELECT predicate, not by index membership alone. The broader index membership for stored local artifacts therefore does not weaken the existing local-artifact exclusion.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title "Keep cleared artifacts out of the index eviction reads" matches the main change. The PR makes idx_artifacts_last_used a partial index on storage_path IS NOT NULL, so cleared artifacts (rows…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.

Full details: Title check

Explanation

The title "Keep cleared artifacts out of the index eviction reads" matches the main change. The PR makes idx_artifacts_last_used a partial index on storage_path IS NOT NULL, so cleared artifacts (rows with NULL storage_path) are excluded from the eviction reads. The schema.go changes and the new cache-eviction index tests both support this. The title is specific and does not describe unrelated work.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 784c07da-1fda-4bc7-96e9-8787c45e1acc

Commits

Reviewing files that changed from the base of the PR and between 13ae3b9 and ca14ed9.

Files selected for processing: 1

• internal/database/schema.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.