ityonemo / ityonemo/vsr

Flaky tests: HeartbeatTest and ViewChangeTest fail intermittently

Open
#3 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Elixir
Stars
19
Forks
2
PR merge metrics
No merged PRs in 30d

Description

## Problem

Two tests fail intermittently (roughly 50-80% of runs):

- `HeartbeatTest` "primary sends heartbeats and backup detects primary failure" (heartbeat_test.exs:109)
- `ViewChangeTest` "collects StartViewChange votes and sends DoViewChange when majority reached" (view_change_test.exs:130)

Both manifest as `TelemetryHelper` timeouts.

## Root Causes

### HeartbeatTest — wrong node killed + crash on broadcast

1. **Wrong primary identification**: The test names nodes `:"primary_xxx"` / `:"backup1_xxx"`, but `primary_for_view/2` sorts by `:erlang.term_to_binary/1`, where `:"backup1_..."` sorts *before* `:"primary_..."`. So for view 0 the actual primary is `backup1`, not `primary`. The test stops the wrong node, and the real primary keeps sending heartbeats — so `primary_timeout` never fires.

2. **Crash on broadcast to dead node**: After the stopped node's name is unregistered, the surviving backups eventually call `start_manual_view_change/1`, which broadcasts `StartViewChange` via `send/2` to the dead node's atom name. Since the name is no longer registered, `send/2` raises `ArgumentError: invalid destination`, crashing the backup GenServers before the telemetry event can be observed.

3. **Telemetry listener attached too late**: The test attaches the `[:timer, :primary_timeout]` listener *after* calling `GenServer.stop/2`, creating a window where the event fires before anyone is listening.

### ViewChangeTest — TOCTOU race on intermediate state

The test uses a live 3-node cluster and asserts on `state.view_change_votes` after sending a `StartViewChange`. However, receiving a `StartViewChange` triggers a broadcast to all replicas, which can cause the view change to *complete* (clearing `view_change_votes`) before `VsrServer.dump/1` runs. This is a time-of-check-time-of-use race.

## Note on `send_vsr/3`

The default `send_vsr/3` implementation uses `send/2`, which crashes when the destination is an unregistered atom name. This is fine for PID-based node IDs (where `send/2` to a dead PID silently succeeds) and for custom transports like Maelstrom, but means the default Erlang-distribution transport is fragile when using registered names — a single dead peer kills the sender on broadcast. The test fixes work around this, but a production fix might want to guard `send/2` or use `Process.whereis/1` first.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with heartbeat_test.exs:109 and view_change_test.exs:130, then inspect primary_for_view/2, TelemetryHelper listener setup, and send_vsr/3. Reproduce the intermittent failures repeatedly and trace the node selection, broadcast, and view-change timing. Done means both tests run reliably without TelemetryHelper timeouts or crashed GenServers.

Written by the indexing model from the issue text.

Assessment

Tech stack
elixir
Domain
distributed-systems, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.