fix: prevent Task.run from propagating dependency-resolution failures - #4
Merged
Merged
Conversation
…Result - Rename internal _ErrorData dataclass to public ErrorData, exported from the package root, while keeping _ErrorContextFormatter private. - TaskResult now records elapsed_seconds and attempts for every run, laying the groundwork for a process execution report.
Add failed_results/failed_tasks_results dicts alongside the existing failed_tasks set so callers can access the TaskResult (including ErrorData) for each errored task, not just its name.
Introduce TaskStatus, TaskReportEntry, and ProcessExecutionReport with a from_result() builder that classifies every task as success, errored, or skipped and exposes its function, args, kwargs, result/error, elapsed time, and attempt count.
TaskStatus moves to task.py (PENDING/SUCCESS/ERRORED/SKIPPED) so TaskResult can carry it directly. worked becomes a derived property (status == SUCCESS), keeping the existing call sites working.
…onReport ProcessRunner now tracks a single dict[str, TaskResult] (self.results), seeded with PENDING entries for every task and transitioned to SUCCESS/ERRORED/SKIPPED as tasks resolve. This replaces the separate passed_results/failed_tasks/failed_results/skipped_tasks bookkeeping and fixes the latent divergence between failed_tasks and failed_results for cascade-skipped tasks. Process.run() now returns a ProcessExecutionReport directly, built via ProcessExecutionReport.from_results(process, results). ProcessResult and from_result are removed as part of this breaking change. BREAKING CHANGE: Process.run() returns ProcessExecutionReport instead of ProcessResult. Use report.successes / report.errored / report.skipped instead of passed_tasks_results / errored_tasks / skipped_tasks, and report.entries[name].result / .error instead of passed_tasks_results[name] / failed_tasks_results[name].
Remove stale references to ProcessResult, passed_tasks_results, failed_tasks, errored_tasks, and skipped_tasks now that Process.run() returns a ProcessExecutionReport backed by a single results dict.
_resolve_args ran outside the retry try/except, so a failure injecting an upstream result propagated out of Task.run. Sequential execution crashed the whole run while parallel execution swallowed it -- an asymmetry. Move arg resolution inside the error handling so it always returns an ERRORED result (attempts=0), and factor the failure-logging path into _errored_result. Adds TaskResult named constructors (pending/skipped/success/errored) used by the new error path, plus tests pinning the no-propagation guarantee.
Split the cascade-skip query from its mutation: _has_failed_dep is now a pure predicate and callers record SKIPPED explicitly. Adopt the TaskResult named constructors and the existing `worked` property instead of re-deriving `status == SUCCESS` by hand.
oliverm91
added a commit
that referenced
this pull request
Jun 19, 2026
Add aiosmtpd as a dev dependency and a `smtp_server` fixture (conftest.py) that runs a real in-process SMTP server capturing delivered messages. Email-send tests now exercise the full path — smtplib conversation, MIME serialization, recipients — and assert on what is actually received. - test_report_send.py: TestEmailSendReport rewritten as integration tests (delivered count, From/To/Subject headers, decoded HTML body, content flags, errors_only excludes successes). Called via send_report directly so transport failures propagate. - test_email_themes.py: TestTaskEmailWiring streaming tests send real emails and assert on the received body/subject; added a negative guard that a successful run delivers zero alerts. Pure formatter render-matrix tests kept unchanged. - test_complex_dag_failures.py: OUTCOME #4 now asserts exactly one received email per failing task with the correct theme and Downstream Impact, replacing the smtplib instantiation-count mock. Runtime deps unchanged (aiosmtpd is dev-only; wheel still zero-dependency).
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
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.
Summary
Task.runresolved dependency arguments (_resolve_args) outside theretry
try/except. When injecting an upstream result failed, the exceptionpropagated out of
Task.run— which crashed the whole sequential run,while parallel execution silently swallowed it. This PR removes that asymmetry
and tightens the
TaskResultconstruction surface along the way.What changed
Fix —
Task.runnever propagates (51f4192)(before a single attempt) is returned as an
ERROREDTaskResult(
attempts=0), exactly like a failure inside the task's own function.ErrorData+ wrap) into_errored_result,shared by the resolution-failure path and the retry-exhausted path.
TaskResultnamed constructors —pending(),skipped(),success(...),errored(...)— used by the new error path.Refactor — cleaner state handling (
99704d7)_has_failed_depisnow a pure predicate; callers record
SKIPPEDexplicitly. (Previously_is_unrunnablemutatedresultsas a side effect of a boolean check.)ProcessRunner.TaskResult.workedproperty instead of re-derivingstatus == SUCCESSby hand in the runner and the report builder.Behavior change
A dependency-resolution failure used to raise out of
Process.run(parallel=False).It is now reported as an
ERROREDtask in theProcessExecutionReport, and therest of the (independent) graph continues — matching how every other task
failure is already handled, and matching parallel execution.
Tests
New
tests/test_resolve_args_failure.py:test_run_wraps_resolution_failure— unit: a resolution failure yieldsERROREDwithattempts=0, no raise.test_sequential_process_survives_resolution_failure— end-to-end: sequentialProcess.runno longer crashes (this fails against the previous code).Type of change
feat— new featurefix— bug fixrefactor— no behavior changedocs— documentation onlytest— tests onlychore/ci/build