fix(app): prevent crash when a source's database/table doesn't exist#2335
Draft
MikeShi42 wants to merge 2 commits into
Draft
fix(app): prevent crash when a source's database/table doesn't exist#2335MikeShi42 wants to merge 2 commits into
MikeShi42 wants to merge 2 commits into
Conversation
Sources can point at databases or tables that don't exist on the current ClickHouse server (e.g. a stale config or freshly imported source). Previously, navigating around the app while such a source was selected caused a hard 'Maximum update depth exceeded' crash that originated from schema-discovery hooks. Two underlying bugs: 1. `useTableMetadata`'s queryFn could resolve to `undefined` when the table is missing in `system.tables`. React Query v5 treats `undefined` as an invalid query result and throws 'Query data cannot be undefined'. That synthetic error then propagated through the page and contributed to render loops in consumers that observed the query result. 2. The schema-discovery hooks (`useColumns`, `useJsonColumns`, `useTableMetadata`) used the default 3-retry policy. 'Database does not exist' is deterministic — retrying just amplifies the spurious re-renders that triggered the crash, and hammers the database with useless requests. Additionally, `useFieldExpressionGenerator` returned a fresh `getFieldExpression` closure on every render, which destabilized downstream useMemo dependencies that themselves observe schema state. Memoize the closure so consumers only recompute when `jsonColumns` actually changes. Co-authored-by: Mike Shi <mike@hyperdx.io>
… tableConnection refs Callers like the dashboard / sessions / services pages pass an inline `tcFromSource(source)` for `tableConnection`, producing a fresh object reference every parent render. The lucene-explanation effect in `SearchInputV2` previously depended on that reference and re-ran every render, calling `setParsedEnglishQuery` in a tight loop. When the underlying source's database doesn't exist, the schema lookups inside `genEnglishExplanation` reject; the state update was wrapped in a `.then` with no error handler, so the rejection bubbled as an unhandled promise rejection and tripped the dev-mode error overlay during the render-loop storm. Stabilize the connection by memoizing on a content-derived string key, swallow the schema-lookup error to a console warning, and add a cancel flag so a stale resolution doesn't update state after the connection changed. Co-authored-by: Mike Shi <mike@hyperdx.io>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
E2E Test Results✅ All tests passed • 177 passed • 3 skipped • 1192s
Tests ran across 4 shards in parallel. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When a Source is configured against a ClickHouse database or table that doesn't exist on the current server (e.g. a stale source carried over from another deployment, or a freshly imported config before the data lands), navigating around the app crashed the page with
Maximum update depth exceededoriginating from a Mantine<input>. The console showed:The crash was reproducible by creating a Trace source pointing at a database that doesn't exist (e.g.
otel_v2), selecting it on the Search page, and navigating to/from Service Map (or other pages that observe the same source). Seebefore_fix_crash_screen.pngbelow.Why
Three intertwined issues combined to surface as a hard render-loop crash:
useTableMetadata'squeryFncould resolve toundefinedwhen the underlying table is missing insystem.tables. React Query v5 treatsundefinedas an invalid result and throwsQuery data cannot be undefined. That synthetic error propagated through consumers that render schema-derived UI (filter sidebar, default order-by, etc.) and contributed to render loops.Schema-discovery hooks used the default 3-retry policy. "Database does not exist" is deterministic — retrying just amplifies the spurious re-renders that triggered the crash, and hammers ClickHouse with useless
DESCRIBErequests.SearchInputV2's English-explanation effect re-ran every render. Many callers pass an inlinetcFromSource(source)fortableConnection, which produces a fresh object reference each parent render. The effect depended on that reference, calledgenEnglishExplanation(which performs schema lookups), and updated state in a tight loop. When the underlying schema lookup rejected, the.thenhad no error handler, so the rejection bubbled as an unhandled promise rejection during a render storm.useFieldExpressionGeneratorhad a similar instability — it returned a freshgetFieldExpressionclosure on every render, destabilizinguseMemodeps in callers.What changed
useTableMetadatanow normalizes the missing-table case tonull(instead ofundefined), so React Query no longer throws. Consumers already usetableMetadata?.…, so they see absence either way.useColumns,useJsonColumns, anduseTableMetadatanow useretry: 0. These are deterministic schema queries — retrying is never useful.useFieldExpressionGeneratormemoizes itsgetFieldExpressionclosure withuseCallbackkeyed onjsonColumns, so consumers'useMemos only recompute when the JSON-column set actually changes.SearchInputV2stabilizes thetableConnectiondep via a content-derived key, swallows schema-lookup rejections to aconsole.warn, and adds a cancel flag so a stale resolution can't update state after the connection changed.Walkthrough
Before —
Search → Service Mapwith a source pointing at a non-existent database hard-crashed the page withMaximum update depth exceeded(image captured prior to the fix):Before fix: Runtime Error overlay - Maximum update depth exceeded
After — same path now resolves cleanly: the Service Map page renders with a graceful inline error explaining that the database doesn't exist, and the rest of the navigation continues to work.
after_fix_search_to_service_map_no_crash.mp4
After fix: Service Map shows
Testing
make ci-lint(lint + TypeScript across all packages)yarn nx run @hyperdx/app:ci:unit— 1679 tests passed, 0 failedmain, then verified the same path no longer crashes after these changes (see walkthrough above).Notes / known follow-ups
Service Map → SearchwhilelastSelectedSourceIdin localStorage points at a "bad" source — still surfaces the dev-mode error overlay in some manual sessions. The original user-reported error chain (useMetadata.useJsonColumns→ React Query throwingdata is undefined→ cascade) is fixed; the remaining edge case looks like a separate render-loop interaction inside the Search page's form/source-resolution effects and was not consistently reproducible from a headless harness in this task. Investigating it further may require additional instrumentation arounduseForm({ values })syncing on the Search page.To show artifacts inline, enable in settings.