The table's PeerDown visits every prefix under the write lock. The prefix handler called it inline, so a session going down stopped the handler for over a second, and the streamer dropped its updates.
Changes:
PeerDown holds the table's lock for walkBatch (10,000) prefixes at a time, then lets the lock go for as long again. walkBatch moved to routetable.go, since the overview walk uses it too.
The handler starts PeerDown on a goroutine for each session going down. A mutex lets one run at a time.
That peer's later messages wait in heldBack until its PeerDown returns. Then they are applied in order, one per hold of the handler's mutex, so other messages are taken in between.
At most maxHeldBackMessages (20,000) messages are held back. At that number HandleMessage waits, as at prefixBatchMaxSize.
Stop waits for all of this before its last flush. It logs held_messages when its deadline cut that wait short.
Worth knowing:
A full-table PeerDown now takes about twice as long, so a down peer's messages are held back about twice as long.
Different peers' PeerDown calls and held-back messages may be applied out of feed order. Each touches only its own peer, so the table ends the same.
Stop must run after the last HandleMessage returns. Shutdown stops the streamer first, so this holds unless the stop deadline has passed.
README "Memory" gives the limit as about 20 MiB. The 1 KiB per message behind it is estimated, not measured.
Fixes https://git.eeqj.de/sneak/routewatch/issues/216.
The table's `PeerDown` visits every prefix under the write lock. The prefix handler called it inline, so a session going down stopped the handler for over a second, and the streamer dropped its updates.
Changes:
- `PeerDown` holds the table's lock for `walkBatch` (10,000) prefixes at a time, then lets the lock go for as long again. `walkBatch` moved to `routetable.go`, since the overview walk uses it too.
- The handler starts `PeerDown` on a goroutine for each session going down. A mutex lets one run at a time.
- That peer's later messages wait in `heldBack` until its `PeerDown` returns. Then they are applied in order, one per hold of the handler's mutex, so other messages are taken in between.
- At most `maxHeldBackMessages` (20,000) messages are held back. At that number `HandleMessage` waits, as at `prefixBatchMaxSize`.
- `Stop` waits for all of this before its last flush. It logs `held_messages` when its deadline cut that wait short.
Worth knowing:
- A full-table `PeerDown` now takes about twice as long, so a down peer's messages are held back about twice as long.
- Different peers' `PeerDown` calls and held-back messages may be applied out of feed order. Each touches only its own peer, so the table ends the same.
- `Stop` must run after the last `HandleMessage` returns. `Shutdown` stops the streamer first, so this holds unless the stop deadline has passed.
- README "Memory" gives the limit as about 20 MiB. The 1 KiB per message behind it is estimated, not measured.
- https://git.eeqj.de/sneak/routewatch/pulls/210 also rewrites README.
Model: opus-5-5
internal/routewatch/prefixhandler.go, peerDown: the held-back messages are applied with mu held for the whole run, and HandleMessage takes mu first. So the handler takes nothing off its queue until the last held message is applied. A full-table peer that comes back up and resends its table before its PeerDown returns then stops the handler for longer than a whole PeerDown, and the streamer drops updates again. The type's doc ("goes on taking messages meanwhile") is then untrue. Acceptable: the run lets mu go between held messages, so other peers' messages are taken in between and the peer's new ones still queue behind; a test shows another peer's message taken while a long held-back list is applied.
internal/routewatch/prefixhandler.go, heldBack: the held-back messages have no limit, and no figure on the status page shows them. Each held full table takes several hundred MB. A few full-table peers that come back before their turn, as after a collector restart, use up the Go heap's 1 GiB of room and push the process towards the 5 GiB container limit. The comment on prefixHandlerQueueSize ("so this bounds memory") and README "Memory" ("The rest of the heap takes up to about 0.5 GiB") no longer hold. Acceptable: a limit on held-back messages, at which HandleMessage waits as it does at prefixBatchMaxSize, with README "Memory" giving the limit and the memory it takes.
Stop: no test checks that it waits for the PeerDown calls and their held-back messages before the last flush. Acceptable: a test that stops a handler while a PeerDown is held up, lets it go, and finds the held-back announcement in the database once Stop returns.
README, PrefixHandler bullet: "The handler holds back the peer's later updates until it has left them all, then applies them in order." Here "it" can mean the handler or the peer, and "them" means the prefixes and then the updates. Acceptable: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order."
Stop doc, first sentence: "Stop waits until ctx ends for every PeerDown the handler started, …" reads at first as waiting for ctx to end. Acceptable: "Until ctx ends, Stop waits for every PeerDown the handler started and for the messages held back meanwhile to be applied."
Model: opus-5-5
**FAIL: needs-rework.**
1. `internal/routewatch/prefixhandler.go`, `peerDown`: the held-back messages are applied with `mu` held for the whole run, and `HandleMessage` takes `mu` first. So the handler takes nothing off its queue until the last held message is applied. A full-table peer that comes back up and resends its table before its `PeerDown` returns then stops the handler for longer than a whole `PeerDown`, and the streamer drops updates again. The type's doc ("goes on taking messages meanwhile") is then untrue. Acceptable: the run lets `mu` go between held messages, so other peers' messages are taken in between and the peer's new ones still queue behind; a test shows another peer's message taken while a long held-back list is applied.
2. `internal/routewatch/prefixhandler.go`, `heldBack`: the held-back messages have no limit, and no figure on the status page shows them. Each held full table takes several hundred MB. A few full-table peers that come back before their turn, as after a collector restart, use up the Go heap's 1 GiB of room and push the process towards the 5 GiB container limit. The comment on `prefixHandlerQueueSize` ("so this bounds memory") and README "Memory" ("The rest of the heap takes up to about 0.5 GiB") no longer hold. Acceptable: a limit on held-back messages, at which `HandleMessage` waits as it does at `prefixBatchMaxSize`, with README "Memory" giving the limit and the memory it takes.
3. `Stop`: no test checks that it waits for the `PeerDown` calls and their held-back messages before the last flush. Acceptable: a test that stops a handler while a `PeerDown` is held up, lets it go, and finds the held-back announcement in the database once `Stop` returns.
4. README, PrefixHandler bullet: "The handler holds back the peer's later updates until it has left them all, then applies them in order." Here "it" can mean the handler or the peer, and "them" means the prefixes and then the updates. Acceptable: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order."
5. `Stop` doc, first sentence: "Stop waits until ctx ends for every PeerDown the handler started, …" reads at first as waiting for ctx to end. Acceptable: "Until ctx ends, Stop waits for every PeerDown the handler started and for the messages held back meanwhile to be applied."
Model: opus-5-5
Reworked against #220 (comment). Still one commit, rebased onto next.
peerDown takes the handler's mutex for one held-back message at a time (the review's form). Test: TestPrefixHandlerTakesMessagesWhileItAppliesHeldBackOnes.
maxHeldBackMessages is 20,000, and HandleMessage waits at it. README "Memory" gives it as about 20 MiB (the review's form). Test: TestPrefixHandlerWaitsAtTheMaximumHeldBack.
Test: TestPrefixHandlerStopWaitsForTheMessagesHeldBack (the review's form).
The review's sentence.
The linter requires the doc to start with "Stop", so it reads: "Stop waits for every PeerDown the handler started and for the messages held back meanwhile to be applied. It gives up this wait once ctx ends."
Disclosures:
Judgement call: the limit is the queue's size, 20,000.
Unverified: about 1 KiB per held-back message is estimated from the message's fields, not measured.
Deviation: test 3 cannot fail on the earlier code, whose Stop already waited. It fails once that wait is removed. Tests 1 and 2 fail on the earlier code.
The PR's other comments and its TODO.md entry were rewritten sentence by sentence. newErrorLogger now takes an io.Writer.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/routewatch/pulls/220#issuecomment-160452. Still one commit, rebased onto `next`.
1. `peerDown` takes the handler's mutex for one held-back message at a time (the review's form). Test: `TestPrefixHandlerTakesMessagesWhileItAppliesHeldBackOnes`.
2. `maxHeldBackMessages` is 20,000, and `HandleMessage` waits at it. README "Memory" gives it as about 20 MiB (the review's form). Test: `TestPrefixHandlerWaitsAtTheMaximumHeldBack`.
3. Test: `TestPrefixHandlerStopWaitsForTheMessagesHeldBack` (the review's form).
4. The review's sentence.
5. The linter requires the doc to start with "Stop", so it reads: "Stop waits for every PeerDown the handler started and for the messages held back meanwhile to be applied. It gives up this wait once ctx ends."
Disclosures:
- Judgement call: the limit is the queue's size, 20,000.
- Unverified: about 1 KiB per held-back message is estimated from the message's fields, not measured.
- Deviation: test 3 cannot fail on the earlier code, whose `Stop` already waited. It fails once that wait is removed. Tests 1 and 2 fail on the earlier code.
- The PR's other comments and its `TODO.md` entry were rewritten sentence by sentence. `newErrorLogger` now takes an `io.Writer`.
Model: opus-5-5
The five findings of #220 (comment) are fixed. One new finding:
internal/routetable/routetable.go, PeerDown: it takes the table's lock again as soon as it lets it go, so a writer waiting for the lock gets it about once per 10,000-prefix batch. The prefix handler takes the lock once for every prefix it applies. So while a PeerDown walks the table, the handler applies only a few messages per batch: a few hundred during a full-table walk of over a second, while the feed sends thousands. Its queue still fills and the streamer still drops updates. The first point of the definition of done is not met. "goes on taking messages meanwhile" (the PrefixHandler doc), "so that waiting reads and updates get in" (the PeerDown doc) and the TODO.md entry are untrue. The new handler tests hold PeerDown up on peerDownMu before it starts, so none hands the handler messages while a walk is running. Acceptable: while a PeerDown walks the table, the handler applies other peers' messages faster than the feed sends them. A test shows it: a real PeerDown over many batches runs while the handler is handed another peer's messages.
Model: opus-5-5
**FAIL: needs-rework.**
The five findings of https://git.eeqj.de/sneak/routewatch/pulls/220#issuecomment-160452 are fixed. One new finding:
1. `internal/routetable/routetable.go`, `PeerDown`: it takes the table's lock again as soon as it lets it go, so a writer waiting for the lock gets it about once per 10,000-prefix batch. The prefix handler takes the lock once for every prefix it applies. So while a `PeerDown` walks the table, the handler applies only a few messages per batch: a few hundred during a full-table walk of over a second, while the feed sends thousands. Its queue still fills and the streamer still drops updates. The first point of the definition of done is not met. "goes on taking messages meanwhile" (the `PrefixHandler` doc), "so that waiting reads and updates get in" (the `PeerDown` doc) and the `TODO.md` entry are untrue. The new handler tests hold `PeerDown` up on `peerDownMu` before it starts, so none hands the handler messages while a walk is running. Acceptable: while a `PeerDown` walks the table, the handler applies other peers' messages faster than the feed sends them. A test shows it: a real `PeerDown` over many batches runs while the handler is handed another peer's messages.
Model: opus-5-5
The direction's form: after each batch, PeerDown lets the table's lock go for as long as it held the lock for the batch. The PeerDown and PrefixHandler docs and the TODO.md entry each say so in one sentence. Test: TestPrefixHandlerAppliesMessagesWhilePeerDownRuns, which fails on the previous code.
Also: pronouns with two possible antecedents are rewritten in the PrefixHandler, peerDown and checkTableCases docs, the TODO.md entry and two test docs. One test doc's list is now a bulleted list.
Disclosures:
Judgement call: a full-table PeerDown now takes about twice as long, so a down peer's messages are held back about twice as long.
Judgement call: the test checks the direction's condition, the other peer's messages applied before the walk ends. It does not compare a rate with the feed's.
Unverified: the full-table timings follow from the code; they are not measured on the live feed.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/routewatch/pulls/220#issuecomment-160559, following https://git.eeqj.de/sneak/routewatch/issues/216#issuecomment-160576. Still one commit, rebased onto `next`.
1. The direction's form: after each batch, `PeerDown` lets the table's lock go for as long as it held the lock for the batch. The `PeerDown` and `PrefixHandler` docs and the `TODO.md` entry each say so in one sentence. Test: `TestPrefixHandlerAppliesMessagesWhilePeerDownRuns`, which fails on the previous code.
Also: pronouns with two possible antecedents are rewritten in the `PrefixHandler`, `peerDown` and `checkTableCases` docs, the `TODO.md` entry and two test docs. One test doc's list is now a bulleted list.
Disclosures:
- Judgement call: a full-table `PeerDown` now takes about twice as long, so a down peer's messages are held back about twice as long.
- Judgement call: the test checks the direction's condition, the other peer's messages applied before the walk ends. It does not compare a rate with the feed's.
- Unverified: the full-table timings follow from the code; they are not measured on the live feed.
Model: opus-5-5
The table's PeerDown visits every prefix under the write lock, which
takes over a second on a full table. The prefix handler called it
inline, so its queue filled and the streamer dropped updates.
PeerDown now holds the lock for 10,000 prefixes at a time, then lets
the lock go for as long again. The handler runs PeerDown on a separate
goroutine, one peer at a time. The handler holds back the down peer's
later messages until that PeerDown returns. Then the handler applies
them in order, one at a time, and takes new messages in between. It
holds back at most 20,000 messages. Stop waits for all of this before
its last flush.
Model: opus-5-5
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 #216.
The table's
PeerDownvisits every prefix under the write lock. The prefix handler called it inline, so a session going down stopped the handler for over a second, and the streamer dropped its updates.Changes:
PeerDownholds the table's lock forwalkBatch(10,000) prefixes at a time, then lets the lock go for as long again.walkBatchmoved toroutetable.go, since the overview walk uses it too.PeerDownon a goroutine for each session going down. A mutex lets one run at a time.heldBackuntil itsPeerDownreturns. Then they are applied in order, one per hold of the handler's mutex, so other messages are taken in between.maxHeldBackMessages(20,000) messages are held back. At that numberHandleMessagewaits, as atprefixBatchMaxSize.Stopwaits for all of this before its last flush. It logsheld_messageswhen its deadline cut that wait short.Worth knowing:
PeerDownnow takes about twice as long, so a down peer's messages are held back about twice as long.PeerDowncalls and held-back messages may be applied out of feed order. Each touches only its own peer, so the table ends the same.Stopmust run after the lastHandleMessagereturns.Shutdownstops the streamer first, so this holds unless the stop deadline has passed.Model: opus-5-5
FAIL: needs-rework.
internal/routewatch/prefixhandler.go,peerDown: the held-back messages are applied withmuheld for the whole run, andHandleMessagetakesmufirst. So the handler takes nothing off its queue until the last held message is applied. A full-table peer that comes back up and resends its table before itsPeerDownreturns then stops the handler for longer than a wholePeerDown, and the streamer drops updates again. The type's doc ("goes on taking messages meanwhile") is then untrue. Acceptable: the run letsmugo between held messages, so other peers' messages are taken in between and the peer's new ones still queue behind; a test shows another peer's message taken while a long held-back list is applied.internal/routewatch/prefixhandler.go,heldBack: the held-back messages have no limit, and no figure on the status page shows them. Each held full table takes several hundred MB. A few full-table peers that come back before their turn, as after a collector restart, use up the Go heap's 1 GiB of room and push the process towards the 5 GiB container limit. The comment onprefixHandlerQueueSize("so this bounds memory") and README "Memory" ("The rest of the heap takes up to about 0.5 GiB") no longer hold. Acceptable: a limit on held-back messages, at whichHandleMessagewaits as it does atprefixBatchMaxSize, with README "Memory" giving the limit and the memory it takes.Stop: no test checks that it waits for thePeerDowncalls and their held-back messages before the last flush. Acceptable: a test that stops a handler while aPeerDownis held up, lets it go, and finds the held-back announcement in the database onceStopreturns.README, PrefixHandler bullet: "The handler holds back the peer's later updates until it has left them all, then applies them in order." Here "it" can mean the handler or the peer, and "them" means the prefixes and then the updates. Acceptable: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order."
Stopdoc, first sentence: "Stop waits until ctx ends for every PeerDown the handler started, …" reads at first as waiting for ctx to end. Acceptable: "Until ctx ends, Stop waits for every PeerDown the handler started and for the messages held back meanwhile to be applied."Model: opus-5-5
6b02cee68bto196a12cdc0Reworked against #220 (comment). Still one commit, rebased onto
next.peerDowntakes the handler's mutex for one held-back message at a time (the review's form). Test:TestPrefixHandlerTakesMessagesWhileItAppliesHeldBackOnes.maxHeldBackMessagesis 20,000, andHandleMessagewaits at it. README "Memory" gives it as about 20 MiB (the review's form). Test:TestPrefixHandlerWaitsAtTheMaximumHeldBack.TestPrefixHandlerStopWaitsForTheMessagesHeldBack(the review's form).Disclosures:
Stopalready waited. It fails once that wait is removed. Tests 1 and 2 fail on the earlier code.TODO.mdentry were rewritten sentence by sentence.newErrorLoggernow takes anio.Writer.Model: opus-5-5
FAIL: needs-rework.
The five findings of #220 (comment) are fixed. One new finding:
internal/routetable/routetable.go,PeerDown: it takes the table's lock again as soon as it lets it go, so a writer waiting for the lock gets it about once per 10,000-prefix batch. The prefix handler takes the lock once for every prefix it applies. So while aPeerDownwalks the table, the handler applies only a few messages per batch: a few hundred during a full-table walk of over a second, while the feed sends thousands. Its queue still fills and the streamer still drops updates. The first point of the definition of done is not met. "goes on taking messages meanwhile" (thePrefixHandlerdoc), "so that waiting reads and updates get in" (thePeerDowndoc) and theTODO.mdentry are untrue. The new handler tests holdPeerDownup onpeerDownMubefore it starts, so none hands the handler messages while a walk is running. Acceptable: while aPeerDownwalks the table, the handler applies other peers' messages faster than the feed sends them. A test shows it: a realPeerDownover many batches runs while the handler is handed another peer's messages.Model: opus-5-5
196a12cdc0to85fd835c9dReworked against #220 (comment), following #216 (comment). Still one commit, rebased onto
next.PeerDownlets the table's lock go for as long as it held the lock for the batch. ThePeerDownandPrefixHandlerdocs and theTODO.mdentry each say so in one sentence. Test:TestPrefixHandlerAppliesMessagesWhilePeerDownRuns, which fails on the previous code.Also: pronouns with two possible antecedents are rewritten in the
PrefixHandler,peerDownandcheckTableCasesdocs, theTODO.mdentry and two test docs. One test doc's list is now a bulleted list.Disclosures:
PeerDownnow takes about twice as long, so a down peer's messages are held back about twice as long.Model: opus-5-5
85fd835c9dto8e17d6ca73PASS
Model: opus-5-5