test_runner: fix flaky retry defects from review

Fix eight defects in the flaky retry loop, each covered by a regression
test added in the previous commit:

- do not retry a flaky expectFailure test that unexpectedly passes
- retry a flaky parent when a failure surfaces only through a subtest
- publish the tracing channel end per attempt so spans stay balanced
- inherit flaky only from a suite parent, not a test to its subtests
- reset the MockTracker between retries
- run a test-level after hook only after the final attempt
- abort the previous attempt's signal so its work is torn down
- publish a flaky expectFailure throw's error to the node.test channel

The remaining intermediate-attempt subtest output inflation across
retries is the documented known limitation and is unchanged.

Signed-off-by: sangwook <rewq5991@gmail.com>

Author: Han5991

lib/internal/test_runner/test.js

@@ -744,7 +744,10 @@ class Test extends AsyncResource {
     this.expectedAssertions = plan;
     this.cancelled = false;
     if (flaky === undefined || flaky === null) {
-      this.flakyRetries = this.parent?.flakyRetries ?? 0;
+      // Inherit flaky only from a suite parent: a suite propagates it to its
+      // test-cases, but a test must not propagate it to its own subtests.
+      this.flakyRetries = this.parent?.reportedType === 'suite' ?
+        (this.parent.flakyRetries ?? 0) : 0;
     } else if (typeof flaky === 'boolean') {
       this.flakyRetries = flaky ? kDefaultFlakyRetries : 0;
     } else if (typeof flaky === 'number') {
@@ -1379,6 +1382,9 @@ class Test extends AsyncResource {
         this.error = null;
         this.passed = false;
         this.cancelled = false;
+        // Restore any mocks applied by the previous attempt so the retry starts
+        // clean (the MockTracker is otherwise only reset in postRun).
+        this.mock?.reset();
         ArrayPrototypeSplice(this.diagnostics, baseDiagnosticsCount);
         // Reset assertion bookkeeping; re-arm the plan if one was requested via
         // the `plan` option so that `t.plan(N)` does not accumulate across
@@ -1405,9 +1411,13 @@ class Test extends AsyncResource {
         // is resolved by the snapshot manager; it is intentionally not reset from
         // here because doing so would corrupt sibling tests in the same file. The
         // final attempt's snapshots win (last-write-wins).
-        // A fresh abort controller per attempt prevents an abort from a previous
-        // attempt's timeout/cancel from being observed as an external abort by
-        // the retry decision below.
+        // Abort the previous attempt's controller so any signal-bound work it
+        // started (e.g. a timer awaiting t.signal) is torn down instead of
+        // leaking. The retry decision for that attempt has already been made, so
+        // this cannot be misread as an external abort. A fresh controller then
+        // backs the next attempt, preventing the previous abort from being
+        // observed as an external abort by the retry decision below.
+        this.abortController.abort();
         this.abortController = new AbortController();
         this.signal = this.abortController.signal;
         afterEach = runOnce(async () => {
@@ -1493,14 +1503,6 @@ class Test extends AsyncResource {
         await afterEach();
         await after();
       } catch (err) {
-        // Publish to the tracing channel's error stream only on the final
-        // attempt; intermediate retries of a flaky test are silent. For a
-        // non-flaky test (maxAttempts === 1) this always fires, as before.
-        const willRetry = attempt < maxAttempts &&
-          !(this.signal.aborted || this.outerSignal?.aborted);
-        if (!willRetry && channelContext !== null && testChannel.error.hasSubscribers) {
-          publishError(err);
-        }
         if (isTestFailureError(err)) {
           if (err.failureType === kTestTimeoutFailure && this.flakyRetries === 0) {
             // Non-flaky timeouts are classified as cancelled, unchanged.
@@ -1516,23 +1518,49 @@ class Test extends AsyncResource {
         } else {
           this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
         }
+        // The pass/fail outcome is now known (e.g. an expectFailure throw is a
+        // pass, so it is not retried). Publish to the tracing channel's error
+        // stream and run the test-level `after` hook only on the final attempt;
+        // intermediate retries of a flaky test are silent and keep `after` for
+        // the end. For a non-flaky test (maxAttempts === 1) the error always
+        // publishes, as before.
+        const willRetry = attempt < maxAttempts &&
+          !this.passed &&
+          !this.expectFailure &&
+          !(this.signal.aborted || this.outerSignal?.aborted);
+        if (!willRetry && channelContext !== null && testChannel.error.hasSubscribers) {
+          publishError(err);
+        }
         try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
-        try { await after(); } catch { /* Ignore error. */ }
+        if (!willRetry) {
+          try { await after(); } catch { /* Ignore error. */ }
+        }
       } finally {
         stopPromise?.[SymbolDispose]();
       }
 
+      // Publish the tracing channel `end` once per attempt so each `start`
+      // (published when the body runs) is paired, keeping spans balanced even
+      // when a flaky test is retried.
+      if (channelContext !== null && testChannel.end.hasSubscribers) {
+        publishEnd();
+      }
+
       // Decide whether to retry based on the resulting state, not on whether the
       // attempt threw: a non-throwing failure via this.fail() must also be
-      // retried. An external abort (user signal, parent, or process cancellation)
-      // is intentional and stops retrying immediately. What remains -
-      // timeouts and ordinary failures - is retried.
-      if (this.passed) {
+      // retried, and a failure that surfaces only through a subtest (promoted to
+      // the parent later, in postRun) is detected here so the parent still
+      // retries. An external abort and an expectFailure verdict stop retrying
+      // immediately.
+      const subtestsFailed = this.flakyRetries > 0 &&
+        ArrayPrototypeSome(this.subtests, (subtest) => !subtest.passed && !subtest.isTodo);
+      if (this.passed && !subtestsFailed) {
         break;
       }
       const shouldRetry =
         attempt < maxAttempts &&
-        !this.passed &&
+        (!this.passed || subtestsFailed) &&
+        !this.expectFailure &&
         !(this.signal.aborted || this.outerSignal?.aborted);
       if (!shouldRetry) {
         break;
@@ -1546,11 +1574,6 @@ class Test extends AsyncResource {
       this.abortController.abort();
     }
 
-    // Publish diagnostics_channel end event if the channel has subscribers (in both success and error cases)
-    if (channelContext !== null && testChannel.end.hasSubscribers) {
-      publishEnd();
-    }
-
     if (this.parent !== null || typeof this.hookType === 'string') {
       // Clean up the test. Then, try to report the results and execute any
       // tests that were pending due to available concurrency.

test/fixtures/test-runner/flaky/parent-subtest-retry.js

@@ -1,17 +1,14 @@
 'use strict';
 // A flaky parent whose only failure comes from a SUBTEST must be retried: the
 // child fails on attempts 1-2 and passes on attempt 3, so the parent must keep
-// retrying and ultimately pass. The state file records the attempt count.
+// retrying and ultimately pass with retryCount === 2.
 const { test } = require('node:test');
 const assert = require('node:assert');
-const { writeFileSync } = require('node:fs');
 
-const stateFile = process.env.FLAKY_STATE;
 let attempt = 0;
 
 test('parent retries when subtest fails', { flaky: 3 }, async (t) => {
   attempt++;
-  writeFileSync(stateFile, String(attempt));
   await t.test('child', () => {
     if (attempt < 3) {
       assert.fail(`child fails on attempt ${attempt}`);

test/parallel/test-runner-flaky.js

@@ -378,10 +378,16 @@ test('flaky: expectFailure that unexpectedly passes is not retried', async () =>
 });
 
 test('flaky: parent retries when only a subtest fails', async () => {
-  const { code, state } = await spawnFlaky(
-    fixtures.path('test-runner/flaky/parent-subtest-retry.js'), 'parent-subtest.txt');
-  assert.strictEqual(state, '3', `parent body ran ${state}x; expected 3 (retry until child passes)`);
-  assert.strictEqual(code, 0, `parent must ultimately pass, got exit ${code}`);
+  // The parent body never throws; the failure surfaces only through the child,
+  // which is promoted to the parent in postRun. The fix detects that before the
+  // retry decision, so the parent retries until the child passes (attempt 3) and
+  // its pass carries retryCount === 2. (Leaked intermediate-attempt children are
+  // the separate, documented subtest-output limitation, so this asserts only the
+  // parent's retry outcome via the run() event stream.)
+  const events = await collectEvents(fixtures.path('test-runner/flaky/parent-subtest-retry.js'));
+  const passes = byName(events.pass, 'parent retries when subtest fails');
+  assert.strictEqual(passes.length, 1, 'the flaky parent must ultimately pass');
+  assert.strictEqual(passes[0].retryCount, 2, 'parent retried until the subtest passed on attempt 3');
 });
 
 test('flaky: tracing channel start/end stay balanced across retries', async () => {