Skip to content

Improve mixclient test reliability #3651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
davecgh merged 2 commits into from
Apr 4, 2026
+59 −38
Merged

Improve mixclient test reliability #3651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75ddb9f
mixclient: Disable logs to backend after test finish
jrick Apr 1, 2026
5aedf72
mixclient: Avoid test hangs caused by epoch ticker
jrick Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 47 additions & 33 deletions mixing/mixclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,9 @@ type Client struct {

logger slog.Logger

testTickC chan struct{}
testHooks map[hook]hookFunc
testWaiting chan struct{}
testTickC chan time.Time
testHooks map[hook]hookFunc
}

// NewClient creates a wallet's mixing client manager.
Expand Down Expand Up @@ -668,26 +669,28 @@ func (c *Client) waitForEpoch(ctx context.Context) (time.Time, error) {
now := time.Now().UTC()
epoch := now.Truncate(c.epoch).Add(c.epoch)
duration := epoch.Sub(now)
timer := time.NewTimer(duration)
select {
case <-ctx.Done():
if !timer.Stop() {
<-timer.C
}
return epoch, ctx.Err()
case <-c.stopping:
if !timer.Stop() {
<-timer.C
}
c.runWG.Wait()
return epoch, ErrStopping
case <-c.testTickC:
if !timer.Stop() {
<-timer.C

testWaiting := c.testWaiting
var timerC, testTickC <-chan time.Time
if testWaiting == nil {
timerC = time.After(duration)
}

for {
select {
case <-ctx.Done():
return epoch, ctx.Err()
case <-c.stopping:
c.runWG.Wait()
return epoch, ErrStopping
case <-timerC:
return epoch, nil
case testWaiting <- struct{}{}:
testWaiting = nil
testTickC = c.testTickC
case testEpoch := <-testTickC:
return testEpoch, nil
}
return time.Now(), nil
case <-timer.C:
return epoch, nil
}
}

Expand All @@ -700,6 +703,11 @@ func (p *peer) msgJitter() time.Duration {
// small amount of jitter is added to help avoid timing deanonymization
// attacks.
func (c *Client) prDelay(ctx context.Context, p *peer) error {
// No delay in tests.
if c.testTickC != nil {
return nil
}

now := time.Now().UTC()
epoch := now.Truncate(c.epoch).Add(c.epoch)
sendBefore := epoch.Add(-timeoutDuration - maxJitter)
Expand All @@ -717,18 +725,13 @@ func (c *Client) prDelay(ctx context.Context, p *peer) error {
<-timer.C
}
return ctx.Err()
case <-c.testTickC:
if !timer.Stop() {
<-timer.C
}
return nil
case <-timer.C:
return nil
}
}

func (c *Client) testTick() {
c.testTickC <- struct{}{}
func (c *Client) testTick(t time.Time) {
c.testTickC <- t
}

func (c *Client) testHook(stage hook, ps *pairedSessions, s *sessionRun, p *peer) {
Expand Down Expand Up @@ -858,12 +861,23 @@ func (c *Client) epochTicker(ctx context.Context) error {
if err != nil {
return err
}
timerC := time.After(timeoutDuration + 2*time.Second)
testWaiting := c.testWaiting
var testTickC <-chan time.Time
for {
select {
case <-ctx.Done():
return err

select {
case <-time.After(timeoutDuration + 2*time.Second):
case <-c.testTickC:
case <-ctx.Done():
return err
case <-timerC:

case testWaiting <- struct{}{}:
testWaiting = nil
testTickC = c.testTickC
continue
case <-testTickC:
}
break
}
c.mu.Lock()
c.removeUnresponsiveDuringEpoch(prevPRs, uint64(firstEpoch.Unix()))
Expand Down
16 changes: 11 additions & 5 deletions mixing/mixclient/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ const (

func newTestClient(w *testWallet, logger slog.Logger) *Client {
c := NewClient(w)
c.testTickC = make(chan struct{})
c.testWaiting = make(chan struct{})
c.testTickC = make(chan time.Time)
c.SetLogger(logger)
return c
}
Expand Down Expand Up @@ -223,7 +224,8 @@ func TestHonest(t *testing.T) {
<-doneRun
}()

c.testTick()
<-c.testWaiting
Copy link
Copy Markdown
Member

@davecgh davecgh Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing to hold this PR up over, but one of the things I have noticed over time with the tests (and especially more recently when working on the connection manager bits) is that naked channel waits like this often end up making tests hang when things aren't working properly.

I have found that ensuring that all channel waits in tests select across a timeout channel works well to prevent hangs when things aren't working as intended.

For example, something like:

select {
case <-c.testWaiting:
case <-time.After(time.Second):   // or whatever is a reasonable timeout for the expected scenarios
	t.Fatal("test synchronization timeout")
}

c.testTick(time.Now().Truncate(time.Second))

var g errgroup.Group
for i := range peers {
Expand All @@ -235,7 +237,8 @@ func TestHonest(t *testing.T) {

go func() {
for {
c.testTick()
<-c.testWaiting
c.testTick(time.Now().Truncate(time.Second))
select {
case <-ctx.Done():
return
Expand Down Expand Up @@ -329,8 +332,11 @@ func testDisruption(t *testing.T, misbehavingID *identity, h hook, f hookFunc) {
}()

testTick := func() {
c.testTick()
c2.testTick()
<-c.testWaiting
<-c2.testWaiting
t := time.Now().Truncate(time.Second)
c.testTick(t)
c2.testTick(t)
}
testTick()

Expand Down
1 change: 1 addition & 0 deletions mixing/mixclient/log_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func useTestLogger(t *testing.T) (slog.Logger, func()) {
l.SetLevel(slog.LevelTrace)
mixpool.UseLogger(l)
return l, func() {
l.SetLevel(slog.LevelOff)
mixpool.UseLogger(slog.Disabled)
}
}