Forward CancellationToken through the build pipeline

[rdeago]

Summary

bv subcommands received a CancellationToken but discarded it, so Ctrl+C could not
interrupt an in-flight restore/build/test/pack — the spawned dotnet ran to
completion on its own. This threads the token end-to-end.

• BuildPipeline (RunThroughAsync/RunRangeAsync/RunAsync + step methods) and
  DotNetService (the five public methods + RunDotNetAsync) now take and forward a
  CancellationToken to the process runner. RunRangeAsync stops launching further
  steps once the token is signalled; CleanAsync honors it between per-project deletions.
• On cancellation, CliWrap kills the entire dotnet process tree (no orphaned children)
  and throws OperationCanceledException; Program maps that to exit code 130.
• DocFxService (in-process DocFX API) and GitService (LibGit2Sharp) spawn no child
  processes, so they're untouched.

Test plan

• dotnet bv pack — clean/restore/build/test/pack all green; artifacts produced.
• ReSharper inspectcode --severity=WARNING — 0 results.
• Manual: Ctrl+C during bv build terminates dotnet promptly and bv exits 130
  (not exercised — interactive; code path + CliWrap contract verified instead).

Closes #282

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 2m 10s —— View job

---

PR Review

• Gather context
• Read changed files
• Analyze changes
• Post review feedback

---

Overall this is a clean, correct, and complete implementation. The token threads end-to-end with no gaps, and the exit-code mapping is exactly right. A few observations below — none are blockers.

---

Correctness

RunRangeAsync check placement (BuildPipeline.cs:78) — placing ThrowIfCancellationRequested() before each RunAsync call (not just at the bottom of the loop) is the right call. Cancellation between steps is fast and doesn't race with an in-flight dotnet process. Good.

CleanAsync loop check (BuildPipeline.cs:111) — the five solution-level deletes at the top (.vs, caches, temp, artifacts, test results) happen before any cancellation check, but those are fast, fixed-count operations; the check in the per-project loop is where it matters. No issue.

OperationCanceledException catch ordering (Program.cs:112) — correctly placed before BuildFailedException so exit 130 wins unambiguously. Exit code 130 (128 + SIGINT) is the right POSIX convention.

NuGetPushAllAsync between-push gap (DotNetService.cs:234-243) — there's no explicit ThrowIfCancellationRequested() between packages, but each _processRunner.RunAsync(...) receives the token and will throw if it's already signalled at call time. Consistent with the rest of the PR.

ReleaseCommand — CreateReleaseAsync / PublishAsync without a token (ReleaseCommand.cs:84, 283) — these are GitHub API calls, not child-process spawns, so leaving them token-free is consistent with the PR's stated scope. Acceptable.

---

Minor observations

CleanAsync is synchronous (BuildPipeline.cs:101-118) — the method takes a CancellationToken, does purely synchronous work, and returns Task.CompletedTask. This is pre-existing; the PR only adds the loop check. No action needed, just noting it for awareness.

OperationCanceledException message (Program.cs:114) — "Operation cancelled." is technically accurate but could be slightly more actionable (e.g. "Build cancelled." or "Cancelled." to match what bv does). Purely cosmetic — leave it if you like it as-is.

---

One thing worth double-checking

The comment on line 95 (// Safe: the handler is removed in the finally below before cts is disposed at end of scope.) was already there before this PR. There is a narrow theoretical race: the OS could queue a Ctrl-C event just after Console.CancelKeyPress -= OnCancel runs but before the event handler has finished executing on its thread, which would call cts.Cancel() after cts is disposed. In practice this window is negligible and this is pre-existing behavior, so it's out of scope — but it's worth keeping in mind if this area is ever revisited. The existing comment slightly overstates the safety guarantee.

---

Summary: no bugs, no style violations, and the token is forwarded correctly through every code path in scope. Ready to merge.

[rdeago]

Thanks for the review! On the one point you flagged as out-of-scope: rather than just softening the comment on Program.cs:95, I closed the race it described.

Console.CancelKeyPress -= OnCancel doesn't wait for an in-flight handler invocation, so a Ctrl-C landing just before the finally could call cts.Cancel() after cts.Dispose() — throwing ObjectDisposedException on the handler thread. I replaced the using var cts with explicit disposal in the finally, and put both cts.Dispose() and the handler's cts.Cancel() behind a shared Lock (the handler no-ops once the source is disposed). The two can no longer race.

The two cosmetic notes (the "Operation cancelled." wording and CleanAsync being synchronous) I've left as-is.

@claude please review again.

[claude]

Claude finished @rdeago's task in 2m 31s —— View job

---

PR Review (follow-up)

• Gather context
• Read updated Program.cs
• Analyze the Lock-based fix for the cts disposal race
• Post review feedback

---

The fix is correct. Here's the full reasoning:

How the race is closed

The critical invariant is that cts.Cancel() and cts.Dispose() can never run concurrently — they both hold cancelGate before touching cts. Three interleavings are possible once finally starts:

Handler runs...	Result
Entirely before finally acquires the lock	Cancel() → ctsDisposed = true; Dispose() — safe, Cancel-before-Dispose is well-defined
Concurrently, handler wins the lock first	Same as above — the lock serializes them
Concurrently, finally wins the lock first	ctsDisposed = true; Dispose() → handler sees ctsDisposed, returns without calling Cancel()

ObjectDisposedException on the handler thread is now impossible.

Points checked

e.Cancel = true is set before the lock (Program.cs:99) — this is intentional and correct. In the no-op path (where ctsDisposed is already true), suppressing the default Ctrl-C termination is harmless: we're in the finally block, the command has finished, and the process is about to exit normally anyway. Setting it unconditionally also avoids the need to unset it in the non-no-op path.

System.Threading.Lock — the .NET 9+ Lock type (not object), confirmed correct given the project targets the .NET 10 SDK (global.json). The lock statement on a Lock uses Lock.EnterScope() rather than Monitor.Enter(), so there's no accidental box-and-lock-separate-object mistake possible here.

// ReSharper disable once AccessToModifiedClosure (Program.cs:103) and // ReSharper disable once AccessToDisposedClosure (Program.cs:109) — both suppressions are correctly placed and correctly motivated. The first suppresses the false-positive on the intentionally-mutable ctsDisposed flag; the second suppresses the false-positive on cts.Cancel() (which is safe because the lock guarantees Dispose() hasn't run yet).

ctsDisposed = true is set before cts.Dispose(), not after — this is the right order. Setting the flag first ensures any concurrent handler that acquires the lock sees true and skips Cancel(), even during the instant Dispose() is executing.

Comment accuracy (Program.cs:89–92) — the updated comment correctly describes the actual hazard and the chosen mitigation. No overstatement.

---

Nothing to change. The fix is minimal, correct, and well-commented. Ready to merge.