Start no prefix flush after the stop deadline (closes #188)

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

internal/routewatch/batchwrite_internal_test.go

@@ -317,7 +317,9 @@ func stopWhileAWriteIsBlocked(
 
 // TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline also checks the
 // routing table's changes: the flush whose delete is held wrote those of the
-// fill, so Stop must log the one of the extra prefix as not written.
+// fill, so Stop must log the one of the extra prefix as not written. The flush
+// timer must not fire. A flush it started before the fill would be the one
+// held, and it would not have written the fill's change.
 func TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline(t *testing.T) {
 	t.Parallel()
 
@@ -326,8 +328,8 @@ func TestPrefixHandlerSkipsItsLastWriteAfterTheStopDeadline(t *testing.T) {
 	var logs bytes.Buffer
 
 	store := newBlockingStore()
-	h := NewPrefixHandler(store, newErrorLogger(&logs),
-		routetable.New(routetable.Params{}))
+	h := newPrefixHandler(store, newErrorLogger(&logs),
+		routetable.New(routetable.Params{}), longerThanAnyTest)
 	fill := slices.Repeat([]string{testV4Prefix}, prefixBatchSize)
 	// The peer's session goes down first, so the write held is the delete of
 	// its routes, and Stop must log that peer down too.

internal/routewatch/prefixhandler.go

@@ -130,6 +130,16 @@ func (u prefixUpdate) announcesRoute() bool {
 // the updates to table, the routing table read from db at startup.
 func NewPrefixHandler(
 	db database.Store, logger *logger.Logger, table *routetable.Table,
+) *PrefixHandler {
+	return newPrefixHandler(db, logger, table, prefixBatchTimeout)
+}
+
+// newPrefixHandler is NewPrefixHandler with the flush timer's interval,
+// batchTimeout, in place of prefixBatchTimeout. A test that the timer must not
+// disturb gives an interval longer than the test runs.
+func newPrefixHandler(
+	db database.Store, logger *logger.Logger, table *routetable.Table,
+	batchTimeout time.Duration,
 ) *PrefixHandler {
 	h := &PrefixHandler{
 		db:          db,
@@ -144,7 +154,7 @@ func NewPrefixHandler(
 	h.batchTaken.L = &h.mu
 
 	// Start the flush timer goroutine
-	go h.flushLoop()
+	go h.flushLoop(batchTimeout)
 
 	return h
 }
@@ -315,11 +325,11 @@ func (h *PrefixHandler) peerSessionDown(peer string) {
 }
 
 // flushLoop runs in a goroutine, the only one that writes the batch: it
-// flushes when the batch is full, periodically, and once more on Stop.
-func (h *PrefixHandler) flushLoop() {
+// flushes when the batch is full, every batchTimeout, and once more on Stop.
+func (h *PrefixHandler) flushLoop(batchTimeout time.Duration) {
 	defer close(h.done)
 
-	ticker := time.NewTicker(prefixBatchTimeout)
+	ticker := time.NewTicker(batchTimeout)
 	defer ticker.Stop()
 
 	for {
@@ -340,7 +350,7 @@ func (h *PrefixHandler) flushLoop() {
 
 		select {
 		case <-ticker.C:
-			if time.Since(h.lastFlush) >= prefixBatchTimeout {
+			if time.Since(h.lastFlush) >= batchTimeout {
 				h.flushBatch()
 			}
 		case <-h.batchFullCh:

internal/routewatch/prefixhandler_internal_test.go

@@ -980,6 +980,11 @@ func TestPrefixHandlerStopLogsNothingAfterADeleteThatSucceeds(t *testing.T) {
 	}
 }
 
+// longerThanAnyTest is a flush timer interval that no test lasts. A test whose
+// prefix handler must flush only when its batch is full and on Stop gives it to
+// newPrefixHandler.
+const longerThanAnyTest = time.Hour
+
 // prefixHandlerWithoutTimer returns a prefix handler that writes to store and
 // applies the updates to an empty routing table. It is built without
 // NewPrefixHandler's flush goroutine, so the test flushes itself.

internal/routewatch/prefixhandler_table_internal_test.go

@@ -341,15 +341,14 @@ func TestPrefixHandlerWritesManyTableChangesOverSeveralFlushes(t *testing.T) {
 }
 
 // TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten stops a prefix handler,
-// with no deadline, whose routing table holds more changed pairs than two
-// writes take. Its last flush writes one write's worth, so Stop must log the
-// pairs that no write took.
+// with no deadline, whose routing table holds more changed pairs than one write
+// takes. Its last flush writes one write's worth, so Stop must log the pairs
+// that no write took. The flush timer must not fire, or its writes could leave
+// no pairs for Stop to log.
 func TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten(t *testing.T) {
 	t.Parallel()
 
-	// More than two writes take, so that pairs are left even when the flush
-	// timer writes once before Stop.
-	const pairs = 2*maxTableChangesPerWrite + 1
+	const pairs = maxTableChangesPerWrite + 1
 
 	var logs bytes.Buffer
 
@@ -357,7 +356,7 @@ func TestPrefixHandlerStopLogsTheTableChangesLeftUnwritten(t *testing.T) {
 	table := routetable.New(routetable.Params{})
 	announceManyPrefixes(table, pairs)
 
-	h := NewPrefixHandler(store, newErrorLogger(&logs), table)
+	h := newPrefixHandler(store, newErrorLogger(&logs), table, longerThanAnyTest)
 	h.Stop(t.Context())
 
 	written := 0