From 534d10d71973345aa40489064c050762ba2d4011 Mon Sep 17 00:00:00 2001 From: user Date: Fri, 4 Sep 2026 05:32:19 +0000 Subject: [PATCH] fix(service): make ApplyUserMode apply stage transactional WIP: the apply loop issued independent UPDATEs, so a failure partway through '+w-o' left '+w' persisted while the caller reported total failure, contradicting the doc comment. Collapse the parsed ops to the final value of each flag and write them in one transaction via the new db.SetSessionUserModes. --- internal/db/queries.go | 65 +++++++++++++++++++++++ internal/service/service.go | 101 +++++++++++++++++------------------- 2 files changed, 114 insertions(+), 52 deletions(-) diff --git a/internal/db/queries.go b/internal/db/queries.go index d1abc25..3463f24 100644 --- a/internal/db/queries.go +++ b/internal/db/queries.go @@ -2415,6 +2415,71 @@ func (database *Database) SetChannelUserLimit( return nil } +// SetSessionUserModes applies a set of user-mode flag +// changes to a session inside a single transaction, so a +// multi-mode change such as "+w-o" is all-or-nothing. A nil +// pointer means the caller did not mention that mode and +// the stored value must be left untouched. +func (database *Database) SetSessionUserModes( + ctx context.Context, + sessionID int64, + wallops *bool, + oper *bool, +) error { + if wallops == nil && oper == nil { + return nil + } + + transaction, err := database.conn.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } + + if wallops != nil { + if _, err := transaction.ExecContext( + ctx, + `UPDATE sessions SET is_wallops = ? WHERE id = ?`, + boolToInt(*wallops), sessionID, + ); err != nil { + _ = transaction.Rollback() + + return fmt.Errorf( + "set session wallops: %w", err, + ) + } + } + + if oper != nil { + if _, err := transaction.ExecContext( + ctx, + `UPDATE sessions SET is_oper = ? WHERE id = ?`, + boolToInt(*oper), sessionID, + ); err != nil { + _ = transaction.Rollback() + + return fmt.Errorf("set session oper: %w", err) + } + } + + if err := transaction.Commit(); err != nil { + _ = transaction.Rollback() + + return fmt.Errorf("commit user modes: %w", err) + } + + return nil +} + +// boolToInt renders a Go bool as the 0/1 integer used for +// boolean columns in the SQLite schema. +func boolToInt(value bool) int { + if value { + return 1 + } + + return 0 +} + // SetSessionWallops sets the wallops (+w) flag on a // session. func (database *Database) SetSessionWallops( diff --git a/internal/service/service.go b/internal/service/service.go index c147651..b4c3a49 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -956,8 +956,11 @@ type userModeOp struct { // rejects malformed input (empty string, no leading sign, // bare sign with no mode letters, unknown mode letters, // +o which must be set via OPER) with an IRCError. On -// failure, no persistent change is made. On success, the -// resulting mode string is returned. +// failure, no persistent change is made: parsing happens +// before any write, and the writes themselves run inside a +// single database transaction that is rolled back whole if +// any statement fails. On success, the resulting mode +// string is returned. func (s *Service) ApplyUserMode( ctx context.Context, sessionID int64, @@ -968,12 +971,19 @@ func (s *Service) ApplyUserMode( return "", err } - for _, op := range ops { - if err := s.applySingleUserMode( - ctx, sessionID, op.char, op.adding, - ); err != nil { - return "", err - } + wallops, oper, err := collapseUserModeOps(ops) + if err != nil { + return "", err + } + + if err := s.db.SetSessionUserModes( + ctx, sessionID, wallops, oper, + ); err != nil { + s.log.Error( + "apply user modes failed", "error", err, + ) + + return "", fmt.Errorf("apply user modes: %w", err) } return s.QueryUserMode(ctx, sessionID) @@ -1048,56 +1058,43 @@ func isKnownUserModeChar(modeChar rune) bool { } } -// applySingleUserMode applies one already-validated user -// mode character to the session. parseUserModeString must -// have validated the character and sign before this runs; -// the default branch here is defence-in-depth only. -func (s *Service) applySingleUserMode( - ctx context.Context, - sessionID int64, - modeChar rune, - adding bool, -) error { - switch modeChar { - case 'w': - err := s.db.SetSessionWallops( - ctx, sessionID, adding, - ) - if err != nil { - s.log.Error( - "set wallops mode failed", "error", err, - ) +// collapseUserModeOps reduces an already-parsed operation +// list to the final desired value of each user mode flag. +// A nil result for a flag means the mode string never +// mentioned it, so it must be left untouched. Later +// operations win over earlier ones for the same letter +// (e.g. "+w-w" ends with wallops off), which matches the +// left-to-right semantics of applying each op in turn. +// parseUserModeString must have validated every character +// and sign before this runs; the default branch here is +// defence-in-depth only. +func collapseUserModeOps( + ops []userModeOp, +) (wallops, oper *bool, err error) { + unknownFlag := &IRCError{ + Code: irc.ErrUmodeUnknownFlag, + Params: nil, + Message: "Unknown MODE flag", + } - return fmt.Errorf("set wallops: %w", err) - } - case 'o': - if adding { - return &IRCError{ - Code: irc.ErrUmodeUnknownFlag, - Params: nil, - Message: "Unknown MODE flag", + for _, op := range ops { + switch op.char { + case 'w': + val := op.adding + wallops = &val + case 'o': + if op.adding { + return nil, nil, unknownFlag } - } - err := s.db.SetSessionOper( - ctx, sessionID, false, - ) - if err != nil { - s.log.Error( - "clear oper mode failed", "error", err, - ) - - return fmt.Errorf("clear oper: %w", err) - } - default: - return &IRCError{ - Code: irc.ErrUmodeUnknownFlag, - Params: nil, - Message: "Unknown MODE flag", + val := false + oper = &val + default: + return nil, nil, unknownFlag } } - return nil + return wallops, oper, nil } // broadcastNickChange notifies channel peers of a nick