Repository navigation
fix(compaction): unexport metric fields to prevent mutex bypass - #480
Conversation
Unexport Manager (TotalJobsCompleted, TotalJobsFailed, TotalFilesCompacted, TotalBytesSaved, TotalManifestsRecov) and BaseTier (TotalCompactions, TotalFilesCompacted, TotalBytesSaved) metric fields so they can only be accessed through the mutex-protected Stats()/GetBaseStats() methods.
There was a problem hiding this comment.
Code Review
This pull request unexports the compaction metric fields in both the Manager and BaseTier structs to prevent data races from external access, routing all reads through their respective mutex-protected stats methods. The reviewer recommended introducing a thread-safe helper method on BaseTier to manage updates to these metrics, ensuring that package-level writes do not bypass the mutex and cause data races.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Addressed the finding — added @github-copilot-code-review[bot] re-review please. |
|
@gemini-code-assist re-review please — added |
There was a problem hiding this comment.
Code Review
This pull request unexports compaction metric fields in both the Manager and BaseTier structs to prevent potential data races from unprotected access. It also introduces a thread-safe RecordCompaction helper method on BaseTier to safely increment metrics under a mutex lock, and updates the release notes accordingly. As there are no review comments, I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Summary
Unexports metric fields on
ManagerandBaseTierthat were previously exported, allowing direct reads/writes that bypass the mutex-protectedStats()/GetBaseStats()accessors.Closes #350.
Changes
internal/compaction/manager.go:TotalJobsCompleted→totalJobsCompleted,TotalJobsFailed→totalJobsFailed,TotalFilesCompacted→totalFilesCompacted,TotalBytesSaved→totalBytesSaved,TotalManifestsRecov→totalManifestsRecovinternal/compaction/tier.go:TotalCompactions→totalCompactions,TotalFilesCompacted→totalFilesCompacted,TotalBytesSaved→totalBytesSavedRELEASE_NOTES_2026.06.2.md: Added bug fix entryAll usages are internal to the
compactionpackage.Test Plan
go build ./internal/compaction/...— cleango build ./cmd/... ./internal/...— clean (no external callers)go test ./internal/compaction/... -v— all pass