fix(st-k6z): serialize the counts refresh read-modify-write across processes - #109
Conversation
…ocesses atomicfile stopped torn writes; it did not stop lost updates. The launchd --all run, a manual `strand counts`, and the server's per-write `strand counts <repo>` spawns each read counts.json + counts-mtimes, compute, and replace both files — last writer wins wholesale. Take an advisory flock on the cache dir for the whole refresh, opened before the first read. Non-blocking poll so ctx cancellation and a 2-minute wait cap stay honored: a wedged holder fails the run loudly instead of piling up blocked refreshers behind launchd's every-minute fire.
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds per-cache advisory locking for counts refreshes. Refresh now serializes its read-modify-write workflow, handles cancellation and timeout, and includes concurrency regression tests. ChangesCounts refresh locking
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The PR serializes the counts refresh read-modify-write and adds focused concurrency and timeout tests. Remaining follow-up is limited to strengthening one test assertion and confirming platform support, with no actionable merge-blocking risk beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Refresh
participant WithLock
participant RefreshLocked
participant CountsFiles
Refresh->>WithLock: Acquire cache-directory lock
WithLock->>RefreshLocked: Run read-modify-write workflow
RefreshLocked->>CountsFiles: Write counts rows and merged state
CountsFiles-->>RefreshLocked: Persisted data
RefreshLocked-->>WithLock: Return refresh result
WithLock-->>Refresh: Release lock
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/counts/lock_test.go (1)
119-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error identity, not only non-nil.
The current check passes for any error, including an unrelated failure such as a lock-file open error. Assert
errors.Is(err, context.Canceled)so the test proves the cancellation path. Note also thatlockWaitanderrLockBusystay uncovered; consider makinglockWaitinjectable if you want that path tested.♻️ Proposed stronger assertion
- if err == nil { - t.Fatal("withLock returned nil while the lock was held") - } + if !errors.Is(err, context.Canceled) { + t.Fatalf("withLock error = %v, want context.Canceled", err) + }Add the import:
import ( "context" + "errors" "os"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/counts/lock_test.go` around lines 119 - 125, Update the withLock test around the existing err == nil assertion to verify errors.Is(err, context.Canceled), importing the errors package as needed. Keep the callback assertion and ensure the test specifically validates cancellation rather than merely any non-nil error; leave lockWait injection out of scope unless required for this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/counts/lock_test.go`:
- Around line 119-125: Update the withLock test around the existing err == nil
assertion to verify errors.Is(err, context.Canceled), importing the errors
package as needed. Keep the callback assertion and ensure the test specifically
validates cancellation rather than merely any non-nil error; leave lockWait
injection out of scope unless required for this assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2498531e-5dcd-49c2-bbeb-97197981d00e
📒 Files selected for processing (3)
internal/counts/lock.gointernal/counts/lock_test.gointernal/counts/refresh.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e987c20ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| select { | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("counts: waiting for refresh lock: %w", ctx.Err()) | ||
| case <-time.After(lockPoll): |
There was a problem hiding this comment.
When another refresh holds .refresh.lock for more than 10 seconds, the production post-write path cannot cancel this wait: defaultRefreshCounts discards the context from goBackground(10*time.Second, ...), and counts.Run invokes refresh with context.Background(). Consequently, this new cancellation branch never observes either the server timeout or Server.Stop, so every queued post-write goroutine may wait for the full two-minute lockWait, while Stop blocks on bgWG; expose and use a context-aware Run path here.
Useful? React with 👍 / 👎.
Codex review: defaultRefreshCounts discarded goBackground's context and counts.Run used context.Background(), so a slow lock holder could keep every queued post-write goroutine alive past its 10s timeout and pin Server.Stop's bgWG wait behind lockWait. Add counts.RunContext for in-process callers that own a lifetime; Run keeps context.Background() as the CLI root. Cover the lockWait and cancellation paths with named-error assertions (CodeRabbit).
|
Assessed both reviews: 1 accepted (P2, real), 1 accepted (nit). Pushed as @chatgpt-codex-connector — propagate the server deadline into lock acquisition. Correct, and the sharpest catch here. Fixed by adding @coderabbitai — assert the error identity, not just non-nil. Fair; the old assertion would have passed on an unrelated lock-file open error. Split into two named-error tests, which also closes the coverage gap you flagged: One correction to my own PR body: the server's post-write refresh calls
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Summary
Testing
|
1 participant
Closes the lost-update race CodeRabbit raised on #108 and deferred as out of scope.
The bug
atomicfile.WriteFile(#108) stopped torn writes, not lost updates. Three writers race the same two files:strand counts --allstrand counts [dir...]strand counts <repo>spawn (defaultRefreshCounts, fired on every bead write)Each reads
counts.json+counts-mtimes, computes, then atomically replaces both. Whichever finishes last wins wholesale, silently dropping the others' rows.writeState's merge-on-write narrows its own window but does not close it — the merge read is itself unserialized.The fix
An advisory
flockon<cacheDir>/.refresh.lock, held for the wholerefresh— opened before the first read, since the reads are the R of the read-modify-write.refreshnow doesMkdirAll+ lock; the old body isrefreshLocked.Taken non-blocking on a 50ms poll rather than a blocking
LOCK_EX, so both ctx cancellation and a 2-minute wait cap stay honored: a wedged holder makes the run fail loudly instead of accumulating blocked refreshers behind launchd's every-minute fire. flock is kernel-side and per-fd, so a crashed holder releases automatically (no stale-marker problem) and it serializes goroutines in one process as well as separate processes.The two
NOTE (RMW race…)comments #108 left behind are retired — replaced by a pointer to where the serialization now lives.Tests
TestRefreshSerializesConcurrentRuns— two refreshes over disjoint repos, overlapping in time via a gated source; both repos must land incounts.jsonandcounts-mtimes. Mutation-checked: revertingrefreshto callrefreshLockeddirectly fails it (no counts.json row for repo-b).TestRefreshLockTimesOutOnWedgedHolder— a held lock makes a secondwithLockreturn an error without runningfn(cancelled ctx stands in for the 2-minute deadline).Gate
make checkgreen:go vet,golangci-lint0 issues,go test -race -count=1 ./...all packages.pack-driftskipped — upstream unreachable, pre-existing.bead: st-k6z
@codex review
Summary by CodeRabbit
Bug Fixes
Tests