Created Started Cancelled
Fix the asynchronous runner hanging on exceptions that escape a test Fixes #110 ## The bug A test that throws an exception classified as asynchronous makes the asynchronous runner hang. The minimal repro from @bristermitten: ```haskell data Boom = Boom deriving (Show) instance Exception Boom where toException = asyncExceptionToException fromException = asyncExceptionFromException main = sydTest $ it "boom" (throwIO Boom :: IO ()) ``` Before this change, with the asynchronous runner: ``` repro: thread blocked indefinitely in an MVar operation ``` After, matching what the synchronous runner has always done: ``` repro: Boom ``` This shape is not exotic: `effectful`'s `Error` effect uses exactly the same `asyncExceptionToException` design, which is how @bristermitten ran into it. ## The regression test `sydtest-test/test/Test/Syd/Runner/AsynchronousSpec.hs` defines an exception whose `toException` is `asyncExceptionToException`, runs a one-test forest that throws it, and asserts that the runner re-throws it instead of hanging. One test per asynchronous entry point (`runSpecForestAsynchronously` and `runSpecForestInterleavedWithOutputAsynchronously`), each wrapped in a 10 second `System.Timeout.timeout` so a regression shows up as a failure rather than a stuck suite. Both tests fail by timing out on `development` and pass with the fix. ## The root cause `Test.Syd.Run.exceptionHandlers` re-throws `SomeAsyncException` instead of turning it into a test result. That is deliberate (#80): swallowing it would break `race`/`concurrently` and Ctrl-C. In the asynchronous runner, that re-thrown exception escapes `job` before it reaches `putMVar var result`, so: - the worker thread dies, and `withJobQueueWorkers` uses `withAsync` without `link`, so nothing notices; - the runner's `blockUntilDone` sees the worker's count released and carries on, so the runner finishes normally; - the printer/waiter thread is left in `race (readMVar failFastVar) (takeMVar var)` on a result nobody will ever put, so `concurrently` waits forever on it. ## The fix The fail-fast `MVar ()` becomes an `MVar Abort` that says *why* the suite is stopping: `AbortFailFast` or `AbortException SomeException`. - A worker records an exception escaping the test in that var before re-throwing it. - The printer and waiter now share `awaitTestResult`, which races the abort var against the result var: fail-fast still means "no result for this test", an exception is re-thrown. - The re-throw makes `concurrently` cancel the runner, so the exception leaves `runSpecForest*Asynchronously` the way it leaves the synchronous runners. I kept the existing behaviour of letting the exception propagate rather than reporting it as a test failure, so this does not walk back #80 and the two runners now agree. If you would rather see such a test reported as a failure, that is a separate decision and a bigger change. ### One extra hang fixed along the way `putMVar failFastVar ()` is now `tryPutMVar`. With `--fail-fast` and two tests failing at the same time, the second worker blocked on an already full `MVar`, which left it counted as working forever and hung the runner at its next `waitForWorkersDone`. There is no regression test for this one: it needs two workers to fail inside the same window, which I could not make deterministic without adding hooks to the runner. ## Things to check manually - `nix flake check` passes here. During one run the `sydtest-test` derivation failed, but the kernel log showed a global OOM kill in that window and the same derivation built clean afterwards; worth a second look if you see it too. - The `Abort`/`abortVar` naming: the var is no longer only about fail-fast, so I renamed it, which is most of the diff's line count. - Whether re-throwing rather than reporting is the behaviour you want long term (see above). - Fail-fast behaviour in the asynchronous runner in general, since `tryPutMVar` changes which worker wins the race to set it (nothing reads the payload differently, but it is the one behavioural change outside the bug). --- ## Update: the trim, and a crash my regression test was causing Two follow-up commits. **The trim.** The catch around a test is now `recordEscapingExceptions`, a named tee: it re-throws whatever it caught, so `SomeException` there does not change which exceptions reach the caller, only which one is reported if several race. Having a name for it means the reason the catch has to be that broad is documented once, at the definition, instead of inline at the call site: *any* exception escaping before the result is put leaves the collecting thread waiting forever. The runner's abort check also no longer enumerates both constructors, since both stop the scheduler identically. **The crash.** The `sydtest-test` suite aborted with `free(): invalid pointer` in 2 of 4 full `nix flake check` runs, always mid-line during output. It was my regression test, not the fix. Each test tears a nested runner down mid-flight, which cancels its worker, printer and scheduler threads; doing that alongside the rest of the suite is what destabilised it. Marking the spec `sequential` fixes it. I isolated it by load-testing the `sydtest-test` derivation while rebuilding the other checks concurrently: | variant | failures | | --- | --- | | `development` | 0/4 isolated, 0/2 under load | | fix, new spec deleted | 0/3 under load | | fix + spec running in parallel | 1/2 under load, 2/4 full flake checks | | fix + spec marked `sequential` | 0/4 under load | Worth your judgement: a real user hitting this bug gets the exception and the process exits, so a runner torn down mid-flight never has to coexist with continued work. My test is unusual in carrying on afterwards, which is why I think `sequential` is the honest fix rather than papering over something users would hit. But it does suggest cancelling a sydtest runner and then continuing to use the process is not safe, and I did not chase that to a root cause. Supersedes the OOM note in "Things to check manually" above: the first failure coincided with a real kernel OOM event, which sent me down the wrong path, but the crash was reproducible and is explained above.