mirror of
https://github.com/coder/coder.git
synced 2025-07-18 14:17:22 +00:00
feat: fail CI when pubsub.Publish
calls are found in db transactions (#17903)
Publishing inside a db transaction can lead to database connection starvation/contention since it requires its own connection. This ruleguard rule (one-shotted by Claude Sonnet 3.7 and finalized by @Emyrk) will detect two of the following 3 instances: ```go type Nested struct { ps pubsub.Pubsub } func TestFail(t *testing.T) { t.Parallel() db, ps := dbtestutil.NewDB(t) nested := &Nested{ ps: ps, } // will catch this _ = db.InTx(func(_ database.Store) error { _, _ = fmt.Printf("") _ = ps.Publish("", []byte{}) return nil }, nil) // will catch this _ = db.InTx(func(_ database.Store) error { _ = nested.ps.Publish("", []byte{}) return nil }, nil) // will NOT catch this _ = db.InTx(func(_ database.Store) error { blah(ps) return nil }, nil) } func blah(ps pubsub.Pubsub) { ps.Publish("", []byte{}) } ``` The ruleguard doesn't recursively introspect function calls so only the first two cases will be guarded against, but it's better than nothing. <img width="1444" alt="image" src="https://github.com/user-attachments/assets/8ffa0d88-16a0-41a9-9521-21211910dec9" /> --------- Signed-off-by: Danny Kopping <dannykopping@gmail.com> Co-authored-by: Steven Masley <stevenmasley@gmail.com>
This commit is contained in:
@ -134,6 +134,42 @@ func databaseImport(m dsl.Matcher) {
|
|||||||
Where(m.File().PkgPath.Matches("github.com/coder/coder/v2/codersdk"))
|
Where(m.File().PkgPath.Matches("github.com/coder/coder/v2/codersdk"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// publishInTransaction detects calls to Publish inside database transactions
|
||||||
|
// which can lead to connection starvation.
|
||||||
|
//
|
||||||
|
//nolint:unused,deadcode,varnamelen
|
||||||
|
func publishInTransaction(m dsl.Matcher) {
|
||||||
|
m.Import("github.com/coder/coder/v2/coderd/database/pubsub")
|
||||||
|
|
||||||
|
// Match direct calls to the Publish method of a pubsub instance inside InTx
|
||||||
|
m.Match(`
|
||||||
|
$_.InTx(func($x) error {
|
||||||
|
$*_
|
||||||
|
$_ = $ps.Publish($evt, $msg)
|
||||||
|
$*_
|
||||||
|
}, $*_)
|
||||||
|
`,
|
||||||
|
// Alternative with short variable declaration
|
||||||
|
`
|
||||||
|
$_.InTx(func($x) error {
|
||||||
|
$*_
|
||||||
|
$_ := $ps.Publish($evt, $msg)
|
||||||
|
$*_
|
||||||
|
}, $*_)
|
||||||
|
`,
|
||||||
|
// Without catching error return
|
||||||
|
`
|
||||||
|
$_.InTx(func($x) error {
|
||||||
|
$*_
|
||||||
|
$ps.Publish($evt, $msg)
|
||||||
|
$*_
|
||||||
|
}, $*_)
|
||||||
|
`).
|
||||||
|
Where(m["ps"].Type.Is("pubsub.Pubsub")).
|
||||||
|
At(m["ps"]).
|
||||||
|
Report("Avoid calling pubsub.Publish() inside database transactions as this may lead to connection deadlocks. Move the Publish() call outside the transaction.")
|
||||||
|
}
|
||||||
|
|
||||||
// doNotCallTFailNowInsideGoroutine enforces not calling t.FailNow or
|
// doNotCallTFailNowInsideGoroutine enforces not calling t.FailNow or
|
||||||
// functions that may themselves call t.FailNow in goroutines outside
|
// functions that may themselves call t.FailNow in goroutines outside
|
||||||
// the main test goroutine. See testing.go:834 for why.
|
// the main test goroutine. See testing.go:834 for why.
|
||||||
|
Reference in New Issue
Block a user