Skip to content

Don't leave nodes Running forever when a subtree is cancelled - #133

Merged
thomasjm merged 4 commits into
masterfrom
fix-cancelled-nodes-left-running
Aug 29, 2026
Merged

thomasjm merged 4 commits into
masterfrom
fix-cancelled-nodes-left-running

Conversation

@thomasjm

@thomasjm thomasjm commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

printFailureReport walks every node calling waitForTree, which retries on NotStarted and Running. After a cancelled run some nodes were left un-Done for good, so it hung forever — 4.5 hours on CI before I killed it.

Cause

Two gaps, both needed to close it:

  1. runInAsync installed withException (recordExceptionInStatus ...) inside the action passed to managedAsyncWithUnmask, which wraps it in bracketedAction. That bookkeeping has an interruptible atomically, so an exception delivered as the thread started killed it before the handler existed: nothing wrote a status and the parent's Running stood forever.
  2. cancelAllChildrenWith only marked a subtree when the immediate child was NotStarted. If the child was Running it got cancelled, but its own un-started descendants were left behind. It also runs on a thread that is itself unwinding, so it could be interrupted partway and abandon the rest of its children.

f09656d fixed part of (2); the CI hang was (1).

Changes

  • managedAsyncWithUnmaskAndHandler: installs the handler around the whole thread body, bookkeeping included. runInAsync uses it.
  • markUnfinishedNodesDone (RunTree.hs): give a terminal status to everything in a subtree that lacks one. Used by cancelAllChildrenWith and cancelNode, in a finally under uninterruptibleMask_ so it survives being interrupted mid-cancel.
  • runNodesConcurrently: its sync-exception path used a bare mapM_ cancel asyncs, which had gap (2); routed through cancelAllChildrenWith.
  • runInAsync only writes Running if the node is still NotStarted, so a finalized subtree can't be resurrected. Retires the -- TODO: fix race condition with writing to runTreeStatus comment.
  • Export optionsWarnOnLongExecutionMs / optionsCancelOnLongExecutionMs, previously command-line only, so the test can set them.

Test

CancelOnLongExecution.hs: the CI shape (one introduceWith, 8-lane pool, 60 tests) run 20 times with a 200ms cancel, asserting nothing is left un-Done. ~6s. Fails 6/6 runs unfixed, passes 15/15 fixed. Fixing only (1) leaves NotStarted stragglers in 3/10 runs.

Not touched: printFailureReport still calls waitForTree per node from a finalizer, where blocking is never right — by then Sandwich.hs has waited on the roots and every formatter async. Formatters/Print.hs too. Better as its own change.

@thomasjm
thomasjm merged commit 71f7ac9 into master Aug 29, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant