fix(dtwin): bound and offload graph reads so Graph Chat can't hang the app (#114)#116
Conversation
|
Fiifi Botchway seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This PR hardens Graph Chat and other dtwin graph-read endpoints against app-wide hangs by (1) offloading blocking graph DB work off the single uvicorn event loop and (2) bounding graph reads (time + result size) so runaway traversals get cancelled per-request instead of pinning pooled DB sessions indefinitely. It also adds an admin-tunable UI/API for these bounds and surfaces a “resource pressure” advisory when the blocking pool saturates.
Changes:
- Added centralized graph-read limit resolution (
statement_timeout+ Graph Chat result cap) with env/admin override precedence and clamping. - Wrapped dtwin graph-read routes (GraphQL/triples/neighbors/stats) in
run_blocking, plus worker-pool auto-sizing + saturation telemetry for UI advisories. - Added admin-only Settings endpoints + UI fields to view/save the bounds; updated the dtwin chat agent’s loopback timeout to track the server-side statement timeout.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/back/core/query_limits.py |
New resolver for graph read timeout + Graph Chat result cap (override/env/default + clamp). |
src/back/core/graphdb/lakebase/LakebaseFlatStore.py |
Applies Postgres statement_timeout for Lakebase graph reads. |
src/back/core/graphdb/lakebase/LakebaseBase.py |
Clears statement_timeout for DDL/migration cursor paths on reused pooled connections. |
src/back/core/databricks/SQLWarehouse.py |
Adds optional statement_timeout_s bracket + reset for bounded warehouse queries. |
src/back/core/triplestore/delta/DeltaTripleStore.py |
Routes bounded graph reads through SQLWarehouse.execute_query(..., statement_timeout_s=...). |
src/back/core/helpers/DatabricksHelpers.py |
Auto-sizes blocking executor, tracks in-flight tasks, exposes saturation stats. |
src/back/core/helpers/__init__.py |
Re-exports get_blocking_pool_stats. |
src/api/routers/internal/dtwin.py |
Offloads blocking graph operations to thread pool; adds resource-pressure payload to chat responses; caps triples/find limit. |
src/api/routers/internal/settings.py |
Adds GET /settings/graph-limits and POST /settings/save-graph-limits. |
src/back/objects/session/GlobalConfigService.py |
Persists/applies graph timeout + cap overrides in global config; adds get/set helpers. |
src/back/objects/domain/SettingsService.py |
Adds SettingsService API for get/save of graph limits (admin-gated). |
src/front/templates/settings.html |
Adds admin-only “Graph read limits” inputs to Settings UI. |
src/front/static/config/js/settings.js |
Loads/saves graph limits via new settings endpoints. |
src/front/static/query/js/query-chat.js |
Displays resource-pressure advisory (inline + toast) when server flags saturation. |
src/agents/agent_dtwin_chat/tools.py |
Derives loopback HTTP timeout from server-side graph timeout + margin. |
tests/units/core/test_query_limits.py |
Unit tests for limit resolution order + clamping. |
tests/units/core/test_graph_query_bounds.py |
Regression tests for bounded warehouse reads + pool auto-tune + saturation flag. |
docs/issues/graph-chat-event-loop-hang.md |
Written incident-style doc for root cause, tradeoffs, and fix approach. |
changelogs/v0.6.1/briancastelino_2026-07-09.log |
Changelog entry documenting the resilience fix and touched files/tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Bound graph reads so a runaway traversal is cancelled | ||
| # server-side instead of pinning the connection (and, via the | ||
| # loopback agent, the event loop). Reset by ``_cursor`` for the | ||
| # DDL path which may legitimately run longer. | ||
| cur.execute(f"SET statement_timeout = {int(timeout_ms)}") |
There was a problem hiding this comment.
Good catch - confirmed. The pool hands out autocommit=True connections, so this was a session GUC that persisted to the next borrower. Fixed in 4375e88: the read is now wrapped in try/finally with RESET statement_timeout, so the bound is scoped to this read only. Safe in autocommit - a statement_timeout cancel leaves no open transaction to abort, so the RESET runs cleanly on both the success and error paths. Added TestLakebaseStatementTimeoutReset covering both paths.
| def _opt_int(key: str): | ||
| if key not in data or data[key] is None or data[key] == "": | ||
| return None | ||
| return int(data[key]) |
There was a problem hiding this comment.
Fixed in 4375e88. _opt_int now wraps the int() in try/except (TypeError, ValueError) and raises ValidationError(f"{key} must be an integer") from exc, matching the error-handling convention already used elsewhere in this router.
| from back.core.query_limits import get_graph_query_timeout_s as _effective | ||
|
|
||
| val = self.get(host, token, registry_cfg, "graph_query_timeout_s", "") | ||
| if val and str(val).isdigit() and int(val) > 0: | ||
| return int(val) | ||
| return _effective() |
There was a problem hiding this comment.
Fixed in 4375e88. The getter now re-applies the persisted value through the central clamp (set_graph_query_timeout_override(int(s) or None)) and returns get_graph_query_timeout_s() from back.core.query_limits, so the Settings UI always shows the same bounded value the DB enforces (0 = unset preserved).
| seconds = max(0, int(seconds)) | ||
| set_graph_query_timeout_override(seconds or None) | ||
| return self._save( | ||
| host, token, registry_cfg, {"graph_query_timeout_s": seconds} | ||
| ) |
There was a problem hiding this comment.
Fixed in 4375e88. The setter now persists the clamped effective value: to_save = 0 if seconds <= 0 else get_graph_query_timeout_s(). So the stored config can never differ from the enforced bound, and re-saving from the UI is idempotent.
| from back.core.query_limits import get_graph_chat_result_cap as _effective | ||
|
|
||
| val = self.get(host, token, registry_cfg, "graph_chat_result_cap", "") | ||
| if val and str(val).isdigit() and int(val) > 0: | ||
| return int(val) | ||
| return _effective() |
There was a problem hiding this comment.
Fixed in 4375e88, same pattern as the timeout getter: persisted value is routed through set_graph_chat_result_cap_override(...) and the method returns the clamped effective cap from back.core.query_limits.
| """Persist and apply the Graph Chat triple result cap (``0`` = unset).""" | ||
| count = max(0, int(count)) | ||
| set_graph_chat_result_cap_override(count or None) | ||
| return self._save( | ||
| host, token, registry_cfg, {"graph_chat_result_cap": count} | ||
| ) |
There was a problem hiding this comment.
Fixed in 4375e88. Now persists to_save = 0 if count <= 0 else get_graph_chat_result_cap(), keeping config/UI consistent with the enforced cap.
…bs#116) - Reset Lakebase statement_timeout in a finally so the per-read bound can't leak to the next pooled (autocommit) borrower and cancel a bulk write/DDL. - save-graph-limits _opt_int raises ValidationError on non-integer input instead of a 500. - GlobalConfigService get/set for graph timeout + chat cap route persisted values through the central clamp so config/UI match the enforced effective bound. - Add Lakebase timeout-reset regression tests (success + error paths).
Addressed all Copilot review feedback in
|
benoitcayladbx
left a comment
benoitcayladbx
left a comment
There was a problem hiding this comment.
Review from @benoitcayladbx
Thanks for the thorough write-up and for already addressing all 6 Copilot comments in 4375e88 — the finally-reset, ValidationError on _opt_int, and the central-clamp routing are the right calls. The design is solid and this is clearly the right fix for #114.
One blocker before we can merge this: the PR targets master, but our integration branch is develop.
develop is currently 54 commits ahead of master (v0.7.0 vs v0.6.1). Please rebase onto develop — or retarget the PR — before we proceed. A few things to watch for during that rebase:
Rebase notes
-
dtwin.pyconflicts —developalready importsrun_blockingand uses it for several routes, but it also migrated fromget_triplestore→get_graphdband fromget_databricks_credentials→get_triplestore_sql_credentials. When rebasing, please audit that the newly wrapped routes (triples/find,graphql/execute,neighbors,sync/stats) don't end up double-wrapped withrun_blocking. -
DatabricksHelpers.pyconflicts —developaddedresolve_delta_warehouse_id,get_delta_databricks_credentials, andget_triplestore_sql_credentials. These don't conflict in purpose but the file has diverged enough that it'll need a careful manual merge. -
query_limits.py— net-new file, should apply cleanly. Same for the new tests and docs. -
Changelog —
developis at v0.7.0, so the entry belongs inchangelogs/v0.7.0/rather thanchangelogs/v0.6.1/.
Before merging
- Rebase onto
develop(or retarget todevelop) - Resolve conflicts in
dtwin.pyandDatabricksHelpers.py, no double-run_blocking - Move changelog to
changelogs/v0.7.0/briancastelino_2026-07-09.log - CI green on the rebased branch (
uv run pytest -q -m "not scenario"+pre-commit run --all-files)
Once those are done this is ready to merge. The rest of the checklist (CLA ✅, eval waiver ✅, conventional commit title ✅, incident doc ✅) is all good.
…e app (databrickslabs#114) Graph Chat could freeze the entire app until redeploy: a broad question drove a slow, unbounded graph read that ran directly on the single uvicorn event loop and pinned a DB session indefinitely. - Offload blocking DB work in internal /dtwin graph routes via run_blocking so it never runs on the event loop. - Add configurable graph-read statement_timeout on both backends (Lakebase SET statement_timeout, warehouse SET STATEMENT_TIMEOUT), reset on release. - Auto-tune the blocking thread pool from instance vCPU count; expose get_blocking_pool_stats and surface a resource-pressure advisory (inline + toast). - Admin Settings controls for statement timeout and chat result cap; align agent loopback HTTP timeout above the server bound. Complements (does not replace) databrickslabs#112 (unindexed _sync tables) and databrickslabs#115 (alias-expansion perf). Closes databrickslabs#114.
…bs#116) - Reset Lakebase statement_timeout in a finally so the per-read bound can't leak to the next pooled (autocommit) borrower and cancel a bulk write/DDL. - save-graph-limits _opt_int raises ValidationError on non-integer input instead of a 500. - GlobalConfigService get/set for graph timeout + chat cap route persisted values through the central clamp so config/UI match the enforced effective bound. - Add Lakebase timeout-reset regression tests (success + error paths).
…slabs#116) Rebased from master onto develop. develop already migrated triplestore->graphdb and offloads graph routes via run_blocking, so kept only the unique bounding/advisory layer. - Port bounded read to graphdb/delta/DeltaFlatStore.execute_query (old DeltaTripleStore deleted on develop). - dtwin.py: keep resource-pressure advisory + triples/find result-cap clamp; offloading inherited from develop. - settings.js: keep loadGraphLimits + save handler on develop's Graph DB tab. - Move changelog to changelogs/v0.7.0/; update DeltaFlatStore test.
4375e88 to
f55698b
Compare
|
@benoitcayladbx Rebased onto Rebase approachThe branch was cut from Conflict resolution (per your checklist)
Testing
Final diff vs |
Summary
Graph Chat could hang the entire app until redeploy: a broad question drove a slow graph read (deep BFS, unfiltered
triples/find, or a heavy GraphQL resolver) that (a) ran directly on the single uvicorn event loop, starving every other request, and (b) was unbounded in time and result size, pinning a DB session indefinitely. This PR makes graph reads resilient — it bounds every read server-side, offloads the blocking work off the event loop, auto-sizes the worker pool to the instance, and surfaces an admin-tunable advisory under genuine saturation. A slow query now degrades to a clean per-request "cancelled" error instead of a global freeze.Linked Issue / milestone
Closes #114. Complements (does not replace) #112 (unindexed
_synctables after Lakeflow sync) and #115 (expand_uri_aliases/get_triples_for_subjectsperf), as raised by @ulsmo — those reduce how often the bound is hit; this PR ensures hitting it never freezes the app.Type of change
Author checklist
uv run pytestgreen locally — partial:uv/psycopgunavailable in author env; ran psycopg-free subset → 39 passed. Reviewer/CI to run full suite.pre-commit run --all-files— run in CI (uvunavailable locally).src/agents/**touched (agent_dtwin_chat/tools.py): eval waiver — only derives loopback HTTP timeout from server statement timeout; no prompt/tool-schema/LLM-decision change.gsd-*references re-introduced.Test plan
uv run pytest tests/ -m "not e2e and not property and not eval" --cov-fail-under=90— reviewer/CI.python -m pytest tests/units/core/test_query_limits.py tests/units/core/test_graph_query_bounds.py tests/units/core/test_sql_warehouse.py -q→ 39 passed, no regression.Reviewer hint
triples/find/graphql/execute/neighbors/sync/statsbodies wrapped inrun_blocking; no state mutated across the thread boundary.0on both backends' pooled connections so a bound never leaks to the next borrower.