Skip to content

fix(rpc): close WaitGroup Add/Wait race in Engine#1776

Merged
ernado merged 1 commit into
mainfrom
fix/rpc-engine-close-race
Jun 14, 2026
Merged

fix(rpc): close WaitGroup Add/Wait race in Engine#1776
ernado merged 1 commit into
mainfrom
fix/rpc-engine-close-race

Conversation

@ernado

@ernado ernado commented Jun 14, 2026

Copy link
Copy Markdown
Member

Summary

Fixes a data race between Engine.Close and Engine.Do reported by the race detector (seen e.g. in gotd/botapi's TestRunEndToEnd).

Do checked the atomic closed flag and then called e.wg.Add(1) as a separate step, while Close stored the flag and called e.wg.Wait(). Between Do's check and its Add, Close could mark the engine closed and have Wait return on a zero counter, after which Do would call Add(1) — violating the sync.WaitGroup contract that an Add raising the counter from zero must happen-before Wait.

WARNING: DATA RACE
Write at ... by goroutine 248:
  rpc.(*Engine).Close()   engine.go:293   (wg.Wait)
  rpc.(*Engine).ForceClose()
  ...
Previous read at ... by goroutine 264:
  rpc.(*Engine).Do()      engine.go:81    (wg.Add)
  ...

Fix

Guard the close-check and wg.Add(1) together under the existing mux so they cannot interleave with Close:

  • closed becomes a mux-guarded bool (was an atomic uint32).
  • Do takes mux, returns ErrEngineClosed if closed, else wg.Add(1), then unlocks.
  • Close sets closed = true under mux, then wg.Wait() outside the lock (so pending requests can still Done).
  • Removes the now-unused isClosed helper.

Testing

go test -race -count=1 ./rpc/... ./mtproto/... passes.

🤖 Generated with Claude Code

Do checked the atomic closed flag and then called wg.Add(1) as a
separate step, while Close stored the flag and called wg.Wait(). Between
Do's check and its Add, Close could mark the engine closed and have Wait
return on a zero counter, then Do would Add(1) — violating the
WaitGroup contract that an Add raising the counter from zero must
happen-before Wait. This is the data race the detector reported between
Engine.Close and Engine.Do.

Guard the close check and wg.Add together under the existing mux so they
cannot interleave with Close. closed becomes a mux-guarded bool; Close
sets it under the lock and waits outside it. Removes the now-unused
isClosed helper.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.23%. Comparing base (5bd59ff) to head (3ed5cd2).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1776      +/-   ##
==========================================
+ Coverage   71.10%   71.23%   +0.12%     
==========================================
  Files         501      503       +2     
  Lines       23501    23577      +76     
==========================================
+ Hits        16711    16795      +84     
+ Misses       5569     5550      -19     
- Partials     1221     1232      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ernado ernado merged commit 406a51b into main Jun 14, 2026
14 checks passed
@ernado ernado deleted the fix/rpc-engine-close-race branch June 14, 2026 04:22
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