Skip to content

fix(githubevents): runtime hardening (data race, error wrapping, go floor) - #285

Merged
cbrgm merged 3 commits into
mainfrom
runtime-hardening
Aug 12, 2026
Merged

fix(githubevents): runtime hardening (data race, error wrapping, go floor)#285
cbrgm merged 3 commits into
mainfrom
runtime-hardening

Conversation

@cbrgm

@cbrgm cbrgm commented Aug 12, 2026

Copy link
Copy Markdown
Owner

What

Three runtime-hardening fixes from the post-merge review of main.

  • Fix a data race on the handler maps. Writes (On*/SetOn*) took mu.Lock, but the handle* read paths read the maps with no lock, so registering a handler while an event was being dispatched raced on the map (the RWMutex only ever guarded writes). The read paths now RLock, copy the handler slices, and RUnlock before dispatching, so a callback can still register mid-dispatch without deadlocking.
  • Wrap request errors. HandleEventRequest used %s + a trailing \n, so callers couldn't errors.Is/As the underlying go-github error. Now %w, no newline.
  • Lower the go directive from the 1.26.5 patch-pin (chore(deps): update go module directive to v1.26.5 #275) to 1.25.0, the actual floor both go-github v90 and x/sync require. .go-version still drives the CI toolchain.

Adds a -race test and a make test-race CI step so this class of bug is caught going forward.

Why

The race is the real one: the RWMutex looked like it protected reads but its RLock was never called, so anyone registering handlers concurrently with serving could hit fatal 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

$ CGO_ENABLED=1 go test -race ./githubevents/     # new TestNoDataRace* : was DATA RACE, now ok
ok  github.com/cbrgm/githubevents/v2/githubevents

$ make test        # full suite green, coverage 85.5%
$ make apidiff      # >> API compatible (go-github version-only changes ignored)
$ golangci-lint run # 0 issues

The -race test fails on main (concurrent map read/write) and passes here.

Checklist

  • Tests added/updated (race_test.go, run under -race in CI via make test-race)
  • No breaking changes (unexported internals + error wrapping; apidiff compatible; go floor lowered, not raised)
  • Readable commit history (3 commits: race fix, error wrap, go directive)
  • AI code review considered and comments resolved

cbrgm added 3 commits August 12, 2026 15:42
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.
@cbrgm
cbrgm enabled auto-merge (squash) August 12, 2026 13:44
@cbrgm
cbrgm merged commit d334acc into main Aug 12, 2026
5 checks passed
@cbrgm
cbrgm deleted the runtime-hardening branch August 12, 2026 13:45
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.

1 participant