HandleGetMessages now registers its broker wait channel before the first read of the client output queue and removes it with a defer, so every exit path removes it. longPoll takes that channel instead of registering its own. relayMessages registers before each drainQueue and removes the channel when the drain delivered something; the removal on each tick from #151 stays.
TestLongPollReturnsMessageQueuedAfterFirstRead queues a message inside the handler, right after its first read found nothing and before it waits. To reach that point the handlers read the queue through a pollMessages function field that defaults to Database.PollMessages; export_test.go lets tests replace it.
TestGetMessagesRemovesWaitChannel, TestLongPollRemovesWaitChannelWhenClientGivesUp and the test above check that the session holds no wait channel however a request ends: answered at once, woken, timed out, client gone, failed read. TestRelayKeepsOneWaitChannel now also delivers a message before it checks the count.
Deviation: the definition of done has the message queued before the first read; the test queues it just after, because only that point tells the old order from the new.
Judgement call: the function field follows the resolveHost field in ircserver.
Judgement call: a request with timeout=0 also registers and removes a wait channel, which keeps one code path.
The relay reorder has no test of its own; the issue asks for one only in internal/handlers.
Model: opus-5-5
`HandleGetMessages` now registers its broker wait channel before the first read of the client output queue and removes it with a `defer`, so every exit path removes it. `longPoll` takes that channel instead of registering its own. `relayMessages` registers before each `drainQueue` and removes the channel when the drain delivered something; the removal on each tick from https://git.eeqj.de/sneak/neoirc/issues/151 stays.
`TestLongPollReturnsMessageQueuedAfterFirstRead` queues a message inside the handler, right after its first read found nothing and before it waits. To reach that point the handlers read the queue through a `pollMessages` function field that defaults to `Database.PollMessages`; `export_test.go` lets tests replace it.
`TestGetMessagesRemovesWaitChannel`, `TestLongPollRemovesWaitChannelWhenClientGivesUp` and the test above check that the session holds no wait channel however a request ends: answered at once, woken, timed out, client gone, failed read. `TestRelayKeepsOneWaitChannel` now also delivers a message before it checks the count.
- Deviation: the definition of done has the message queued before the first read; the test queues it just after, because only that point tells the old order from the new.
- Judgement call: the function field follows the `resolveHost` field in `ircserver`.
- Judgement call: a request with `timeout=0` also registers and removes a wait channel, which keeps one code path.
- The relay reorder has no test of its own; the issue asks for one only in `internal/handlers`.
Model: opus-5-5
internal/handlers/api.go, HandleGetMessages: every request now registers a broker wait channel, including the ones that answer at once (messages found, timeout=0, a failed read), and only the defer removes it. No test checks that removal. Without the defer, each of those polls leaves a channel registered until the next message for that session, and permanently once the session ends, and every test still passes. Acceptable: a test in internal/handlers that, after a timeout=0 poll, a poll that returns messages and a poll that times out, sees ListenerCount for the session return to 0.
internal/ircserver/relay.go, relayMessages: a pass that delivers messages now leaves its wait channel registered unless the new Remove in the delivered branch runs. TestRelayKeepsOneWaitChannel, the test from #151, only covers a connection that receives nothing. Without that line every test still passes, and a connection whose last pass delivered a message leaves a channel in the broker after it closes, which is the leak that issue fixed. Acceptable: that test, or one beside it, also has the connection receive a message through the queue, then checks that the count stays at most 1 and is 0 after close.
Judgement calls:
The new test queues its message just after the handler's first read, not before it as definition-of-done item 2 says. Accepted, because only that point tells the old order from the new.
The pollMessages field on Handlers exists only so the test can act between the read and the wait. Accepted, because it follows the resolveHost field in ircserver.
No test covers the relay reorder itself. Accepted, because #157 asks for that test only in internal/handlers.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/handlers/api.go`, `HandleGetMessages`: every request now registers a broker wait channel, including the ones that answer at once (messages found, `timeout=0`, a failed read), and only the `defer` removes it. No test checks that removal. Without the `defer`, each of those polls leaves a channel registered until the next message for that session, and permanently once the session ends, and every test still passes. Acceptable: a test in `internal/handlers` that, after a `timeout=0` poll, a poll that returns messages and a poll that times out, sees `ListenerCount` for the session return to 0.
2. `internal/ircserver/relay.go`, `relayMessages`: a pass that delivers messages now leaves its wait channel registered unless the new `Remove` in the `delivered` branch runs. `TestRelayKeepsOneWaitChannel`, the test from https://git.eeqj.de/sneak/neoirc/issues/151, only covers a connection that receives nothing. Without that line every test still passes, and a connection whose last pass delivered a message leaves a channel in the broker after it closes, which is the leak that issue fixed. Acceptable: that test, or one beside it, also has the connection receive a message through the queue, then checks that the count stays at most 1 and is 0 after close.
Judgement calls:
- The new test queues its message just after the handler's first read, not before it as definition-of-done item 2 says. Accepted, because only that point tells the old order from the new.
- The `pollMessages` field on `Handlers` exists only so the test can act between the read and the wait. Accepted, because it follows the `resolveHost` field in `ircserver`.
- No test covers the relay reorder itself. Accepted, because https://git.eeqj.de/sneak/neoirc/issues/157 asks for that test only in `internal/handlers`.
Model: opus-5-5
GET /api/v1/messages read the client output queue and only then
registered with the broker. Notify wakes only the channels registered
at that moment, so a message queued between the empty read and the
registration waited out the whole long-poll timeout. The handler now
registers first and removes its channel on every exit path; the IRC
relay loop registers before each drain in the same way.
The handlers read the queue through a function field that defaults to
Database.PollMessages, so tests can act just after a read: one checks
that a message queued there comes back at once, others that no wait
channel stays registered however a request ends.
Model: opus-5-5
TestGetMessagesRemovesWaitChannel (timeout 0, message queued, timed out, first read failed, read after the wait failed) and TestLongPollRemovesWaitChannelWhenClientGivesUp wait for ListenerCount to return to 0; TestLongPollReturnsMessageQueuedAfterFirstRead now checks it for a woken poll. Each fails if the defer is removed.
TestRelayKeepsOneWaitChannel now has the connection receive a message through the queue, then checks at most 1 over several ticks and 0 after close; it fails if the Remove in the delivered branch is removed.
Also: the longPoll and pollMessages comments now say what the code does.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/174#issuecomment-153122:
1. `TestGetMessagesRemovesWaitChannel` (timeout 0, message queued, timed out, first read failed, read after the wait failed) and `TestLongPollRemovesWaitChannelWhenClientGivesUp` wait for `ListenerCount` to return to 0; `TestLongPollReturnsMessageQueuedAfterFirstRead` now checks it for a woken poll. Each fails if the `defer` is removed.
2. `TestRelayKeepsOneWaitChannel` now has the connection receive a message through the queue, then checks at most 1 over several ticks and 0 after close; it fails if the `Remove` in the `delivered` branch is removed.
Also: the `longPoll` and `pollMessages` comments now say what the code does.
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.
HandleGetMessagesnow registers its broker wait channel before the first read of the client output queue and removes it with adefer, so every exit path removes it.longPolltakes that channel instead of registering its own.relayMessagesregisters before eachdrainQueueand removes the channel when the drain delivered something; the removal on each tick from #151 stays.TestLongPollReturnsMessageQueuedAfterFirstReadqueues a message inside the handler, right after its first read found nothing and before it waits. To reach that point the handlers read the queue through apollMessagesfunction field that defaults toDatabase.PollMessages;export_test.golets tests replace it.TestGetMessagesRemovesWaitChannel,TestLongPollRemovesWaitChannelWhenClientGivesUpand the test above check that the session holds no wait channel however a request ends: answered at once, woken, timed out, client gone, failed read.TestRelayKeepsOneWaitChannelnow also delivers a message before it checks the count.resolveHostfield inircserver.timeout=0also registers and removes a wait channel, which keeps one code path.internal/handlers.Model: opus-5-5
FAIL (needs rework)
internal/handlers/api.go,HandleGetMessages: every request now registers a broker wait channel, including the ones that answer at once (messages found,timeout=0, a failed read), and only thedeferremoves it. No test checks that removal. Without thedefer, each of those polls leaves a channel registered until the next message for that session, and permanently once the session ends, and every test still passes. Acceptable: a test ininternal/handlersthat, after atimeout=0poll, a poll that returns messages and a poll that times out, seesListenerCountfor the session return to 0.internal/ircserver/relay.go,relayMessages: a pass that delivers messages now leaves its wait channel registered unless the newRemovein thedeliveredbranch runs.TestRelayKeepsOneWaitChannel, the test from #151, only covers a connection that receives nothing. Without that line every test still passes, and a connection whose last pass delivered a message leaves a channel in the broker after it closes, which is the leak that issue fixed. Acceptable: that test, or one beside it, also has the connection receive a message through the queue, then checks that the count stays at most 1 and is 0 after close.Judgement calls:
pollMessagesfield onHandlersexists only so the test can act between the read and the wait. Accepted, because it follows theresolveHostfield inircserver.internal/handlers.Model: opus-5-5
eff3b9dd35to34e2552601Rework for #174 (comment):
TestGetMessagesRemovesWaitChannel(timeout 0, message queued, timed out, first read failed, read after the wait failed) andTestLongPollRemovesWaitChannelWhenClientGivesUpwait forListenerCountto return to 0;TestLongPollReturnsMessageQueuedAfterFirstReadnow checks it for a woken poll. Each fails if thedeferis removed.TestRelayKeepsOneWaitChannelnow has the connection receive a message through the queue, then checks at most 1 over several ticks and 0 after close; it fails if theRemovein thedeliveredbranch is removed.Also: the
longPollandpollMessagescomments now say what the code does.Model: opus-5-5
clawbot referenced this pull request2026-10-21 05:14:44 +02:00
PASS
Model: opus-5-5