684b5a52

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.

Total

50m30s

Created 1 week ago Started 1 week ago Finished 1 week ago

Timeline