feat: allow passing a startup script per connection#352
Conversation
Run optional SQL on every new pooled connection (MySQL/SQLite after_connect, Postgres deadpool post_create) so session settings like set_config apply to every query. Editable via a new "Advanced" tab in the connection editor. Closes TabularisDB#350
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Files Reviewed (3 files)
Previous Issue Resolution
Previous Review Summaries (3 snapshots, latest commit 2186106)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2186106)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 30b5548)Status: No Issues Found | Recommendation: Merge The previous CRITICAL issue has been resolved. Changes verified
Files Reviewed (2 incremental files)
Previous review (commit 14eeac9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (15 files)
Reviewed by kimi-k2.6-20260420 · Input: 50.3K · Output: 5.5K · Cached: 157.6K |
build_connection_key hashed only connection_id/host/tls, so editing a saved connection's startup script reused the cached pool and the new script never ran until app restart. Fold a SHA-256 of the trimmed script into the key (only when set, so script-free pools keep their keys). Addresses the critical review finding on TabularisDB#352.
|
Good catch — fixed in 30b5548.
Added two unit tests: the key changes when the script changes, and a blank/whitespace script is treated as absent (no key fragmentation). |
|
Thanks for this PR. Couple of things before this goes in:
|
A broken startup script now fails fast with a clearly attributed error instead of a misleading pool timeout or generic connection error: - MySQL/SQLite preflight the script on a throwaway connection. sqlx swallows after_connect errors and retries until the acquire timeout, reporting "pool timed out"; the preflight surfaces "Startup script failed: ..." up front. Connection failures are returned verbatim so a bad host/credentials is not mislabelled as a script error. - Postgres post_create reports the startup script as the cause. Also swap the Advanced-tab startup-script textarea for the shared SqlEditorWrapper (Monaco) for syntax highlighting, theming and font settings, matching the editor and other SQL fields.
|
Both addressed in 2186106. 1. Error story. Kept it hard-blocking — for the RLS-bypass case a half-configured session silently serving queries is worse than a clear failure — but made the failure fast and clearly attributed, and distinct from a connectivity error. Worth flagging what I found while doing this: for MySQL/SQLite the old behaviour wasn't actually "bubbles straight out". sqlx swallows
One residual edge: a script that passes preflight but fails non-deterministically on a later pooled connection still hits sqlx's retry → 2. Editor. Swapped the textarea for The SQLite integration test now asserts the error is attributed ( |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| })?; | ||
| let mut pool_options = sqlx::sqlite::SqlitePoolOptions::new().max_connections(5); // SQLite has lower concurrency needs | ||
| if let Some(script) = startup_script(params) { | ||
| run_sqlite_startup_script(&options, &script).await?; |
There was a problem hiding this comment.
WARNING: SQLite startup script preflight lacks timeout
MySQL preflight is wrapped in tokio::time::timeout(connect_timeout, ...) so a hanging script surfaces as a clear timeout error rather than blocking indefinitely. The SQLite preflight has no such guard — a startup script that encounters a database lock or runs an unbounded query would block pool creation forever.
Consider adding a timeout around run_sqlite_startup_script so SQLite behaves consistently with MySQL and users aren't left waiting indefinitely.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 947325b. Wrapped the SQLite preflight in a 30s timeout to match MySQL, so a broken (or stalled-mount/custom-VFS) script can never wedge pool creation. cargo test --lib startup_script still passes and returns immediately.
…meout The MySQL preflight was wrapped in the connect timeout but SQLite's was not. SQLite is file-based so a hang is unlikely, but a custom VFS or a path on a stalled network mount could wedge pool creation indefinitely. Wrap it in a 30s timeout to match MySQL.
2 participants
Closes #350.
Adds an optional startup script to a connection — SQL run on every new pooled connection, so session settings like
SELECT set_config('app.bypass_rls', 'on', false);(or anySET) apply to every query regardless of which pooled connection serves it. This is the DataGrip-style "startup script" requested in the issue, and makes RLS-bypass-in-dev practical.Changes
ConnectionParams.startup_script, persisted with the connection (non-secret).after_connect, Postgres via deadpoolpost_create. Blank/whitespace scripts are skipped; multiple statements are separated by;.Tests
SQLite pool tests (no DB server needed) cover: script runs on connect, blank script is skipped, invalid script surfaces an error.