mirror of
https://github.com/coder/coder.git
synced 2026-06-03 04:58:23 +00:00
ci: add InTx linter replacing ruleguard rule (#24422)
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.
This commit is contained in:
@@ -60,7 +60,7 @@ func TestNestedInTx(t *testing.T) {
|
||||
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")
|
||||
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,
|
||||
|
||||
@@ -274,7 +274,7 @@ func (b WorkspaceBuildBuilder) Do() WorkspaceResponse {
|
||||
err := b.db.InTx(func(tx database.Store) error {
|
||||
//nolint:revive // calls do on modified struct
|
||||
b.db = tx
|
||||
resp = b.doInTX()
|
||||
resp = b.doInTX() // intxcheck:ignore // b.db is reassigned to tx on the line above
|
||||
return nil
|
||||
}, nil)
|
||||
require.NoError(b.t, err)
|
||||
|
||||
@@ -490,7 +490,7 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object
|
||||
}
|
||||
|
||||
if b.templateVersionPresetID == uuid.Nil {
|
||||
presetID, err := prebuilds.FindMatchingPresetID(b.ctx, b.store, templateVersionID, names, values)
|
||||
presetID, err := prebuilds.FindMatchingPresetID(b.ctx, store, templateVersionID, names, values)
|
||||
if err != nil {
|
||||
return BuildError{http.StatusInternalServerError, "find matching preset", err}
|
||||
}
|
||||
@@ -528,7 +528,7 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object
|
||||
return BuildError{code, "insert workspace build", err}
|
||||
}
|
||||
|
||||
task, err := b.getWorkspaceTask()
|
||||
task, err := b.getWorkspaceTask(store)
|
||||
if err != nil {
|
||||
return BuildError{http.StatusInternalServerError, "get task by workspace id", err}
|
||||
}
|
||||
@@ -677,11 +677,11 @@ func (b *Builder) getTemplateVersionID() (uuid.UUID, error) {
|
||||
|
||||
// getWorkspaceTask returns the task associated with the workspace, if any.
|
||||
// If no task exists, it returns (nil, nil).
|
||||
func (b *Builder) getWorkspaceTask() (*database.Task, error) {
|
||||
func (b *Builder) getWorkspaceTask(store database.Store) (*database.Task, error) {
|
||||
if b.hasTask != nil {
|
||||
return b.task, nil
|
||||
}
|
||||
t, err := b.store.GetTaskByWorkspaceID(b.ctx, b.workspace.ID)
|
||||
t, err := store.GetTaskByWorkspaceID(b.ctx, b.workspace.ID)
|
||||
if err != nil {
|
||||
if xerrors.Is(err, sql.ErrNoRows) {
|
||||
b.hasTask = ptr.Ref(false)
|
||||
@@ -1382,7 +1382,7 @@ func (b *Builder) checkUsage() error {
|
||||
return BuildError{http.StatusInternalServerError, "Failed to fetch template version", err}
|
||||
}
|
||||
|
||||
task, err := b.getWorkspaceTask()
|
||||
task, err := b.getWorkspaceTask(b.store)
|
||||
if err != nil {
|
||||
return BuildError{http.StatusInternalServerError, "Failed to fetch workspace task", err}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user