Skip to content

fix: log execution errors server-side (stream, Lakebase, SQL Warehouse)#255

Merged
jamesbroadhead merged 8 commits intomainfrom
fix/log-query-errors-server-side
Apr 27, 2026
Merged

fix: log execution errors server-side (stream, Lakebase, SQL Warehouse)#255
jamesbroadhead merged 8 commits intomainfrom
fix/log-query-errors-server-side

Conversation

@jamesbroadhead
Copy link
Copy Markdown
Contributor

Summary

  • Query/stream execution errors were only sent to the browser via SSE or recorded in telemetry spans, but never logged server-side
  • This made runtime failures invisible in databricks apps logs, hindering debugging and monitoring
  • Adds logger.error() calls in error paths across:
    • StreamManager — catch block in _processGeneratorInBackground()
    • LakebaseV1Connectorquery() and transaction() catch blocks
    • SQLWarehouseConnectorexecuteStatement() and _pollForStatementResult() catch blocks

What's NOT changed

  • Genie connector — already has logger.error() on all error paths
  • Files connector — errors from traced() are re-thrown and logged by the files plugin's _handleApiError()
  • Intentional silent catches (SSE write failures, retry loops) are left as-is

Test plan

  • Trigger a query error (e.g. invalid warehouse config) and verify it appears in databricks apps logs
  • Trigger a Lakebase query/transaction failure and verify server-side logging
  • Trigger a SQL Warehouse statement failure and verify server-side logging
  • Verify errors are still sent to the browser via SSE as before

Fixes #243

This pull request was AI-assisted by Isaac.

Query execution errors were only sent to the browser via SSE but never
logged server-side, making them invisible in `databricks apps logs`.
Add logger.error() call in the StreamManager catch block so errors
appear in application logs for debugging and monitoring.

Fixes #243
Extends the same pattern from stream-manager to connector catch blocks
that previously only recorded errors in telemetry spans.

Co-authored-by: Isaac
Log only error.message and error code instead of the full error object
(%O) to avoid leaking SQL query strings into server logs. pg errors
store the raw query in a .query property which %O would dump.

Co-authored-by: Isaac
Verifies that when connectors or stream manager log errors, only the
error message and code appear in output — not the raw SQL statement
or parameter values from the error object.

Co-authored-by: Isaac
Copy link
Copy Markdown
Collaborator

@MarioCadenas MarioCadenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things before merge:

[Required] SQL Warehouse: asymmetric abort gatingpackages/appkit/src/connectors/sql-warehouse/client.ts

The new logger.error in executeStatement (~L240) sits inside the existing if (!isAborted) { ... } block, but the one in _pollForStatementResult (~L385) is not gated. The same failure bubbling through both layers will log in one and stay silent in the other depending on cancel state. Pick one policy and apply it uniformly.

[Recommended] StreamManager: aborts will spam error logspackages/appkit/src/stream/stream-manager.ts:265

_categorizeError maps AbortErrorSTREAM_ABORTED, which is a normal client cancel (user navigates away). After this PR every cancel becomes a logger.error("Stream execution failed: ...") line. Suggest branching on errorCode === STREAM_ABORTED and logging at info/debug, or skipping.

[Follow-up] pg .message is still a leak surface

The redaction tests use synthetic Error objects with custom .query/.parameters. Realistic pg errors like invalid input syntax for type uuid: "<sensitive>" put the leak directly in .message, which we do log. Either allowlist by error.code (log a stable class label instead of free-form message) or at least add one test with a realistic pg-shaped error (code: '22P02', message containing the rejected literal) so a future change can't silently regress to String(error). This is the 6th time error-message-leakage has surfaced in reviews of this repo — probably worth a stricter policy.

Nit: docs/static/appkit-ui/styles.gen.css has unrelated regenerated lines (removed .max-w-full, added [&_table]:overflow-x-auto, etc.). Looks like a stale rebuild — drop from this PR?

Otherwise direction is right and the proactive redaction tests are appreciated.

- Quiet StreamManager: log STREAM_ABORTED at info instead of error so
  normal client cancels do not produce error-level log spam.
- Make SQL Warehouse abort logging uniform: drop logger.error from
  _pollForStatementResult; let executeStatement be the single (gated)
  point of error logging so the same failure is never double-logged
  and never silent depending on cancel state.
- Lock in pg-shaped error redaction: add a test using a realistic pg
  error (code 22P02 with a sensitive literal in .message) so a future
  change to e.g. String(error) cannot silently re-introduce error.query
  or error.parameters into the log.
- Reorder imports per biome organize-imports.

Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
@jamesbroadhead
Copy link
Copy Markdown
Contributor Author

@MarioCadenas — pushed 9c9f5b6 addressing all four points; CI is green. Quick map back to your review:

  • [Required] SQL Warehouse asymmetric abort gating → made it uniform by removing logger.error from _pollForStatementResult. executeStatement's catch (gated on isAborted) is now the single point of error logging, so a polling failure can never be both logged and silent depending on cancel state, and we don't double-log when the same error bubbles through both layers. Updated the polling error test accordingly (drops the stmt-123 assertion since the outer log no longer carries the statement id).

  • [Recommended] StreamManager abort spam → branched on errorCode === SSEErrorCode.STREAM_ABORTED and demoted client cancels to logger.info("Stream aborted by client (code=%s)"). Non-abort errors still go through logger.error as before.

  • [Follow-up] pg .message leak surface → went with the test-lock-in option. Added redacts pg-shaped error fields when message also leaks the literal in lakebase-v1.test.ts using code: 22P02 and a UUID literal directly in .message (the realistic shape you described). It asserts the code and stable message prefix are logged, and that error.query / non-message parameter values stay out — so a future regression to String(error) (which would re-introduce error.query) breaks the test. Agreed on the broader policy point — happy to follow up with allowlist-by-code redaction in a separate PR if you'd like, since this one is already cross-connector.

  • Nit: styles.gen.css → resolved by merging main; the file is no longer in the PR diff.

Re-requesting your review.

@jamesbroadhead jamesbroadhead merged commit 390d9e5 into main Apr 27, 2026
7 checks passed
@jamesbroadhead jamesbroadhead deleted the fix/log-query-errors-server-side branch April 27, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Runtime query errors not logged server-side

2 participants