Skip to content

fix(compaction): unexport metric fields to prevent mutex bypass - #480

Merged
xe-nvdk merged 4 commits into
mainfrom
fix/unexport-compaction-metrics
Jun 4, 2026
Merged

xe-nvdk merged 4 commits into
mainfrom
fix/unexport-compaction-metrics

Conversation

@xe-nvdk

@xe-nvdk xe-nvdk commented Jun 4, 2026

Copy link
Copy Markdown
Member

Summary

Unexports metric fields on Manager and BaseTier that were previously exported, allowing direct reads/writes that bypass the mutex-protected Stats() / GetBaseStats() accessors.

Closes #350.

Changes

  • internal/compaction/manager.go: TotalJobsCompleted → totalJobsCompleted, TotalJobsFailed → totalJobsFailed, TotalFilesCompacted → totalFilesCompacted, TotalBytesSaved → totalBytesSaved, TotalManifestsRecov → totalManifestsRecov
  • internal/compaction/tier.go: TotalCompactions → totalCompactions, TotalFilesCompacted → totalFilesCompacted, TotalBytesSaved → totalBytesSaved
  • RELEASE_NOTES_2026.06.2.md: Added bug fix entry

All usages are internal to the compaction package.

Test Plan

  • go build ./internal/compaction/... — clean
  • go build ./cmd/... ./internal/... — clean (no external callers)
  • go test ./internal/compaction/... -v — all pass
  • Deep review with configuration matrix — no findings

Ignacio Van Droogenbroeck added 2 commits June 4, 2026 11:30
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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread internal/compaction/tier.go
@xe-nvdk

xe-nvdk commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

Addressed the finding — added RecordCompaction(filesCompacted int, bytesSaved int64) thread-safe helper on BaseTier so tier implementations within the package cannot bypass t.mu when incrementing metrics.

@github-copilot-code-review[bot] re-review please.

@xe-nvdk

xe-nvdk commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@gemini-code-assist re-review please — added RecordCompaction helper on BaseTier per your finding.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xe-nvdk
xe-nvdk merged commit c19523c into main Jun 4, 2026
5 checks passed
@xe-nvdk
xe-nvdk deleted the fix/unexport-compaction-metrics branch June 4, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

low(compaction): Exported metric fields bypass mutex

1 participant