OPC/goauto
2
0
Fork 0
Code Issues 200 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(syb): #340 phase 3 review — recheck reads must be locking, not plain

Browse Source 
writeRecomputeChanges' purchase_task/return_match rechecks (added in the
previous phase 3 commit) used plain SELECT COUNT(*). On MySQL 8.4 under
REPEATABLE-READ, a plain read inside a transaction reuses the snapshot
taken at that transaction's first read (recomputeChanges' own SELECT), so
a purchase_task or return_match row committed by another connection AFTER
that snapshot was invisible to the recheck even after the syb_product row's
FOR UPDATE lock was granted — the row lock only serializes writers against
each other, it doesn't force a later plain read to see newer committed
data. Confirmed against a real local MySQL 8.4 server with two connections:
after the other transaction committed a task, plain COUNT(*) returned 0
while COUNT(*) ... FOR SHARE correctly returned 1. Every existing test
passed anyway because this package's tests run on SQLite, which has no
multi-connection snapshot isolation to reproduce the race at all.

Fix: both rechecks now use clause.Locking{Strength: "SHARE"} (a locking/
"current" read is enough since they only need to observe committed rows,
not lock them further). Split the three per-row queries (row FOR UPDATE,
purchase_task FOR SHARE, return_match FOR SHARE) into small query-builder
helpers (sybProductRowLockQuery / purchaseTaskLockedQuery /
returnMatchLockedQuery) so writeRecomputeChanges consumes them and a test
can independently assert their generated SQL.

New test: TestRecheckQueriesUseLockingReads opens a DryRun gorm session
against the MySQL dialector (mysql.New with SkipInitializeWithVersion,
DisableAutomaticPing — no real network connection is ever made) and pins
the exact SQL shape via db.ToSQL:
  SELECT * FROM `syb_product` WHERE id = 1 FOR UPDATE
  SELECT count(*) FROM `purchase_task` WHERE syb_product_id = 1 FOR SHARE
  SELECT count(*) FROM `return_match` WHERE syb_product_id = 1
    AND active_syb_product_id IS NOT NULL FOR SHARE
The test's own comment documents why SQLite cannot reproduce this race
(gorm's SQLite driver drops clause.Locking entirely; SQLite also has no
multi-connection REPEATABLE-READ snapshot semantics to begin with).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTDbDcwbDw1TSAcE6wfh2F
This commit is contained in:
QiuSW
2026-09-29 15:43:37 +08:00
co-authored by Claude Opus 5.5
parent baa20af530
commit cd4bd6acb1
2 changed files with 149 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/app/goauto/sybproductfilter/recompute.go
+60 -9
View File
@@ -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 {
server/app/goauto/sybproductfilter/recompute_locking_test.go
+89
View File
@@ -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)
}
}