From 59fd722d3721bbf1b05abeb4fdfdcbaedaac4d59 Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Sun, 22 Feb 2026 16:55:59 +0100 Subject: [PATCH] refactor(channelserver): standardize on BeginTxx for all repository transactions Replace db.Begin() with db.BeginTxx(context.Background(), nil) across all 8 remaining call sites in repo_guild.go, repo_guild_rp.go, repo_festa.go, and repo_event.go. Use deferred Rollback() instead of explicit rollback at each error return, eliminating 15 manual rollback calls. --- docs/technical-debt.md | 16 +++------------- server/channelserver/repo_event.go | 6 ++++-- server/channelserver/repo_festa.go | 6 ++++-- server/channelserver/repo_guild.go | 27 ++++++++++++--------------- server/channelserver/repo_guild_rp.go | 13 +++++++------ 5 files changed, 30 insertions(+), 38 deletions(-) diff --git a/docs/technical-debt.md b/docs/technical-debt.md index 2b53f146f..9b72c85e9 100644 --- a/docs/technical-debt.md +++ b/docs/technical-debt.md @@ -114,19 +114,9 @@ The pattern `m.Field, _ = stringsupport.SJISToUTF8(...)` appears across: A malformed SJIS string from a client yields an empty string with no log output, making garbled text impossible to debug. The error return should at least be logged at debug level. -### 6. Inconsistent transaction API +### ~~6. Inconsistent transaction API~~ (Fixed) -`repo_guild.go` mixes two transaction styles in the same file: - -```go -// Line 175 — no context, old style -tx, err := r.db.Begin() - -// Line 518 — sqlx-idiomatic, with context -tx, err := r.db.BeginTxx(context.Background(), nil) -``` - -Should standardize on `BeginTxx` throughout. The `Begin()` calls cannot carry a context for cancellation or timeout. +**Status:** Fixed. All transaction call sites now use `BeginTxx(context.Background(), nil)` with deferred rollback, replacing the old `Begin()` + manual rollback pattern across `repo_guild.go`, `repo_guild_rp.go`, `repo_festa.go`, and `repo_event.go`. ### ~~7. `LoopDelay` config has no Viper default~~ (Fixed) @@ -162,4 +152,4 @@ Based on impact and the momentum from recent repo-interface refactoring: 5. ~~**Fix `fmt.Sprintf` in logger calls**~~ — **Done** 6. ~~**Add `LoopDelay` Viper default**~~ — **Done** 7. **Log SJIS decoding errors** — improves debuggability for text issues -8. **Standardize on `BeginTxx`** — consistency fix in `repo_guild.go` +8. ~~**Standardize on `BeginTxx`**~~ — **Done** diff --git a/server/channelserver/repo_event.go b/server/channelserver/repo_event.go index f6c7a7c45..eaae596e7 100644 --- a/server/channelserver/repo_event.go +++ b/server/channelserver/repo_event.go @@ -1,6 +1,7 @@ package channelserver import ( + "context" "time" "github.com/jmoiron/sqlx" @@ -79,13 +80,14 @@ func (r *EventRepository) UpdateEventQuestStartTimes(updates []EventQuestUpdate) if len(updates) == 0 { return nil } - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() + for _, u := range updates { if _, err := tx.Exec("UPDATE event_quests SET start_time = $1 WHERE id = $2", u.StartTime, u.ID); err != nil { - _ = tx.Rollback() return err } } diff --git a/server/channelserver/repo_festa.go b/server/channelserver/repo_festa.go index ca3d61b34..c588bfae7 100644 --- a/server/channelserver/repo_festa.go +++ b/server/channelserver/repo_festa.go @@ -1,6 +1,7 @@ package channelserver import ( + "context" "database/sql" "github.com/jmoiron/sqlx" @@ -181,16 +182,17 @@ func (r *FestaRepository) RegisterGuild(guildID uint32, team string) error { // SubmitSouls records soul submissions for a character within a transaction. func (r *FestaRepository) SubmitSouls(charID, guildID uint32, souls []uint16) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() + for i, s := range souls { if s == 0 { continue } if _, err := tx.Exec(`INSERT INTO festa_submissions VALUES ($1, $2, $3, $4, now())`, charID, guildID, i, s); err != nil { - _ = tx.Rollback() return err } } diff --git a/server/channelserver/repo_guild.go b/server/channelserver/repo_guild.go index ef64e1894..f9eca11e5 100644 --- a/server/channelserver/repo_guild.go +++ b/server/channelserver/repo_guild.go @@ -1,6 +1,7 @@ package channelserver import ( + "context" "database/sql" "errors" "fmt" @@ -168,10 +169,11 @@ func (r *GuildRepository) ListAll() ([]*Guild, error) { // Create creates a new guild and adds the leader as its first member. func (r *GuildRepository) Create(leaderCharID uint32, guildName string) (int32, error) { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return 0, err } + defer func() { _ = tx.Rollback() }() var guildID int32 err = tx.QueryRow( @@ -179,13 +181,11 @@ func (r *GuildRepository) Create(leaderCharID uint32, guildName string) (int32, guildName, leaderCharID, ).Scan(&guildID) if err != nil { - _ = tx.Rollback() return 0, err } _, err = tx.Exec(`INSERT INTO guild_characters (guild_id, character_id) VALUES ($1, $2)`, guildID, leaderCharID) if err != nil { - _ = tx.Rollback() return 0, err } @@ -207,10 +207,11 @@ func (r *GuildRepository) Save(guild *Guild) error { // Disband removes a guild, its members, and cleans up alliance references. func (r *GuildRepository) Disband(guildID uint32) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() stmts := []string{ "DELETE FROM guild_characters WHERE guild_id = $1", @@ -219,17 +220,14 @@ func (r *GuildRepository) Disband(guildID uint32) error { } for _, stmt := range stmts { if _, err := tx.Exec(stmt, guildID); err != nil { - _ = tx.Rollback() return err } } if _, err := tx.Exec("UPDATE guild_alliances SET sub1_id=sub2_id, sub2_id=NULL WHERE sub1_id=$1", guildID); err != nil { - _ = tx.Rollback() return err } if _, err := tx.Exec("UPDATE guild_alliances SET sub2_id=NULL WHERE sub2_id=$1", guildID); err != nil { - _ = tx.Rollback() return err } @@ -244,13 +242,13 @@ func (r *GuildRepository) RemoveCharacter(charID uint32) error { // AcceptApplication deletes the application and adds the character to the guild. func (r *GuildRepository) AcceptApplication(guildID, charID uint32) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() if _, err := tx.Exec(`DELETE FROM guild_applications WHERE character_id = $1`, charID); err != nil { - _ = tx.Rollback() return err } @@ -258,7 +256,6 @@ func (r *GuildRepository) AcceptApplication(guildID, charID uint32) error { INSERT INTO guild_characters (guild_id, character_id, order_index) VALUES ($1, $2, (SELECT MAX(order_index) + 1 FROM guild_characters WHERE guild_id = $1)) `, guildID, charID); err != nil { - _ = tx.Rollback() return err } @@ -275,18 +272,18 @@ func (r *GuildRepository) CreateApplication(guildID, charID, actorID uint32, app // CreateApplicationWithMail atomically creates an application and sends a notification mail. func (r *GuildRepository) CreateApplicationWithMail(guildID, charID, actorID uint32, appType GuildApplicationType, mailSenderID, mailRecipientID uint32, mailSubject, mailBody string) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() + if _, err := tx.Exec( `INSERT INTO guild_applications (guild_id, character_id, actor_id, application_type) VALUES ($1, $2, $3, $4)`, guildID, charID, actorID, appType); err != nil { - _ = tx.Rollback() return err } if _, err := tx.Exec(mailInsertQuery, mailSenderID, mailRecipientID, mailSubject, mailBody, 0, 0, true, false); err != nil { - _ = tx.Rollback() return err } return tx.Commit() @@ -312,14 +309,14 @@ func (r *GuildRepository) RejectApplication(guildID, charID uint32) error { // ArrangeCharacters reorders guild members by updating their order_index values. func (r *GuildRepository) ArrangeCharacters(charIDs []uint32) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() for i, id := range charIDs { if _, err := tx.Exec("UPDATE guild_characters SET order_index = $1 WHERE character_id = $2", 2+i, id); err != nil { - _ = tx.Rollback() return err } } diff --git a/server/channelserver/repo_guild_rp.go b/server/channelserver/repo_guild_rp.go index ea52af8ba..3cc037c9f 100644 --- a/server/channelserver/repo_guild_rp.go +++ b/server/channelserver/repo_guild_rp.go @@ -1,6 +1,9 @@ package channelserver -import "time" +import ( + "context" + "time" +) // AddMemberDailyRP adds RP to a member's daily total. func (r *GuildRepository) AddMemberDailyRP(charID uint32, amount uint16) error { @@ -56,36 +59,34 @@ func (r *GuildRepository) SetRoomExpiry(guildID uint32, expiry time.Time) error // then updates the guild's rp_reset_at timestamp. // Uses SELECT FOR UPDATE to prevent concurrent rollovers from racing. func (r *GuildRepository) RolloverDailyRP(guildID uint32, noon time.Time) error { - tx, err := r.db.Begin() + tx, err := r.db.BeginTxx(context.Background(), nil) if err != nil { return err } + defer func() { _ = tx.Rollback() }() + // Lock the guild row and re-check whether rollover is still needed. var rpResetAt time.Time if err := tx.QueryRow( `SELECT COALESCE(rp_reset_at, '2000-01-01'::timestamptz) FROM guilds WHERE id = $1 FOR UPDATE`, guildID, ).Scan(&rpResetAt); err != nil { - _ = tx.Rollback() return err } if !rpResetAt.Before(noon) { // Another goroutine already rolled over; nothing to do. - _ = tx.Rollback() return nil } if _, err := tx.Exec( `UPDATE guild_characters SET rp_yesterday = rp_today, rp_today = 0 WHERE guild_id = $1`, guildID, ); err != nil { - _ = tx.Rollback() return err } if _, err := tx.Exec( `UPDATE guilds SET rp_reset_at = $1 WHERE id = $2`, noon, guildID, ); err != nil { - _ = tx.Rollback() return err } return tx.Commit()