Fixed infinite loading after idle connection loss on Linux Desktop. #6308#9800
Fixed infinite loading after idle connection loss on Linux Desktop. #6308#9800ZoroXL wants to merge 4 commits intopgadmin-org:masterfrom
Conversation
WalkthroughAdds a server-side liveness check Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
480-517:⚠️ Potential issue | 🔴 CriticalThis visibility handler closes over stale Query Tool state, preventing the connection status check from running properly.
Inside a mount-only
useEffect, the listener capturesqtStateonce and never sees its updates. Sinceconnected_oncestarts asfalse, the condition at line 490 (if(qtState.params?.trans_id && qtState.connected_once)) will never execute even after the connection succeeds. Additionally, if the connection switches later, the listener will use a staletrans_idfrom mount time rather than the updated one.The listener also lacks cleanup, creating a memory leak.
Use a ref to access current state:
♻️ Suggested fix
+ const qtStateRef = useRef(qtState); + useEffect(() => { + qtStateRef.current = qtState; + }, [qtState]); + useEffect(()=>{ getSQLScript(); initializeQueryTool(); @@ - document.addEventListener('visibilitychange', function() { + const onVisibilityChange = function() { if(document.hidden) { setQtStatePartial({is_visible: false}); } else { setQtStatePartial({is_visible: true}); @@ - if(qtState.params?.trans_id && qtState.connected_once) { - fetchConnectionStatus(api, qtState.params.trans_id) + const {params, connected_once} = qtStateRef.current; + if(params?.trans_id && connected_once) { + fetchConnectionStatus(api, params.trans_id) .then(({data: respData}) => { if(respData.data) { setQtStatePartial({ connected: true, connection_status: respData.data.status, @@ - }); + }; + document.addEventListener('visibilitychange', onVisibilityChange); + return ()=>document.removeEventListener('visibilitychange', onVisibilityChange); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 480 - 517, The visibilitychange listener closes over stale qtState and never cleans up; change the effect to store current state in a ref (e.g., qtStateRef) that you update whenever qtState changes, then inside the mount-only useEffect have the listener read qtStateRef.current (instead of qtState) to check params?.trans_id and connected_once and call fetchConnectionStatus, use parseApiError for errors as before, and add cleanup by removing the same event listener on unmount; keep setQtStatePartial and response handling unchanged but reference qtStateRef.current for up-to-date values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 491-505: The status-response handler currently drops any pending
notifications; update the fetchConnectionStatus(...).then handler so it forwards
respData.data.notifies to the same notification path used elsewhere instead of
discarding them — e.g., when handling the successful branch (respData.data) and
the error branch, check for respData.data.notifies and dispatch them via the
existing mechanism (the same way the poller pushes into
QUERY_TOOL_EVENTS.PUSH_NOTICE) or call the existing notifier helper so
notifications received for qtState.params.trans_id are preserved; modify the
block around fetchConnectionStatus, setQtStatePartial, and
respData.data.notifies to forward notifies accordingly.
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 1416-1439: Add a new abstract method connection_ping(self) to the
BaseConnection class in abstract.py to formalize the contract (matching the
psycopg3 implementation's signature) so callers like the code that invokes
connection_ping() on connection objects won't get AttributeError; then update
any driver implementations that lack it to implement connection_ping (e.g.,
ensure the psycopg3 connection class's connection_ping semantics—execute
lightweight "SELECT 1", close on failure and return bool—are mirrored or
appropriately adapted in other drivers/test doubles).
---
Outside diff comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 480-517: The visibilitychange listener closes over stale qtState
and never cleans up; change the effect to store current state in a ref (e.g.,
qtStateRef) that you update whenever qtState changes, then inside the mount-only
useEffect have the listener read qtStateRef.current (instead of qtState) to
check params?.trans_id and connected_once and call fetchConnectionStatus, use
parseApiError for errors as before, and add cleanup by removing the same event
listener on unmount; keep setQtStatePartial and response handling unchanged but
reference qtStateRef.current for up-to-date values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f2a8fb2-0dc9-4321-95b9-48fec6a970b6
📒 Files selected for processing (5)
web/pgadmin/browser/utils.pyweb/pgadmin/static/js/tree/tree_nodes.tsweb/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsxweb/pgadmin/utils/driver/psycopg3/connection.pyweb/pgadmin/utils/driver/psycopg3/server_manager.py
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
Outdated
Show resolved
Hide resolved
| def connection_ping(self): | ||
| """ | ||
| Check if the connection is actually alive by executing a lightweight | ||
| query. Unlike connected(), which only inspects local state, this | ||
| sends traffic to the server and will detect stale / half-open TCP | ||
| connections that were silently dropped by firewalls or the OS while | ||
| pgAdmin was idle. | ||
|
|
||
| Returns True if alive, False otherwise. | ||
| """ | ||
| if not self.connected(): | ||
| return False | ||
| try: | ||
| cur = self.conn.cursor() | ||
| cur.execute("SELECT 1") | ||
| cur.close() | ||
| return True | ||
| except Exception: | ||
| try: | ||
| self.conn.close() | ||
| except Exception: | ||
| pass | ||
| self.conn = None | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Base contract:"
rg -n -C2 'def ping\(|def connection_ping\(' web/pgadmin/utils/driver/abstract.py
echo
echo "Call sites:"
rg -n -C2 'connection_ping\(' web/pgadmin/browser/utils.py web/pgadmin/utils/driver
echo
echo "Implementations:"
rg -n -C2 'def connection_ping\(' web/pgadmin/utils/driverRepository: pgadmin-org/pgadmin4
Length of output: 1836
🏁 Script executed:
fd -t f 'connection.py' web/pgadmin/utils/driverRepository: pgadmin-org/pgadmin4
Length of output: 111
🏁 Script executed:
fd -type d web/pgadmin/utils/driverRepository: pgadmin-org/pgadmin4
Length of output: 234
🏁 Script executed:
fd --type d --maxdepth 1 '' web/pgadmin/utils/driverRepository: pgadmin-org/pgadmin4
Length of output: 98
🏁 Script executed:
ls -la web/pgadmin/utils/driver/Repository: pgadmin-org/pgadmin4
Length of output: 416
🏁 Script executed:
head -50 web/pgadmin/utils/driver/psycopg3/connection.py | grep -E "^import|^from|class.*\("Repository: pgadmin-org/pgadmin4
Length of output: 1035
🏁 Script executed:
rg -n "^class.*BaseConnection" web/pgadmin/utils/driver/psycopg3/connection.pyRepository: pgadmin-org/pgadmin4
Length of output: 100
🏁 Script executed:
rg -n "class.*BaseConnection" web/pgadmin/utils/driver/Repository: pgadmin-org/pgadmin4
Length of output: 296
🏁 Script executed:
rg -n "def connection\(" web/pgadmin/browser/utils.py -A 5 | head -20Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
rg -n "manager.*=" web/pgadmin/browser/utils.py | head -10Repository: pgadmin-org/pgadmin4
Length of output: 236
🏁 Script executed:
rg -n "PG_DEFAULT_DRIVER" web/pgadmin/browser/utils.pyRepository: pgadmin-org/pgadmin4
Length of output: 175
🏁 Script executed:
rg -n "PG_DEFAULT_DRIVER\s*=" config.py 2>/dev/null | head -5Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
fd config.py | head -5Repository: pgadmin-org/pgadmin4
Length of output: 304
🏁 Script executed:
rg -n "PG_DEFAULT_DRIVER" web/config.pyRepository: pgadmin-org/pgadmin4
Length of output: 98
🏁 Script executed:
rg -n -B5 -A10 "connection_ping\(\)" web/pgadmin/browser/utils.pyRepository: pgadmin-org/pgadmin4
Length of output: 1042
🏁 Script executed:
rg -n "connection_ping" web/pgadmin --type py | grep -E "mock|Mock|patch|test"Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
rg -n "connection_ping" web/pgadminRepository: pgadmin-org/pgadmin4
Length of output: 325
connection_ping() must be added to the BaseConnection abstract contract.
Currently, web/pgadmin/browser/utils.py:441 calls connection_ping() on the connection object without this method being defined in web/pgadmin/utils/driver/abstract.py. While only psycopg3 is currently deployed, this violates the driver abstraction contract and would cause AttributeError if any alternate driver or test double were introduced.
Add the abstract method to BaseConnection in abstract.py and ensure all driver implementations provide it.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 1433-1433: Do not catch blind exception: Exception
(BLE001)
[error] 1436-1437: try-except-pass detected, consider logging the exception
(S110)
[warning] 1436-1436: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 1416 - 1439,
Add a new abstract method connection_ping(self) to the BaseConnection class in
abstract.py to formalize the contract (matching the psycopg3 implementation's
signature) so callers like the code that invokes connection_ping() on connection
objects won't get AttributeError; then update any driver implementations that
lack it to implement connection_ping (e.g., ensure the psycopg3 connection
class's connection_ping semantics—execute lightweight "SELECT 1", close on
failure and return bool—are mirrored or appropriately adapted in other
drivers/test doubles).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/pgadmin/utils/driver/abstract.py (1)
210-217: Clarifyconnection_ping()vsping()contract in the base API.Line 210 adds
connection_ping(), butping()(Line 226) remains and the class-level abstract-method docs only describeping(). Please explicitly document the behavioral split to prevent inconsistent implementations across drivers.Proposed doc clarification
- * ping() - - Implement this method to ping the server. There are times, a connection - has been lost, but - the connection driver does not know about it. This - can be helpful to figure out the actual reason for query failure. + * connection_ping() + - Implement a lightweight liveness probe that sends traffic to the server + (e.g., simple query) to detect stale/half-open TCP connections. + + * ping() + - Implement this method to ping the server for existing behavior/callers. + If both methods exist, document when each one should be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/driver/abstract.py` around lines 210 - 217, Update the abstract base docs to explicitly describe the behavioral contract split between connection_ping() and ping(): state that connection_ping() must perform a lightweight network-level check that returns True/False only for connection aliveness (no side effects), while ping() may perform higher-level application/DB checks and should raise exceptions on failure or return driver-specific status; update the class docstring and the docstrings on the two abstract methods (connection_ping and ping) to reflect this, include expectations about return types and error handling so driver implementations of connection_ping() and ping() are unambiguous.web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
493-518: Pull the connection-status refresh into one helper.This block and the interval poller above already do the same fetch/update/notify work in two places. A shared
refreshConnectionStatus()will make future reconnect fixes much less likely to land in only one path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 493 - 518, Extract the duplicate fetch/update/notify logic into a single helper function, e.g. refreshConnectionStatus(api, trans_id), that calls fetchConnectionStatus(api, trans_id), updates state via setQtStatePartial({connected, connection_status, connection_status_msg}) and fires notifications via eventBus.current.fireEvent(QUERY_TOOL_EVENTS.PUSH_NOTICE, ...); then replace the inline block and the interval poller call with a call to refreshConnectionStatus(api, params.trans_id) and ensure the catch path still uses parseApiError(error) to set connection_status_msg and logs errors consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 522-523: The effect currently only removes the document
'visibilitychange' listener but leaves the layout listeners attached; capture
the callback references you pass into qtPanelDocker.eventBus.registerListener
for LAYOUT_EVENTS.CLOSING and LAYOUT_EVENTS.ACTIVE (e.g. name them
onLayoutClosing and onLayoutActive), and in the effect cleanup call the
corresponding unregister/deregister method on qtPanelDocker.eventBus to remove
those listeners (along with the existing
document.removeEventListener(onVisibilityChange)), ensuring you check
qtPanelDocker exists before unregistering so no stale closures remain after
unmount.
---
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 493-518: Extract the duplicate fetch/update/notify logic into a
single helper function, e.g. refreshConnectionStatus(api, trans_id), that calls
fetchConnectionStatus(api, trans_id), updates state via
setQtStatePartial({connected, connection_status, connection_status_msg}) and
fires notifications via
eventBus.current.fireEvent(QUERY_TOOL_EVENTS.PUSH_NOTICE, ...); then replace the
inline block and the interval poller call with a call to
refreshConnectionStatus(api, params.trans_id) and ensure the catch path still
uses parseApiError(error) to set connection_status_msg and logs errors
consistently.
In `@web/pgadmin/utils/driver/abstract.py`:
- Around line 210-217: Update the abstract base docs to explicitly describe the
behavioral contract split between connection_ping() and ping(): state that
connection_ping() must perform a lightweight network-level check that returns
True/False only for connection aliveness (no side effects), while ping() may
perform higher-level application/DB checks and should raise exceptions on
failure or return driver-specific status; update the class docstring and the
docstrings on the two abstract methods (connection_ping and ping) to reflect
this, include expectations about return types and error handling so driver
implementations of connection_ping() and ping() are unambiguous.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d89f1f51-1f05-47bc-bda8-1564281721ae
📒 Files selected for processing (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsxweb/pgadmin/utils/driver/abstract.py
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
Outdated
Show resolved
Hide resolved
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)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
468-509:⚠️ Potential issue | 🟡 MinorCancel the debounced ACTIVE handler during cleanup.
On Lines 468-509,
deregisterListener(...)stops futureLAYOUT_EVENTS.ACTIVEdeliveries, but a trailing_.debounce(...)call that was already queued can still run after unmount and invokesetQtStatePartial(...)/docker.current.focus(...)on a stale instance. CallonLayoutActive.cancel()in the cleanup too.🧹 Minimal fix
return ()=>{ document.removeEventListener('visibilitychange', onVisibilityChange); + onLayoutActive.cancel(); if(qtPanelDocker?.eventBus) { qtPanelDocker.eventBus.deregisterListener(LAYOUT_EVENTS.CLOSING, onLayoutClosing); qtPanelDocker.eventBus.deregisterListener(LAYOUT_EVENTS.ACTIVE, onLayoutActive); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 468 - 509, The debounced ACTIVE handler onLayoutActive can still run after the component unmounts causing stale setQtStatePartial/docker.current.focus calls; in the cleanup where you remove the visibility listener and deregister event listeners (deregisterListener for LAYOUT_EVENTS.ACTIVE), also call onLayoutActive.cancel() to cancel any pending debounced invocation of onLayoutActive so it cannot run after unmount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 487-500: The document visibility handler onVisibilityChange
currently marks every Query Tool instance visible and triggers
refreshConnectionStatus for all tabs; change it to only set is_visible and call
refreshConnectionStatus when this component is the active panel: check the
current active panel id (the same source used for LAYOUT_EVENTS.ACTIVE) against
this component's qtPanelId before setting is_visible true or calling
refreshConnectionStatus, leaving inactive instances untouched; use
qtStateRef.current, params.trans_id, connected_once and refreshConnectionStatus
as before but gated by the active-panel check and do not rely on global document
visibility alone to resume polling.
---
Outside diff comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 468-509: The debounced ACTIVE handler onLayoutActive can still run
after the component unmounts causing stale
setQtStatePartial/docker.current.focus calls; in the cleanup where you remove
the visibility listener and deregister event listeners (deregisterListener for
LAYOUT_EVENTS.ACTIVE), also call onLayoutActive.cancel() to cancel any pending
debounced invocation of onLayoutActive so it cannot run after unmount.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2473c38-20fb-46d9-a369-ee8900c1a528
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
| const onVisibilityChange = function() { | ||
| if(document.hidden) { | ||
| setQtStatePartial({is_visible: false}); | ||
| } else { | ||
| setQtStatePartial({is_visible: true}); | ||
| // When the tab becomes visible again after being hidden (e.g. user | ||
| // switched away on Linux Desktop), immediately check the connection | ||
| // status. This ensures a dead connection is detected right away | ||
| // instead of waiting for the next poll interval, which was disabled | ||
| // while the tab was hidden. | ||
| const {params, connected_once} = qtStateRef.current; | ||
| if(params?.trans_id && connected_once) { | ||
| refreshConnectionStatus(params.trans_id); | ||
| } |
There was a problem hiding this comment.
Don't mark every Query Tool instance visible on window focus.
On Lines 487-500, the document-level listener unconditionally flips is_visible back to true and runs refreshConnectionStatus(...) whenever the window becomes visible again. If multiple Query Tool tabs are open, the inactive ones will also resume polling until another LAYOUT_EVENTS.ACTIVE arrives. Please keep layout-active state separate here and only re-enable polling for the active qtPanelId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`
around lines 487 - 500, The document visibility handler onVisibilityChange
currently marks every Query Tool instance visible and triggers
refreshConnectionStatus for all tabs; change it to only set is_visible and call
refreshConnectionStatus when this component is the active panel: check the
current active panel id (the same source used for LAYOUT_EVENTS.ACTIVE) against
this component's qtPanelId before setting is_visible true or calling
refreshConnectionStatus, leaving inactive instances untouched; use
qtStateRef.current, params.trans_id, connected_once and refreshConnectionStatus
as before but gated by the active-panel check and do not rely on global document
visibility alone to resume polling.
1 participant
When pgAdmin is left idle for an extended period on Linux Desktop, the database connection gets silently dropped. Instead of showing the reconnect dialog, the application hangs with an infinite spinner.
Fixes:
connection_ping()to detect stale/half-open TCP connections thatconnected()misses (it only checks local state).connection_ping()in the Object Explorerchildren()endpoint to detect dead connections before they cause a hang.Refs #6308, #9700, #8279
Summary by CodeRabbit
New Features
Bug Fixes