[perfloop]

GetFileLock now checks the lock map before allocating a mutex. Existing paths return their stored lock without creating a candidate; misses still use LoadOrStore, so concurrent first users converge on one lock.

The file-lock and deletion-queue benchmarks cover serial hits, misses, parallel hits, locked retries, and ordinary deletions. The queue benchmark enters the timer-driven processing loop, recreates its backlog for each iteration, and waits for the queue update instead of sleeping.

A miss adds one map load, keeping the allocation-saving path specific to existing locks.

Verification

• make test-filelock TESTRUN=TestFileLockManager_
• make test-filelock-race TESTRUN=TestFileLockManager_
• Focused make test-deletion queue checks
• make lint, make license-check, and make fmt-check

Workload: 1,000 read-locked deletion retries through Queue.processingLoop at a one-second cadence with a 30-second retry delay

Metric	Before	After	Change
allocs/op	20472	18472	9.8% lower
Checks: 5 passed.

---

Generated by Perfloop. Measurements and checks.

---

Note

Low Risk
Hot-path optimization and test tightening; lock semantics for existing and concurrent first-time paths are unchanged.

Overview
GetFileLock now does a fast Load when a path already has a lock, so repeat lookups avoid allocating a throwaway RWMutex on every call. First-time paths still use LoadOrStore, so concurrent misses converge on a single mutex.

Test coverage adds TestFileLockManager_ConcurrentMissUsesOneLock (100 goroutines on one path) and makes TestQueue_BackgroundProcessing wait with require.Eventually instead of a fixed sleep, reducing flakiness on slow CI.

^{Reviewed by Cursor Bugbot for commit 215aa38. Bugbot is set up for automated code reviews on this repo. Configure here.}

[greptile-apps]

Greptile Summary

The PR optimizes existing file-lock lookups by avoiding unnecessary mutex allocation while preserving miss convergence through LoadOrStore.

• Adds a concurrency test confirming simultaneous first lookups receive the same lock.
• Replaces a fixed delay in deletion-queue testing with condition-based polling.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
storage/fd/filelockmanager.go	Adds a fast path for existing locks while retaining atomic lock creation for concurrent misses.
storage/fd/filelockmanager_test.go	Verifies that concurrent misses for one path converge on a single mutex.
storage/deletion/queue_test.go	Makes background-deletion verification condition-based rather than dependent on a fixed sleep.

_{Reviews (2): Last reviewed commit: "chore(fd): trim the benchmark scaffoldin..." | Re-trigger Greptile}

[greptile-apps]

Benchmark output path resolves incorrectly

When a caller supplies a directory-containing relative output path such as bench/filelock.test, the recipe creates its parent from the repository root but resolves the binary path after changing into storage/fd, causing the target to fail or place the binary somewhere other than the requested location. The bench-deletion target has the same mismatch.

[perfloop]

Addressed in fbf2d0a92d38. Response to this feedback.

Resolved both targets to absolute paths before changing directories, so relative bench/filelock.test and bench/deletion.test outputs are created beneath the repository root.

[ovaistariq]

Reviewed with a two-lens finder pass plus adversarial verification of every candidate finding, and by building and running the added benchmarks locally.

The production change is correct

storage/fd/filelockmanager.go is sound. The fast path is a plain Load; the miss path still uses LoadOrStore and returns the loaded value when loaded is true, so two callers racing on the same path still receive the same lock instance. TryLock semantics are unchanged, which matters because fileManager.Remove depends on a failed TryLock to skip an in-use file. TestFileLockManager_ConcurrentMissUsesOneLock covers the identity property. No correctness finding here, and the allocation the PR removes is real.

The added benchmarks do not measure what they name

1. Two of the three new benchmarks time a hard-coded sleep. BenchmarkQueueProcessingLoopUnlockedDeletionBacklog and BenchmarkQueueProcessingLoopLockedRetryBacklog both put time.Sleep(lockedRetryLoopTicks * queue.config.ProcessInterval) — 2 × 1s — inside the timed region. Built and ran on the PR branch:

BenchmarkQueueProcessBatchLockedRetryBacklog-16            1    10356250 ns/op   18032 allocs/op
BenchmarkQueueProcessingLoopUnlockedDeletionBacklog-16     1  2001341042 ns/op   17262 allocs/op
BenchmarkQueueProcessingLoopLockedRetryBacklog-16          1  2000700541 ns/op   18061 allocs/op

The direct-ProcessBatch benchmark measures the same 1,000-entry workload at 10.4 ms; the two loop benchmarks report 2.000 s. It is worse than dilution: processingLoop runs in a background goroutine, so its tick executes concurrently with the sleep and Stop() waits for max(2s, 1s + ProcessBatch). Any regression that keeps ProcessBatch under ~1 s produces no change at all in ns/op. At the measured 10.4 ms baseline that covers roughly a 95× regression. allocs/op still carries signal — and that is the metric this PR targets — but the timing columns cannot detect what they were added to guard. Synchronizing on loop completion (poll queue.processed/queue.failed) or shortening ProcessInterval would fix it.

2. BenchmarkQueueProcessBatchLockedRetryBacklog is not steady-state. The queue is created once outside the b.N loop and reused. Each iteration enqueues 1,000 entries that all fail their TryLock and get re-inserted at now+RetryDelay, which the batch never reaches again, so the queue grows by ~1,000 entries per iteration. Measured on one machine back to back: ~3.0 ms/op at -benchtime=20x, and 19–30 ms/op at -benchtime=400x. Because go test -bench chooses b.N from observed speed, a slower baseline binary lands on a smaller b.N and is scored against a smaller backlog than the faster PR binary, which can invert an A/B comparison. The two loop benchmarks already recreate the queue per iteration under StopTimer; doing the same here would fix it.

3. Minor. b.Fatal is called from a RunParallel worker goroutine (b.Fatal must be called from the goroutine running the benchmark function; use b.Error or record and fail after). BenchmarkGetFileLockMiss times RemoveFileLock alongside the miss it means to measure, so the reported cost is the pair rather than the miss.

Suggestion

The lock change is good and I would take it as is. The benchmarks are the part worth another pass, since a benchmark that cannot move when the code regresses is a false guarantee for whoever changes this path next.

One note on scope: storage/deletion/queue_bench_test.go benchmarks the deletion queue rather than the file-lock manager. It exercises the lock path indirectly, but it may belong in its own change.

[perfloop]

Addressed in a17e676f18f8. Response to this feedback.

Reworked the queue benchmarks to start Queue.processingLoop, recreate the backlog for every iteration, and wait for the batch's queue-key update instead of sleeping. The hit and miss benchmarks now isolate their intended operations, parallel failures are reported after RunParallel, and the queue coverage remains here because it exercises the deletion caller.

[ovaistariq]

Rebased onto main and trimmed to the change itself (215aa38): the Load-then-LoadOrStore lookup, its concurrent-miss test, and the deletion-queue test's polling wait are kept; the two benchmark files and the Makefile targets that existed only to run them are dropped. 49-line diff now. Ready to merge.