◐ Off-By-One · answer catalog

go-claim-returning-post-update-zero

2 answer(s)godockergodocker

go-claim-returning-post-update-zero

📦 Source in repository (JSON)

Answer 1

Root cause. ClaimDomainCredits used:

UPDATE accounts SET shadow_credits = 0, is_claimed = true
 WHERE id = $1 AND is_claimed = false
RETURNING shadow_credits

RETURNING emits the post-update row. Since the same statement sets shadow_credits = 0, every claim reported 0 credits converted — even though the conversion succeeded.

Fix. Replace RETURNING with a data-modifying CTE that captures the pre-update value before the write runs, then projects it out (~/claimcredits/store.go):

const claimDomainCreditsSQL = `
WITH pre AS (
    SELECT shadow_credits
      FROM accounts
     WHERE id = $1 AND is_claimed = false
     FOR UPDATE
),
upd AS (
    UPDATE accounts
       SET shadow_credits = 0,
           is_claimed = true
     WHERE id = $1 AND is_claimed = false
)
SELECT shadow_credits FROM pre`

func ClaimDomainCredits(ctx context.Context, q Querier, accountID int64) (int64, error) {
    var converted int64
    err := q.QueryRow(ctx, claimDomainCreditsSQL, accountID).Scan(&converted)
    if errors.Is(err, pgx.ErrNoRows) {
        return 0, ErrNotClaimed // missing or already claimed: nothing converted
    }
    if err != nil {
        return 0, fmt.Errorf("claim domain credits: %w", err)
    }
    return converted, nil
}

Why this is correct: - pre reads the pre-update shadow_credits (the amount actually converted); upd performs the conversion; the outer SELECT projects the pre value. RETURNING is gone, so the post-update zero cannot leak in. - pre and upd are one statement, so the read and write share a snapshot — the returned value is the amount the UPDATE converted, with no separate transaction needed. - FOR UPDATE on pre is the concurrency hardening: a racing claim blocks on the row lock, re-evaluates is_claimed = false against the winner's commit, and returns ErrNotClaimed instead of double-reporting. errors.Is(err, pgx.ErrNoRows) → ErrNotClaimed covers the missing/already-claimed no-op path.

The naive UPDATE … RETURNING version is preserved in naive_test.go as a regression reference.

Evidence & signatures

**RED → GREEN workflow, verified live:**

1. **RED** — With the naive SQL as production, `go test -v` failed at both layers:
   - Real PG: `expected 500 credits converted, got 0` (basic claim), `first claim: got=0` (already-claimed), `concurrent claim: unexpected outcome got=0`
   - pgxmock: `could not match actual sql: "UPDATE accounts SET shadow_credits = 0 … RETURNING shadow_credits" with expected regexp "WITH pre AS"`
2. **GREEN** — After the CTE fix, `go test -race -count=1 ./...` → **`ok claimcredits`**, 9/9 passing.

**Tests** (`store_test.go` = pgxmock; `integration_test.go` = real PostgreSQL 18 on <ip-address>:55433, `accounts` table recreated per test):

| Test | Layer | Outcome |
|---|---|---|
| `ClaimDomainCredits_ReturnsPreUpdateAmount` | pgxmock | returns 500 (pre-update), not 0 |
| `ClaimDomainCredits_AlreadyClaimed` | pgxmock | `ErrNotClaimed`, 0 converted |
| `Naive_ReturnsPostUpdateZero` | pgxmock | pins the original bug: naive SQL → 0 |
| Integration `…ReturnsAmountConverted` | real PG | returns 500; row ends `shadow_credits=0, is_claimed=true` |
| Integration `…NaiveReturnsPostUpdateZero` | real PG | naive SQL returns 0 while row IS converted (bug proof) |
| Integration `…AlreadyClaimed` | real PG | 2nd claim → `ErrNotClaimed`, row unchanged |
| Integration `…ZeroCredits` | real PG | claiming a 0-credit account succeeds, returns 0 (legit zero, not an error) |
| Integration `…MissingAccount` | real PG | nonexistent id → `ErrNotClaimed` |
| Integration `…ConcurrentClaims_ExactlyOneWins` | real PG + `-race` | 4 goroutines: exactly 1 wins (500), 3 get `ErrNotClaimed`; stable across 3×5 repeated runs |

**Edge case found beyond the spec — plain CTE is racy.** A deterministic lock-contention experiment (T0 holds a row `FOR UPDATE`, T1/T2 run the CTE, T0 commits) showed the plain CTE without `FOR UPDATE` reports `[500, 500]` — the loser reports 500 credits it never converted (its `pre` already ran before the winner's commit). Adding `FOR UPDATE` to `pre` makes it report `[500, -1]` — exactly one claim converts, the loser gets `ErrNotClaimed`. This is why the production SQL includes `FOR UPDATE`, and it's guarded by the 4-worker race test.
{"model": "deepseek-v4-flash", "problem_class": "go-claim-returning-post-update-zero", "result": "passed", "tests": 9}

Answer 2

Root cause. ClaimDomainCredits used:

UPDATE accounts SET shadow_credits = 0, is_claimed = true
 WHERE id = $1 AND is_claimed = false
RETURNING shadow_credits

RETURNING emits the post-update row. Since the same statement sets shadow_credits = 0, every claim reported 0 credits converted — even though the conversion succeeded.

Fix. Replace RETURNING with a data-modifying CTE that captures the pre-update value before the write runs, then projects it out (~/claimcredits/store.go):

const claimDomainCreditsSQL = `
WITH pre AS (
    SELECT shadow_credits
      FROM accounts
     WHERE id = $1 AND is_claimed = false
     FOR UPDATE
),
upd AS (
    UPDATE accounts
       SET shadow_credits = 0,
           is_claimed = true
     WHERE id = $1 AND is_claimed = false
)
SELECT shadow_credits FROM pre`

func ClaimDomainCredits(ctx context.Context, q Querier, accountID int64) (int64, error) {
    var converted int64
    err := q.QueryRow(ctx, claimDomainCreditsSQL, accountID).Scan(&converted)
    if errors.Is(err, pgx.ErrNoRows) {
        return 0, ErrNotClaimed // missing or already claimed: nothing converted
    }
    if err != nil {
        return 0, fmt.Errorf("claim domain credits: %w", err)
    }
    return converted, nil
}

Why this is correct: - pre reads the pre-update shadow_credits (the amount actually converted); upd performs the conversion; the outer SELECT projects the pre value. RETURNING is gone, so the post-update zero cannot leak in. - pre and upd are one statement, so the read and write share a snapshot — the returned value is the amount the UPDATE converted, with no separate transaction needed. - FOR UPDATE on pre is the concurrency hardening: a racing claim blocks on the row lock, re-evaluates is_claimed = false against the winner's commit, and returns ErrNotClaimed instead of double-reporting. errors.Is(err, pgx.ErrNoRows) → ErrNotClaimed covers the missing/already-claimed no-op path.

The naive UPDATE … RETURNING version is preserved in naive_test.go as a regression reference.

Evidence & signatures

**RED → GREEN workflow, verified live:**

1. **RED** — With the naive SQL as production, `go test -v` failed at both layers:
   - Real PG: `expected 500 credits converted, got 0` (basic claim), `first claim: got=0` (already-claimed), `concurrent claim: unexpected outcome got=0`
   - pgxmock: `could not match actual sql: "UPDATE accounts SET shadow_credits = 0 … RETURNING shadow_credits" with expected regexp "WITH pre AS"`
2. **GREEN** — After the CTE fix, `go test -race -count=1 ./...` → **`ok claimcredits`**, 9/9 passing.

**Tests** (`store_test.go` = pgxmock; `integration_test.go` = real PostgreSQL 18 on <ip-address>:55433, `accounts` table recreated per test):

| Test | Layer | Outcome |
|---|---|---|
| `ClaimDomainCredits_ReturnsPreUpdateAmount` | pgxmock | returns 500 (pre-update), not 0 |
| `ClaimDomainCredits_AlreadyClaimed` | pgxmock | `ErrNotClaimed`, 0 converted |
| `Naive_ReturnsPostUpdateZero` | pgxmock | pins the original bug: naive SQL → 0 |
| Integration `…ReturnsAmountConverted` | real PG | returns 500; row ends `shadow_credits=0, is_claimed=true` |
| Integration `…NaiveReturnsPostUpdateZero` | real PG | naive SQL returns 0 while row IS converted (bug proof) |
| Integration `…AlreadyClaimed` | real PG | 2nd claim → `ErrNotClaimed`, row unchanged |
| Integration `…ZeroCredits` | real PG | claiming a 0-credit account succeeds, returns 0 (legit zero, not an error) |
| Integration `…MissingAccount` | real PG | nonexistent id → `ErrNotClaimed` |
| Integration `…ConcurrentClaims_ExactlyOneWins` | real PG + `-race` | 4 goroutines: exactly 1 wins (500), 3 get `ErrNotClaimed`; stable across 3×5 repeated runs |

**Edge case found beyond the spec — plain CTE is racy.** A deterministic lock-contention experiment (T0 holds a row `FOR UPDATE`, T1/T2 run the CTE, T0 commits) showed the plain CTE without `FOR UPDATE` reports `[500, 500]` — the loser reports 500 credits it never converted (its `pre` already ran before the winner's commit). Adding `FOR UPDATE` to `pre` makes it report `[500, -1]` — exactly one claim converts, the loser gets `ErrNotClaimed`. This is why the production SQL includes `FOR UPDATE`, and it's guarded by the 4-worker race test.
{"model": "deepseek-v4-flash", "problem_class": "go-claim-returning-post-update-zero", "result": "passed", "tests": 9}
Generated from the verified corpus · MIT licensedBack to the catalog