fix: Remove operation ordering to prevent data orphaning in storage#69
fix: Remove operation ordering to prevent data orphaning in storage#69lucasoares merged 6 commits intomainfrom
Conversation
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
Co-authored-by: lucasoares <10624972+lucasoares@users.noreply.github.com>
cezar-tech
changed the title
| // Cache should NOT be called since storage failed | ||
| // No expectations set for cache - if called, test will fail |
| // When storage fails, cache should not be called, preserving data integrity | ||
|
|
||
| mockStorage := mocks.NewMockStorage(mockCtrl) | ||
| // Storage fails - should happen first |
There was a problem hiding this comment.
Comment removed as requested in commit 74c2c86.
There was a problem hiding this comment.
Comments were already removed in commit 74c2c86.
| // This test verifies that storage is called before cache to prevent orphaning | ||
| // When storage fails, cache should not be called, preserving data integrity |
There was a problem hiding this comment.
Comments removed as requested in commit 74c2c86.
There was a problem hiding this comment.
Comments were already removed in commit 74c2c86.
| @@ -0,0 +1 @@ | |||
| 3F000058A772FB05F975C1532B7FD31ED63831A9 | |||
There was a problem hiding this comment.
why is this file needed ?
There was a problem hiding this comment.
@copilot add this file to git igore and dont commit it in the pr
There was a problem hiding this comment.
This file was accidentally committed during development. It has been removed in commit 74c2c86.
|
|
||
| mockCache := mocks.NewMockCache(mockCtrl) | ||
| mockCache.EXPECT().Remove(gomock.Any(), "q1", []string{"1", "2"}).Return(int64(2), nil) | ||
| // Cache should NOT be called since storage failed - the new order prevents cache orphaning |
|
@cezar-tech Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
@copilot solve the comments on the pr |
Co-authored-by: cezar-tech <56096912+cezar-tech@users.noreply.github.com>
…mmits Co-authored-by: cezar-tech <56096912+cezar-tech@users.noreply.github.com>
Problem
The
Queue.Removemethod was removing data from cache first, then from storage. This caused a critical issue where if storage removal failed after cache removal succeeded, data would become orphaned in storage without corresponding cache entries. Since the housekeeper scans cache to generate removal operations, these orphaned storage documents could never be cleaned up.Solution
Inverted the removal order to storage-first, then cache. This ensures:
Pulloperation already handles this scenario by detecting storage inconsistencies and automatically cleaning up stale cache entriesChanges Made
Queue.Removemethod ininternal/queue/queue.goto remove from storage before cacheTestQueueRemoveCacheError: Now tests storage success → cache failure scenarioTestQueueRemoveStorageError: Now tests storage failure (cache not called)TestRemoveExceedingMessagesRemoveErrorShouldResultError: Updated for new behaviorTestQueueRemoveStorageFirstPreventsOrphaningto validate orphaning preventionValidation
✅ All Remove-related tests pass (18 tests)
✅ All service layer Remove tests pass (3 tests)
✅ Unit test suite runs successfully (307 tests, 7 skipped)
This is a minimal, surgical change that fixes the core data integrity issue while maintaining full backward compatibility in terms of API and return values.
Fixes #68.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.