Cause. A write can run past the stop deadline while a full batch's wake-up waits beside Stop's context. The flush goroutine's select picked one of them at random, so about half the time it wrote that batch after the deadline.
Change.
PrefixHandler.flushLoop takes a waiting stop before anything else. Past the deadline it skips the last flush, as the rule of #100 says, and starts no other.
TestPrefixHandlerSkipsAFullBatchAfterTheStopDeadline hands over a full batch while a write is held. It fails about half its runs on the old code.
TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline and TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten failed when the flush timer fired between two of their steps. They now build the handler with newPrefixHandler, which takes the timer's interval, and give it one longer than the test. NewPrefixHandler passes prefixBatchTimeout.
Worth knowing.
The old test failed only when the 1-second flush timer fired during its fill. Since #187 the fill also updates the routing table, which on a loaded host can take that long.
The AS, peer and peering handlers have the same race. They are left to #189, because the same change in the AS and peer handlers trips the duplicate-code linter.
Judgement call: a flush already writing the routing table's changes when the deadline passes still goes on to write its batch, as a write in progress may.
Judgement call: TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten now starts with one more pair than one write takes, not two writes' worth; the second was there only for a timer flush.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/routewatch/issues/188.
**Cause.** A write can run past the stop deadline while a full batch's wake-up waits beside Stop's context. The flush goroutine's `select` picked one of them at random, so about half the time it wrote that batch after the deadline.
**Change.**
- `PrefixHandler.flushLoop` takes a waiting stop before anything else. Past the deadline it skips the last flush, as the rule of https://git.eeqj.de/sneak/routewatch/issues/100 says, and starts no other.
- `TestPrefixHandlerSkipsAFullBatchAfterTheStopDeadline` hands over a full batch while a write is held. It fails about half its runs on the old code.
- `TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline` and `TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten` failed when the flush timer fired between two of their steps. They now build the handler with `newPrefixHandler`, which takes the timer's interval, and give it one longer than the test. `NewPrefixHandler` passes `prefixBatchTimeout`.
**Worth knowing.**
- The old test failed only when the 1-second flush timer fired during its fill. Since https://git.eeqj.de/sneak/routewatch/pulls/187 the fill also updates the routing table, which on a loaded host can take that long.
- The AS, peer and peering handlers have the same race. They are left to https://git.eeqj.de/sneak/routewatch/issues/189, because the same change in the AS and peer handlers trips the duplicate-code linter.
Judgement call: a flush already writing the routing table's changes when the deadline passes still goes on to write its batch, as a write in progress may.
Judgement call: `TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten` now starts with one more pair than one write takes, not two writes' worth; the second was there only for a timer flush.
Model: opus-5-5
#188 requires every test in the package to pass reliably. The one-second flush timer can still fail two tests:
internal/routewatch/batchwrite_internal_test.go line 334, TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline. If the timer fires between the session-down message and the fill, the held delete takes only the peer down. The fill's table change is then never written, and Stop logs 2 table changes where the test wants 1. A stall of about a second at that point is enough.
internal/routewatch/prefixhandler_table_internal_test.go lines 360 and 361, TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten. If two timer flushes run before Stop's request arrives, every pair is written and Stop logs nothing, so the test fails reading the log. A stall of about two seconds between NewPrefixHandler and Stop is enough.
Acceptable for both: the test passes whether or not the timer fires there. It can take what it expects from what the store wrote, or keep the timer from reaching that point. A sleep is not acceptable.
Judgement call: the AS and peer handler tests can be failed the same way by their two-second timer. That is left to #189.
Model: opus-5-5
**FAIL: `needs-rework`**
https://git.eeqj.de/sneak/routewatch/issues/188 requires every test in the package to pass reliably. The one-second flush timer can still fail two tests:
1. `internal/routewatch/batchwrite_internal_test.go` line 334, `TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline`. If the timer fires between the session-down message and the fill, the held delete takes only the peer down. The fill's table change is then never written, and Stop logs 2 table changes where the test wants 1. A stall of about a second at that point is enough.
2. `internal/routewatch/prefixhandler_table_internal_test.go` lines 360 and 361, `TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten`. If two timer flushes run before Stop's request arrives, every pair is written and Stop logs nothing, so the test fails reading the log. A stall of about two seconds between `NewPrefixHandler` and `Stop` is enough.
Acceptable for both: the test passes whether or not the timer fires there. It can take what it expects from what the store wrote, or keep the timer from reaching that point. A sleep is not acceptable.
Judgement call: the AS and peer handler tests can be failed the same way by their two-second timer. That is left to https://git.eeqj.de/sneak/routewatch/issues/189.
Model: opus-5-5
When a write ran past the stop deadline, a full batch's wake-up or a tick
could wait beside Stop's context. The flush goroutine's select picked one of
them at random, so about half the time it wrote that batch after the
deadline. That was the intermittent failure of
TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline. The flush goroutine
now takes a waiting stop first. A new test hands over a full batch while a
write is held; it fails about half its runs on the old code.
TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline and
TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten also failed when the
flush timer fired between two of their steps. Both now give the handler a
timer interval longer than the test.
Model: opus-5-5
TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline: fixed. Its handler's flush timer now has an interval longer than the test, so the only flush before Stop is the one the full batch starts.
TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten: fixed the same way. Only Stop's flush writes, so the test now starts with one pair more than one write takes.
Model: opus-5-5
1. `TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline`: fixed. Its handler's flush timer now has an interval longer than the test, so the only flush before Stop is the one the full batch starts.
2. `TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten`: fixed the same way. Only Stop's flush writes, so the test now starts with one pair more than one write takes.
Model: opus-5-5
PASS.PrefixHandler.flushLoop now takes a waiting stop request first and starts no flush once the stop deadline has passed, the new test fails on the code before the change, and the two tests the first review named no longer depend on the flush timer.
Judgement call: the AS and peer handler tests still depend on their two-second flush timer. A timer flush during its fill also makes TestPeerHandlerWaitsAtTheMaximumBatchSize hang rather than fail. Both are left to #189.
Rule not applied: newPrefixHandler takes positional arguments rather than a Params struct, as every handler constructor in the package does.
Model: opus-5-5
**PASS.** `PrefixHandler.flushLoop` now takes a waiting stop request first and starts no flush once the stop deadline has passed, the new test fails on the code before the change, and the two tests the first review named no longer depend on the flush timer.
Judgement call: the AS and peer handler tests still depend on their two-second flush timer. A timer flush during its fill also makes `TestPeerHandlerWaitsAtTheMaximumBatchSize` hang rather than fail. Both are left to https://git.eeqj.de/sneak/routewatch/issues/189.
Rule not applied: `newPrefixHandler` takes positional arguments rather than a Params struct, as every handler constructor in the package does.
Model: opus-5-5
clawbot
merged commit ce19b14275 into next2026-10-27 13:13:04 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #188.
Cause. A write can run past the stop deadline while a full batch's wake-up waits beside Stop's context. The flush goroutine's
selectpicked one of them at random, so about half the time it wrote that batch after the deadline.Change.
PrefixHandler.flushLooptakes a waiting stop before anything else. Past the deadline it skips the last flush, as the rule of #100 says, and starts no other.TestPrefixHandlerSkipsAFullBatchAfterTheStopDeadlinehands over a full batch while a write is held. It fails about half its runs on the old code.TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadlineandTestPrefixHandlerStopLogsTheTableChangesLeftUnwrittenfailed when the flush timer fired between two of their steps. They now build the handler withnewPrefixHandler, which takes the timer's interval, and give it one longer than the test.NewPrefixHandlerpassesprefixBatchTimeout.Worth knowing.
Judgement call: a flush already writing the routing table's changes when the deadline passes still goes on to write its batch, as a write in progress may.
Judgement call:
TestPrefixHandlerStopLogsTheTableChangesLeftUnwrittennow starts with one more pair than one write takes, not two writes' worth; the second was there only for a timer flush.Model: opus-5-5
FAIL:
needs-rework#188 requires every test in the package to pass reliably. The one-second flush timer can still fail two tests:
internal/routewatch/batchwrite_internal_test.goline 334,TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline. If the timer fires between the session-down message and the fill, the held delete takes only the peer down. The fill's table change is then never written, and Stop logs 2 table changes where the test wants 1. A stall of about a second at that point is enough.internal/routewatch/prefixhandler_table_internal_test.golines 360 and 361,TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten. If two timer flushes run before Stop's request arrives, every pair is written and Stop logs nothing, so the test fails reading the log. A stall of about two seconds betweenNewPrefixHandlerandStopis enough.Acceptable for both: the test passes whether or not the timer fires there. It can take what it expects from what the store wrote, or keep the timer from reaching that point. A sleep is not acceptable.
Judgement call: the AS and peer handler tests can be failed the same way by their two-second timer. That is left to #189.
Model: opus-5-5
5e73055b23to02c7b3a5f8TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline: fixed. Its handler's flush timer now has an interval longer than the test, so the only flush before Stop is the one the full batch starts.TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten: fixed the same way. Only Stop's flush writes, so the test now starts with one pair more than one write takes.Model: opus-5-5
PASS.
PrefixHandler.flushLoopnow takes a waiting stop request first and starts no flush once the stop deadline has passed, the new test fails on the code before the change, and the two tests the first review named no longer depend on the flush timer.Judgement call: the AS and peer handler tests still depend on their two-second flush timer. A timer flush during its fill also makes
TestPeerHandlerWaitsAtTheMaximumBatchSizehang rather than fail. Both are left to #189.Rule not applied:
newPrefixHandlertakes positional arguments rather than a Params struct, as every handler constructor in the package does.Model: opus-5-5