fix(sqllab): surface stacktrace in SQL Lab error responses (#28248)

[rusackas]

SUMMARY

Fixes #28248. SQL Lab query execution errors were silently discarding the Python traceback in two ways:

1. Traceback swallowed in logs — Both execution paths called logger.debug("Query %d: %s", query_id, ex) without exc_info, so the full traceback never appeared in Celery worker logs.
2. Stacktrace missing from error payload — handle_query_error (legacy sql_lab.py) and _handle_query_error (newer celery_task.py) returned payloads with status, error, and errors keys only — no stacktrace key, leaving users and operators with no way to find the root cause.

Fix:

• Changed logger.debug(...) → logger.exception(...) in the outer except Exception blocks in both get_sql_results and execute_sql_task
• Added logger.exception(...) in both handle_query_error and _handle_query_error
• Added a stacktrace key (populated with traceback.format_exc()) to the returned payload dicts in both functions
• Added import traceback to both files

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend-only fix, no UI changes.

TESTING INSTRUCTIONS

# All 6 regression tests should now pass:
pytest tests/unit_tests/test_sqllab_error_stacktrace.py -v
# Expected: 6 passed

ADDITIONAL INFORMATION

• Has associated issue: Add Missing Stacktrace information on errors, SQL lab query execution #28248
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

Closes #28248

[bito-code-review]

Code Review Agent Run #0da1b9

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• tests/unit_tests/test_sqllab_error_stacktrace.py - 2

  • NameError: payload out of scope · Line 145-145
  • NameError: payload out of scope · Line 259-259

Review Details

• Files reviewed - 1 · Commit Range: dc225ef..dc225ef

  • tests/unit_tests/test_sqllab_error_stacktrace.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.94%. Comparing base (a33fcb0) to head (25e0b5d).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/sql/execution/celery_task.py	0.00%	5 Missing ⚠️
superset/sql_lab.py	20.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40585      +/-   ##
==========================================
- Coverage   63.97%   63.94%   -0.03%     
==========================================
  Files        2654     2658       +4     
  Lines      142768   143019     +251     
  Branches    32837    32870      +33     
==========================================
+ Hits        91329    91454     +125     
- Misses      49878    50001     +123     
- Partials     1561     1564       +3     

Flag	Coverage Δ
hive	39.75% <10.00%> (+0.03%)	⬆️
mysql	58.39% <10.00%> (-0.04%)	⬇️
postgres	58.47% <10.00%> (-0.04%)	⬇️
presto	41.36% <10.00%> (+0.02%)	⬆️
python	59.95% <10.00%> (-0.04%)	⬇️
sqlite	58.13% <10.00%> (-0.04%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Surfaces Python tracebacks for SQL Lab query failures in both the legacy (superset/sql_lab.py) and newer (superset/sql/execution/celery_task.py) execution paths, so operators and users get root-cause information instead of a bare error string.

Changes:

• Switch logger.debug(...) → logger.exception(...) in the outer except blocks of get_sql_results and execute_sql_task, and add logger.exception(...) inside handle_query_error/_handle_query_error.
• Add a stacktrace key (populated from traceback.format_exc()) to the dict returned by both error handlers.
• Add unit/regression tests asserting both the logging and payload behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
superset/sql_lab.py	Imports traceback, logs exception with traceback, and adds stacktrace to the error payload.
superset/sql/execution/celery_task.py	Mirrors the legacy fix in the newer Celery execution path.
tests/unit_tests/sql/execution/test_celery_task.py	Adds a test for the empty-traceback branch in _handle_query_error.
tests/unit_tests/test_sqllab_error_stacktrace.py	New regression suite covering logging + payload behavior for both execution paths.

[bito-code-review]

Code Review Agent Run #458df8

Actionable Suggestions - 0

Additional Suggestions - 1

• superset/sql/execution/celery_task.py - 1

  • Missing error logging in handler · Line 103-103
    The removed `logger.exception` call ensured all errors handled by `_handle_query_error` were logged at ERROR level. Without it, errors at line 358 are silently lost from application logs since that call site has no logging. Restore the logger call to maintain error visibility.
    Code suggestion

    --- superset/sql/execution/celery_task.py (lines 100-111) ---
     100:          query.set_extra_json_key("errors", errors_payload)
     101: 
     102:      db.session.commit()  # pylint: disable=consider-using-transaction
     103: +    logger.exception("Query %s failed: %s", query.id, ex)
     104:      payload.update(
     105:          {"status": query.status.value, "error": msg, "errors": errors_payload}
     106:      )
     107:      if app.config.get("SHOW_STACKTRACE"):
     108:          if stacktrace := traceback.format_exc():
     109:              payload["stacktrace"] = stacktrace
     110:      if troubleshooting_link := app.config.get("TROUBLESHOOTING_LINK"):
     111:          payload["link"] = troubleshooting_link
     112:      return payload

Review Details

• Files reviewed - 4 · Commit Range: dc225ef..25e0b5d

  • superset/sql/execution/celery_task.py
  • superset/sql_lab.py
  • tests/unit_tests/sql/execution/test_celery_task.py
  • tests/unit_tests/test_sqllab_error_stacktrace.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

Switching to comment-only review

[aminghadersohi]

PR #40585 — Code Review

HEAD SHA: 25e0b5dc32b6944f45a21c6e98c78465bcfe20c4
Diff: 4 production lines changed, 350+ lines of new tests across 4 files.
Mode: code
Second opinion: Not needed — small focused fix, no security/migrations/multi-tenant/submodule changes.

---

Scan Coverage (all 19 run)

#	Scan	Result
1	Inline imports without circular-import comment	Pre-existing violations (_serialize_payload, _serialize_result_set) — not introduced by this PR
2	SQL injection	0 violations
3	RLS bypass	0 violations
4	Hardcoded credentials	0 violations
5	Debug logging in exception handlers	Fixed (debug→exception) ✓
6	Broad except Exception	Pre-existing, intentional Celery task pattern
7	Missing type hints	0 violations in changed lines
8	Mutable default arguments	0 violations
9	Exception chaining	0 violations
10	Assert in production	0 violations
11	TODO/FIXME	Pre-existing TODO in sql_lab.py (not introduced by PR)
12	Dead code	0 violations
13	Walrus operator	Correct usage in if/assignment context
14	Config access inconsistency	NIT — see below
15	Test false positives	MEDIUM — see below
16	Silent fallbacks	0 violations
17	DRY	Acceptable — the stacktrace block is duplicated across the two files, but they're parallel implementations
18	Structural source-inspection tests	MEDIUM — see below
19	Error message exposure	Properly gated behind SHOW_STACKTRACE

---

MEDIUM

M1 — traceback.format_exc() is always truthy; the inner guard never filters anything

traceback.format_exc() — in Python 3.11 and earlier — returns 'NoneType: None\n' (not an empty string) when no exception is currently active. That string is truthy, so the if stacktrace := traceback.format_exc(): guard never actually suppresses a spurious stacktrace if _handle_query_error were called outside an except block.

In practice this is fine for the current call sites (both are inside except handlers), but the guard is misleading: it reads as "only include the stacktrace if there is one," while in reality it always fires when SHOW_STACKTRACE is True. Several tests validate the supposedly-false branch by mocking format_exc to return "" — a state that can't happen naturally in Python 3.

# celery_task.py:106-108 / sql_lab.py:126-128
if app.config.get("SHOW_STACKTRACE"):
    if stacktrace := traceback.format_exc():   # always True in practice
        payload["stacktrace"] = stacktrace

Consider using ex.__traceback__ directly, which makes the intent clear and is immune to sys.exc_info() state at call time:

if app.config.get("SHOW_STACKTRACE") and ex.__traceback__ is not None:
    payload["stacktrace"] = "".join(
        traceback.format_exception(type(ex), ex, ex.__traceback__)
    )

M2 — Structural source-inspection tests are fragile and don't test runtime behavior

test_legacy_get_sql_results_outer_except_logs_exc_info and test_new_execute_sql_task_outer_except_logs_exc_info (in test_sqllab_error_stacktrace.py, lines 145 and 258) use inspect.getsource() to check that the string "logger.exception(" appears anywhere in the function body. This pattern has several failure modes:

• Passes if the string is inside a comment
• Passes if logger.exception is in a different, unrelated except block within the function
• Fails for semantically equivalent alternatives (e.g., log.exception(...) or logger.error(..., exc_info=True))

A behavioral test is more reliable:

def test_get_sql_results_outer_except_logs_with_exc_info(mocker):
    mock_logger = mocker.patch("superset.sql_lab.logger")
    mocker.patch("superset.sql_lab.execute_sql_statements", side_effect=RuntimeError("boom"))
    mocker.patch("superset.sql_lab.get_query", return_value=mock_query)
    get_sql_results(123, "SELECT 1", ...)
    mock_logger.exception.assert_called_once()
    # or: assert any(call[1].get("exc_info") for call in mock_logger.error.call_args_list)

M3 — test_handle_query_error_no_stacktrace_when_format_exc_empty is a false positive

test_celery_task.py:181 does not set SHOW_STACKTRACE=True. Without that flag the outer if app.config.get("SHOW_STACKTRACE"): check is already falsy, so traceback.format_exc is never called and the "stacktrace" not in payload assertion passes trivially — regardless of whether the inner if stacktrace := ... guard exists at all. The format_exc mock is dead.

The intent appears to be to test "SHOW_STACKTRACE=True, but format_exc returns empty → no stacktrace key". That scenario is already covered by test_handle_query_error_omits_stacktrace_when_show_stacktrace_enabled_but_empty (line 221). This test should either be removed, or updated to add mocker.patch.dict(current_app.config, {"SHOW_STACKTRACE": True}).

---

NIT

N1 — traceback.format_exc() is called after db.session.commit()

Both _handle_query_error (celery_task.py:103) and handle_query_error (sql_lab.py) call db.session.commit() before calling traceback.format_exc(). If commit() raises a secondary exception (unlikely but possible under heavy load or connection failure), sys.exc_info() at that point reflects the commit failure, not the original query error. Capturing the stacktrace from ex.__traceback__ (as shown in M1) avoids this entirely.

N2 — app.config["TROUBLESHOOTING_LINK"] vs app.config.get("SHOW_STACKTRACE") inconsistency in sql_lab.py

sql_lab.py:129 uses bracket access for TROUBLESHOOTING_LINK (will KeyError if the key is absent from config):

if troubleshooting_link := app.config["TROUBLESHOOTING_LINK"]:

The new code two lines above uses safe .get():

if app.config.get("SHOW_STACKTRACE"):

The corresponding code in celery_task.py uses .get() for both config keys consistently. TROUBLESHOOTING_LINK has a default in config.py so this won't blow up in production, but the inconsistency is mildly jarring. Worth aligning while handle_query_error is being touched.

N3 — No frontend change to render stacktrace in the SQL Lab UI

The stacktrace key is added to the error payload that's stored in the results backend and returned to the client, but there are no frontend changes to surface it. Inspecting the sqlLab.ts types and action handlers, there's no stacktrace field in the QueryObject interface. Unless existing generic error display code already forwards arbitrary payload keys, the stack trace is only visible to operators reading the results backend directly or via raw API responses.

If exposing this to UI users was part of the intent, a follow-up issue or a note in the PR description would help track it. If it's purely operator-facing (i.e., logs only), the payload["stacktrace"] in the stored results is extra data that goes unread.

---

PRAISE

The fix is minimal and surgical. Both the logging upgrade (debug → exception with implicit exc_info) and the payload addition are applied consistently across both execution paths. The SHOW_STACKTRACE gate is the right design choice — raw Python tracebacks exposing module paths and library versions are off by default.

The celery_task._handle_query_error correctly handles the TIMED_OUT status preservation (unlike its legacy counterpart in sql_lab.py that still has the commented-out TODO about it) — good that the new code doesn't repeat that technical debt.

The test_sqllab_error_stacktrace.py file has genuinely useful regression tests for the payload inclusion/exclusion branches, and the try: raise RuntimeError("boom") / except ... as ex: pattern correctly ensures sys.exc_info() is populated when calling the handlers.