fitzee opened a new pull request, #43977:
URL: https://github.com/apache/superset/pull/43977
### SUMMARY
Scheduled chart-data exports can fail on socket timeouts or HTTP errors
without actionable, safe transport diagnostics. This change was developed
against `apache/superset` master at `1fb6b98941345a99b12240f482dcbd72caadd34e`,
rather than porting a downstream implementation.
**Upstream behavior inspected**
- CSV/Excel exports prefer the saved query-context POST path, including
post-processing and export pagination. A legacy GET remains when query-context
generation fails.
- Both paths use a single blocking urllib request. Socket timeouts fall
through the generic export failure handler; HTTP error bodies are not surfaced.
- Reports already have an execution deadline and delivery/cleanup reserves.
- Both first-run/error and success/grace states emit
`report_execution_terminal` again when recording the error-notification result.
**Publication baseline**
- Cherry-picked the tested change onto the existing fork master
(`faf7c34c0a9bef8358a20af9cf82a1e9f754b241`) without updating any default
branch. The cherry-pick was conflict-free and has the same stable patch ID as
the original tested commit. Unit and CSV integration tests were rerun on this
branch. The Apache PR diff contains only the nine intended files; no workflows.
A non-mutating merge-tree check against Apache master produces tree
`2962dce921b7ba534b37a7a24a671b7e24a47e0f`, identical to the original tested
commit.
**Changes**
- Add shared, report-specific transport handling around the existing GET and
POST calls. No query-context, authentication, post-processing, or endpoint
rewrite.
- Classify HTTP, timeout, and network failures separately. Direct and
urllib-wrapped connection/read timeouts map to the existing CSV/Excel timeout
exceptions.
- Add `ALERT_REPORTS_CSV_REQUEST_RETRY = False`. Opting in permits at most
one retry for transient connection/read failures or HTTP 429/500/502/503/504.
Other HTTP statuses, permanent network failures, and requests without a finite
timeout do not retry.
- Back off 0.5 seconds, honoring numeric Retry-After up to 2 seconds;
longer/date-based/invalid delays defer to existing scheduling policy. Attempts
and backoff share the original timeout allowance, capped by the execution
deadline and phase reserves. An exhausted 60-second request does **not**
receive another 60 seconds.
- Close legacy GET responses on success/read failure, and close HTTP error
responses.
- Record error-notification bookkeeping separately from terminal emission.
Preserve notification delivery, its historical database audit row/grace-period
marker, and the original execution error. No new lifecycle abstraction or
schema.
**Security/logging considerations**
- Log existing schedule/chart context, a fixed endpoint path without any
query string, error category, status, elapsed time, timeout, and attempt. No
custom metrics or metric tags.
- Read at most 4097 HTTP response bytes to enforce a 4096-byte limit. Retain
only known Superset error types from at most four JSON errors. Redact free-form
messages and arbitrary fields; omit non-JSON/oversized bodies. Unknown 400
response text remains intentionally unavailable rather than risking SQL,
payload, or credential disclosure.
- Do not log request URLs, query payloads, cookies, authorization headers,
remote reason strings, or raw chained transport exceptions. Regression tests
include secret sentinels and full rendered exception chains.
- This is a reliability and diagnostic-hardening change, not a vulnerability
report.
**Risks and rollback**
- A timed-out server query may continue executing; retrying POST exports can
increase query load. Retries are opt-in and limited to one. Whole-schedule
retries remain unchanged and can compound load if also enabled.
- urllib socket timeouts are not strict wall-clock cancellation. Existing
report task limits remain responsible for interrupting in-flight work; this
change does not increase those limits.
- Diagnostic redaction deliberately trades arbitrary error detail for
privacy. Consumers expecting repeated terminal messages will see one terminal
emission instead; notification audit history is retained.
- Disable `ALERT_REPORTS_CSV_REQUEST_RETRY` to roll back retries. Revert
this commit for the complete rollback; no migration is required.
**Deferred**
- Chart/query tuning, increasing request/task budgets, query cancellation,
and transport/POST architecture changes.
- Broader embedded-dataframe retry/normalization work and changes to
whole-schedule retry policy.
- Production workload validation before enabling retries; no live
browser/end-to-end test was performed.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: backend-only change. Before: generic CSV failure and
repeated terminal emission during error notification. After: safe classified
diagnostics, normalized timeouts, optional budget-sharing retry, and one
terminal emission with separate notification bookkeeping.
### TESTING INSTRUCTIONS
Tests ran in `/tmp/superset-csv-venv` with repository base requirements and
test dependencies installed (the initial system environment lacked
dependencies). Activate that environment before the following commands:
```bash
source /tmp/superset-csv-venv/bin/activate
python -m pytest tests/unit_tests/commands/report/chart_data_test.py
tests/unit_tests/commands/report/execute_test.py
tests/unit_tests/utils/csv_tests.py -q
# 217 passed
python -m pytest tests/unit_tests/commands/report/
tests/unit_tests/utils/csv_tests.py
tests/unit_tests/utils/test_report_execution.py -q
# 405 passed
python -m pytest tests/integration_tests/reports/commands_tests.py -k csv -q
# 6 passed, 121 deselected
PYTHONPATH="$PWD${PYTHONPATH:+:$PYTHONPATH}" pre-commit run --files $(git
diff --name-only origin/master...HEAD)
# All applicable changed-file hooks passed, including ruff-format, ruff,
pylint and mypy
pre-commit run mypy --files $(git diff --name-only origin/master...HEAD)
# Passed
```
Coverage includes HTTP 400/no retry and bounded redaction; direct/wrapped
timeouts on GET/POST and CSV/Excel; retry success/exhaustion; retryable
429/5xx; Retry-After; budget exhaustion and post-backoff deadline rechecks;
response closure; payload/header/URL/exception-chain non-disclosure; and single
terminal emission even when error notification fails. The CSV integration
failure test also verifies the terminal emission and retained notification
audit history.
Manual verification on a configured worker: schedule a CSV chart report,
verify normal export output is unchanged, then simulate HTTP 400 and a short
transient 503 at the internal chart-data endpoint. Confirm one request for 400,
one by default for 503, at most two for 503 with the retry option enabled, safe
worker diagnostics, and preserved owner error notification. A failure consuming
the entire request allowance must not retry. Repeat for an Excel attachment.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Adds an opt-in configuration setting; no API or database changes.
Implementation and tests were AI-assisted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
