Skip to content

fix(state): fire FakeClock.After only once Advance reaches the deadline - #240

Draft
reka18 wants to merge 3 commits into
mainfrom
fix/fakeclock-after
Draft

reka18 wants to merge 3 commits into
mainfrom
fix/fakeclock-after

Conversation

@reka18

@reka18 reka18 commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

What this change does

FakeClock.After sent its deadline into the channel at call time, so a receive returned before any Advance. A test that selected on Clock.After (a silence timeout, a reconnect grace) saw every timeout fire at once.

Now After(d) registers a waiter with its deadline, and Advance fires every waiter the clock reaches, sending each its deadline.

  • Order: earliest deadline first, and the first armed when deadlines tie. A goroutine already blocked on several waiters wakes on the earliest one.
  • Zero or negative d: After fires at once with the current instant, as time.After does. A negative d used to send an instant in the past.
  • No Advance: a receive with no Advance far enough now blocks, and the changelog says so. In this repository only durable's two clock-seam tests relied on the old instant fire. They now check that the channel waits, then advance the wrapped clock.
  • Scheduler: unchanged. It drives after transitions through Now and Tick, not After.

Why

Host code that tests its timeouts with FakeClock passed or failed for the wrong reason.

Relates to: closes #219

Tests:

  • The new tests cover waiting for the deadline, the value sent, zero and negative durations, and one large Advance over several waiters.
  • They also cover the order a receiver parked with testing/synctest sees, ties included, and concurrent After and Advance under -race.
  • On main, the wait, non-positive and every-reached tests always fail. The order test fails in most runs: there both channels are ready at once, so select picks at random.

Checklist

  • Commits are signed off (git commit -s) per the DCO
  • Conventional commit messages (type: subject)
  • mage check passes locally (lint + race tests + govulncheck): go vet and -race tests pass for state, durable and cluster; lint and govulncheck run in CI
  • Tests added/updated for the change
  • Public API changes documented (godoc) and, if applicable, noted for the changelog

FakeClock.After sent its deadline into the channel at call time, so a
receive returned before any Advance and every timeout a host selected on
fired at once. After now registers a waiter, and Advance fires each one
the clock reaches, earliest deadline first, sending that deadline.
After(d) with d of zero or less still fires at once, as time.After does.

durable's clock tests relied on the instant fire; they now advance the
wrapped clock first.

Closes #219.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
The deadline-order test only checked each waiter's value, so removing
the sort or reversing the tie-break passed. A goroutine already blocked
on several waiters does see the order, so the new test parks a receiver
with synctest and asserts it wakes on the earliest deadline, and on the
first armed when deadlines tie. The old test keeps its checks under a
name that says what they cover.

durable's clock tests now also check that After waits for the wrapped
clock. The docs and changelog say that a receive with no Advance blocks,
and that After(d <= 0) now sends the current instant.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
Say that only a negative d changed what After sends, that a waiter fires
when the clock reaches its deadline, and that an unfired waiter stays
registered until an Advance reaches it. Reflow the godoc and name the
waiters in the order test's failure message.

Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
@reka18 reka18 added ready All ACR dimensions confirmed - clear to merge confirmed:correctness ACR clean: correctness confirmed:regression-safety ACR clean: regression-safety confirmed:concurrency-discipline ACR clean: concurrency-discipline confirmed:replay-determinism ACR clean: replay-determinism confirmed:api-compatibility ACR clean: api-compatibility confirmed:missed-context ACR clean: missed-context confirmed:human-readability ACR clean: human-readability labels Nov 9, 2026
@reka18

reka18 commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

ACR on this PR (one AI reviewer per dimension)

All seven dimensions are confirmed after two rounds.

Round 1 had 2 distinct blockers across 7 reviewers, after merging duplicates. Both are fixed in 85f1eb7:

  • Ordering test: the deadline-order test didn't check order. Removing the sort, or reversing the tie-break, still passed (correctness, missed-context and human-readability). A new test parks a receiver with testing/synctest and asserts it wakes on the earliest deadline, and on the first armed when deadlines tie. Both mutations now fail it.
  • PR description: it said every new test fails on main (human-readability). Corrected.
  • Non-blockers folded in: the changelog says a receive with no Advance now blocks, and how a negative d changed. durable's clock tests check that After waits for the wrapped clock.

Round 2 re-ran correctness, missed-context and human-readability: 0 blockers. Its wording non-blockers are fixed in 54d4bf7.

Disagreement, resolved: three reviewers (regression-safety, concurrency, replay) said the firing order can't be observed, because every channel is buffered. Correctness showed that it can: a goroutine already blocked on several channels gets the first one sent. I kept the order and tested it. A probe confirmed it in 2000 of 2000 runs. The alternative was to drop the sort and the promise. Ruled: keep the sort and the order promise. The sort adds about 0.8 µs per Advance with 10 waiters due, and about 0.9 ms with 100,000.

Not changed, recorded:

  • Unfired waiters: they stay registered until an Advance reaches them, so a re-arm loop that never advances grows the list. This is documented. There's no pending-count helper.
  • Sends outside the lock: the order test wouldn't catch them. That only matters across concurrent Advance calls, and nothing promises order there.
  • mage check: not run locally, because lint isn't installed here. go vet and the -race tests pass for state, durable and cluster. CI runs the rest.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confirmed:api-compatibility ACR clean: api-compatibility confirmed:concurrency-discipline ACR clean: concurrency-discipline confirmed:correctness ACR clean: correctness confirmed:human-readability ACR clean: human-readability confirmed:missed-context ACR clean: missed-context confirmed:regression-safety ACR clean: regression-safety confirmed:replay-determinism ACR clean: replay-determinism ready All ACR dimensions confirmed - clear to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

state: FakeClock.After must fire only after Advance reaches the deadline

1 participant