Skip to content

recycle_on_failure and retries alternative - #157

Open
christiangnrd wants to merge 20 commits into
mainfrom
retries
Open

recycle_on_failure and retries alternative#157
christiangnrd wants to merge 20 commits into
mainfrom
retries

Conversation

@christiangnrd

@christiangnrd christiangnrd commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

While reviewing #148 I wasn't too happy with how the retries feature was reimplementing a simple version of the package so I started looking into reusing the same test machinery for the retries too.

Failing tests show up as yellow if they are to be retried.

Example output:

Running 2 tests using 1 parallel jobs. If this is too many concurrent jobs, specify the `--jobs=N` argument to the tests, or set the `JULIA_CPU_THREADS` environment variable.
               │   Test   │ ──────────────── CPU ──────────────── │
Test  (Worker) │ time (s) │ GC (s) │ GC % │ Alloc (MB) │ RSS (MB) │
fails      (7) │     0.05 │   failed at 2026-08-08T15:10:13.996
passes     (7) │     0.00 │   0.00 │  0.0 │       0.00 │   325.20 │
Retrying 1 failed test  (1)
fails      (8) │     0.05 │   failed at 2026-08-08T15:10:15.526
Retrying 1 failed test  (2)
fails      (8) │     0.00 │   failed at 2026-08-08T15:10:15.731

Test Summary: | Pass  Fail  Total  Time
  Overall     |    1     1      2  3.3s
    passes    |    1            1  0.0s
    fails     |          1      1  0.0s
    FAILURE

Error in testset fails:
Test Failed at REPL[18]:2
  Expression: false

It's mostly working. Only 2 things left to do:

  • Figure out the failing test
  • Fixup docs to sound less like Claude

Close #148.

@christiangnrd
christiangnrd force-pushed the retries branch 2 times, most recently from 1e4b669 to 5dc71e1 Compare August 8, 2026 18:15
@giordano

giordano commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

A review from the bot:

The unification changes retry semantics in ways the docs don't reflect, and there's one correctness edge worth fixing:

The interrupted guard is now dead, and an interrupt can eat failure records. interrupted is only set in the outer catch, which jumps past the retry block — so at the if retries > 0 && !interrupted check it can never be true. Meanwhile the actual interrupt path inside a worker task calls stop_work() and returns, so nothing propagates to the outer catch: the retry block then runs with done[] == true, does filter!(r -> r.test ∉ retry_tests, results.value) up front, and spawns tasks that hit done[] && return before pushing replacement records — the interrupted run's failures silently vanish from the summary. Guarding the retry block on !done[] (instead of the dead interrupted flag) fixes both problems; alternatively, drop each old record only when its retry actually completes rather than bulk-filtering before the phase.

"Single fresh worker" is no longer guaranteed. The retry phase takes whatever comes out of worker_pool and only spawns fresh if it's nothing or dead. In the common case the pool holds nothings (workers stopped at tests_to_start == 0) so you do get a fresh worker — but with serial tests configured, the live serial worker is returned to the pool and can be handed to the retry phase, so retried tests may run on a used worker, and that worker's sibling nothings mean a second live process can exist during retries. Relatedly, the old loop always stopped the worker after a repeat failure; now that only happens with recycle_on_failure=true, so one persistently-failing retry can contaminate the next retried test in the same round. I'd force fresh-worker + recycle-on-nonpass semantics inside retry phases (it's the slow path; the cost is negligible and it's the documented contract) — or, if the reuse is intentional, rewrite the "deliberately quiesced… single fresh worker… stopped before the next retry" paragraph in advanced.md, whose example output ("passed on retry" / "failed again") is stale for the new format anyway. The "retried test runs alone" test still doesn't cover serial_position=:after, which is exactly the configuration where the invariant now breaks.

Smaller items:

  • tests_to_start was initialized to length(tests) and every retry task still does atomic_sub!, driving it negative. Nothing checks == 0 afterwards today, but it silently breaks the counter's invariant for anyone who does later; skip the decrement for retry phases or document why negative is fine.
  • retries > 0 && (io_ctx.nonpass_color[] = :yellow) runs even when retries will be skipped (--quickfail, and the interrupt case above), so failures print yellow with no red pass ever coming. Use the same condition as the retry block.
  • The color flip is written by the coordinator while the printer task may still be draining queued messages from the previous round, so a late-printed earlier-round failure can pick up the final round's red. Cosmetic, but trivially avoided by flipping the color via a printer-channel message instead of mutating the Ref directly.
  • Banner nits: the singular branch of "failed test$(tests_n > 1 ? "s" : " ")" appends a trailing space instead of nothing, and color=:white is near-invisible on light terminals — plain printstyled without a color (or :yellow, matching the scheme) would read better.
  • Negative retries still isn't validated.

@christiangnrd

christiangnrd commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

This is ready for review.

@giordano From the robot's reveiw, I ignored the test_to_start comment, since that was preexisting and only picked up because I moved the code into its own function. Might be worth a separated PR I didn't really look into it. I also ignored the negative retries comment and the banner nit. I purposely added the space so the retry number is always in the same column (I doubt anyone will or should be retrying more than 9 times), and the :white thing has nothing to do with this PR.

@michel2323 Do you mind trying it out to ensure that this keeps the functionality you originally intended with #148?

@christiangnrd

This comment was marked as resolved.

Base automatically changed from move to main August 10, 2026 19:49
michel2323 and others added 14 commits August 10, 2026 16:49
A test that corrupts process-wide state — the motivating case is a GPU
whose driver ends up in a state where every subsequent allocation in the
process fails — poisons every later test scheduled onto the same worker,
turning one bad test into a cascade of failed files. The Distributed-
based harness this package was extracted from recycled a worker after
any failed test; restore that behavior behind `recycle_on_failure =
true`, alongside the existing max-rss and crash recycling.

With `retries = N`, tests that did not pass are re-run up to N times
after the main run completes: sequentially, on a single fresh worker,
with all other workers stopped. Parallel test runs create resource
contention (several workers sharing one GPU or a limited amount of RAM),
so a failure can mean "lost the resource race" rather than "broken":
re-running on an otherwise-idle system distinguishes the two. Tests that
failed due to contention reliably pass on the idle retry, while
deterministic failures fail again and are reported exactly once — only
the final attempt of each test enters the results, and retried tests are
visibly marked in the output.

Both options default to off.
Add a "Failure Handling" section to the advanced usage guide covering both
options: why worker recycling after a failure is useful (process-wide state
corruption cascading onto later tests on the same worker) and what the retry
environment guarantees (all other workers stopped, sequential re-run on a
fresh worker, only the final attempt reported, retry worker recycled after a
repeat failure).

Also mention them in the feature list on the front page, and add a best
practice warning against using retries to paper over genuinely broken tests.
For `recycle_on_failure`, run a fixed sequence of failing and passing tests
with a single job and count the workers created: the default reuses one
worker for all of them, while `recycle_on_failure=true` needs a fresh worker
after each failure.

For `retries`, use a test that fails on its first attempt and passes on any
subsequent one (recording attempts in a file, since each attempt runs in a
different process) to check that a test rescued by a retry is reported as
passing, and that it is the only worker alive while it runs. A persistently
failing test is checked to exhaust its retries and still be reported exactly
once. Also cover that retries are off by default and skipped under
`--quickfail`.

@giordano giordano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking this new design, better integrated with the existing infrastructure. Thanks a lot for working on it! Should this PR be updated on top of main to include #162?

One thought I had was: should we think about an API to only allow certain tests to be retried? You may have few tests that you know are flaky, but don't want to waste time retrying more robust tests that are likely genuinely failing. However I'm concerned this API may become too convoluted, for perhaps little gain.

@christiangnrd

Copy link
Copy Markdown
Collaborator Author

Should this PR be updated on top of main to include #162?

The stack already took care of that!

However I'm concerned this API may become too convoluted, for perhaps little gain.

A lot of my ideas for this package stay ideas for this reason haha. Were you picturing test name filtering or something more fancy? Either way this can probably be implemented in a non-breaking way (ie future PR)

@giordano

Copy link
Copy Markdown
Collaborator

The stack already took care of that!

Oh, I missed these two PRs were stacked, neat.

Were you picturing test name filtering or something more fancy?

Yeah, something like that, but didn't think of how to pass the list of names.

Either way this can probably be implemented in a non-breaking way (ie future PR)

Uhm, ok, you're probably right.

@giordano giordano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michel2323 it'd be nice to get your feedback, and if you can test it, but this looks good to me

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.

3 participants