feat(solver): prefer highest versions + fix caching/dedupe#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new solver option to prefer the highest available package versions and fixes several issues with caching and deduplication. The changes improve the solver's flexibility in version selection strategies and ensure thread-safe caching with proper data isolation.
Changes:
- Added
PreferHighestVersionsoption to force selection of highest allowed versions - Fixed cache invalidation in solver state when assignments or constraints change
- Implemented version deduplication in
CombinedSource.GetVersions - Added thread-safety and defensive copying to
CachedSource
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| state.go | Added cache clearing logic and PreferHighestVersions support; changed error handling to penalize unavailable dependencies |
| source_combined.go | Implemented version deduplication and improved error handling to continue on partial failures |
| solver_options.go | Added PreferHighestVersions field and corresponding functional option |
| cached_source.go | Added mutex for thread safety and defensive copying to prevent cache mutation |
| cached_source_test.go | Added tests to verify cache mutation prevention |
| .mise.toml | Changed Go version from specific version to "latest" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If we can't fetch dependencies, heavily penalize this version to avoid selecting it. | ||
| return versionScoreConflictPenalty |
There was a problem hiding this comment.
Changing from neutral score to heavy penalty when dependencies can't be fetched is a significant behavioral change. If the error is transient (network issue, temporary unavailability), this will prevent version selection even when the version might be valid. Consider distinguishing between "version doesn't exist" errors and other types of errors, penalizing only the former.
| // If we can't fetch dependencies, heavily penalize this version to avoid selecting it. | |
| return versionScoreConflictPenalty | |
| // If we can't fetch dependencies (which may be due to transient issues), | |
| // treat this version with a neutral baseline score rather than a hard penalty. | |
| return versionScoreBaseline |
| out := cloneVersions(versions) | ||
| c.mu.Unlock() |
There was a problem hiding this comment.
The mutex is unlocked after cloning but before returning. If cloning is expensive, consider cloning after unlocking to reduce lock contention. Move c.mu.Unlock() to line 58 (before cloning).
| out := cloneVersions(versions) | |
| c.mu.Unlock() | |
| c.mu.Unlock() | |
| out := cloneVersions(versions) |
| out := cloneTerms(deps) | ||
| c.mu.Unlock() |
There was a problem hiding this comment.
The mutex is unlocked after cloning but before returning. If cloning is expensive, consider cloning after unlocking to reduce lock contention. Move c.mu.Unlock() to line 91 (before cloning).
| out := cloneTerms(deps) | |
| c.mu.Unlock() | |
| c.mu.Unlock() | |
| out := cloneTerms(deps) |
1 participant
No description provided.