A second /connect in neoirc-cli left the first poll loop running. Both loops then polled with the new client, so a message could be shown twice. cmdConnect also wrote stopPoll without a.mu while the first loop read it.
What changed:
cmdConnect closes the earlier loop's channel in the locked step that replaces the client and the cursor, and gives the new loop a new one.
pollLoop checks its own channel under a.mu, before each poll and again before it moves the cursor.
TestSecondConnectStopsFirstPollLoop fails if the earlier loop polls again after a second /connect.
TestSecondConnectIgnoresHeldPoll holds the earlier loop's poll across a second /connect, then answers it with a cursor and a message. It fails if that answer moves the cursor or is shown.
TestSecondConnectAfterFailedPoll holds that poll the same way, then fails it. It fails if the earlier loop polls again.
What a reader may trip over:
A stopped loop is not interrupted mid-poll. It ends when that poll returns, up to 15 seconds later, and shows nothing from it.
Disclosures:
Not covered by a test: that both checks of the channel, and the write of stopPoll, happen under a.mu. A test could only fail on them with a hook added to the code.
Judgement call: the tests close stopPoll themselves. /quit would also stop the UI, which runUI's cleanup then stops again: a data race inside tview.
Judgement call: the two held-poll tests start the earlier loop themselves, so they can wait for it to end.
Model: opus-5-5
A second `/connect` in `neoirc-cli` left the first poll loop running. Both loops then polled with the new client, so a message could be shown twice. `cmdConnect` also wrote `stopPoll` without `a.mu` while the first loop read it.
What changed:
- `cmdConnect` closes the earlier loop's channel in the locked step that replaces the client and the cursor, and gives the new loop a new one.
- `pollLoop` checks its own channel under `a.mu`, before each poll and again before it moves the cursor.
- `TestSecondConnectStopsFirstPollLoop` fails if the earlier loop polls again after a second `/connect`.
- `TestSecondConnectIgnoresHeldPoll` holds the earlier loop's poll across a second `/connect`, then answers it with a cursor and a message. It fails if that answer moves the cursor or is shown.
- `TestSecondConnectAfterFailedPoll` holds that poll the same way, then fails it. It fails if the earlier loop polls again.
What a reader may trip over:
- A stopped loop is not interrupted mid-poll. It ends when that poll returns, up to 15 seconds later, and shows nothing from it.
Disclosures:
- Not covered by a test: that both checks of the channel, and the write of `stopPoll`, happen under `a.mu`. A test could only fail on them with a hook added to the code.
- Judgement call: the tests close `stopPoll` themselves. `/quit` would also stop the UI, which `runUI`'s cleanup then stops again: a data race inside tview.
- Judgement call: the two held-poll tests start the earlier loop themselves, so they can wait for it to end.
Model: opus-5-5
internal/cli/app.go, pollLoop: no test covers the second check of stop, the one after PollMessages returns. Remove that check and every test still passes. The PR body itself says the check is needed: without it, a poll already waiting at the earlier server when /connect runs comes back and moves the new connection's cursor, and the new loop can then show messages twice or skip them. Acceptable: a test in internal/cli/app_internal_test.go where the earlier connection's poll is still held at its server when the second /connect runs, and is then answered with a last_id and a message. The test fails if that answer moves the cursor or the message is shown.
Judgement call: an earlier loop that took a batch just before /connect can still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.
Model: opus-5-5
FAIL:
- `internal/cli/app.go`, `pollLoop`: no test covers the second check of `stop`, the one after `PollMessages` returns. Remove that check and every test still passes. The PR body itself says the check is needed: without it, a poll already waiting at the earlier server when `/connect` runs comes back and moves the new connection's cursor, and the new loop can then show messages twice or skip them. Acceptable: a test in `internal/cli/app_internal_test.go` where the earlier connection's poll is still held at its server when the second `/connect` runs, and is then answered with a `last_id` and a message. The test fails if that answer moves the cursor or the message is shown.
Judgement call: an earlier loop that took a batch just before `/connect` can still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.
Model: opus-5-5
Finding: added TestSecondConnectIgnoresHeldPoll in internal/cli/app_internal_test.go. It fails when the second stop check in pollLoop is removed: the late answer moves the cursor and its message is shown.
Judgement call: the test starts the earlier connection's poll loop itself, so it waits on a channel for that loop to end instead of sleeping.
Model: opus-5-5
Rework:
- Finding: added `TestSecondConnectIgnoresHeldPoll` in `internal/cli/app_internal_test.go`. It fails when the second `stop` check in `pollLoop` is removed: the late answer moves the cursor and its message is shown.
- Judgement call: the test starts the earlier connection's poll loop itself, so it waits on a channel for that loop to end instead of sleeping.
Model: opus-5-5
internal/cli/app.go, pollLoop: no test covers the check of stop before each poll. Remove that check and every test still passes, yet the earlier loop then polls the new server: if the earlier connection's poll fails and the second /connect runs while that loop waits to retry, the loop wakes and polls with the new client. Acceptable: a test in internal/cli/app_internal_test.go that fails when that check is removed, for example one where the first server fails every poll and the test fails if the second server then gets a poll from the earlier loop.
Judgement call: an earlier loop that took a batch just before /connect can still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.
Judgement call: the other commands still read a.client without a.mu. They run one at a time, in turn with /connect, its only writer, so this is not a race, and the issue leaves it alone.
Judgement call: TestSecondConnectIgnoresHeldPoll sets up the earlier connection and starts its poll loop itself. It sets what /connect sets and the second /connect is real, so it still tests that case.
Model: opus-5-5
FAIL:
- `internal/cli/app.go`, `pollLoop`: no test covers the check of `stop` before each poll. Remove that check and every test still passes, yet the earlier loop then polls the new server: if the earlier connection's poll fails and the second `/connect` runs while that loop waits to retry, the loop wakes and polls with the new client. Acceptable: a test in `internal/cli/app_internal_test.go` that fails when that check is removed, for example one where the first server fails every poll and the test fails if the second server then gets a poll from the earlier loop.
The finding of https://git.eeqj.de/sneak/neoirc/pulls/236#issuecomment-160698 is fixed.
Judgement call: an earlier loop that took a batch just before `/connect` can still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.
Judgement call: the other commands still read `a.client` without `a.mu`. They run one at a time, in turn with `/connect`, its only writer, so this is not a race, and the issue leaves it alone.
Judgement call: `TestSecondConnectIgnoresHeldPoll` sets up the earlier connection and starts its poll loop itself. It sets what `/connect` sets and the second `/connect` is real, so it still tests that case.
Model: opus-5-5
A second /connect started a new poll loop without stopping the first.
The first loop then polled with the new client too, so each message
could be shown twice. It also read stopPoll without a.mu while
cmdConnect wrote it.
cmdConnect now closes the earlier loop's channel in the same locked
step that replaces the client and the cursor, and hands the new
channel to the new loop. The loop checks its channel under a.mu
before each poll and again before it moves the cursor, so a stopped
loop never polls with the new client or moves the new cursor.
Model: opus-5-5
Finding: added TestSecondConnectAfterFailedPoll in internal/cli/app_internal_test.go. It holds the earlier loop's poll across a second /connect, then fails it, and fails if the earlier loop polls again; removing the check of stop before each poll makes it fail.
The server that holds every poll is now the newHoldingServer helper, which TestSecondConnectStopsFirstPollLoop uses too.
Disclosure, also in the PR body: no test covers that both checks of stop, and the write of stopPoll, happen under a.mu. A test could only fail on them with a hook added to the code.
Model: opus-5-5
Rework:
- Finding: added `TestSecondConnectAfterFailedPoll` in `internal/cli/app_internal_test.go`. It holds the earlier loop's poll across a second `/connect`, then fails it, and fails if the earlier loop polls again; removing the check of `stop` before each poll makes it fail.
- The server that holds every poll is now the `newHoldingServer` helper, which `TestSecondConnectStopsFirstPollLoop` uses too.
- Disclosure, also in the PR body: no test covers that both checks of `stop`, and the write of `stopPoll`, happen under `a.mu`. A test could only fail on them with a hook added to the code.
Model: opus-5-5
PASS: a second /connect now stops the earlier poll loop. Only the new connection's loop polls, moves the cursor and shows messages, and stopPoll is read and written only under a.mu. The findings of #236 (comment) and #236 (comment) are fixed.
Judgement call: the stopped loop's last poll to the earlier server stays open until that server answers, up to 15 seconds, and nothing from it is used. I read that as meeting "exactly one poll loop runs", since interrupting it would need a change to the API client that the issue does not ask for.
Judgement call: an earlier loop that took a batch just before /connect can still be showing it while the new loop starts. Each message is still shown once.
Judgement call: if the checks of stop or the write of stopPoll move outside a.mu, no test fails and the race detector reports nothing. The issue asks for no test of that, and the code meets it.
Model: opus-5-5
PASS: a second `/connect` now stops the earlier poll loop. Only the new connection's loop polls, moves the cursor and shows messages, and `stopPoll` is read and written only under `a.mu`. The findings of https://git.eeqj.de/sneak/neoirc/pulls/236#issuecomment-160698 and https://git.eeqj.de/sneak/neoirc/pulls/236#issuecomment-160810 are fixed.
Judgement call: the stopped loop's last poll to the earlier server stays open until that server answers, up to 15 seconds, and nothing from it is used. I read that as meeting "exactly one poll loop runs", since interrupting it would need a change to the API client that the issue does not ask for.
Judgement call: an earlier loop that took a batch just before `/connect` can still be showing it while the new loop starts. Each message is still shown once.
Judgement call: if the checks of `stop` or the write of `stopPoll` move outside `a.mu`, no test fails and the race detector reports nothing. The issue asks for no test of that, and the code meets it.
Model: opus-5-5
clawbot
merged commit 7968be00e7 into next2026-11-02 05:38:08 +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.
A second
/connectinneoirc-clileft the first poll loop running. Both loops then polled with the new client, so a message could be shown twice.cmdConnectalso wrotestopPollwithouta.muwhile the first loop read it.What changed:
cmdConnectcloses the earlier loop's channel in the locked step that replaces the client and the cursor, and gives the new loop a new one.pollLoopchecks its own channel undera.mu, before each poll and again before it moves the cursor.TestSecondConnectStopsFirstPollLoopfails if the earlier loop polls again after a second/connect.TestSecondConnectIgnoresHeldPollholds the earlier loop's poll across a second/connect, then answers it with a cursor and a message. It fails if that answer moves the cursor or is shown.TestSecondConnectAfterFailedPollholds that poll the same way, then fails it. It fails if the earlier loop polls again.What a reader may trip over:
Disclosures:
stopPoll, happen undera.mu. A test could only fail on them with a hook added to the code.stopPollthemselves./quitwould also stop the UI, whichrunUI's cleanup then stops again: a data race inside tview.Model: opus-5-5
FAIL:
internal/cli/app.go,pollLoop: no test covers the second check ofstop, the one afterPollMessagesreturns. Remove that check and every test still passes. The PR body itself says the check is needed: without it, a poll already waiting at the earlier server when/connectruns comes back and moves the new connection's cursor, and the new loop can then show messages twice or skip them. Acceptable: a test ininternal/cli/app_internal_test.gowhere the earlier connection's poll is still held at its server when the second/connectruns, and is then answered with alast_idand a message. The test fails if that answer moves the cursor or the message is shown.Judgement call: an earlier loop that took a batch just before
/connectcan still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.Model: opus-5-5
2bb7a78340toc8c7ac665dRework:
TestSecondConnectIgnoresHeldPollininternal/cli/app_internal_test.go. It fails when the secondstopcheck inpollLoopis removed: the late answer moves the cursor and its message is shown.Model: opus-5-5
FAIL:
internal/cli/app.go,pollLoop: no test covers the check ofstopbefore each poll. Remove that check and every test still passes, yet the earlier loop then polls the new server: if the earlier connection's poll fails and the second/connectruns while that loop waits to retry, the loop wakes and polls with the new client. Acceptable: a test ininternal/cli/app_internal_test.gothat fails when that check is removed, for example one where the first server fails every poll and the test fails if the second server then gets a poll from the earlier loop.The finding of #236 (comment) is fixed.
Judgement call: an earlier loop that took a batch just before
/connectcan still be showing it while the new loop starts. Each message is still shown once and the cursor does not move, so this is not a finding.Judgement call: the other commands still read
a.clientwithouta.mu. They run one at a time, in turn with/connect, its only writer, so this is not a race, and the issue leaves it alone.Judgement call:
TestSecondConnectIgnoresHeldPollsets up the earlier connection and starts its poll loop itself. It sets what/connectsets and the second/connectis real, so it still tests that case.Model: opus-5-5
c8c7ac665dtoce3a989321clawbot referenced this pull request2026-11-02 04:56:43 +01:00
ce3a989321tofaf2c861c1Rework:
TestSecondConnectAfterFailedPollininternal/cli/app_internal_test.go. It holds the earlier loop's poll across a second/connect, then fails it, and fails if the earlier loop polls again; removing the check ofstopbefore each poll makes it fail.newHoldingServerhelper, whichTestSecondConnectStopsFirstPollLoopuses too.stop, and the write ofstopPoll, happen undera.mu. A test could only fail on them with a hook added to the code.Model: opus-5-5
PASS: a second
/connectnow stops the earlier poll loop. Only the new connection's loop polls, moves the cursor and shows messages, andstopPollis read and written only undera.mu. The findings of #236 (comment) and #236 (comment) are fixed.Judgement call: the stopped loop's last poll to the earlier server stays open until that server answers, up to 15 seconds, and nothing from it is used. I read that as meeting "exactly one poll loop runs", since interrupting it would need a change to the API client that the issue does not ask for.
Judgement call: an earlier loop that took a batch just before
/connectcan still be showing it while the new loop starts. Each message is still shown once.Judgement call: if the checks of
stopor the write ofstopPollmove outsidea.mu, no test fails and the race detector reports nothing. The issue asks for no test of that, and the code meets it.Model: opus-5-5