fix(connection): say what is holding the pool, not just that it is full - #727
Conversation
Reported from the app: ordinary use filled a five-connection pool, and the error said only that it had reached its limit of five — a number the user had never chosen and could not act on. Their words were "no idea what's going on with the different connections", which is the right complaint about that message. I could not reproduce the exhaustion. `pool_leak_repro.rs` drives the paths ordinary use goes through — connect, list databases, list tables, run a query, stage and roll back a write — against a live MariaDB and watches the pool. Everything comes back: held=2, idle=2, stable over six rounds, and identical to a plain sqlx pool built as a control. So this does not claim to fix the cause. It makes the next occurrence explainable instead of mysterious. The app already tracks which queries are in flight on which connection; the message now uses it: "Unraid" is using all 5 of its connections and none freed up within 10s. 3 queries are still running. Wait for them, or raise "Max pool size" on the profile — a backup, a schema refresh and a query each hold one at the same time. And when nothing of ours is running, which is the shape of an actual leak, it says so and does not offer advice that fits the other case: This app has nothing running on them, which means they are held by work that ended without releasing its connection. Reconnecting clears it. Please report it — that is a bug in SQLPilot, not a setting you have wrong. A `tracing::warn` records the census at the same moment, so a log from the next occurrence names the connection, the count held, and what was running. The test is kept, with the mistake it taught me written into it. My first run read held=2, idle=1 after every operation and I took it for a leak of exactly one connection. It was not: `num_idle` does not update the instant a query ends, so measuring straight after an `await` shows a connection still out. The census now settles before it counts, and the module comment says why, because the false reading is convincing.
EVWorth
left a comment
There was a problem hiding this comment.
Explaining the pool failure is the right direction, and the settle-before-counting note in pool_leak_repro.rs is a good find to keep. One real problem: running counts only editor queries, so the message's "nothing running, this is a bug" case also fires for ordinary backup and schema-refresh concurrency. Details are inline. There's also a small ordering nit on the warn!.
Note that the new repro tests are #[ignore]d live-DB tests, so the green CI result doesn't run them.
Generated by Claude Code
| let running = self | ||
| .in_flight | ||
| .iter() | ||
| .filter(|entry| entry.value() == &connection_id) | ||
| .count(); |
There was a problem hiding this comment.
This only counts work that went through QueryExecutor::execute, and only once its first prelude row arrived (in_flight.insert at the CONNECTION_ID() row). Plenty of other code takes connections from the same pool and is never counted:
SchemaInspector(schema/inspector.rs, everyget_poolcall)- backups (
backup/writer.rs) and restores (restore/runner.rs) mas-admin- direct
get_pooluse insrc/commands/mod.rsandsrc/mcp/workspace.rs
Also, mcp/workspace.rs can be handed a different QueryExecutor, which has its own in_flight map.
So a backup plus a few schema refreshes can fill all five connections with running == 0. The user then sees the "that is a bug in SQLPilot, please report it" message for exactly the ordinary concurrency the other branch of the message describes. That makes the 0 case the least reliable one, even though the PR presents it as the leak signal.
Two options:
- Count every user of the pool, for example with a counter or guard in
ConnectionManageraround acquire, labelled by who took the connection. That also gives thewarn!census real detail. - Keep this count but soften the 0 case to something like "no queries from the editor are running; schema browsing, backups or restores may still hold them", and drop the "bug in SQLPilot" claim until the count covers everything.
Generated by Claude Code
| .iter() | ||
| .filter(|entry| entry.value() == &connection_id) | ||
| .count(); | ||
| if matches!(e, sqlx::Error::PoolTimedOut) { |
There was a problem hiding this comment.
Minor: this warn! fires before describe_pool_error checks held == 0. So a server that went away (empty pool, acquire times out) still logs "Pool ran out of connections" with held=0, the exact confusion a_pool_holding_nothing_has_not_run_out_of_anything guards against in the message. Checking held > 0 here as well keeps the log consistent with the message.
Generated by Claude Code
…r's lane main now gives the agent and backups/restores their own connection lanes (#732), so this PR's running-query count and message are reworked on top of that: - The in-flight map records each query's lane, and the count covers only interactive-lane queries. An agent's queries hold none of the editor's connections and were inflating the count; a live test now shows it. - With no editor query running, the message no longer calls it a bug in SQLPilot outright. It names what else holds these connections (schema browsing, the admin panel) and that reconnecting clears a real leak. - The "Pool ran out of connections" warning only fires when the lane is actually holding connections; an empty one timing out is a server that went away. - Agent and job lanes keep their own messages from #732. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019uLE7rJcohc7WDPstJ7yCL
|
Taking this over to finish it on top of #732 (connection lanes). Pushed 6df54fc, a merge of
Verified on MySQL 8.0 and MariaDB 11: Generated by Claude Code |
Reported from the app: ordinary use filled a five-connection pool, and the
error said only that it had reached its limit of five — a number the user never
chose and couldn't act on. Their words were "no idea what's going on with the
different connections", which is the right complaint about that message.
I could not reproduce it
pool_leak_repro.rsdrives the paths ordinary use takes — connect, listdatabases, list tables, run a query, stage and roll back a write — against a
live MariaDB, watching the pool after each.
So this does not claim to fix the cause. It makes the next occurrence
explainable instead of mysterious.
What it does
The app already tracks which queries are in flight on which connection and
never used it. Now:
And when nothing of ours is running — the actual shape of a leak — it says
something different rather than advising you to wait for work that isn't there:
A
tracing::warnrecords the census at the same moment, so the next occurrenceleaves a log line naming the connection, the count held, and what was running.
Those two messages split the problem in half: concurrency versus a real leak.
The mistake the test now carries
My first run showed
held=2, idle=1after every operation and I read it asa leak of exactly one connection — I was one step from "fixing" the health
watcher. It was a measurement error:
num_idledoesn't update the instant aquery ends, so counting straight after an
awaitshows a connection still out.The census settles before counting, and the module comment says why, because
the false reading is genuinely convincing.
Deliberately not done
Raising the default from 5. It's tight for an app that can hold four
connections legitimately — but raising it now would hide whichever of the two
causes this actually is.
🤖 Generated with Claude Code