fix(rush): treat non-terminal statuses as non-terminal

[aramissennyeydd]

Summary

The pnpm-sync plugin is triggering multiple times during cobuilds instead of a single time after a proper build. This may be contributing to an increased rate of Writer already closed errors that we've been seeing.

Details

While debugging a failure when turning on injected-deps and pnpm-sync, I came across a consistent Writer already closed error. I think this is due to pnpm-sync trying to write debug logs to a closed terminal (investigation still pending) - however, as part of my investigation, I saw pnpm-sync copying the same files over and over again with a log like

pnpm-sync: Starting operation for /packages/lib-doordash-api/node_modules/.pnpm-sync.json
--
  | pnpm-sync: Synced 101 files in 6 ms
  | Get completed_state()_package(@doordash/lib-dd-server)_phase(_phase:test): null
  | pnpm-sync: Starting operation for /packages/lib-dd-server/node_modules/.pnpm-sync.json
  | pnpm-sync: Synced 8 files in 1 ms
  | Get completed_state()_package(@doordash/lib-dd)_phase(_phase:test): null
  | pnpm-sync: Starting operation for /packages/lib-dd/node_modules/.pnpm-sync.json
  | pnpm-sync: Synced 20 files in 2 ms

I'm pretty confident this is caused by the afterExecuteOperation hook actually getting called for states like Executing and Waiting instead of only being called for terminal statuses.

How it was tested

Ran a cobuild against the sharded test repo we have and ran a cobuild locally and verified that I only saw pnpm-sync logs for completed operations instead of all cobuild attempts.

Impacted documentation

[dmichon-msft]

This goes after _afterExecuteOperation because that call can change the status (in addition to the status change in the error handler)

[aramissennyeydd]

@dmichon-msft From my read of this code, _onOperationComplete expects (or at least expected) a terminal operation to then handle propagating various states to other operations - the actual status changes happen in OperationExecutionRecord.executeAsync?

[dmichon-msft]

The behavior of _onOperationComplete varies depending on the current status of record.status, and both this._afterExecuteOperation(record) and the error handler thereof are allowed to alter that status, importantly by potentially changing a success to a failure.

Even more importantly, _onOperationComplete is the call that unblocks the execution of dependent operations, and various plugins use afterExecuteOperation to perform work that must complete before those dependent operations can start.