Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements fail-fast behavior in the task execution pipeline where remaining tasks are skipped when a dependency fails. This prevents wasting time executing tasks that are guaranteed to fail and provides clear feedback about why tasks were skipped.
- Adds tracking of task failures in
ExecutionPlan::executeto skip subsequent tasks - Introduces
ExecutionFailure::SkippedDueToFailedDependencystatus for skipped tasks - Creates comprehensive snap tests demonstrating fail-fast behavior in both simple and nested task scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/vite_task/src/schedule.rs | Implements fail-fast logic by tracking failures and creating skipped execution statuses |
| packages/cli/snap-tests/fail-fast/package.json | Defines test scripts with dependencies to demonstrate fail-fast behavior |
| packages/cli/snap-tests/fail-fast/failure.js | Simple Node.js script that exits with code 1 to simulate task failure |
| packages/cli/snap-tests/fail-fast/steps.json | Test configuration specifying commands and environment variables |
| packages/cli/snap-tests/fail-fast/snap.txt | Expected output showing skipped tasks with clear dependency failure messages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if let Ok(exit_status) = status.execution_result { | ||
| if exit_status != 0 { | ||
| has_failed = true; | ||
| } |
There was a problem hiding this comment.
The failure tracking logic only checks for non-zero exit codes but ignores Err cases. If execution_result is Err(ExecutionFailure::...), the task has failed but has_failed won't be set to true, allowing subsequent tasks to execute when they should be skipped.
| } | |
| } | |
| } else { | |
| // If execution_result is Err, treat as failure | |
| has_failed = true; |
| → Cache miss: no previous cache entry found | ||
| ······················································· | ||
| [3] test: $ vite run script3 ⊘ (skipped: dependency failed) | ||
| → Cache miss: no previous cache entry found |
There was a problem hiding this comment.
It's weird that skipped tasks still report cache status, since they don't look up cache at all.
| let mut has_failed = false; | ||
| for step in self.steps { | ||
| execution_statuses.push(Self::execute_resolved_task(step, workspace).await?); | ||
| if has_failed { |
There was a problem hiding this comment.
This incorrectly skips tasks that are not yet executed but do not depend on the failed tasks.
There was a problem hiding this comment.
Yes, it looks like it can't be handled simply and needs to be determined based on the task_graph
fengmk2
force-pushed
the
09-25-feat_task_support_fast_failure_exit
branch
from
f3c721a to
7217da0
Compare
fengmk2
changed the base branch from
graphite-base/202
to
09-25-chore_task_remove_unuse_empty_line
fengmk2
force-pushed
the
09-25-feat_task_support_fast_failure_exit
branch
from
7217da0 to
2705211
Compare
fengmk2
changed the base branch from
09-25-chore_task_remove_unuse_empty_line
to
graphite-base/202
fengmk2
force-pushed
the
09-25-feat_task_support_fast_failure_exit
branch
from
2705211 to
b94cbbf
Compare
fengmk2
force-pushed
the
graphite-base/202
branch
from
0bf3cde to
b8e0dc3
Compare
fengmk2
force-pushed
the
09-25-feat_task_support_fast_failure_exit
branch
from
b94cbbf to
7c54618
Compare
TL;DR
Implement fail-fast behavior in the task execution pipeline to skip remaining tasks when a dependency fails.
What changed?
ExecutionPlan::executeto track task failures and skip subsequent tasksExecutionFailure::SkippedDueToFailedDependencystatus for skipped taskspackages/cli/snap-tests/fail-fast/to verify the behaviorHow to test?
Run the new snap test in the
fail-fastdirectory:The output should show that when
script2fails,script3is skipped. Similarly, in the nested case, whentestfails (due toscript2failing),script4is also skipped.Why make this change?
This change improves the developer experience by not wasting time running tasks that are guaranteed to fail due to a dependency failure. It also provides clear feedback about why certain tasks were skipped, making it easier to diagnose and fix issues in the task pipeline.