JoinChannel in internal/service/service.go runs GetOrCreateChannel, CountChannelMembers, checkJoinRestrictions and then JoinChannel or JoinChannelAsOperator (internal/db/queries.go) as separate statements. The database handle has one connection, but it is released between statements, so two requests interleave statement by statement and each decides on a count the other is about to change. Seen on eceb5f2, ten trials each:
A channel with +l 2 and one member, two concurrent JOINs: both answered joined, three members, every trial. README.md lists +l as enforced.
A channel that does not exist yet, two concurrent JOINs: both read a count of 0, both take the creator path, both become operator, and the second skips every join check, every trial. README.md: "The first user to join a channel becomes its operator".
A JOIN concurrent with the last member's PART: GetOrCreateChannel returns the row, the PART deletes it, and the member INSERT fails its foreign key; JoinChannel returns an error that is not an IRCError, so the HTTP API answers 500internal error (handleServiceError), every trial; the IRC listener only logs it (joinChannel in internal/ircserver/commands.go) and the client hears nothing. The joiner is in no channel and the channel is gone. README.md reserves 500 for server failures.
Fix: run the join as one unit: a transaction (BEGIN IMMEDIATE, which also serialises it against a PART's delete) that creates the channel if missing, counts the members, applies the checks, inserts the member and commits. With one connection in the pool other statements wait on it, which is what is wanted. The 500 then goes away, since the delete cannot run between the lookup and the insert. PartChannel and KickUser (PartChannel then DeleteChannelIfEmpty) are two statements too; the same kind of transaction there keeps a join from landing between them.
Definition of done
With +l N, concurrent JOINs never take a channel past N members; of concurrent first JOINs exactly one becomes operator; a JOIN concurrent with the last PART either joins and keeps the channel, or gets 403, never 500.
Tests under -race for the three cases, in internal/service or internal/handlers, with enough concurrent attempts that today's code fails them (ten pairs reproduce every time).
One reviewed PR against next.
Model: fable-5-1
`JoinChannel` in `internal/service/service.go` runs `GetOrCreateChannel`, `CountChannelMembers`, `checkJoinRestrictions` and then `JoinChannel` or `JoinChannelAsOperator` (`internal/db/queries.go`) as separate statements. The database handle has one connection, but it is released between statements, so two requests interleave statement by statement and each decides on a count the other is about to change. Seen on `eceb5f2`, ten trials each:
- A channel with `+l 2` and one member, two concurrent JOINs: both answered `joined`, three members, every trial. `README.md` lists `+l` as enforced.
- A channel that does not exist yet, two concurrent JOINs: both read a count of 0, both take the creator path, both become operator, and the second skips every join check, every trial. `README.md`: "The first user to join a channel becomes its operator".
- A JOIN concurrent with the last member's PART: `GetOrCreateChannel` returns the row, the PART deletes it, and the member INSERT fails its foreign key; `JoinChannel` returns an error that is not an `IRCError`, so the HTTP API answers `500` `internal error` (`handleServiceError`), every trial; the IRC listener only logs it (`joinChannel` in `internal/ircserver/commands.go`) and the client hears nothing. The joiner is in no channel and the channel is gone. `README.md` reserves 500 for server failures.
Fix: run the join as one unit: a transaction (`BEGIN IMMEDIATE`, which also serialises it against a PART's delete) that creates the channel if missing, counts the members, applies the checks, inserts the member and commits. With one connection in the pool other statements wait on it, which is what is wanted. The 500 then goes away, since the delete cannot run between the lookup and the insert. `PartChannel` and `KickUser` (`PartChannel` then `DeleteChannelIfEmpty`) are two statements too; the same kind of transaction there keeps a join from landing between them.
## Definition of done
1. With `+l N`, concurrent JOINs never take a channel past N members; of concurrent first JOINs exactly one becomes operator; a JOIN concurrent with the last PART either joins and keeps the channel, or gets 403, never 500.
2. Tests under `-race` for the three cases, in `internal/service` or `internal/handlers`, with enough concurrent attempts that today's code fails them (ten pairs reproduce every time).
3. One reviewed PR against `next`.
Model: fable-5-1
clawbot
self-assigned this 2026-10-20 17:14:50 +02: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.
JoinChannelininternal/service/service.gorunsGetOrCreateChannel,CountChannelMembers,checkJoinRestrictionsand thenJoinChannelorJoinChannelAsOperator(internal/db/queries.go) as separate statements. The database handle has one connection, but it is released between statements, so two requests interleave statement by statement and each decides on a count the other is about to change. Seen oneceb5f2, ten trials each:+l 2and one member, two concurrent JOINs: both answeredjoined, three members, every trial.README.mdlists+las enforced.README.md: "The first user to join a channel becomes its operator".GetOrCreateChannelreturns the row, the PART deletes it, and the member INSERT fails its foreign key;JoinChannelreturns an error that is not anIRCError, so the HTTP API answers500internal error(handleServiceError), every trial; the IRC listener only logs it (joinChannelininternal/ircserver/commands.go) and the client hears nothing. The joiner is in no channel and the channel is gone.README.mdreserves 500 for server failures.Fix: run the join as one unit: a transaction (
BEGIN IMMEDIATE, which also serialises it against a PART's delete) that creates the channel if missing, counts the members, applies the checks, inserts the member and commits. With one connection in the pool other statements wait on it, which is what is wanted. The 500 then goes away, since the delete cannot run between the lookup and the insert.PartChannelandKickUser(PartChannelthenDeleteChannelIfEmpty) are two statements too; the same kind of transaction there keeps a join from landing between them.Definition of done
+l N, concurrent JOINs never take a channel past N members; of concurrent first JOINs exactly one becomes operator; a JOIN concurrent with the last PART either joins and keeps the channel, or gets 403, never 500.-racefor the three cases, ininternal/serviceorinternal/handlers, with enough concurrent attempts that today's code fails them (ten pairs reproduce every time).next.Model: fable-5-1