chore: simplify, modernize (Go 1.26), update deps#84
Conversation
- rpc.go: drop misleading `stderr` alias for stdlib errors (no import conflict)
- locker.go: remove dead `item.callback` field (stored but never read)
- locker.go: drop redundant `Store(0)` on freshly-zeroed atomic.Uint64 fields
- locker.go: drop redundant `writerCount.Store(0)` in w==0 branch of lockRead
- locker.go: fix inconsistency guards: `&&` → `||` (locker.go:170,267,493) — the
error message "should be zero writers AND zero readers" makes the intent clear;
the guard must fire when EITHER is non-zero, not only when both are
- locker.go: simplify exists() wildcard branch to `return A || B`
- locker.go: remove unreachable `if !ok` block in updateTTL (ok is always true
at that point; the early-return on !ok precedes it)
- locker.go: drop redundant `lockID` param from callback type — it always equals
the captured `id`; use the closure variable directly
- locker.go: replace `goto loop` with `for { select { … ; continue } ; break }`
There was a problem hiding this comment.
Pull request overview
This PR simplifies lock internals and updates a small RPC import alias cleanup while retaining the existing locking API behavior.
Changes:
- Removes unused callback storage and redundant atomic zero stores.
- Simplifies TTL callback control flow and wildcard existence checks.
- Updates inconsistency checks from
&&to||in several lock acquisition paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
locker.go |
Simplifies lock bookkeeping, callback signatures, TTL update flow, and inconsistency checks. |
rpc.go |
Replaces the aliased errors import with the standard import name. |
Comments suppressed due to low confidence (1)
locker.go:171
- This newly broadened inconsistency check returns without releasing
resourceMuin this branch. With||, a notification that leaves only one of the counters non-zero now takes this path and can leave the resource locked, causing subsequent operations on the resource to block; release the mutex before returning, as the other inconsistency checks do.
if r.writerCount.Load() != 0 || r.readerCount.Load() != 0 {
l.log.Error("inconsistent state, should be zero writers and zero readers",
"resource", res,
"id", id,
"writers", r.writerCount.Load(),
"readers", r.readerCount.Load())
return false
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
More reviews will be available in 25 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR refactors the lock callback contract to remove the lock ID parameter and updates all acquisition paths accordingly. The TTL timeout loop is converted from goto-based to for-select structure. A bug in the errors package import is fixed, and test coverage is added for TTL expiry, read-to-write promotion, and wildcard exists behavior. ChangesLock callback refactoring and TTL loop improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 76.98% 77.15% +0.16%
==========================================
Files 4 4
Lines 691 674 -17
==========================================
- Hits 532 520 -12
+ Misses 136 135 -1
+ Partials 23 19 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The writer-wait inconsistency check returned without releasing resourceMu, unlike the sibling checks in the promotion arm and lockRead. An inconsistent state would leak the operation+release semaphores and wedge the resource; release the mutex before returning, matching the siblings.
These lock() branches were only reached by the random fuzz test, so their coverage flickered between runs and dropped on this branch. Add three deterministic tests (driven through the RPC client) plus a small container helper, restoring patch/project coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/lock_test.go`:
- Around line 706-712: The goroutine started via wg.Go currently calls
require.NoError(t, e.Error...) which is unsafe; change it to propagate errors to
the main test goroutine (or use testify/assert from the main goroutine) instead
of calling t.FailNow() from a non-test goroutine: e.g., have the goroutine send
any received error on a dedicated error channel (or set a shared
atomic/variable) and then in the main test goroutine after wg.Wait() or via
select on that error channel assert/require the error there; update the code
around wg.Go, the ch receive case, and the stop handling to forward e.Error
rather than invoking require inside the goroutine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23162dda-2264-4881-b5f2-c19dac1bd233
📒 Files selected for processing (3)
locker.gorpc.gotests/lock_test.go
require.NoError calls t.FailNow()/runtime.Goexit, which is only safe on the test goroutine; from the container error-watch goroutine it can panic or leak. Use assert.NoError, which records the failure without FailNow.
lock() self-bounds to its wait timeout, so the goroutine + buffered channel + 15s select watchdog were redundant; a plain blocking call exercises the same wait-then-acquire arm and matches the file's idiom.
Applied fixes (S: 6, M: 1, R: 1)
Simplify (S)
item.callbackfield — stored but never readStore(0)on freshly-zeroedatomic.Uint64fields (two sites inlock()/lockRead())writerCount.Store(0)in thew==0branch oflockReadexists()wildcard branch:if A||B{return true};return false→return A||Bif !okblock inupdateTTL(the guard is alwaystrueat that point)lockIDparam from thecallbacktype (always equals capturedid); replacegoto loopwithfor{select{…;continue};break}Modernize (M)
rpc.go: drop misleadingstderr "errors"alias — no import conflict existsReview / bug fix (R)
lock()andlockRead():&&→||at three sites (lines 170, 267, 493). The surrounding log message reads "should be zero writers and zero readers", confirming the check must fire when either count is non-zero, not only when both are.Deps
go get -u all && go mod tidyproduced no changes — all dependencies were already current.Summary by CodeRabbit
Bug Fixes
Refactor
Tests