Repository navigation
Count cached metadata against max_size and evict it LRU - #507
Conversation
…n drops whichever was used least recently, so metadata no longer piles up for good (git-pkgs#461). Metadata rows record when they were last served, an artifact not yet served counts from its download, and Arch stores a database blob again if eviction removed it under a refreshed row.
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Please address durable metadata cleanup, move the data backfill out of the schema migration, and surface the new eviction metric in analytics.
|
|
||
| // Metadata goes now too: readers hold the whole document in memory, | ||
| // and a row refetched meanwhile refetches again on a missing blob. | ||
| if err := store.Delete(ctx, path); err != nil { |
There was a problem hiding this comment.
The new metadata path deletes the metadata_cache row before deleting the blob, and queues cleanup only after a storage error. A process crash between the row deletion and this call leaves a blob with neither a cache row nor a pending-delete record, so it disappears from max_size accounting and cannot be reclaimed by the queue. Delete the metadata row and enqueue its storage path in one database transaction, then let cleanup remove the blob. Please cover interruption and retry behavior.
| return fmt.Errorf("adding column last_accessed_at to metadata_cache: %w", err) | ||
| } | ||
| } | ||
| if err := backfillMetadataLastAccessed(db); err != nil { |
There was a problem hiding this comment.
Please keep this data backfill outside the schema migration. backfillMetadataLastAccessed loads every existing row into memory, then performs one UPDATE per row in a single transaction before startup can finish. Move it into bounded, resumable work and keep eviction safe while last_accessed_at is still NULL. Cover an upgrade with existing rows and an interrupted backfill.
|
|
||
| MetadataEvicted = prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Name: "proxy_metadata_evicted_total", |
There was a problem hiding this comment.
The new proxy_metadata_evicted_total metric is absent from /ui/analytics and metricSurface, contrary to CONTRIBUTING.md. TestEveryMetricIsSurfaced also does not instantiate this counter, so the normal test order hides the omission. Reproduced with go test ./internal/server -run 'TestEveryMetricIsSurfaced|TestEvictLRU_CountsAndEvictsMetadata' -count=2. Add the analytics display, the metricSurface entry, and a RecordMetadataEvicted call in the coverage test setup.
…, and cover the new counter in the metric surface test.
…ction, so a crash before the blob is deleted leaves it to reclaim instead of orphaning it.
…0 instead of inside the migration, resuming after a restart, and until a row is backfilled eviction orders it by its write time.
Fixes #461.
max_sizenow counts cached metadata as well as artifacts, and eviction drops whatever was served least recently.metadata_cachegetslast_accessed_at(migration 014, backfilled fromupdated_at), and every cached read records a hit, batched like artifact hits./statsis unchanged. New metric:proxy_metadata_evicted_total.Tested live with max_size 5MB on SQLite and Postgres: floods of sumdb hash tiles and large npm packuments always went back to 5 MB after the next eviction pass. On main the same setup stayed over the limit.
Not covered: storage can exceed the limit until the next pass (once a minute), and a metadata flood now evicts artifacts too.