Skip to content

Fix two races when a replica starts - #175

Merged
jaredthivener merged 1 commit into
mainfrom
atomic-viewer-report
Oct 7, 2026
Merged

jaredthivener merged 1 commit into
mainfrom
atomic-viewer-report

Conversation

@jaredthivener

Copy link
Copy Markdown
Owner

The Tests job for #173 failed once on main, on the Postgres run, in a test that change did not touch:

tests/test_lamp.py::test_a_lost_database_degrades_instead_of_crashing
    assert client.get(STATUS).status_code == 200  # the lamp has been seen once
E   AssertionError: assert 503 == 200

Because that run failed, #173 was never published by itself; #174's run, a minute later, passed and published both.

The failure is not a test problem. A replica that has just started can answer its first request with 503, in two ways.

1. The check was recorded before the answer arrived

catch_up() asks the database twice: who is watching, then the lamp's state. It set hub.checked_at between the two. A request arriving in that gap saw no state and a check made a moment ago, so it did not ask for itself and reported the lamp unreachable.

Fix: record the check after the state is in.

2. Two first reports from one replica collided

Store.pulse() ran an UPDATE and, if no row was updated, an INSERT. At startup the background loop and the first request both report. On Postgres both updates find nothing, both insert, and the second fails on the primary key. SQLite is unaffected because its writers take turns.

Fix: one INSERT ... ON CONFLICT DO UPDATE, which both databases support.

Checked

Against a local Postgres 17 and SQLite, ten runs each:

Test Before After
test_a_request_during_the_first_sync_is_not_turned_away (new) fails 10/10 on both, with the same assert 503 == 200 passes 10/10 on both
test_a_replica_reporting_twice_at_once_is_counted_once (new) fails 10/10 on Postgres with UniqueViolation; passes on SQLite passes 10/10 on both
test_a_lost_database_degrades_instead_of_crashing (the CI failure) failed 0/40 locally; the race needs CI's timing passes 10/10 on both

The whole suite passes on both (14 tests).

Which of the two caused the CI failure cannot be told from its log: both end in the same 503.

馃 Generated with Claude Code

A replica that has just started could answer its first request with
503. The Tests job failed on this once, on Postgres, at
test_a_lost_database_degrades_instead_of_crashing.

- catch_up() recorded the check as done before the lamp's state had
  arrived. A request in that gap found no state and a recent check,
  and reported the lamp unreachable. The check is now recorded last.
- Store.pulse() updated a replica's row and inserted it if nothing was
  updated. Two reports at once (the background loop and a first
  request) both inserted, and on Postgres the second failed on the
  primary key. It is now one INSERT ... ON CONFLICT DO UPDATE.

Each has a test that failed ten times out of ten before the change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jaredthivener
jaredthivener merged commit 0a93425 into main Oct 7, 2026
8 checks passed
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