fix(billing): guard syncJob lifecycle with a dedicated mutex#1706
Conversation
Init and Close mutated the syncJob cron field without holding a lock, so concurrent or repeated Init calls could race on the field and leak a started cron. Adds a dedicated syncJobMu across the four billing services and makes the blob GetAll cache read race-safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFour billing service structs ( ChangesBilling Service syncJob Concurrency Fixes
Blob Resources Repository Cache Read Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
billing/checkout/service.go (1)
175-181:⚠️ Potential issue | 🟠 MajorUse a single
Stop()result inCloseinstead of stopping twice.At Line 179 you already wait for shutdown completion, but Line 180 calls
Stop()again and returns that second context'sErr(). That return value is detached from the stop you actually awaited and can yield flaky/non-actionable close errors.💡 Proposed fix
func (s *Service) Close() error { s.syncJobMu.Lock() defer s.syncJobMu.Unlock() if s.syncJob != nil { - <-s.syncJob.Stop().Done() - return s.syncJob.Stop().Err() + stopCtx := s.syncJob.Stop() + <-stopCtx.Done() + s.syncJob = nil } return nil }Apply the same pattern in:
billing/customer/service.go(Line 384-390)billing/subscription/service.go(Line 142-148)Note:
billing/invoice/service.gohas a different issue—it callsStop().Err()without awaiting completion and should also be fixed, but with a different approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4313c410-b272-4223-9359-a21f45816dcb
📒 Files selected for processing (9)
billing/checkout/service.gobilling/checkout/service_concurrent_test.gobilling/customer/service.gobilling/customer/service_concurrent_test.gobilling/invoice/service.gobilling/invoice/service_concurrent_test.gobilling/subscription/service.gobilling/subscription/service_concurrent_test.gointernal/store/blob/resources_repository.go
Close called Cron.Stop() twice and returned the second context's Err(), which races the stop goroutine and intermittently returns context.Canceled. Wait on Stop().Done() and return nil. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 27712777114Coverage increased (+0.2%) to 43.804%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Summary
Synchronize access to the
syncJobcron field across the four billing services, and make the blob resource cache read inGetAllrace-safe.Changes
syncJobMu(distinct from the sync-operation mutexmu) to the customer, checkout, subscription, and invoice services; guardsyncJobreads/writes inInitandClose.initSyncJobso the lock usesdeferwhile the credit-overdraft setup stays outside the lock.blob.ResourcesRepository.GetAll: re-readrepo.cachedunder the mutex afterrefresh()instead of reading it unlocked.TestService_InitClose_Concurrentto each billing service.Test Plan
go test -race ./billing/subscription/ ./billing/checkout/ ./billing/customer/ ./billing/invoice/go build ./billing/... ./internal/store/blob/...andgo vetpassSQL Safety (if your PR touches
*_repository.goorgoqu.*)resources_repository.gochange is concurrency-only (mutex placement); no SQL orgoququery building touched.🤖 Generated with Claude Code