Fix Sequel postgres SQL threading race condition#2739
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a Postgres thread-safety patch to the BOSH Director's DB configuration. It validates Sequel's upstream cached-string behavior via new specs, then adds Config.apply_postgres_thread_safety_patch which, when Sequel::Postgres::Adapter is present, prepends a module overriding execute_query to call String.new(sql).freeze before super. The patch is idempotent, invoked after establishing the DB connection, and safely no-ops if the adapter is absent; unit tests verify SQL copying, freezing, arg forwarding, and idempotency. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bosh-director/lib/bosh/director/config.rb (1)
353-393:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the Postgres thread-safety patch on the active DB adapter (not just
Sequel::Postgres::Adapterbeing loaded)
configure_dbcallsapply_postgres_thread_safety_patchfor every connection, but the patch guard only checksdefined?(Sequel::Postgres::Adapter). If Sequel’s Postgres adapter constant is already loaded (e.g., from earlier requires/tests) while running with a non-Postgres adapter likemysql2, the patch still flips@postgres_thread_safety_patchedand prepends the module incorrectly.Suggested fix
- apply_postgres_thread_safety_patch + apply_postgres_thread_safety_patch(connection_config['adapter']) ... - def apply_postgres_thread_safety_patch + def apply_postgres_thread_safety_patch(adapter_name) + return unless adapter_name == 'postgres' return unless defined?(Sequel::Postgres::Adapter) return if `@postgres_thread_safety_patched`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bosh-director/lib/bosh/director/config.rb` around lines 353 - 393, The patch currently only checks for defined?(Sequel::Postgres::Adapter) which can be true even when the active connection uses a non-Postgres adapter; update apply_postgres_thread_safety_patch so it only flips `@postgres_thread_safety_patched` and prepends the module when the active DB adapter is Postgres (e.g., inspect the actual DB/adapter in use from the connection passed into configure_db or via Sequel.database/Sequel::DATABASES to confirm adapter_scheme == :postgres or database_type == :postgres) before applying the patch; keep the `@postgres_thread_safety_patched` guard but move/extend the check to verify the real adapter (refer to apply_postgres_thread_safety_patch, configure_db, `@postgres_thread_safety_patched`, and Sequel::Postgres::Adapter).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rb`:
- Around line 55-63: Isolate the non-postgres-adapter context by ensuring
Sequel::Postgres::Adapter is undefined when testing
apply_postgres_thread_safety_patch: wrap the examples in a block that uses
RSpec's hide_const("Sequel::Postgres::Adapter") (or temporarily remove/restore
the constant) and reset
described_class.instance_variable_set(:`@postgres_thread_safety_patched`, nil)
before calling described_class.send(:apply_postgres_thread_safety_patch) so the
spec deterministically verifies that the method does not raise and does not set
`@postgres_thread_safety_patched`.
In `@src/bosh-director/spec/unit/bosh/director/sequel_spec.rb`:
- Around line 38-45: The test named 'concurrent callers receive the same
object_id, demonstrating the shared-buffer race' currently calls
dataset.select_sql sequentially; change it to spawn multiple real threads that
each call dataset.select_sql and store the returned String's object_id (use
Thread.new to call dataset.select_sql and push results into a thread-safe array,
then join all threads), then assert ids.uniq.length == 1; reference the existing
dataset variable and the select_sql calls so the concurrent behavior is
exercised across threads rather than sequentially.
---
Outside diff comments:
In `@src/bosh-director/lib/bosh/director/config.rb`:
- Around line 353-393: The patch currently only checks for
defined?(Sequel::Postgres::Adapter) which can be true even when the active
connection uses a non-Postgres adapter; update
apply_postgres_thread_safety_patch so it only flips
`@postgres_thread_safety_patched` and prepends the module when the active DB
adapter is Postgres (e.g., inspect the actual DB/adapter in use from the
connection passed into configure_db or via Sequel.database/Sequel::DATABASES to
confirm adapter_scheme == :postgres or database_type == :postgres) before
applying the patch; keep the `@postgres_thread_safety_patched` guard but
move/extend the check to verify the real adapter (refer to
apply_postgres_thread_safety_patch, configure_db,
`@postgres_thread_safety_patched`, and Sequel::Postgres::Adapter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90d3261e-a9d1-4b54-a5e9-c6f80e3a8e92
📒 Files selected for processing (3)
src/bosh-director/lib/bosh/director/config.rbsrc/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rbsrc/bosh-director/spec/unit/bosh/director/sequel_spec.rb
There was a problem hiding this comment.
Pull request overview
This PR introduces a temporary mitigation in bosh-director for a Sequel/Postgres concurrency issue where a cached mutable SQL String can be mutated across threads and end up being sent to Postgres (e.g., with a (conn: NNNNN) prefix), causing PG::SyntaxError.
Changes:
- Apply a Postgres-specific monkey patch after
Sequel.connectthat wrapsSequel::Postgres::Adapter#execute_queryto passString.new(sql).freeze, ensuring each call uses an isolated buffer. - Add unit specs to validate the patch behavior (copy + freeze, args passthrough, idempotency, and no-op when the adapter isn’t loaded).
- Add specs that pin upstream Sequel
Dataset#select_sqlcaching behavior so future Sequel upgrades surface when the workaround may no longer be needed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/bosh-director/lib/bosh/director/config.rb |
Calls a new apply_postgres_thread_safety_patch from configure_db and prepends a wrapper module onto Sequel::Postgres::Adapter#execute_query. |
src/bosh-director/spec/unit/bosh/director/sequel_spec.rb |
Adds “upstream behavior pinning” specs around Dataset#select_sql caching and a unit-level demonstration of the String.new(sql).freeze pattern. |
src/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rb |
Adds focused tests for patch behavior, idempotency, and safe no-op when the Postgres adapter isn’t defined. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
from
918cd92 to
90a3d56
Compare
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
beyhan
left a comment
beyhan
left a comment
There was a problem hiding this comment.
It looks good to me. I just have the two minor points I commented.
rkoster
left a comment
rkoster
left a comment
There was a problem hiding this comment.
Tests could be better, but probably not worth it, since we intent to remove this code once we bump to ruby 4.x.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bosh-director/lib/bosh/director/config.rb (1)
381-392:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the idempotency check depend on the adapter, not on
Configstate.
Config.clearon Lines 80-92 nils every class ivar, including@postgres_thread_safety_patched, but it does not undo theprependonSequel::Postgres::Adapter. A laterconfigure_dbcall can therefore stack another wrapper onto the same adapter class, so the “apply only once” guarantee is not actually stable across reconfiguration.Suggested fix
+ POSTGRES_THREAD_SAFETY_PATCH = Module.new do + private + + def execute_query(sql, args) + super(String.new(sql).freeze, args) + end + end + def apply_postgres_thread_safety_patch return unless defined?(Sequel::Postgres::Adapter) - return if `@postgres_thread_safety_patched` + return if Sequel::Postgres::Adapter < POSTGRES_THREAD_SAFETY_PATCH - `@postgres_thread_safety_patched` = true - Sequel::Postgres::Adapter.prepend(Module.new do - private - - def execute_query(sql, args) - super(String.new(sql).freeze, args) - end - end) + Sequel::Postgres::Adapter.prepend(POSTGRES_THREAD_SAFETY_PATCH) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bosh-director/lib/bosh/director/config.rb` around lines 381 - 392, The idempotency check currently uses Config instance state (`@postgres_thread_safety_patched`) which is cleared by Config.clear and doesn't prevent repeated prepends on Sequel::Postgres::Adapter; change apply_postgres_thread_safety_patch to create or reference a persistent module (e.g., POSTGRES_THREAD_SAFETY_PATCH = Module.new) and check Sequel::Postgres::Adapter.ancestors.include?(POSTGRES_THREAD_SAFETY_PATCH) (or an equivalent adapter-level marker) before calling Sequel::Postgres::Adapter.prepend, instead of relying on `@postgres_thread_safety_patched`, so the prepend only happens once per adapter regardless of Config.clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/bosh-director/lib/bosh/director/config.rb`:
- Around line 381-392: The idempotency check currently uses Config instance
state (`@postgres_thread_safety_patched`) which is cleared by Config.clear and
doesn't prevent repeated prepends on Sequel::Postgres::Adapter; change
apply_postgres_thread_safety_patch to create or reference a persistent module
(e.g., POSTGRES_THREAD_SAFETY_PATCH = Module.new) and check
Sequel::Postgres::Adapter.ancestors.include?(POSTGRES_THREAD_SAFETY_PATCH) (or
an equivalent adapter-level marker) before calling
Sequel::Postgres::Adapter.prepend, instead of relying on
`@postgres_thread_safety_patched`, so the prepend only happens once per adapter
regardless of Config.clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f88df64c-cdbe-4215-b4e9-7550a07b92d6
📒 Files selected for processing (2)
src/bosh-director/lib/bosh/director/config.rbsrc/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rb
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bosh-director/lib/bosh/director/config.rb (1)
381-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the idempotency check survive
Config.clear.
@postgres_thread_safety_patchedlives onBosh::Director::Config, butclearnils all class ivars atsrc/bosh-director/lib/bosh/director/config.rb:80-92without removing the module already prepended ontoSequel::Postgres::Adapter. After a clear/reconfigure cycle in the same process, this method will prepend another wrapper and keep growing the ancestor chain. Please key the guard off the adapter itself instead (for example, prepend a named module constant and return whenSequel::Postgres::Adapter < ThatModule).Possible fix
+ POSTGRES_THREAD_SAFETY_PATCH = Module.new do + private + + def execute_query(sql, args) + super(String.new(sql).freeze, args) + end + end + def apply_postgres_thread_safety_patch(adapter_name) return unless adapter_name == 'postgres' return unless defined?(Sequel::Postgres::Adapter) - return if `@postgres_thread_safety_patched` + return if Sequel::Postgres::Adapter < POSTGRES_THREAD_SAFETY_PATCH - `@postgres_thread_safety_patched` = true - Sequel::Postgres::Adapter.prepend(Module.new do - private - - def execute_query(sql, args) - super(String.new(sql).freeze, args) - end - end) + Sequel::Postgres::Adapter.prepend(POSTGRES_THREAD_SAFETY_PATCH) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bosh-director/lib/bosh/director/config.rb` around lines 381 - 387, The idempotency guard using `@postgres_thread_safety_patched` can be lost by Config.clear and causes repeated prepends; instead create a named module constant (e.g. POSTGRES_THREAD_SAFETY_PATCH_MODULE) containing the wrapper and in apply_postgres_thread_safety_patch check the adapter itself (return if Sequel::Postgres::Adapter < POSTGRES_THREAD_SAFETY_PATCH_MODULE) before prepending that constant onto Sequel::Postgres::Adapter; replace the instance-variable guard (`@postgres_thread_safety_patched`) with this module-based ancestor check so repeated clear/reconfigure cycles do not re-prepend the wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rb`:
- Around line 58-60: The test uses %w[...] which produces only strings so the
nil branch of apply_postgres_thread_safety_patch is never exercised; change the
test data to include a real nil (e.g. ['mysql2', 'sqlite', nil]) so
described_class.send(:apply_postgres_thread_safety_patch, adapter) is invoked
with adapter == nil and adapter.inspect in the example name still behaves as
expected; update the iterated array in the spec accordingly to cover the
adapter_name == nil case.
---
Outside diff comments:
In `@src/bosh-director/lib/bosh/director/config.rb`:
- Around line 381-387: The idempotency guard using
`@postgres_thread_safety_patched` can be lost by Config.clear and causes repeated
prepends; instead create a named module constant (e.g.
POSTGRES_THREAD_SAFETY_PATCH_MODULE) containing the wrapper and in
apply_postgres_thread_safety_patch check the adapter itself (return if
Sequel::Postgres::Adapter < POSTGRES_THREAD_SAFETY_PATCH_MODULE) before
prepending that constant onto Sequel::Postgres::Adapter; replace the
instance-variable guard (`@postgres_thread_safety_patched`) with this module-based
ancestor check so repeated clear/reconfigure cycles do not re-prepend the
wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba8785d8-7d88-4ead-ac69-d252daf40272
📒 Files selected for processing (2)
src/bosh-director/lib/bosh/director/config.rbsrc/bosh-director/spec/unit/bosh/director/postgres_thread_safety_patch_spec.rb
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
2 times, most recently
from
8aa4a65 to
cab9619
Compare
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
from
cab9619 to
96bacea
Compare
aramprice
dismissed
rkoster’s stale review
internal ticket id's removed from code
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
2 times, most recently
from
9a64f25 to
6c15687
Compare
|
|
aramprice
left a comment
aramprice
left a comment
There was a problem hiding this comment.
EasyCLA appears confused by the git-bot co-signing on the latest commit.
We can override, or you can re-push the commit without any git-bot attribution, and that should fix it.
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
/easycla |
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
2 times, most recently
from
13fe959 to
91594eb
Compare
Sequel's Dataset#select_sql caches and returns the same mutable String object to all concurrent callers. Under high concurrency, log_connection_yield prepends "(conn: NNNNN)" in-place to that shared buffer, which is then sent verbatim to Postgres as SQL: PG::SyntaxError: ERROR: syntax error at or near "conn" Add apply_postgres_thread_safety_patch to Config#configure_db, called after Sequel.connect when the postgres adapter is loaded. The patch prepends a module onto Sequel::Postgres::Adapter that wraps execute_query to pass String.new(sql).freeze to super, giving each call its own frozen independent C-level string buffer so concurrent PQexec calls cannot interfere. An idempotency guard prevents double-application if configure_db is called multiple times during startup. Specs pin the upstream Sequel caching behaviour so any gem upgrade that fixes the issue upstream will alert us to remove the patch. Unit tests cover: String.new.freeze isolation, args passthrough, idempotency, and no-op for non-postgres adapters. We have a threading test that exposes the issue. The corruption occurs in the form of SQL the query string being mutated before being sent to Postgres. Examples of the mutated thread are this string being sent to Postgres (conn: 152) SELECT * FROM "bug_templates" INNER JOIN "join_r... The string corruption occurs in all of the tested 3.3 ruby releases; 3.3.9, 3.3.11, 3.4.9 The string corruption does not occur in ruby 4.0.5 This patch will be short lived until the bump to ruby 4.0 There is a test that fails intentionally once the runtime is upgraded, prompting removal of Config#apply_postgres_thread_safety_patch and its call site. postgres_thread_safety_patch_spec is DB-agnostic [TNZ-104200, TNZ-103317] ai-assisted=yes
colins
force-pushed
the
TNZ-104200-sequel-threading-mitigation
branch
from
91594eb to
7263d8c
Compare
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
Sequel's Dataset#select_sql caches and returns the same mutable String object to all concurrent callers. Under high concurrency, log_connection_yield prepends "(conn: NNNNN)" in-place to that shared buffer, which is then sent verbatim to Postgres as SQL:
PG::SyntaxError: ERROR: syntax error at or near "conn"
Add apply_postgres_thread_safety_patch to Config#configure_db, called after Sequel.connect when the postgres adapter is loaded. The patch prepends a module onto Sequel::Postgres::Adapter that wraps execute_query to pass String.new(sql).freeze to super, giving each call its own frozen independent C-level string buffer so concurrent PQexec calls cannot interfere.
An idempotency guard prevents double-application if configure_db is called multiple times during startup.
Specs pin the upstream Sequel caching behaviour so any gem upgrade that fixes the issue upstream will alert us to remove the patch. Unit tests cover: String.new.freeze isolation, args passthrough, idempotency, and no-op for non-postgres adapters.
We have a threading test that exposes the issue. The corruption occurs in the form of SQL the query string being mutated before being sent to Postgres. Examples of the mutated thread are this string being sent to Postgres
(conn: 152) SELECT * FROM "bug_templates" INNER JOIN "join_r...
The string corruption occurs in all of the tested 3.3 ruby release; 3.3.9, 3.3.11, 3.4.9
The string corruption does not occur in ruby 4.0.5
This patch will be short lived until the bump to ruby 4.0
[TNZ-104200, TNZ-103317]
ai-assisted=yes
What is this change about?
A customer reported Postgres failures caused by a syntax error in the received SQL string. This seems to have
been caused by a concurrency problem that was mutating a string. This patches the symptom of the failure.
The long term fix is to bumnp ruby to 4.0+
Please provide contextual information.
We created an issue with the sequel gem but it appears that ruby is the real cuplrit.
jeremyevans/sequel#2368
What tests have you run against this PR?
cd src/spec
bundle exec rake spec:unit:parallel
How should this change be described in bosh release notes?
A temporary fix to mitigate a string corruption race condition that only affects the ruby 3.3 releases
Does this PR introduce a breaking change?
No
Tag your pair, your PM, and/or team!
@aramprice