From 87dc2478a9f6259a68342932625199c6eaaefb8a Mon Sep 17 00:00:00 2001
From: Danny Kopping <danny@coder.com>
Date: Mon, 19 May 2025 16:52:51 +0200
Subject: [PATCH] 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>
---
 scripts/rules.go | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/scripts/rules.go b/scripts/rules.go
index 4e16adad06..4175287567 100644
--- a/scripts/rules.go
+++ b/scripts/rules.go
@@ -134,6 +134,42 @@ func databaseImport(m dsl.Matcher) {
 		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
 // functions that may themselves call t.FailNow in goroutines outside
 // the main test goroutine. See testing.go:834 for why.