mirror of
https://github.com/coder/coder.git
synced 2026-06-03 13:08:25 +00:00
Ethan
55e525fc28
Replace the old `InTx` ruleguard rule in `scripts/rules.go` with a custom in-tree `go/analysis` analyzer under `scripts/intxcheck/`. The new analyzer catches the same direct and pass-through misuse classes as before, plus two new classes the pattern-matcher couldn't reach: - **Indirect same-package helper misuse** — flags `p.someHelper(ctx)` inside `InTx` when the helper body uses the outer store (the PR #24369 bug class). - **Nested dangerous closures** — descends into `go func() { ... }()`, `defer func() { ... }()`, and immediately-invoked function literals. The analyzer uses semantic `types.Object` identity instead of raw expression string comparison, which avoids false positives from closure-local shadowing and catches simple aliases like `outer := s.db` and `alias := s`. This PR also fixes three real outer-store-inside-transaction bugs the new analyzer surfaced: - `coderd/wsbuilder/wsbuilder.go`: `FindMatchingPresetID` and `getWorkspaceTask` now use the inner transaction store instead of `b.store`. - `enterprise/dbcrypt/dbcrypt.go`: `ensureEncrypted` now calls `s.InsertDBCryptKey` (the tx-wrapped store) instead of `db.InsertDBCryptKey`. The `dbCrypt.InTx` method wraps the raw tx in a new `*dbCrypt`, so `s.InsertDBCryptKey` still dispatches through the encryption layer. Two call sites need `// intxcheck:ignore` suppressions. Both are one-off patterns that only look like misuse because the analyzer doesn't track assignments — proving them safe would require full dataflow analysis, which is well beyond what a targeted lint like this should attempt: - `coderd/database/dbfake/dbfake.go` — `b.db` is reassigned to `tx` on the preceding line, so `b.doInTX()` actually uses the transaction. The analyzer sees the original `b.db` identity and flags it. - `coderd/database/db_test.go` — test intentionally passes the outer store to `require.Equal` to assert that nested `InTx` returns the same handle. Suppressions use `// intxcheck:ignore` instead of `//nolint:intxcheck` because `intxcheck` runs as a standalone `go/analysis` tool outside golangci-lint. golangci-lint's `nolintlint` checker flags `//nolint` directives for linters it doesn't control, so we use a custom comment prefix to avoid that conflict.
97 lines
2.6 KiB
Go
97 lines
2.6 KiB
Go
package database_test
|
|
|
|
import (
|
|
"context"
|
|
"database/sql"
|
|
"testing"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/lib/pq"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
|
"github.com/coder/coder/v2/coderd/database/dbtime"
|
|
"github.com/coder/coder/v2/coderd/database/migrations"
|
|
)
|
|
|
|
func TestSerializedRetry(t *testing.T) {
|
|
t.Parallel()
|
|
if testing.Short() {
|
|
t.SkipNow()
|
|
}
|
|
|
|
sqlDB := testSQLDB(t)
|
|
db := database.New(sqlDB)
|
|
|
|
called := 0
|
|
txOpts := &database.TxOptions{Isolation: sql.LevelSerializable}
|
|
err := db.InTx(func(tx database.Store) error {
|
|
// Test nested error
|
|
return tx.InTx(func(tx database.Store) error {
|
|
// The easiest way to mock a serialization failure is to
|
|
// return a serialization failure error.
|
|
called++
|
|
return &pq.Error{
|
|
Code: "40001",
|
|
Message: "serialization_failure",
|
|
}
|
|
}, txOpts)
|
|
}, txOpts)
|
|
require.Error(t, err, "should fail")
|
|
// The double "execute transaction: execute transaction" is from the nested transactions.
|
|
// Just want to make sure we don't try 9 times.
|
|
require.Equal(t, err.Error(), "transaction failed after 3 attempts: execute transaction: execute transaction: pq: serialization_failure", "error message")
|
|
require.Equal(t, called, 3, "should retry 3 times")
|
|
}
|
|
|
|
func TestNestedInTx(t *testing.T) {
|
|
t.Parallel()
|
|
if testing.Short() {
|
|
t.SkipNow()
|
|
}
|
|
|
|
uid := uuid.New()
|
|
sqlDB := testSQLDB(t)
|
|
err := migrations.Up(sqlDB)
|
|
require.NoError(t, err, "migrations")
|
|
|
|
db := database.New(sqlDB)
|
|
err = db.InTx(func(outer database.Store) error {
|
|
return outer.InTx(func(inner database.Store) error {
|
|
//nolint:gocritic
|
|
require.Equal(t, outer, inner, "should be same transaction") // intxcheck:ignore // intentional: test asserts nested InTx returns same store
|
|
|
|
_, err := inner.InsertUser(context.Background(), database.InsertUserParams{
|
|
ID: uid,
|
|
Email: "coder@coder.com",
|
|
Username: "coder",
|
|
HashedPassword: []byte{},
|
|
CreatedAt: dbtime.Now(),
|
|
UpdatedAt: dbtime.Now(),
|
|
RBACRoles: []string{},
|
|
LoginType: database.LoginTypeGithub,
|
|
})
|
|
return err
|
|
}, nil)
|
|
}, nil)
|
|
require.NoError(t, err, "outer tx: %w", err)
|
|
|
|
user, err := db.GetUserByID(context.Background(), uid)
|
|
require.NoError(t, err, "user exists")
|
|
require.Equal(t, uid, user.ID, "user id expected")
|
|
}
|
|
|
|
func testSQLDB(t testing.TB) *sql.DB {
|
|
t.Helper()
|
|
|
|
connection, err := dbtestutil.Open(t)
|
|
require.NoError(t, err)
|
|
|
|
db, err := sql.Open("postgres", connection)
|
|
require.NoError(t, err)
|
|
t.Cleanup(func() { _ = db.Close() })
|
|
|
|
return db
|
|
}
|