Skip to content

fix: prevent Task.run from propagating dependency-resolution failures - #4

Merged
oliverm91 merged 10 commits into
mainfrom
feature/process-execution-report
Jun 15, 2026
Merged

oliverm91 merged 10 commits into
mainfrom
feature/process-execution-report

Conversation

@oliverm91

Copy link
Copy Markdown
Owner

Summary

Task.run resolved dependency arguments (_resolve_args) outside the
retry try/except. When injecting an upstream result failed, the exception
propagated out of Task.run — which crashed the whole sequential run,
while parallel execution silently swallowed it. This PR removes that asymmetry
and tightens the TaskResult construction surface along the way.

What changed

Fix — Task.run never propagates (51f4192)

  • Arg resolution now runs inside the error handling, so any failure
    (before a single attempt) is returned as an ERRORED TaskResult
    (attempts=0), exactly like a failure inside the task's own function.
  • Factored the failure path (log + ErrorData + wrap) into _errored_result,
    shared by the resolution-failure path and the retry-exhausted path.
  • Added TaskResult named constructors — pending(), skipped(),
    success(...), errored(...) — used by the new error path.

Refactor — cleaner state handling (99704d7)

  • Split the cascade-skip query from its mutation: _has_failed_dep is
    now a pure predicate; callers record SKIPPED explicitly. (Previously
    _is_unrunnable mutated results as a side effect of a boolean check.)
  • Adopted the named constructors across ProcessRunner.
  • Use the existing TaskResult.worked property instead of re-deriving
    status == SUCCESS by 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 ERRORED task in the ProcessExecutionReport, and the
rest 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 yields
    ERRORED with attempts=0, no raise.
  • test_sequential_process_survives_resolution_failure — end-to-end: sequential
    Process.run no longer crashes (this fails against the previous code).

Type of change

  • feat — new feature
  • fix — bug fix
  • refactor — no behavior change
  • docs — documentation only
  • test — tests only
  • chore / ci / build

oliverm91 added 10 commits June 14, 2026 20:08
…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
oliverm91 merged commit d42406f into main Jun 15, 2026
16 checks passed
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).
@oliverm91
oliverm91 deleted the feature/process-execution-report branch June 20, 2026 01:43
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.

1 participant