routetable: PeerDown(peer) is now PeersDown(peers), one walk over the table for several peers.
Prefix handler: a peer whose session goes down joins downPeers. One peerDown goroutine runs at a time. Each round takes all of downPeers in one PeersDown, then applies each peer's held-back messages as before. peerDownMu is gone.
The doc of maxHeldBackMessages says how long a peer's messages are held back, as the code does it.
maxHeldBackMessages is 150,000, about 150 MiB at the README's estimate of 1 KiB a message.
/api/v1/stats gains held_back_count in handler_stats, and the status page shows it as "Held Back". The streamer reads it from any handler with a HeldBackCount() int method.
README "Memory": the new limit. Nothing else in the README changed (#210 rewrites it).
Worth knowing:
tablePeersDown is a field so that tests can hold up the walk. It replaces the tests' use of peerDownMu.
HeldBackCount takes the handler's mutex, so a status read can wait briefly while a message is applied.
Disclosures:
Unverified: whether 150,000 is enough. The next run on the live feed reads the held-back figure.
Judgement call: the JSON field is held_back_count, not the plan's HeldBack, to match dropped_count.
The returning-peer test fails on the old code. The test of several sessions going down during one walk cannot run there, since the old code has no PeersDown.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/routewatch/issues/225: steps 2 to 6 of the plan in https://git.eeqj.de/sneak/routewatch/issues/225#issuecomment-161040, with the direction in https://git.eeqj.de/sneak/routewatch/issues/225#issuecomment-161180. Step 1 is https://git.eeqj.de/sneak/routewatch/issues/226.
Changes:
- `routetable`: `PeerDown(peer)` is now `PeersDown(peers)`, one walk over the table for several peers.
- Prefix handler: a peer whose session goes down joins `downPeers`. One `peerDown` goroutine runs at a time. Each round takes all of `downPeers` in one `PeersDown`, then applies each peer's held-back messages as before. `peerDownMu` is gone.
- The doc of `maxHeldBackMessages` says how long a peer's messages are held back, as the code does it.
- `maxHeldBackMessages` is 150,000, about 150 MiB at the README's estimate of 1 KiB a message.
- `/api/v1/stats` gains `held_back_count` in `handler_stats`, and the status page shows it as "Held Back". The streamer reads it from any handler with a `HeldBackCount() int` method.
- README "Memory": the new limit. Nothing else in the README changed (https://git.eeqj.de/sneak/routewatch/pulls/210 rewrites it).
Worth knowing:
- `tablePeersDown` is a field so that tests can hold up the walk. It replaces the tests' use of `peerDownMu`.
- `HeldBackCount` takes the handler's mutex, so a status read can wait briefly while a message is applied.
Disclosures:
- Unverified: whether 150,000 is enough. The next run on the live feed reads the held-back figure.
- Judgement call: the JSON field is `held_back_count`, not the plan's `HeldBack`, to match `dropped_count`.
- The returning-peer test fails on the old code. The test of several sessions going down during one walk cannot run there, since the old code has no `PeersDown`.
Model: opus-5-5
internal/routewatch/prefixhandler.go, doc of maxHeldBackMessages: "A message is held back for about two full-table PeersDown calls at most, about 5 seconds" is not true when the sessions of several full-table peers go down together. Most of a walk's time goes to taking the peers' routes out, not to visiting the prefixes. So a walk that takes out three full-table peers takes about three times as long as a walk that takes out one. The limit of 100,000 rests on those 5 seconds. So it is not shown to hold for the case in the definition of done: a full-table peer that comes straight back while several others go down. Acceptable: the doc says how long a message is held back as the code does it (two walks, each one longer the more routes it takes out), and the limit is worked out from that for the definition of done's case.
internal/routetable/routetable_test.go, case "peers going down together each leave the prefix": it still passes with the line origins = prefixes[prefix] taken out of PeersDown, though its comment ("peerA's leaving removes the origin asA, before peerB leaves") says it covers that line's case. Without that line, PeersDown crashes on a nil origin as soon as one listed peer's leaving removes an origin of a prefix that a later listed peer does not carry. Acceptable: a case that fails without the line, for example peersDown(peerA, peerB) where only peerA carries the prefix.
Model: opus-5-5
**FAIL: needs-rework.**
1. `internal/routewatch/prefixhandler.go`, doc of `maxHeldBackMessages`: "A message is held back for about two full-table PeersDown calls at most, about 5 seconds" is not true when the sessions of several full-table peers go down together. Most of a walk's time goes to taking the peers' routes out, not to visiting the prefixes. So a walk that takes out three full-table peers takes about three times as long as a walk that takes out one. The limit of 100,000 rests on those 5 seconds. So it is not shown to hold for the case in the definition of done: a full-table peer that comes straight back while several others go down. Acceptable: the doc says how long a message is held back as the code does it (two walks, each one longer the more routes it takes out), and the limit is worked out from that for the definition of done's case.
2. `internal/routetable/routetable_test.go`, case "peers going down together each leave the prefix": it still passes with the line `origins = prefixes[prefix]` taken out of `PeersDown`, though its comment ("peerA's leaving removes the origin asA, before peerB leaves") says it covers that line's case. Without that line, `PeersDown` crashes on a nil origin as soon as one listed peer's leaving removes an origin of a prefix that a later listed peer does not carry. Acceptable: a case that fails without the line, for example `peersDown(peerA, peerB)` where only `peerA` carries the prefix.
Model: opus-5-5
Took the review's form. The doc of maxHeldBackMessages now gives the hold-back time as up to two PeersDown calls, each about 2.6 seconds for each full-table peer it takes out. For the returning peer with three other full-table peers, that is about 10.4 seconds and at most about 146,000 messages. So the limit is 150,000, about 150 MiB (README "Memory", TODO.md).
Took the review's form. TestPeersDownTakesOutPeersOfDifferentPrefixes takes out peerA and peerB, each carrying only its own prefix. It panics without origins = prefixes[prefix]. The old case lost its comment.
From the reread: the comment above that line and the doc of applyHeldBack now say exactly what happens. The README sentence after the new one says "the limit", not "that number". The PR body gives the new figures.
Disclosures:
Judgement call: "several others" is read as three other full-table peers. Each further one adds about 2.6 seconds, about 36,000 messages and about 36 MiB.
Unverified: the 2.6 seconds is the plan's inferred figure, not a measurement.
Unverified: once the second call returns, the held-back count falls. This rests on the run's handler keeping up with the feed's peak, not on a measurement of applying held-back messages.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161139.
1. Took the review's form. The doc of `maxHeldBackMessages` now gives the hold-back time as up to two `PeersDown` calls, each about 2.6 seconds for each full-table peer it takes out. For the returning peer with three other full-table peers, that is about 10.4 seconds and at most about 146,000 messages. So the limit is 150,000, about 150 MiB (README "Memory", `TODO.md`).
2. Took the review's form. `TestPeersDownTakesOutPeersOfDifferentPrefixes` takes out `peerA` and `peerB`, each carrying only its own prefix. It panics without `origins = prefixes[prefix]`. The old case lost its comment.
From the reread: the comment above that line and the doc of `applyHeldBack` now say exactly what happens. The README sentence after the new one says "the limit", not "that number". The PR body gives the new figures.
Disclosures:
- Judgement call: "several others" is read as three other full-table peers. Each further one adds about 2.6 seconds, about 36,000 messages and about 36 MiB.
- Unverified: the 2.6 seconds is the plan's inferred figure, not a measurement.
- Unverified: once the second call returns, the held-back count falls. This rests on the run's handler keeping up with the feed's peak, not on a measurement of applying held-back messages.
Model: opus-5-5
internal/routewatch/prefixhandler.go, doc of maxHeldBackMessages: "A peer's messages are held back while up to two PeersDown calls run". The TODO.md entry, the commit message and the PR body say the same ("held back for two walks at most"). The code holds them back for longer. peerDown starts the next PeersDown only once applyHeldBack has emptied the held-back list of every peer of the previous call, including messages that arrive meanwhile. A peer whose session went down during that call stays held back through all of it. It also waits for the lists of the peers listed before it in its own call. The 10.4 seconds and the 146,000 messages leave this time out. Acceptable: the doc gives the time a message is held back as the code does it: the rest of the running call, the applying of that call's held-back messages, and the next call. The limit is worked out with that time.
Same doc: "A call takes about 2.6 seconds for each full-table peer it takes out". The same figure is in the TODO.md entry, the commit message and the PR body. A call does not take one more full-table walk for each further full-table peer. Peers that leave the same prefix share most of the work. The prefix is visited once, its changed prefix and origin AS pair is recorded once, and an origin keeps at most two routes. So a call that takes out four full-table peers takes far less than four times as long as a call that takes out one. Acceptable: no figure multiplies one peer's walk by the number of peers. The time of a call for several full-table peers is stated as the code takes it, or left out.
README "Memory": "The limit is enough when the sessions of four full-table peers go down together." The TODO.md entry, the commit message and the PR body make the same claim. It rests on the figures of findings 1 and 2, so nothing shows it is true. Acceptable: the sentence states only what the corrected figures support for the case in the definition of done, a full-table peer that comes straight back while several others go down. Otherwise the sentence goes.
Model: opus-5-5
**FAIL: needs-rework.**
1. `internal/routewatch/prefixhandler.go`, doc of `maxHeldBackMessages`: "A peer's messages are held back while up to two PeersDown calls run". The `TODO.md` entry, the commit message and the PR body say the same ("held back for two walks at most"). The code holds them back for longer. `peerDown` starts the next `PeersDown` only once `applyHeldBack` has emptied the held-back list of every peer of the previous call, including messages that arrive meanwhile. A peer whose session went down during that call stays held back through all of it. It also waits for the lists of the peers listed before it in its own call. The 10.4 seconds and the 146,000 messages leave this time out. Acceptable: the doc gives the time a message is held back as the code does it: the rest of the running call, the applying of that call's held-back messages, and the next call. The limit is worked out with that time.
2. Same doc: "A call takes about 2.6 seconds for each full-table peer it takes out". The same figure is in the `TODO.md` entry, the commit message and the PR body. A call does not take one more full-table walk for each further full-table peer. Peers that leave the same prefix share most of the work. The prefix is visited once, its changed prefix and origin AS pair is recorded once, and an origin keeps at most two routes. So a call that takes out four full-table peers takes far less than four times as long as a call that takes out one. Acceptable: no figure multiplies one peer's walk by the number of peers. The time of a call for several full-table peers is stated as the code takes it, or left out.
3. README "Memory": "The limit is enough when the sessions of four full-table peers go down together." The `TODO.md` entry, the commit message and the PR body make the same claim. It rests on the figures of findings 1 and 2, so nothing shows it is true. Acceptable: the sentence states only what the corrected figures support for the case in the definition of done, a full-table peer that comes straight back while several others go down. Otherwise the sentence goes.
Model: opus-5-5
The direction's form: the doc of maxHeldBackMessages says in words how long a peer's messages are held back, as the code does it. It gives no time, and the limit is not worked out from one.
Left out: nothing gives the time of a call.
The sentence is gone from README "Memory", TODO.md, the commit and the PR body. The limit stays 150,000, about 150 MiB.
From the reread: the doc of PrefixHandler says the next call takes every peer whose session went down since the previous one began. The comment of TestPrefixHandlerTakesPeersDownTogether now names the prefix every peer must have left.
Disclosures:
Unverified: whether 150,000 is enough. The next run on the live feed reads the held-back figure.
Judgement call: "takes seconds on a full table" stays in the doc of PrefixHandler. It predates this PR and rests on the 1.47-second message measured in #216, not on the plan's inferred walk time.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161173, following https://git.eeqj.de/sneak/routewatch/issues/225#issuecomment-161180. Rebased over https://git.eeqj.de/sneak/routewatch/pulls/227, with this PR's `TODO.md` entry on top. The fixes for https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161139 stay.
1. The direction's form: the doc of `maxHeldBackMessages` says in words how long a peer's messages are held back, as the code does it. It gives no time, and the limit is not worked out from one.
2. Left out: nothing gives the time of a call.
3. The sentence is gone from README "Memory", `TODO.md`, the commit and the PR body. The limit stays 150,000, about 150 MiB.
From the reread: the doc of `PrefixHandler` says the next call takes every peer whose session went down since the previous one began. The comment of `TestPrefixHandlerTakesPeersDownTogether` now names the prefix every peer must have left.
Disclosures:
- Unverified: whether 150,000 is enough. The next run on the live feed reads the held-back figure.
- Judgement call: "takes seconds on a full table" stays in the doc of `PrefixHandler`. It predates this PR and rests on the 1.47-second message measured in https://git.eeqj.de/sneak/routewatch/issues/216, not on the plan's inferred walk time.
Model: opus-5-5
Several sentences say a peer's messages are held back until the peer has left every prefix. The code holds them back for longer. After a PeersDown call returns, peerDown applies the held-back messages of the call's peers one peer after another. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer listed after a returning full-table peer stays held back while that peer's table is applied, long after it has left every prefix. Where:
internal/routewatch/prefixhandler.go, doc of HandleMessage: "is held back until the peer has left every prefix".
Same file, doc of PrefixHandler: "The handler holds back a down peer's later messages until the peer has left every prefix."
README.md, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." It was true before this change and is not after it.
README.md, "Memory": "It applies them once the peer has left every prefix." The same.
Acceptable: each of these sentences says when the hold ends as the code does it, in a short sentence, for example "until peerDown applies them". The doc of maxHeldBackMessages already says it in full.
Model: opus-5-5
**FAIL: needs-rework.**
1. Several sentences say a peer's messages are held back until the peer has left every prefix. The code holds them back for longer. After a `PeersDown` call returns, `peerDown` applies the held-back messages of the call's peers one peer after another. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer listed after a returning full-table peer stays held back while that peer's table is applied, long after it has left every prefix. Where:
- `internal/routewatch/prefixhandler.go`, doc of `HandleMessage`: "is held back until the peer has left every prefix".
- Same file, doc of `PrefixHandler`: "The handler holds back a down peer's later messages until the peer has left every prefix."
- `README.md`, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." It was true before this change and is not after it.
- `README.md`, "Memory": "It applies them once the peer has left every prefix." The same.
Acceptable: each of these sentences says when the hold ends as the code does it, in a short sentence, for example "until `peerDown` applies them". The doc of `maxHeldBackMessages` already says it in full.
Model: opus-5-5
Paused for sneak's review: this PR has failed three reviews, so under https://git.eeqj.de/sneak/project-management/issues/43 nothing more is done on it until he has reviewed it.
The three failed reviews:
- https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161139
- https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161173
- https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161205
Model: opus-5-5
clawbot
removed their assignment 2026-11-02 08:27:11 +01:00
sneak
was assigned by clawbot2026-11-02 08:27:11 +01:00
Taking this PR back from sneak: under the new rule (sneak/project-management#43 (comment)) the manager ends the review loop. The third review (#228 (comment)) found only that four sentences give the wrong moment the hold ends. The code, the tests, the limit and every other sentence passed.
The rework changes exactly these four sentences, to this text:
README.md, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." becomes "The handler holds back the peer's later updates. It applies them in order after the peer has left every prefix, once the updates held back before them are applied."
README.md, "Memory", Held-back messages: "It applies them once the peer has left every prefix." becomes "It applies them after the peer has left every prefix, once the messages held back before them are applied."
internal/routewatch/prefixhandler.go, doc of PrefixHandler: "The handler holds back a down peer's later messages until the peer has left every prefix. Then the handler applies them in order, one at a time, and takes new messages in between." becomes "The handler holds back a down peer's later messages until peerDown applies them, as the doc of maxHeldBackMessages describes. It applies them in order, one at a time, and takes new messages in between."
Same file, doc of HandleMessage: "A message from a peer whose session went down is held back until the peer has left every prefix." becomes "A message from a peer whose session went down is held back until peerDown applies it."
It leaves everything else alone: code, tests, maxHeldBackMessages and its doc, the heldBack field comment (it already says the messages wait until they are applied), TODO.md, the commit message and the PR body. make fmt may rewrap the two README paragraphs; nothing else moves. One commit, amended, rebased onto next, gate run, force-pushed.
The next review checks only: that the diff between 6f3fbf0 and the new head is these four sentences and make fmt's rewrapping, that each now matches the code, and that script/cibuild passes on the rebased branch. It passes if so.
Model: opus-5-5
Taking this PR back from sneak: under the new rule (https://git.eeqj.de/sneak/project-management/issues/43#issuecomment-162903) the manager ends the review loop. The third review (https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-161205) found only that four sentences give the wrong moment the hold ends. The code, the tests, the limit and every other sentence passed.
**The rework changes exactly these four sentences, to this text:**
1. `README.md`, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." becomes "The handler holds back the peer's later updates. It applies them in order after the peer has left every prefix, once the updates held back before them are applied."
2. `README.md`, "Memory", Held-back messages: "It applies them once the peer has left every prefix." becomes "It applies them after the peer has left every prefix, once the messages held back before them are applied."
3. `internal/routewatch/prefixhandler.go`, doc of `PrefixHandler`: "The handler holds back a down peer's later messages until the peer has left every prefix. Then the handler applies them in order, one at a time, and takes new messages in between." becomes "The handler holds back a down peer's later messages until peerDown applies them, as the doc of maxHeldBackMessages describes. It applies them in order, one at a time, and takes new messages in between."
4. Same file, doc of `HandleMessage`: "A message from a peer whose session went down is held back until the peer has left every prefix." becomes "A message from a peer whose session went down is held back until peerDown applies it."
**It leaves everything else alone:** code, tests, `maxHeldBackMessages` and its doc, the `heldBack` field comment (it already says the messages wait until they are applied), `TODO.md`, the commit message and the PR body. `make fmt` may rewrap the two README paragraphs; nothing else moves. One commit, amended, rebased onto `next`, gate run, force-pushed.
**The next review checks only:** that the diff between `6f3fbf0` and the new head is these four sentences and `make fmt`'s rewrapping, that each now matches the code, and that `script/cibuild` passes on the rebased branch. It passes if so.
Model: opus-5-5
sneak
was unassigned by clawbot2026-11-02 22:21:07 +01:00
clawbot
self-assigned this 2026-11-02 22:21:07 +01:00
README.md, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." The same holds for README.md, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." Neither matches peerDown. It does not apply held-back messages in the order they were held back. It applies them one peer after another, in the order the peers' sessions went down. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer's updates also wait for updates held back after them, such as a returning full-table peer's table when that peer went down first. An update held back early from a peer whose session went down later is applied after them. Acceptable: each sentence says the peer's updates wait for the held-back updates of the peers whose session went down before it. For example: "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."
Model: opus-5-5
**FAIL: needs-rework.**
1. `README.md`, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." The same holds for `README.md`, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." Neither matches `peerDown`. It does not apply held-back messages in the order they were held back. It applies them one peer after another, in the order the peers' sessions went down. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer's updates also wait for updates held back after them, such as a returning full-table peer's table when that peer went down first. An update held back early from a peer whose session went down later is applied after them. Acceptable: each sentence says the peer's updates wait for the held-back updates of the peers whose session went down before it. For example: "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."
Model: opus-5-5
Correction to #228 (comment): the text I gave for the two README sentences was wrong, as #228 (comment) shows. peerDown applies the held-back updates one peer at a time, in the order the peers' sessions went down.
The rework changes exactly these two sentences, to this text:
README.md, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." becomes "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."
README.md, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." becomes "It applies them after the peer has left every prefix and after the held-back messages of every peer whose session went down before it."
It leaves everything else alone, the two code comments from the last rework included. One commit, amended, rebased onto next, gate run, force-pushed.
The next review checks only: that the diff between ec15f4f and the new head is these two sentences and make fmt's rewrapping, that both match peerDown and applyHeldBack, and that script/cibuild passes on the rebased branch. It passes if so.
Model: opus-5-5
Correction to https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-162950: the text I gave for the two README sentences was wrong, as https://git.eeqj.de/sneak/routewatch/pulls/228#issuecomment-163111 shows. `peerDown` applies the held-back updates one peer at a time, in the order the peers' sessions went down.
**The rework changes exactly these two sentences, to this text:**
1. `README.md`, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." becomes "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."
2. `README.md`, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." becomes "It applies them after the peer has left every prefix and after the held-back messages of every peer whose session went down before it."
**It leaves everything else alone,** the two code comments from the last rework included. One commit, amended, rebased onto `next`, gate run, force-pushed.
**The next review checks only:** that the diff between `ec15f4f` and the new head is these two sentences and `make fmt`'s rewrapping, that both match `peerDown` and `applyHeldBack`, and that `script/cibuild` passes on the rebased branch. It passes if so.
Model: opus-5-5
The prefix handler ran one PeerDown per peer, one after another. When
the sessions of several peers went down together, their walks queued up.
A returning full-table peer's messages stayed held back while the walks
ahead of its own ran, and could reach the limit of 20,000.
The table's PeersDown now takes several peers out of every prefix in one
walk. One walk runs at a time. The next one takes every peer whose
session went down since the previous one began. The doc of
maxHeldBackMessages says how long a peer's messages are held back. The
limit is now 150,000 messages, about 150 MiB. The status page shows how
many messages each handler holds back.
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.
Implements #225: steps 2 to 6 of the plan in #225 (comment), with the direction in #225 (comment). Step 1 is #226.
Changes:
routetable:PeerDown(peer)is nowPeersDown(peers), one walk over the table for several peers.downPeers. OnepeerDowngoroutine runs at a time. Each round takes all ofdownPeersin onePeersDown, then applies each peer's held-back messages as before.peerDownMuis gone.maxHeldBackMessagessays how long a peer's messages are held back, as the code does it.maxHeldBackMessagesis 150,000, about 150 MiB at the README's estimate of 1 KiB a message./api/v1/statsgainsheld_back_countinhandler_stats, and the status page shows it as "Held Back". The streamer reads it from any handler with aHeldBackCount() intmethod.Worth knowing:
tablePeersDownis a field so that tests can hold up the walk. It replaces the tests' use ofpeerDownMu.HeldBackCounttakes the handler's mutex, so a status read can wait briefly while a message is applied.Disclosures:
held_back_count, not the plan'sHeldBack, to matchdropped_count.PeersDown.Model: opus-5-5
FAIL: needs-rework.
internal/routewatch/prefixhandler.go, doc ofmaxHeldBackMessages: "A message is held back for about two full-table PeersDown calls at most, about 5 seconds" is not true when the sessions of several full-table peers go down together. Most of a walk's time goes to taking the peers' routes out, not to visiting the prefixes. So a walk that takes out three full-table peers takes about three times as long as a walk that takes out one. The limit of 100,000 rests on those 5 seconds. So it is not shown to hold for the case in the definition of done: a full-table peer that comes straight back while several others go down. Acceptable: the doc says how long a message is held back as the code does it (two walks, each one longer the more routes it takes out), and the limit is worked out from that for the definition of done's case.internal/routetable/routetable_test.go, case "peers going down together each leave the prefix": it still passes with the lineorigins = prefixes[prefix]taken out ofPeersDown, though its comment ("peerA's leaving removes the origin asA, before peerB leaves") says it covers that line's case. Without that line,PeersDowncrashes on a nil origin as soon as one listed peer's leaving removes an origin of a prefix that a later listed peer does not carry. Acceptable: a case that fails without the line, for examplepeersDown(peerA, peerB)where onlypeerAcarries the prefix.Model: opus-5-5
9187cd47b6to9d37f98239Reworked against #228 (comment).
maxHeldBackMessagesnow gives the hold-back time as up to twoPeersDowncalls, each about 2.6 seconds for each full-table peer it takes out. For the returning peer with three other full-table peers, that is about 10.4 seconds and at most about 146,000 messages. So the limit is 150,000, about 150 MiB (README "Memory",TODO.md).TestPeersDownTakesOutPeersOfDifferentPrefixestakes outpeerAandpeerB, each carrying only its own prefix. It panics withoutorigins = prefixes[prefix]. The old case lost its comment.From the reread: the comment above that line and the doc of
applyHeldBacknow say exactly what happens. The README sentence after the new one says "the limit", not "that number". The PR body gives the new figures.Disclosures:
Model: opus-5-5
FAIL: needs-rework.
internal/routewatch/prefixhandler.go, doc ofmaxHeldBackMessages: "A peer's messages are held back while up to two PeersDown calls run". TheTODO.mdentry, the commit message and the PR body say the same ("held back for two walks at most"). The code holds them back for longer.peerDownstarts the nextPeersDownonly onceapplyHeldBackhas emptied the held-back list of every peer of the previous call, including messages that arrive meanwhile. A peer whose session went down during that call stays held back through all of it. It also waits for the lists of the peers listed before it in its own call. The 10.4 seconds and the 146,000 messages leave this time out. Acceptable: the doc gives the time a message is held back as the code does it: the rest of the running call, the applying of that call's held-back messages, and the next call. The limit is worked out with that time.Same doc: "A call takes about 2.6 seconds for each full-table peer it takes out". The same figure is in the
TODO.mdentry, the commit message and the PR body. A call does not take one more full-table walk for each further full-table peer. Peers that leave the same prefix share most of the work. The prefix is visited once, its changed prefix and origin AS pair is recorded once, and an origin keeps at most two routes. So a call that takes out four full-table peers takes far less than four times as long as a call that takes out one. Acceptable: no figure multiplies one peer's walk by the number of peers. The time of a call for several full-table peers is stated as the code takes it, or left out.README "Memory": "The limit is enough when the sessions of four full-table peers go down together." The
TODO.mdentry, the commit message and the PR body make the same claim. It rests on the figures of findings 1 and 2, so nothing shows it is true. Acceptable: the sentence states only what the corrected figures support for the case in the definition of done, a full-table peer that comes straight back while several others go down. Otherwise the sentence goes.Model: opus-5-5
9d37f98239to6f3fbf027aReworked against #228 (comment), following #225 (comment). Rebased over #227, with this PR's
TODO.mdentry on top. The fixes for #228 (comment) stay.maxHeldBackMessagessays in words how long a peer's messages are held back, as the code does it. It gives no time, and the limit is not worked out from one.TODO.md, the commit and the PR body. The limit stays 150,000, about 150 MiB.From the reread: the doc of
PrefixHandlersays the next call takes every peer whose session went down since the previous one began. The comment ofTestPrefixHandlerTakesPeersDownTogethernow names the prefix every peer must have left.Disclosures:
PrefixHandler. It predates this PR and rests on the 1.47-second message measured in #216, not on the plan's inferred walk time.Model: opus-5-5
FAIL: needs-rework.
Several sentences say a peer's messages are held back until the peer has left every prefix. The code holds them back for longer. After a
PeersDowncall returns,peerDownapplies the held-back messages of the call's peers one peer after another. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer listed after a returning full-table peer stays held back while that peer's table is applied, long after it has left every prefix. Where:internal/routewatch/prefixhandler.go, doc ofHandleMessage: "is held back until the peer has left every prefix".PrefixHandler: "The handler holds back a down peer's later messages until the peer has left every prefix."README.md, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." It was true before this change and is not after it.README.md, "Memory": "It applies them once the peer has left every prefix." The same.Acceptable: each of these sentences says when the hold ends as the code does it, in a short sentence, for example "until
peerDownapplies them". The doc ofmaxHeldBackMessagesalready says it in full.Model: opus-5-5
Paused for sneak's review: this PR has failed three reviews, so under sneak/project-management#43 nothing more is done on it until he has reviewed it.
The three failed reviews:
Model: opus-5-5
Taking this PR back from sneak: under the new rule (sneak/project-management#43 (comment)) the manager ends the review loop. The third review (#228 (comment)) found only that four sentences give the wrong moment the hold ends. The code, the tests, the limit and every other sentence passed.
The rework changes exactly these four sentences, to this text:
README.md, PrefixHandler bullet: "The handler holds back the peer's later updates until the peer has left every prefix, then applies them in order." becomes "The handler holds back the peer's later updates. It applies them in order after the peer has left every prefix, once the updates held back before them are applied."README.md, "Memory", Held-back messages: "It applies them once the peer has left every prefix." becomes "It applies them after the peer has left every prefix, once the messages held back before them are applied."internal/routewatch/prefixhandler.go, doc ofPrefixHandler: "The handler holds back a down peer's later messages until the peer has left every prefix. Then the handler applies them in order, one at a time, and takes new messages in between." becomes "The handler holds back a down peer's later messages until peerDown applies them, as the doc of maxHeldBackMessages describes. It applies them in order, one at a time, and takes new messages in between."HandleMessage: "A message from a peer whose session went down is held back until the peer has left every prefix." becomes "A message from a peer whose session went down is held back until peerDown applies it."It leaves everything else alone: code, tests,
maxHeldBackMessagesand its doc, theheldBackfield comment (it already says the messages wait until they are applied),TODO.md, the commit message and the PR body.make fmtmay rewrap the two README paragraphs; nothing else moves. One commit, amended, rebased ontonext, gate run, force-pushed.The next review checks only: that the diff between
6f3fbf0and the new head is these four sentences andmake fmt's rewrapping, that each now matches the code, and thatscript/cibuildpasses on the rebased branch. It passes if so.Model: opus-5-5
6f3fbf027atoec15f4fd53Applied #228 (comment).
Model: opus-5-5
FAIL: needs-rework.
README.md, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." The same holds forREADME.md, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." Neither matchespeerDown. It does not apply held-back messages in the order they were held back. It applies them one peer after another, in the order the peers' sessions went down. It empties each peer's list, messages that arrive meanwhile included, before it starts on the next peer's. So a peer's updates also wait for updates held back after them, such as a returning full-table peer's table when that peer went down first. An update held back early from a peer whose session went down later is applied after them. Acceptable: each sentence says the peer's updates wait for the held-back updates of the peers whose session went down before it. For example: "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."Model: opus-5-5
Correction to #228 (comment): the text I gave for the two README sentences was wrong, as #228 (comment) shows.
peerDownapplies the held-back updates one peer at a time, in the order the peers' sessions went down.The rework changes exactly these two sentences, to this text:
README.md, PrefixHandler bullet: "It applies them in order after the peer has left every prefix, once the updates held back before them are applied." becomes "It applies them in order after the peer has left every prefix and after the held-back updates of every peer whose session went down before it."README.md, "Memory", Held-back messages: "It applies them after the peer has left every prefix, once the messages held back before them are applied." becomes "It applies them after the peer has left every prefix and after the held-back messages of every peer whose session went down before it."It leaves everything else alone, the two code comments from the last rework included. One commit, amended, rebased onto
next, gate run, force-pushed.The next review checks only: that the diff between
ec15f4fand the new head is these two sentences andmake fmt's rewrapping, that both matchpeerDownandapplyHeldBack, and thatscript/cibuildpasses on the rebased branch. It passes if so.Model: opus-5-5
ec15f4fd53to10112e31a6Applied #228 (comment).
Model: opus-5-5
PASS
Model: opus-5-5