Fix potential issue with cached sync run persisting after a sync is started or ended. #612
Conversation
WalkthroughReplaces a sync.Once-based lazy initialization with a mutex-guarded cached view-sync state, adds explicit cache invalidation on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/dotc1z/sync_runs.go (1)
101-125: Consider avoiding holding the mutex during database operations.The mutex is held while calling
getFinishedSyncandgetLatestUnfinishedSync, which perform database queries. This could cause contention if multiple goroutines attempt to access the cache simultaneously while a slow query is in progress.A common pattern is to check validity under the lock, release the lock during the expensive operation, then re-acquire and re-check before storing:
🔎 Suggested alternative approach (double-check locking)
func (c *C1File) getCachedViewSyncRun(ctx context.Context) (*syncRun, error) { ctx, span := tracer.Start(ctx, "C1File.getCachedViewSyncRun") defer span.End() c.cachedViewSyncMu.Lock() - defer c.cachedViewSyncMu.Unlock() - if c.cachedViewSyncValid { + run, err := c.cachedViewSyncRun, c.cachedViewSyncErr + c.cachedViewSyncMu.Unlock() - return c.cachedViewSyncRun, c.cachedViewSyncErr + return run, err } + c.cachedViewSyncMu.Unlock() - // First try to get a finished full sync - c.cachedViewSyncRun, c.cachedViewSyncErr = c.getFinishedSync(ctx, 0, connectorstore.SyncTypeFull) - if c.cachedViewSyncErr != nil { - return c.cachedViewSyncRun, c.cachedViewSyncErr + // Compute outside the lock + run, err := c.getFinishedSync(ctx, 0, connectorstore.SyncTypeFull) + if err != nil { + c.cachedViewSyncMu.Lock() + c.cachedViewSyncRun, c.cachedViewSyncErr = run, err + c.cachedViewSyncValid = true + c.cachedViewSyncMu.Unlock() + return run, err } - // If no finished sync, try to get an unfinished one - if c.cachedViewSyncRun == nil { - c.cachedViewSyncRun, c.cachedViewSyncErr = c.getLatestUnfinishedSync(ctx, connectorstore.SyncTypeAny) + if run == nil { + run, err = c.getLatestUnfinishedSync(ctx, connectorstore.SyncTypeAny) } - c.cachedViewSyncValid = true - return c.cachedViewSyncRun, c.cachedViewSyncErr + c.cachedViewSyncMu.Lock() + // Re-check in case another goroutine populated the cache + if !c.cachedViewSyncValid { + c.cachedViewSyncRun, c.cachedViewSyncErr = run, err + c.cachedViewSyncValid = true + } + result, resultErr := c.cachedViewSyncRun, c.cachedViewSyncErr + c.cachedViewSyncMu.Unlock() + return result, resultErr }However, if concurrency is not a major concern in the current usage pattern (e.g., single-threaded access), the current implementation is simpler and correct. Please verify whether concurrent access to this method is expected.
pkg/dotc1z/c1file_test.go (1)
598-604: Consider updating misleading comments.The comments on lines 598-599 and 602-604 describe the buggy behavior ("but it will return resource-1... because the cache wasn't invalidated"), but the test assertions expect the correct behavior after the fix. This could confuse future readers.
🔎 Suggested comment update
- // Call ListResources again - it should return resource-2 from the new finished sync (sync2), - // but it will return resource-1 from the cached sync (sync1) instead because the cache wasn't invalidated resp2, err := f.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{}.Build()) require.NoError(t, err) - // This assertion will fail because the cache wasn't invalidated when sync2 finished + // After the fix, the cache is invalidated when sync2 finishes, so we get resource-2 require.Len(t, resp2.GetList(), 1, "should return resource from new sync") require.Equal(t, "resource-2", resp2.GetList()[0].GetId().GetResource(), "should return resource-2 from the new finished sync (sync2), not resource-1 from cached sync (sync1)")
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/dotc1z/c1file.gopkg/dotc1z/c1file_test.gopkg/dotc1z/sync_runs.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/dotc1z/sync_runs.go (2)
pkg/dotc1z/c1file.go (1)
C1File(36-60)pkg/connectorstore/connectorstore.go (2)
SyncTypeFull(14-14)SyncTypeAny(17-17)
pkg/dotc1z/c1file_test.go (2)
pkg/dotc1z/c1file.go (2)
NewC1ZFile(180-216)WithPragma(151-155)pkg/connectorstore/connectorstore.go (1)
SyncTypeFull(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (5)
pkg/dotc1z/sync_runs.go (3)
127-134: LGTM!The
invalidateCachedViewSyncRunhelper correctly clears all cache state under the mutex, ensuring thread-safe invalidation.
557-557: LGTM!Cache invalidation on
StartNewSyncensures that subsequent reads will recompute the cached sync run after a new sync begins.
616-616: LGTM!Cache invalidation on
EndSyncensures that the cached view sync run reflects the newly finished sync state.pkg/dotc1z/c1file.go (1)
49-53: LGTM!The struct fields are correctly defined for the mutex-protected cache pattern:
cachedViewSyncMuas a value (not pointer) is correct for embedded mutexescachedViewSyncValidas an explicit validity flag enables cache invalidationcachedViewSyncErrstores any error from cache populationpkg/dotc1z/c1file_test.go (1)
548-608: Good test coverage for cache invalidation.The test effectively validates that:
- The cache is populated on first
ListResourcescall- Starting and ending a new sync invalidates the cache
- Subsequent
ListResourcescalls return data from the new finished sync
Related to #604
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.