[profiling]

What changed

Retention checks last_accessed_at in the database before clearing an artifact. With batched hit writes enabled, a download can record a hit after the sweep's initial flush while that hit is still only in memory. The retention update can then clear an artifact that was just served.

Serialize batch flushes with retention clears, flush pending artifact hits before the conditional clear, and hold the batch locks through that clear. If writing hits fails, abort the clear and restore the batch. Add a regression test for a hit buffered after the sweep's initial flush.

Validation

• go test ./internal/database ./internal/retention passes.
• go test ./... is blocked by the local Git 2.23.0 binary, which does not support git init --initial-branch used by existing Git-cache tests.
• go vet ./... could not complete because the local Go toolchain panicked while decoding dependency export data.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of activity tracking during cleanup of expired artifacts. Pending activity is processed before cleanup proceeds, helping prevent recorded hits from being lost when cleanup and batch processing overlap.
  • Cleanup now stops if pending activity cannot be processed, preserving the existing data for a later attempt.

[coderabbitai]

Walkthrough

Hit flushes are now serialized. ClearExpiredArtifact flushes pending hits before clearing an artifact and stops if that flush fails. Failed batches are merged back into pending hits.

Changes: Artifact retention and hit flushing

Layer / File(s)	Summary
Serialize hit flushes internal/database/hits.go	A mutex serializes flush operations, including metadata flushing and restoring hits after a write failure.
Flush hits before clearing artifacts internal/database/retention.go, internal/database/retention_test.go	ClearExpiredArtifact flushes pending hits before its transaction and stops if the flush fails. Failed batches are merged back by count and latest timestamp. A regression test checks that clearing does not proceed after a hit is recorded.

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

Change: Bug fix

Suggested labels: bug

Suggested reviewers: simonchrz, pinguinfuss

Merge Risk: Minimal · up to 1a2ea

The change is mergeable after normal checks. No concrete failure in the retention behavior was established.

Security architecture risk: Moderate · up to 1a2ea

The change protects recently used artifacts, but a stalled cleanup operation can now delay unrelated downloads. No expansion of access or permissions was identified.

Retained concerns

• Medium · reliability · inferred: Retention now holds the DB-instance-wide hit-state mutex while flushing all pending artifact hits and completing the clear transaction. Cached downloads, HEAD responses and signed-URL responses record hits synchronously before returning, so a slow or blocked retention write can stall requests for unrelated artifacts on the same instance. The base retention path did not hold this mutex, and ordinary batch flushing releases it before database I/O. Deferred unlocks contain returned errors, but this path has no context-bound database operation to release the mutex during a prolonged wait. This is a newly widened availability failure domain, not a verified attacker-triggered denial of service.

Security review details

Security Blast Radius

• inferred — The introduced failure-containment concern spans cached requests sharing one DB object's hit batch, rather than only the artifact being expired. It does not establish cross-instance blocking, additional database privileges or access to another environment.

Security Findings and Attack Paths

• observed — Artifact requests feed hit accounting before a cached result returns. Retention policy, not request-supplied timing parameters, determines the cutoff used by the clear. This establishes request-path coupling to the new lock, but does not establish that an external requester can prolong the database operation or bypass an access control.

Trust Boundaries and Controls

• observed — The changed control is a persistence-ordering gate: failure to persist pending artifact hits prevents deletion eligibility from being acted upon. Existing conditional SQL and transactional queueing continue to constrain the destructive operation.

Resilience and Maintainability Implications

• inferred — Deferred rollback and unlocks recover local resources when operations return. They do not bound an in-flight database wait: sweep cancellation is checked between candidates, while ClearExpiredArtifact and writeHits use database operations without a passed context. This makes cancellation relevant to the newly shared request-blocking domain.

Hardening Proposals

• proposed — Bound retention database work with cancellation-aware operations while preserving flush-before-clear ordering and failed-batch restoration, so prolonged maintenance waits do not indefinitely hold the shared hit-state mutex.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "Prevent retention from clearing recently served artifacts" matches the main change. The raw summary and PR objectives show that retention clears now flush buffered artifact hits first, hold…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.

Full details: Title check

Explanation

The title "Prevent retention from clearing recently served artifacts" matches the main change. The raw summary and PR objectives show that retention clears now flush buffered artifact hits first, hold the batch locks through the clear, and abort the clear if hit writes fail. These changes stop retention from removing artifacts that were recently served. The title is specific and does not use vague terms. It does not list every detail, such as the mutex in flushHits or the new regression test, and that is acceptable.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 3 files.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

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

[coderabbitai]

Additional comments: 1

internal/database/retention.go: 1

86-94: Trivial · Nitpick

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Holding both hit locks through the clear is intentional. Lock ordering is consistent.

flushHits takes flushMu, then calls take and add, which take mu only briefly. ClearExpiredArtifact takes flushMu, then mu. Both paths use the same order, so no deadlock results.

flushHitsLocked bypasses take and add. It does not re-acquire mu, so the non-reentrant mutex is safe.

One side effect: RecordArtifactHit blocks on mu while the clear transaction runs. This applies to every request that records a hit. The delay is bounded by the transaction time. Watch it if clears slow down on Postgres.

Source: Learnings

Review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: d81219bf-fc18-464c-9ea5-392d19d3eb17

Commits

Reviewing files that changed from the base of the PR and between b6e71f4 and 1a2ea93.

Files selected for processing: 3

• internal/database/hits.go
• internal/database/retention.go
• internal/database/retention_test.go

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

[simonchrz]

Thanks for this. The race is real, and the locking is correct as far as I can see: both paths take flushMu before mu, so they can't deadlock. I'd like the clear to be cheaper before this merges.

What it costs

hits.mu is taken by every cached download and every metadata hit. ClearExpiredArtifact now holds it over two transactions for each artifact it clears. I measured 1000 clears in a row on local SQLite while 8 goroutines recorded hits:

	main	this PR
RecordArtifactHit, median	~100 ns	1.1 ms
RecordArtifactHit, max	5–60 µs	1.7–3.5 ms

That is on a fast local disk. On a slow disk, or with Postgres over a network, each transaction takes longer and so does the wait. Hits are batched so that requests don't wait on database writes.

What it doesn't close

• A request reads the record and opens the object before it records its hit (readStored). A clear in between still goes through.
• Proxies that share a Postgres database don't see each other's buffers.

The delete queue's grace period is what keeps such a download working (see the comment on sweepRetention), so what the race costs is one refetch.

Suggestion

In ClearExpiredArtifact, take hits.mu only to look the artifact up in pending, and return false when it has a hit at or after cutoff. That needs no write under the lock, no flushMu and no flushHitsLocked, and it narrows the window about as much. Your test should pass with it unchanged.