From 20f76bab0087a7ccb44d86efb57b89902e093ad5 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Fri, 16 May 2025 17:02:25 +0200 Subject: [PATCH 1/8] feat: ruleguard to prevent pubsub.Publish in tx Signed-off-by: Danny Kopping --- scripts/rules.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/scripts/rules.go b/scripts/rules.go index 4e16adad06a87..ca0a4be5eaea0 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -134,6 +134,38 @@ 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 deadlocks or other unexpected behavior. +// +//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(` + $db.InTx(func($tx $dbType) $retType { + $*_ + $ps.Publish($*args) + $*_ + }, $*txopts) + `). + Where(m["ps"].Type.Implements("pubsub.Pubsub") || + m["ps"].Text.Matches(`\w+\.pubsub`) || + m["ps"].Text.Matches(`pubsub\.\w+`)). + Report("Avoid calling Publish inside database transactions as this may lead to connection deadlocks. Move the Publish call outside the transaction.") + + // Also catch publish calls on nested fields like c.pubsub.Publish() + m.Match(` + $db.InTx(func($tx $dbType) $retType { + $*_ + $ps.$field.Publish($*args) + $*_ + }, $*txopts) + `). + Where(m["field"].Text == "pubsub"). + Report("Avoid calling 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. From fe31f5d834c9bd940909fdccdd6ee840495ee03f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 16 May 2025 11:10:14 -0500 Subject: [PATCH 2/8] use 'Is' instead of 'Implements' --- scripts/rules.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index ca0a4be5eaea0..738520f401131 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -143,26 +143,22 @@ func publishInTransaction(m dsl.Matcher) { // Match direct calls to the Publish method of a pubsub instance inside InTx m.Match(` - $db.InTx(func($tx $dbType) $retType { + $x.InTx(func($y) error { $*_ - $ps.Publish($*args) + $_ = $ps.Publish($evt, $msg) $*_ - }, $*txopts) - `). - Where(m["ps"].Type.Implements("pubsub.Pubsub") || - m["ps"].Text.Matches(`\w+\.pubsub`) || - m["ps"].Text.Matches(`pubsub\.\w+`)). - Report("Avoid calling Publish inside database transactions as this may lead to connection deadlocks. Move the Publish call outside the transaction.") - - // Also catch publish calls on nested fields like c.pubsub.Publish() - m.Match(` - $db.InTx(func($tx $dbType) $retType { + }, $*_) + `, + // Without catching error return + ` + $x.InTx(func($y) error { $*_ - $ps.$field.Publish($*args) + $ps.Publish($evt, $msg) $*_ - }, $*txopts) + }, $*_) `). - Where(m["field"].Text == "pubsub"). + Where(m["ps"].Type.Is("pubsub.Pubsub")). + At(m["ps"]). Report("Avoid calling Publish inside database transactions as this may lead to connection deadlocks. Move the Publish call outside the transaction.") } From 0def3119568c1ad5f8973843e81f51f5debeba28 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 11:11:04 +0200 Subject: [PATCH 3/8] chore: touch up error Signed-off-by: Danny Kopping --- scripts/rules.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index 738520f401131..4ebc1b209aa27 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -135,7 +135,7 @@ func databaseImport(m dsl.Matcher) { } // publishInTransaction detects calls to Publish inside database transactions -// which can lead to connection deadlocks or other unexpected behavior. +// which can lead to connection starvation. // //nolint:unused,deadcode,varnamelen func publishInTransaction(m dsl.Matcher) { @@ -159,7 +159,7 @@ func publishInTransaction(m dsl.Matcher) { `). Where(m["ps"].Type.Is("pubsub.Pubsub")). At(m["ps"]). - Report("Avoid calling Publish inside database transactions as this may lead to connection deadlocks. Move the Publish call outside the transaction.") + 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 From 50d26fecee07d965a79b743c9a16c1a47c0ffa56 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 13:43:39 +0200 Subject: [PATCH 4/8] chore: add short var declaration Signed-off-by: Danny Kopping --- scripts/rules.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/rules.go b/scripts/rules.go index 4ebc1b209aa27..dd37ec490be49 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -148,6 +148,14 @@ func publishInTransaction(m dsl.Matcher) { $_ = $ps.Publish($evt, $msg) $*_ }, $*_) + `, + // Alternative with short variable declaration + ` + $x.InTx(func($y) error { + $*_ + $_ := $ps.Publish($evt, $msg) + $*_ + }, $*_) `, // Without catching error return ` From cd326adfdf4d642b986b64f3b8de8de2f87cbfd0 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 14:37:18 +0200 Subject: [PATCH 5/8] chore: make fmt Signed-off-by: Danny Kopping --- scripts/rules.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index dd37ec490be49..2e483bef83f58 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -149,8 +149,8 @@ func publishInTransaction(m dsl.Matcher) { $*_ }, $*_) `, - // Alternative with short variable declaration - ` + // Alternative with short variable declaration + ` $x.InTx(func($y) error { $*_ $_ := $ps.Publish($evt, $msg) From b5fb079eee248ce65f726fe8dfc4e681d20fe2fd Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 16:26:44 +0200 Subject: [PATCH 6/8] Update scripts/rules.go Co-authored-by: Steven Masley --- scripts/rules.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/rules.go b/scripts/rules.go index 2e483bef83f58..7ef4635536444 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -143,7 +143,7 @@ func publishInTransaction(m dsl.Matcher) { // Match direct calls to the Publish method of a pubsub instance inside InTx m.Match(` - $x.InTx(func($y) error { + $_.InTx(func($_) error { $*_ $_ = $ps.Publish($evt, $msg) $*_ From 5db1da3fbfd229b1788c96bdf39a74294702a873 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 16:28:19 +0200 Subject: [PATCH 7/8] Revert "Update scripts/rules.go" This reverts commit b5fb079eee248ce65f726fe8dfc4e681d20fe2fd. --- scripts/rules.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/rules.go b/scripts/rules.go index 7ef4635536444..2e483bef83f58 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -143,7 +143,7 @@ func publishInTransaction(m dsl.Matcher) { // Match direct calls to the Publish method of a pubsub instance inside InTx m.Match(` - $_.InTx(func($_) error { + $x.InTx(func($y) error { $*_ $_ = $ps.Publish($evt, $msg) $*_ From 6e681d5994b1ef36accc056a27d655175af01e93 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Mon, 19 May 2025 16:32:18 +0200 Subject: [PATCH 8/8] chore: drop unnecessary identifiers Signed-off-by: Danny Kopping --- scripts/rules.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index 2e483bef83f58..4175287567502 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -143,7 +143,7 @@ func publishInTransaction(m dsl.Matcher) { // Match direct calls to the Publish method of a pubsub instance inside InTx m.Match(` - $x.InTx(func($y) error { + $_.InTx(func($x) error { $*_ $_ = $ps.Publish($evt, $msg) $*_ @@ -151,7 +151,7 @@ func publishInTransaction(m dsl.Matcher) { `, // Alternative with short variable declaration ` - $x.InTx(func($y) error { + $_.InTx(func($x) error { $*_ $_ := $ps.Publish($evt, $msg) $*_ @@ -159,7 +159,7 @@ func publishInTransaction(m dsl.Matcher) { `, // Without catching error return ` - $x.InTx(func($y) error { + $_.InTx(func($x) error { $*_ $ps.Publish($evt, $msg) $*_