would it be easier to just have a set of tokens that can all be canceled in bulk, and where the active one is removed once its work is complete? seems like less state machine management.

We only want to have one active cancellation source at a time. A new build request should cancel the last active one to avoid unnecessary tasks from running.

We only want to have one active cancellation source at a time.

Sure. But that doesn't mean we need a literal single-field.

A new build request should cancel the last active one

A new build sholud cancel all running tasks IMO. Once a task stops running, it can remove it's CT from the set we're tracking.

to avoid unnecessary tasks from running.

Note the subtlety here. Cancelling a token doesn't stop a task from running. It just signals a request to it to stop running. The task stops running when it actually finally returns.

All i'm suggesting is (IMO at least) a simple formalization here that needs less of a mutable field, and less external changing of that field. Instead, all we'd have is a set of active tasks (or, more explicitly, a set of active cancellation tokens). When we kick of the task, we put it's CTS in that set. When the task is done (for any reason), it removes its CTS. Cancellation is the trivial, we walk all CTSs and cancel them. No need to have a single field that we then have to properly lock and keep track of and whatnot.

Instead, all we'd have is a set of active tasks (or, more explicitly, a set of active cancellation tokens).

@CyrusNajmabadi IMO your approach actually makes the code and contracts more difficult to understand. We currently have a very clearly defined mutable, gate protected state and CTS for the active build:

Moving away from a single active CTS to a set of these loses the 1:1 relationship between everything tracked for current build. Additionally, it does not remove the requirement for locking and such, as we still need to track/update the current in-progress state within locks.

I am hesitant to make such a change, especially as part of this PR which just aims to fix a leak in the current design. If you still feel strongly about your proposed design, we can talk about this further in a separate issue. However, we need to get this leak resolved asap, I am getting multiple pings from perf team on getting this fixed as it is showing up in bunch of their test scenarios.

We implemented a primitive for this in Git Extensions and Project System.

So what Sam shows is similar to what i was thinking about. However, i would not block on this for this PR. I would ask that we storngly consider investing time and effort into large-scale refactorings here for clarity and safety. In this type, for example, things seem very fragile, and i feel there may be other design patterns that would make things better. For example roslyn has many forms of: do async, cancellable work, while moving from one immutable state to another. We shoudl consider if one of those patterns would be more viable here to encapsulate the complexity.

THanks!