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. --- ## Update: classify by origin, not by type @bristermitten tried this out and found the fix was only half of one: the hang was gone, but a test using `expectFailing` that failed through `effectful`'s `Error` effect now failed the suite with `Uncaught exception ...: SomeAsyncException` instead of passing. That is correct, and the cause is older than this PR. This section supersedes "The fix" above, which kept the exception propagating. Since 0.17.0.2, which exceptions a test may fail with was decided by their type: `exceptionHandlers` re-threw anything that was a `SomeAsyncException`. `effectful`'s `Error` effect gives its errors exactly that classification, so a test failing through it was re-thrown out of the suite rather than reported as a failure, and `expectFailing` never got to see it. Before 0.17.0.2 only `AsyncException` was re-thrown, so such a test did fail normally; the fix for #80 widened it and took this case along. The asynchronous runner hanging was the same bug showing up worse. So the classification is what changed. A test runs in a thread of its own, and `waitCatch` on that thread is what catches: - whatever the test's own thread throws is a failure of that test, whatever its declared classification; - an exception delivered to the thread waiting on it (Ctrl-C, a `--timeout`, the cancellation that `race`/`concurrently` sends) interrupts the wait, cancels the test and propagates, which is what #80 needs. `exceptionHandlers` is gone along with the classification it implemented. The asynchronous runner keeps the `Abort` machinery from the commits above: no test written with `it` lets an exception escape now, but a worker that dies must still not leave the collecting thread waiting for a result forever. `Test/Syd/Runner/AsynchronousSpec.hs` therefore changed its premise. It used to make an exception escape by throwing an asynchronously-classified one, which no longer escapes, so it would have quietly stopped testing the hang. It now builds a one-test forest by hand whose test function throws a plain `userError`, which keeps that invariant covered for its own sake. The three new tests in `ExceptionSpec.hs` cover the user-visible behaviour: reported as a failure by each runner, and accepted by `expectFailing`. ### Things to check manually - Every test now runs in a thread `forkIO` makes, not the caller's. Tests in the asynchronous runner already ran on `forkIO` threads, so the only change is for `--threads 1`, where a test used to run on the main (bound) thread. - `exceptionHandlers` was public API through `Test.Syd`'s wholesale re-export of `Test.Syd.Run`, and tests that used to take the suite down now fail, so the 0.33.0.1 release commit became 0.34.0.0 rather than gaining a second release on top. - The smaller alternative not taken: keep classifying by type but narrow the re-throw to `AsyncException` and `AsyncCancelled`. Ten lines, but `System.Timeout.Timeout` is not an exported type, so a `--timeout` could not be told apart from a test's own failure and would stop reporting `Timeout!`. - `nix flake check` failed once on `sydtest-mutation-driver-gen` (`MutateSpec.runMutationMode...`, flaky-passing on retry). Rebuilding that derivation interleaved between branches: `development` 2/10, this branch 0/10, so it is not from this change. Every one of those failures is consistent with `classifySyncExceptionAsKilled` turning a transient exception in the child run into `SuiteKilled Nothing`, which reports a kill with no log. Out of scope here, but worth a look on its own.