Skip to content

fix(contracts): report scan FAILED instead of COMPLETED_WITH_ERRORS on pre-results failure#2732

Merged
m1n0 merged 1 commit into
mainfrom
dtl-1800-contract-scan-pending-mark-failed
Jun 3, 2026
Merged

fix(contracts): report scan FAILED instead of COMPLETED_WITH_ERRORS on pre-results failure#2732
m1n0 merged 1 commit into
mainfrom
dtl-1800-contract-scan-pending-mark-failed

Conversation

@m1n0

@m1n0 m1n0 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Fixes DTL-1800. Source incident: SAS-12468.

Problem

When a contract verification fails before producing any check results (e.g. the data source connection fails — Databricks 403 in the incident), core sends an errored, result-less batch to Soda Cloud. The backend maps that to COMPLETED_WITH_ERRORS. For a managed scan still in PENDING, the scan-state machine forbids PENDING -> COMPLETED_WITH_ERRORS (ScanState.PENDING allows only SUBMITTED/FAILED/CANCELED), so the upload returns 400 invalid_scan_state → exit code 4 (RESULTS_NOT_SENT_TO_CLOUD) → the contract launcher raises LibraryExecutionEngineException and pages via Datadog.

Fix

Report such a run as FAILED via mark_scan_as_failed (PENDING -> FAILED is allowed) instead of sending results. The report lands, no false RESULTS_NOT_SENT_TO_CLOUD is raised, and genuine "could not send to Cloud" failures still surface (this only changes the terminal state core requests when the run produced nothing).

  • New predicate CheckCollectionResult.errored_without_results (errored and zero check results).
  • CheckCollectionImpl.verify() (per-file/non-combine path): route to mark_scan_as_failed.
  • session.py combined-upload path (combine_uploads=True subtypes, e.g. Data Standards): skip such results from the combined send and mark the scan FAILED when the whole scan produced nothing to send.

Tests

soda-tests/tests/unit/test_contract_marks_scan_failed_on_connection_error.py — covers both the per-file and combine-upload paths by patching the DuckDB connection to fail. Both assert sodaCoreMarkScanFailed is sent and sodaCoreInsertScanResults is not. Full unit suite: 871 passed, 2 skipped.

🤖 Generated with Claude Code

@m1n0 m1n0 self-assigned this Jun 2, 2026
@m1n0
m1n0 requested a review from Copilot June 2, 2026 14:10
@m1n0
m1n0 marked this pull request as ready for review June 2, 2026 14:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts Soda Cloud reporting for contract/check-collection runs that error before producing any check results, to avoid sending a result-less errored batch that Cloud maps to COMPLETED_WITH_ERRORS (an invalid transition from PENDING for managed scans). Instead, such runs are reported as FAILED via mark_scan_as_failed.

Changes:

  • Add CheckCollectionResult.errored_without_results to detect ERROR runs with zero check_results.
  • Update CheckCollectionImpl.verify() and the session-level combine_uploads executor path to avoid uploading these result-less errored batches and (when appropriate) mark the scan as failed instead.
  • Add unit tests covering both per-file and combined-upload paths on a simulated connection failure.

Reviewed changes

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

File Description
soda-core/src/soda_core/check_collections/base.py Adds errored_without_results and changes per-file upload behavior to mark managed scans FAILED when errors occur before any check results exist.
soda-core/src/soda_core/check_collections/session.py Updates combined-upload grouping to exclude errored_without_results results and optionally mark the scan FAILED if nothing else is sendable.
soda-tests/tests/unit/test_contract_marks_scan_failed_on_connection_error.py Adds unit tests asserting sodaCoreMarkScanFailed is sent and sodaCoreInsertScanResults is not for pre-results failures (both upload modes).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread soda-core/src/soda_core/check_collections/base.py Outdated
Comment thread soda-core/src/soda_core/check_collections/session.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread soda-core/src/soda_core/check_collections/base.py Outdated
Comment thread soda-core/src/soda_core/check_collections/session.py Outdated
Comment thread soda-core/src/soda_core/check_collections/session.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread soda-core/src/soda_core/check_collections/base.py Outdated
Comment thread soda-core/src/soda_core/check_collections/session.py Outdated
…n pre-results failure

When a contract verification fails before producing any check results (e.g. the data
source connection fails), core sent an errored, result-less batch to Soda Cloud, which
the backend maps to COMPLETED_WITH_ERRORS. For a runner-created scan still in PENDING the
scan-state machine forbids PENDING -> COMPLETED_WITH_ERRORS (invalid_scan_state, HTTP 400),
so the upload fails -> exit code 4 (RESULTS_NOT_SENT_TO_CLOUD) -> the contract launcher
raises LibraryExecutionEngineException and pages (incident SAS-12468).

Report such runs as FAILED via mark_scan_as_failed (PENDING -> FAILED is allowed) so the
report lands and no false RESULTS_NOT_SENT_TO_CLOUD is raised. Genuine "could not send to
Cloud" failures still surface.

- Add CheckCollectionResult.errored_without_results (errored, zero check results).
- Route such runs to mark_scan_as_failed in both the per-file verify() path and the session
  combined-upload path (combine_uploads subtypes, e.g. Data Standards).
- Gate on the scan id (EnvConfigHelper.soda_scan_id): ad-hoc runs have no pre-created scan,
  so they keep the normal upload and aren't dropped from Cloud.
- Session fallback marks FAILED only when nothing else was sent and no sibling uploaded or
  failed to send; keeps the first errored result for deterministic logs.
- Stamp the scan id on the result and forward the stored exception so the FAILED scan is
  diagnosable and post-processing failure reporting works.

DTL-1800.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@m1n0
m1n0 force-pushed the dtl-1800-contract-scan-pending-mark-failed branch from e14a1ab to 38961e7 Compare June 2, 2026 20:55
@sonarqubecloud

sonarqubecloud Bot commented Jun 2, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

@Niels-b Niels-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

Just for my own understanding. This fixes an issue only when Soda Cloud creates a scan and it fails. This doesn't affect a scan that you when using Soda Core programmatically. Is that correct?

# Presence of this id is the precondition for reporting a scan as failed
# (mark_scan_as_failed needs it) — not is_running_on_runner, which only reflects
# SODA_INSTRUCTION_ID and can be set without a scan id.
return os.getenv("SODA_SCAN_ID")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't know we had this available in the runner.
As comment says: we don't have this in "local" runs. Would be nice 😄

@m1n0

m1n0 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Correct — gated on SODA_SCAN_ID, which only the runner sets.
Ad-hoc/programmatic core runs have no scan id and take the unchanged upload path (covered by test_ad_hoc_run_without_scan_id_still_uploads_results). On the runner it only changes the errored with zero results case.

@m1n0
m1n0 merged commit a38d2a8 into main Jun 3, 2026
23 checks passed
@m1n0
m1n0 deleted the dtl-1800-contract-scan-pending-mark-failed branch June 3, 2026 07:28
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.

3 participants