feat: lake: make job cancellation a first-class JobState flag - #14835
feat: lake: make job cancellation a first-class JobState flag#14835dennj wants to merge 2 commits into
Conversation
061889d to
7df762f
Compare
|
Reference manual CI status:
|
|
Mathlib CI status (docs):
|
7df762f to
f7bce4a
Compare
|
@marcelolynch is this the kind of native cancellation you were looking for? |
0b087a3 to
c5407c5
Compare
c5407c5 to
7b120ab
Compare
| let imports ← match (← (← root.transImports.fetch).wait) with | ||
| | .ok imports _ => pure imports | ||
| | r@(.error ..) => | ||
| -- Canceled is not a bad import; fail silently (the module's own job reports it). | ||
| if r.isCanceled then failure | ||
| error s!"bad imports (see the '{root.name.toString}' job for details)" |
There was a problem hiding this comment.
Both these cases should probably be handled instead via a variant of wait? that checks the result for cancelled itself and rethrows on canceled (so, it would need to be JobM instead of BaseIO). The other uses of wait? are also impacted by the new cancellation (they are the job monitor caveats), which indicates that wait? is correct vector for this check.
There was a problem hiding this comment.
Added Job.waitUnlessCanceled? : Job α → JobM (Option α), which rethrows via cancelJob on a canceled job, so both sites are back to their original let some x ← … | fallback shape.
Both callers run in FetchM, and lifting JobM into FetchM keeps only the log, not the canceled flag. So at these two sites a cancellation surfaces as a plain error without the flag (as it did with the hand-rolled version). The monitor's wait? uses are BaseIO, so they'd need an explicit isCanceled check instead. I left it here, since a canceled top-level job isn't reachable under --fail-fast alone
This PR makes job cancellation under
--fail-fasta first-class notion: canceled jobs are reported as⊘ Canceledrather than as successes, and a canceled dependency is no longer misreported as abad import.This is the follow-up anticipated in #14797's review, where cancellation is recognized by convention (a trace-level log entry with a marker string). Here it is recorded natively as
JobState.canceled, a per-result flag alongside the existingwantsRebuild, which already has exactly this shape (a per-resultBool, merged with||, read by the monitor). Precedence is therefore defined once, inJobState.merge, for every combinator: failure still dominates, because the monitor keeps computingfailedfrom the log exactly as before and treats a job as canceled only when it did not itself fail.JobResult.isCanceledreads the flag; the marker string and its "for internal use only" constant are removed.Unlike the first-class attempt in #13075, the error type is unchanged. Cancellation rides in state that already flows through every combinator, so
Job.await,ensureJob, and job registration are untouched and no boundary has to translate a cancellation into something lossy, which is what caused the diagnostic-loss and fabricated-error bugs the earlier approach had. Exit codes,--wfail, and log replay are unaffected sincefailedis computed as before.What is still missing
leanprocess already running still elaborates to completion, so fail-fast exit time is bounded by the slowest in-flight module. Killing it cleanly needs thecanceledstate this PR introduces.--fail-fastis still the only thing that sets the token. Graceful Ctrl-C handling and build timeouts are natural future consumers.