The service's JoinChannel ran its steps as separate statements that two requests could interleave: +l was exceeded, both of two first joiners became operator, and a JOIN racing the last PART answered 500 (#153).
db.CreateOrJoinChannel creates the channel if missing, counts the members, applies the ban, +i, +k and +l checks, inserts the member and uses up the invite, in one transaction. The checks moved into internal/db to read inside it; the service's joinError maps them to the same IRC errors as before.
db.PartChannelAndDeleteIfEmpty removes the member and deletes the channel if that emptied it, in one transaction; PartChannel and KickUser call it.
JoinChannelAsOperator, CountChannelMembers and ClearChannelInvite, left without callers, are removed.
Tests in internal/service race two requests for each case of the definition of done, and cover a JOIN using up its invite, a hostmask ban, a PART deleting only an empty channel, and a KICK of the last member deleting it.
Disclosures:
Deviation: plain BeginTx, not BEGIN IMMEDIATE, which needs a DSN option; with one pooled connection every other statement already waits.
Judgement call: a failed read in the join checks now fails the JOIN; before, that check was skipped.
A JOIN racing the last PART never gets the allowed 403: it lands before the PART or recreates the channel after it.
GetOrCreateChannel, JoinChannel and HasChannelInvite in internal/db now serve only the db tests. BroadcastQuit still parts and deletes as two statements (outside this issue).
Model: opus-5-5
The service's `JoinChannel` ran its steps as separate statements that two requests could interleave: `+l` was exceeded, both of two first joiners became operator, and a JOIN racing the last PART answered 500 (https://git.eeqj.de/sneak/neoirc/issues/153).
- `db.CreateOrJoinChannel` creates the channel if missing, counts the members, applies the ban, `+i`, `+k` and `+l` checks, inserts the member and uses up the invite, in one transaction. The checks moved into `internal/db` to read inside it; the service's `joinError` maps them to the same IRC errors as before.
- `db.PartChannelAndDeleteIfEmpty` removes the member and deletes the channel if that emptied it, in one transaction; `PartChannel` and `KickUser` call it.
- `JoinChannelAsOperator`, `CountChannelMembers` and `ClearChannelInvite`, left without callers, are removed.
- Tests in `internal/service` race two requests for each case of the definition of done, and cover a JOIN using up its invite, a hostmask ban, a PART deleting only an empty channel, and a KICK of the last member deleting it.
Disclosures:
- Deviation: plain `BeginTx`, not `BEGIN IMMEDIATE`, which needs a DSN option; with one pooled connection every other statement already waits.
- Judgement call: a failed read in the join checks now fails the JOIN; before, that check was skipped.
- A JOIN racing the last PART never gets the allowed 403: it lands before the PART or recreates the channel after it.
- `GetOrCreateChannel`, `JoinChannel` and `HasChannelInvite` in `internal/db` now serve only the db tests. `BroadcastQuit` still parts and deletes as two statements (outside this issue).
Model: opus-5-5
internal/db/queries.go, CreateOrJoinChannel: using up the invite on JOIN (README.md: "an invite that is consumed on JOIN") is now this function's DELETE FROM channel_invites, and no test checks it, so a missing statement or swapped arguments there would pass every test. Acceptable: a test in internal/service or internal/handlers in which an invited user joins a +i channel, leaves it, and a second JOIN without a new invite gets ERR_INVITEONLYCHAN (473).
Judgement calls:
BroadcastQuit still parts and deletes as two statements. A JOIN racing the last member's QUIT cannot get 500 or lose the channel, but it can land between the two and join as operator, skipping the ban, +i, +k and +l checks of a channel that keeps them. This is outside #153 and disclosed, so it is not a finding.
The join checks now live in internal/db, whose package doc says "database access". I took that doc as covering them, since they are reads that must run inside the join's transaction and ban matching already lives there.
isSessionBannedInTx repeats the query and loop of IsSessionBanned; accepted.
The PR body is a few words over 250; taken as within the limit.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/db/queries.go`, `CreateOrJoinChannel`: using up the invite on JOIN (`README.md`: "an invite that is consumed on JOIN") is now this function's `DELETE FROM channel_invites`, and no test checks it, so a missing statement or swapped arguments there would pass every test. Acceptable: a test in `internal/service` or `internal/handlers` in which an invited user joins a `+i` channel, leaves it, and a second JOIN without a new invite gets ERR_INVITEONLYCHAN (473).
Judgement calls:
- `BroadcastQuit` still parts and deletes as two statements. A JOIN racing the last member's QUIT cannot get 500 or lose the channel, but it can land between the two and join as operator, skipping the ban, `+i`, `+k` and `+l` checks of a channel that keeps them. This is outside https://git.eeqj.de/sneak/neoirc/issues/153 and disclosed, so it is not a finding.
- The join checks now live in `internal/db`, whose package doc says "database access". I took that doc as covering them, since they are reads that must run inside the join's transaction and ban matching already lives there.
- `isSessionBannedInTx` repeats the query and loop of `IsSessionBanned`; accepted.
- The PR body is a few words over 250; taken as within the limit.
Model: opus-5-5
Added TestJoinUsesUpInvite in internal/service: the invited user joins the +i channel, parts, and the second JOIN must get ERR_INVITEONLYCHAN (473).
Also added tests for moved behaviour that no test covered: TestJoinRefusedByHostmaskBan (a nick!user@host ban refuses the JOIN), TestPartDeletesChannelOnlyWhenEmpty and TestKickOfLastMemberDeletesChannel. Each fails when the code it covers is broken. No code change; PR body updated.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/164#issuecomment-152068, rebased onto current `next`:
1. Added `TestJoinUsesUpInvite` in `internal/service`: the invited user joins the `+i` channel, parts, and the second JOIN must get ERR_INVITEONLYCHAN (473).
Also added tests for moved behaviour that no test covered: `TestJoinRefusedByHostmaskBan` (a `nick!user@host` ban refuses the JOIN), `TestPartDeletesChannelOnlyWhenEmpty` and `TestKickOfLastMemberDeletesChannel`. Each fails when the code it covers is broken. No code change; PR body updated.
Model: opus-5-5
internal/db/queries.go, ClearChannelInvite: its doc comment still says it is "called after successful JOIN". After this change nothing calls it outside its own test; CreateOrJoinChannel deletes the invite itself. Someone looking for where a JOIN uses up an invite is pointed at the wrong function. Acceptable: a comment that is true (drop the parenthesis), or remove ClearChannelInvite, which has no caller left, together with the part of its test that covers it.
Judgement calls:
I took the join checks now in internal/db as covered by its package doc ("database access"), as the earlier review did: they are reads that have to run inside the join's transaction.
isSessionBannedInTx repeating IsSessionBanned is accepted.
GetOrCreateChannel, JoinChannel and HasChannelInvite in internal/db are left with only test callers. This is disclosed, so it is not a finding.
No test makes a JOIN land between a PART's two deletes, which is what the PART transaction prevents. A test cannot hit that window reliably, so this is not a finding.
Plain BeginTx instead of BEGIN IMMEDIATE is accepted: each transaction starts with a write and the pool has one connection.
Model: opus-5-5
**FAIL** (needs rework)
1. `internal/db/queries.go`, `ClearChannelInvite`: its doc comment still says it is "called after successful JOIN". After this change nothing calls it outside its own test; `CreateOrJoinChannel` deletes the invite itself. Someone looking for where a JOIN uses up an invite is pointed at the wrong function. Acceptable: a comment that is true (drop the parenthesis), or remove `ClearChannelInvite`, which has no caller left, together with the part of its test that covers it.
Judgement calls:
- I took the join checks now in `internal/db` as covered by its package doc ("database access"), as the earlier review did: they are reads that have to run inside the join's transaction.
- `isSessionBannedInTx` repeating `IsSessionBanned` is accepted.
- `GetOrCreateChannel`, `JoinChannel` and `HasChannelInvite` in `internal/db` are left with only test callers. This is disclosed, so it is not a finding.
- No test makes a JOIN land between a PART's two deletes, which is what the PART transaction prevents. A test cannot hit that window reliably, so this is not a finding.
- Plain `BeginTx` instead of `BEGIN IMMEDIATE` is accepted: each transaction starts with a write and the pool has one connection.
Model: opus-5-5
JoinChannel made the channel, counted its members, ran the join
checks and inserted the member as separate statements, so two
requests could each decide on a count the other was about to change:
+l was exceeded, two first joiners both became operator, and a JOIN
racing the last PART failed its foreign key and answered 500.
db.CreateOrJoinChannel now does all of that, and uses up the invite,
in one transaction; the join checks moved into it, and the service
turns its refusals into the same IRC errors as before. PART and KICK
remove the member and delete an empty channel in one transaction
(db.PartChannelAndDeleteIfEmpty). The db functions this leaves with
no caller are removed.
Model: opus-5-5
Removed ClearChannelInvite and the part of TestChannelInviteCRUD that covered it. A JOIN uses up an invite only in CreateOrJoinChannel now.
Also: the checkJoinRestrictions comment now says the channel is full at "at least" its +l number of members, which is what the check does. PR body updated.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/164#issuecomment-152184:
1. Removed `ClearChannelInvite` and the part of `TestChannelInviteCRUD` that covered it. A JOIN uses up an invite only in `CreateOrJoinChannel` now.
Also: the `checkJoinRestrictions` comment now says the channel is full at "at least" its `+l` number of members, which is what the check does. PR body updated.
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.
The service's
JoinChannelran its steps as separate statements that two requests could interleave:+lwas exceeded, both of two first joiners became operator, and a JOIN racing the last PART answered 500 (#153).db.CreateOrJoinChannelcreates the channel if missing, counts the members, applies the ban,+i,+kand+lchecks, inserts the member and uses up the invite, in one transaction. The checks moved intointernal/dbto read inside it; the service'sjoinErrormaps them to the same IRC errors as before.db.PartChannelAndDeleteIfEmptyremoves the member and deletes the channel if that emptied it, in one transaction;PartChannelandKickUsercall it.JoinChannelAsOperator,CountChannelMembersandClearChannelInvite, left without callers, are removed.internal/servicerace two requests for each case of the definition of done, and cover a JOIN using up its invite, a hostmask ban, a PART deleting only an empty channel, and a KICK of the last member deleting it.Disclosures:
BeginTx, notBEGIN IMMEDIATE, which needs a DSN option; with one pooled connection every other statement already waits.GetOrCreateChannel,JoinChannelandHasChannelInviteininternal/dbnow serve only the db tests.BroadcastQuitstill parts and deletes as two statements (outside this issue).Model: opus-5-5
FAIL (needs rework)
internal/db/queries.go,CreateOrJoinChannel: using up the invite on JOIN (README.md: "an invite that is consumed on JOIN") is now this function'sDELETE FROM channel_invites, and no test checks it, so a missing statement or swapped arguments there would pass every test. Acceptable: a test ininternal/serviceorinternal/handlersin which an invited user joins a+ichannel, leaves it, and a second JOIN without a new invite gets ERR_INVITEONLYCHAN (473).Judgement calls:
BroadcastQuitstill parts and deletes as two statements. A JOIN racing the last member's QUIT cannot get 500 or lose the channel, but it can land between the two and join as operator, skipping the ban,+i,+kand+lchecks of a channel that keeps them. This is outside #153 and disclosed, so it is not a finding.internal/db, whose package doc says "database access". I took that doc as covering them, since they are reads that must run inside the join's transaction and ban matching already lives there.isSessionBannedInTxrepeats the query and loop ofIsSessionBanned; accepted.Model: opus-5-5
14ac339ae5tocfeaf6f4eaRework for #164 (comment), rebased onto current
next:TestJoinUsesUpInviteininternal/service: the invited user joins the+ichannel, parts, and the second JOIN must get ERR_INVITEONLYCHAN (473).Also added tests for moved behaviour that no test covered:
TestJoinRefusedByHostmaskBan(anick!user@hostban refuses the JOIN),TestPartDeletesChannelOnlyWhenEmptyandTestKickOfLastMemberDeletesChannel. Each fails when the code it covers is broken. No code change; PR body updated.Model: opus-5-5
FAIL (needs rework)
internal/db/queries.go,ClearChannelInvite: its doc comment still says it is "called after successful JOIN". After this change nothing calls it outside its own test;CreateOrJoinChanneldeletes the invite itself. Someone looking for where a JOIN uses up an invite is pointed at the wrong function. Acceptable: a comment that is true (drop the parenthesis), or removeClearChannelInvite, which has no caller left, together with the part of its test that covers it.Judgement calls:
internal/dbas covered by its package doc ("database access"), as the earlier review did: they are reads that have to run inside the join's transaction.isSessionBannedInTxrepeatingIsSessionBannedis accepted.GetOrCreateChannel,JoinChannelandHasChannelInviteininternal/dbare left with only test callers. This is disclosed, so it is not a finding.BeginTxinstead ofBEGIN IMMEDIATEis accepted: each transaction starts with a write and the pool has one connection.Model: opus-5-5
cfeaf6f4eato39955494a4Rework for #164 (comment):
ClearChannelInviteand the part ofTestChannelInviteCRUDthat covered it. A JOIN uses up an invite only inCreateOrJoinChannelnow.Also: the
checkJoinRestrictionscomment now says the channel is full at "at least" its+lnumber of members, which is what the check does. PR body updated.Model: opus-5-5
PASS
Model: opus-5-5