fix(githubevents): runtime hardening (data race, error wrapping, go floor) - #285
Merged
Conversation
the handler maps were written under mu.Lock but read in the handle* paths with no lock, so registering a handler while an event is being dispatched raced on the map (the RWMutex only ever guarded writes). the read paths now take an RLock, copy the handler slices, and unlock before dispatching, so callbacks can still register mid-dispatch without deadlocking. adds a -race test and a make test-race CI step.
HandleEventRequest formatted the underlying error with %s and a trailing newline, so callers could not errors.Is/As the go-github validation or parse error. use %w and drop the newline.
go-github v90 and golang.org/x/sync both require go 1.25.0, so that is the real floor. #275 pinned the directive to the 1.26.5 patch release, which needlessly raised the minimum go version for consumers. drop it to the dependency floor; .go-version still drives the CI toolchain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three runtime-hardening fixes from the post-merge review of
main.On*/SetOn*) tookmu.Lock, but thehandle*read paths read the maps with no lock, so registering a handler while an event was being dispatched raced on the map (theRWMutexonly ever guarded writes). The read paths nowRLock, copy the handler slices, andRUnlockbefore dispatching, so a callback can still register mid-dispatch without deadlocking.HandleEventRequestused%s+ a trailing\n, so callers couldn'terrors.Is/Asthe underlying go-github error. Now%w, no newline.godirective from the1.26.5patch-pin (chore(deps): update go module directive to v1.26.5 #275) to1.25.0, the actual floor both go-github v90 and x/sync require..go-versionstill drives the CI toolchain.Adds a
-racetest and amake test-raceCI step so this class of bug is caught going forward.Why
The race is the real one: the
RWMutexlooked like it protected reads but itsRLockwas never called, so anyone registering handlers concurrently with serving could hitfatal error: concurrent map read and map write. The snapshot-under-RLock pattern keeps the lock off the callback execution path (holding it there would deadlock a callback that registers). Behavior is otherwise unchanged: same handlers dispatched, same order, same first-error-wins and panic-recovery semantics. Public API is untouched (apidiff confirms).Testing
The
-racetest fails onmain(concurrent map read/write) and passes here.Checklist
race_test.go, run under-racein CI viamake test-race)