fix: resolve race conditions and cache inconsistencies from post-optimization review #39
Conversation
… uninitialized cache
…h directly & document error handling strategy in Refresh
There was a problem hiding this comment.
Pull request overview
This PR addresses critical race conditions and cache consistency issues identified during post-optimization review. The changes improve thread-safety in the logger, fix middleware argument propagation, ensure cache state consistency, and add comprehensive test coverage.
Changes:
- Fixed race conditions in logger by protecting ColorPrinter access with read locks
- Corrected middleware chain to propagate modified commands and arguments
- Updated cache operations to maintain consistent state (MarkUpgraded now sets Installed flag)
- Changed core initialization to fail-fast on cache errors instead of creating invalid empty cache
- Added comprehensive unit tests for UnifiedCache functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/middleware/middleware.go | Fixed middleware chain to properly propagate modified cmd/args through the chain |
| internal/logger/logger.go | Added RLock protection for ColorPrinter access and removed double-formatting in writeDirect |
| internal/core/core.go | Changed to fail-fast with Fatal error when cache initialization fails |
| internal/brew/unified_cache_test.go | Added comprehensive unit tests for UnifiedCache core functionality (9 test cases) |
| internal/brew/unified_cache.go | Fixed MarkUpgraded to set Installed flag, simplified IsInstalled race condition fix, added error handling comment, optimized splitLines |
| go.mod | Moved testify from indirect to direct dependency for new tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/brew/unified_cache_test.go
Outdated
| assert.False(t, cache.Packages["wget"].Installed) | ||
| assert.Empty(t, cache.Packages["wget"].InstalledVersion) | ||
| assert.False(t, cache.Packages["wget"].Outdated) |
There was a problem hiding this comment.
This test expects the package to remain in the cache with Installed set to false, but the actual MarkUninstalled implementation deletes the package from the cache entirely. These assertions will fail because cache.Packages["wget"] will not exist after calling MarkUninstalled. Either update the test to check that the package is deleted (using assert.NotContains or checking the map directly), or update MarkUninstalled to preserve the package with Installed=false instead of deleting it.
| assert.False(t, cache.Packages["wget"].Installed) | |
| assert.Empty(t, cache.Packages["wget"].InstalledVersion) | |
| assert.False(t, cache.Packages["wget"].Outdated) | |
| assert.NotContains(t, cache.Packages, "wget", "package should be removed from cache after uninstall") |
internal/brew/unified_cache_test.go
Outdated
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // mockRunner implements runner.CommandRunner for testing |
There was a problem hiding this comment.
The comment describes this as "mockRunner" but the type is named "testRunner". Update the comment to match the type name for consistency.
| // mockRunner implements runner.CommandRunner for testing | |
| // testRunner implements runner.CommandRunner for testing |
📝 Description
This PR addresses critical bugs identified during post-optimization code review by GitHub Copilot. The changes fix race conditions, cache state inconsistencies, and logger thread-safety issues introduced during the performance optimization work.
Key Fixes:
MarkUpgradednow correctly sets theInstalledflag when upgrading packageswriteDirectfunctionIsInstalledby simplifying the refresh check logic🔗 Related Issue(s)
🔄 Type of Change
📋 Checklist
🔍 Additional Notes
These fixes address subtle but critical bugs that could cause:
All changes have been tested with
keg list,keg upgrade --check, and full test suite passes.