-
Notifications
You must be signed in to change notification settings - Fork 887
feat: fail CI when pubsub.Publish
calls are found in db transactions
#17903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
214d648
to
0def311
Compare
I definitely think it's a good idea, but my ruleguard-fu is pretty weak. @johnstcn do you mind taking a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also need to match the inline assignment case e.g. if err := ps.Publish(...)
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Correct, the best you can attempt to do is catch passing $x.InTx(func($y) error {
$*_
$_($*_ $ps, $*_)
$*_
}, $*_) There will always be holes 😢. |
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
This reverts commit b5fb079.
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
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:
The ruleguard doesn't recursively introspect function calls so only the first two cases will be guarded against, but it's better than nothing.