Skip to content

fix(pb): make disableProgress race-free with sync/atomic.Bool#511

Merged
chlins merged 1 commit intomodelpack:mainfrom
SAY-5:fix/pb-disable-progress-race
Apr 21, 2026
Merged

fix(pb): make disableProgress race-free with sync/atomic.Bool#511
chlins merged 1 commit intomodelpack:mainfrom
SAY-5:fix/pb-disable-progress-race

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented Apr 20, 2026

What

internal/pb/pb.go:33-40 declares a package-level disableProgress bool that gets written by SetDisableProgress(disable bool) and read from the progress-bar hot path inside (*ProgressBar).Add:

var (
    disableProgress bool
)

func SetDisableProgress(disable bool) {
    disableProgress = disable
}

// ... later
if disableProgress {
    return reader
}

Multiple concurrent pull/push workers call Add from their goroutines while the main goroutine (CLI arg parsing, feature flags, etc.) may still flip disableProgress. The access is unsynchronised, so per #493 go test -race reports a data race, and the Go memory model leaves the behaviour undefined in practice.

Fix

Use sync/atomic.Bool:

var (
    disableProgress atomic.Bool
)

func SetDisableProgress(disable bool) {
    disableProgress.Store(disable)
}

// ... later
if disableProgress.Load() {
    return reader
}

No public-API change — SetDisableProgress(bool) keeps the same signature. Callers and tests continue to pass a plain bool; the race-detector reports clean.

Fixes #493

internal/pb package used an unprotected package-level `disableProgress`
bool that's read by `(*ProgressBar).Add` from the concurrent pull/push
worker goroutines while `SetDisableProgress` flips it on startup.
`go test -race` reports the expected data race, and the Go memory
model leaves the behaviour undefined in practice.

Swap the bool for an atomic.Bool. Store from SetDisableProgress, Load
from Add. No public-API change - callers continue to pass plain bool
to SetDisableProgress.

Fixes modelpack#493

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
Copy link
Copy Markdown

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

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 converts the disableProgress flag to an atomic.Bool to ensure thread-safe access across concurrent goroutines, specifically for pull/push workers. It updates the SetDisableProgress and Add methods to use atomic operations. I have no feedback to provide.

Copy link
Copy Markdown
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

@chlins chlins merged commit 306c518 into modelpack:main Apr 21, 2026
5 checks passed
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.

bug: disableProgress global variable has data race (no atomic/mutex protection)

2 participants