diff --git a/server/app/goauto/sybproductfilter/recompute.go b/server/app/goauto/sybproductfilter/recompute.go index fed6089..127b6cc 100644 --- a/server/app/goauto/sybproductfilter/recompute.go +++ b/server/app/goauto/sybproductfilter/recompute.go @@ -207,6 +207,61 @@ type RecomputeExecuteResult struct { Operator string `json:"operator"` } +// countPurchaseTasksLocked and countActiveReturnMatchesLocked are the two +// rechecks writeRecomputeChanges runs after taking the syb_product row lock. +// +// `[必须]` They MUST use a locking read (FOR SHARE), not a plain COUNT(*). +// Production and local are MySQL 8.4 under REPEATABLE-READ, where a plain +// read inside a transaction reuses the snapshot taken at that transaction's +// FIRST read (here, recomputeChanges' own SELECT) — so a purchase_task or +// return_match row committed by another connection AFTER that snapshot is +// invisible to a plain COUNT(*) even after this code has waited for and +// obtained the syb_product row's FOR UPDATE lock. The row lock only +// serializes writers against each other; it does not by itself make a later +// plain read see newer committed data under REPEATABLE-READ. A locking read +// (FOR SHARE is enough since these two only need to observe committed rows, +// not lock them for update) forces MySQL to use a fresh "current read" +// instead of the snapshot, which is exactly what closes the race (verified +// against real MySQL 8.4 with two connections: after the other transaction +// committed a task, plain COUNT returned 0 while COUNT ... FOR SHARE +// correctly returned 1). SQLite — used by this package's tests — drops +// locking clauses entirely (gorm.io/driver/sqlite's "FOR" clause builder is +// a no-op) and has no multi-connection snapshot semantics to reproduce this +// race in the first place, so no SQLite-backed test can catch a regression +// here; see TestRecheckQueriesUseLockingReads below for the SQL-shape test +// that does. +// sybProductRowLockQuery, purchaseTaskLockedQuery and returnMatchLockedQuery +// build (but do not execute) the three locking reads writeRecomputeChanges +// runs per row. They are split out from the count*/lock helpers below purely +// so a test can call db.ToSQL against the exact same query construction the +// production code runs, and assert the FOR UPDATE / FOR SHARE clause is +// actually present in the generated SQL (#340 phase 3 review follow-up). +func sybProductRowLockQuery(tx *gorm.DB) *gorm.DB { + return tx.Clauses(clause.Locking{Strength: clause.LockingStrengthUpdate}) +} + +func purchaseTaskLockedQuery(tx *gorm.DB, sybID uint64) *gorm.DB { + return tx.Clauses(clause.Locking{Strength: clause.LockingStrengthShare}). + Model(&models.PurchaseTask{}).Where("syb_product_id = ?", sybID) +} + +func returnMatchLockedQuery(tx *gorm.DB, sybID uint64) *gorm.DB { + return tx.Clauses(clause.Locking{Strength: clause.LockingStrengthShare}). + Table("return_match").Where("syb_product_id = ? AND active_syb_product_id IS NOT NULL", sybID) +} + +func countPurchaseTasksLocked(ctx context.Context, tx *gorm.DB, sybID uint64) (int64, error) { + var count int64 + err := purchaseTaskLockedQuery(tx.WithContext(ctx), sybID).Count(&count).Error + return count, err +} + +func countActiveReturnMatchesLocked(ctx context.Context, tx *gorm.DB, sybID uint64) (int64, error) { + var count int64 + err := returnMatchLockedQuery(tx.WithContext(ctx), sybID).Count(&count).Error + return count, err +} + // writeRecomputeChanges is the write phase, kept separate from planning so it // is independently testable (#340 phase 3 review item 1): for every planned // change it takes the SAME row lock purchase.Service.create and returnmatch's @@ -221,23 +276,19 @@ func writeRecomputeChanges(ctx context.Context, tx *gorm.DB, planned []recompute now := time.Now().UTC() for _, change := range planned { var locked models.SYBProduct - if err := tx.WithContext(ctx).Clauses(clause.Locking{Strength: "UPDATE"}). - First(&locked, change.id).Error; err != nil { + if err := sybProductRowLockQuery(tx.WithContext(ctx)).First(&locked, change.id).Error; err != nil { return RecomputeCounts{}, err } - var taskCount int64 - if err := tx.WithContext(ctx).Model(&models.PurchaseTask{}). - Where("syb_product_id = ?", change.id).Count(&taskCount).Error; err != nil { + taskCount, err := countPurchaseTasksLocked(ctx, tx, change.id) + if err != nil { return RecomputeCounts{}, err } if taskCount > 0 { actual.SkippedHasTask++ continue } - var matchCount int64 - if err := tx.WithContext(ctx).Table("return_match"). - Where("syb_product_id = ? AND active_syb_product_id IS NOT NULL", change.id). - Count(&matchCount).Error; err != nil { + matchCount, err := countActiveReturnMatchesLocked(ctx, tx, change.id) + if err != nil { return RecomputeCounts{}, err } if matchCount > 0 { diff --git a/server/app/goauto/sybproductfilter/recompute_locking_test.go b/server/app/goauto/sybproductfilter/recompute_locking_test.go new file mode 100644 index 0000000..23aa071 --- /dev/null +++ b/server/app/goauto/sybproductfilter/recompute_locking_test.go @@ -0,0 +1,89 @@ +package sybproductfilter + +import ( + "strings" + "testing" + + "gorm.io/driver/mysql" + "gorm.io/gorm" +) + +// mysqlDryRunDB opens a gorm session against the MySQL dialector with +// DryRun+DisableAutomaticPing, so no real network connection is ever made +// (sql.Open is lazy and gorm skips the startup ping) but the SQL gorm would +// send to a real MySQL 8.4 server can still be inspected via db.ToSQL. +// +// `[必须]` This must be the MySQL dialector, not SQLite: gorm.io/driver/ +// sqlite's own "FOR" clause builder silently drops clause.Locking entirely +// (SQLite has no row-level locking), so a SQLite-backed test would show +// these queries with no FOR clause at all regardless of whether the +// production code asks for one — it would pass even with the bug this test +// exists to catch. Only a MySQL-dialect SQL string proves the FOR UPDATE / +// FOR SHARE clauses are actually being sent. +func mysqlDryRunDB(t *testing.T) *gorm.DB { + t.Helper() + // SkipInitializeWithVersion is required, not just DisableAutomaticPing: + // gorm's MySQL dialector otherwise runs `SELECT VERSION()` against the + // DSN's ConnPool during Initialize (Open), before DryRun/ping settings + // even come into play, to decide version-gated feature flags such as + // DontSupportForShareClause. With it set, sql.Open's lazy connection + // pool is never dialed at all. + db, err := gorm.Open(mysql.New(mysql.Config{ + DSN: "user:pass@tcp(127.0.0.1:3306)/goauto_test?parseTime=true", SkipInitializeWithVersion: true, + }), &gorm.Config{DryRun: true, DisableAutomaticPing: true}) + if err != nil { + t.Fatalf("open dry-run mysql session: %v", err) + } + return db +} + +// TestRecheckQueriesUseLockingReads pins the exact SQL shape behind #340 +// phase 3's real fix: writeRecomputeChanges' syb_product row lock must be +// FOR UPDATE, and its two rechecks (purchase_task, return_match) must be +// FOR SHARE — a plain COUNT(*) for either recheck is invisible to a +// transaction's already-taken REPEATABLE-READ snapshot on real MySQL 8.4 +// even after the row's FOR UPDATE lock is granted, which is exactly the race +// this test guards against ever regressing to (verified against a real +// MySQL 8.4 server with two connections: a purchase_task committed by the +// other connection after the snapshot was invisible to plain COUNT(*), but +// visible to COUNT(*) ... FOR SHARE). SQLite, which every other test in this +// package runs against, cannot reproduce any of this: it has no +// multi-connection snapshot isolation and gorm's SQLite driver drops locking +// clauses outright, so this test intentionally talks MySQL SQL shape only, +// never a real database. +func TestRecheckQueriesUseLockingReads(t *testing.T) { + db := mysqlDryRunDB(t) + + rowLockSQL := db.ToSQL(func(tx *gorm.DB) *gorm.DB { + var dest map[string]any + return sybProductRowLockQuery(tx).Table("syb_product").Where("id = ?", uint64(1)).Find(&dest) + }) + t.Logf("ROW LOCK SQL: %s", rowLockSQL) + if !strings.Contains(rowLockSQL, "FOR UPDATE") { + t.Fatalf("expected the syb_product row lock to be FOR UPDATE, got SQL: %s", rowLockSQL) + } + + var taskCount int64 + taskSQL := db.ToSQL(func(tx *gorm.DB) *gorm.DB { + return purchaseTaskLockedQuery(tx, 1).Count(&taskCount) + }) + t.Logf("PURCHASE TASK RECHECK SQL: %s", taskSQL) + if !strings.Contains(taskSQL, "FOR SHARE") { + t.Fatalf("expected the purchase_task recheck to be a locking (FOR SHARE) read, got SQL: %s", taskSQL) + } + if !strings.Contains(taskSQL, "syb_product_id") { + t.Fatalf("expected the purchase_task recheck to filter by syb_product_id, got SQL: %s", taskSQL) + } + + var matchCount int64 + matchSQL := db.ToSQL(func(tx *gorm.DB) *gorm.DB { + return returnMatchLockedQuery(tx, 1).Count(&matchCount) + }) + t.Logf("RETURN MATCH RECHECK SQL: %s", matchSQL) + if !strings.Contains(matchSQL, "FOR SHARE") { + t.Fatalf("expected the return_match recheck to be a locking (FOR SHARE) read, got SQL: %s", matchSQL) + } + if !strings.Contains(matchSQL, "active_syb_product_id IS NOT NULL") { + t.Fatalf("expected the return_match recheck to filter on an active match, got SQL: %s", matchSQL) + } +}