Visitar URL original
Avoid finish retry, ensuring teardowns run only once by ump45nose · Pull Request #3488 · avajs/ava · GitHub
Skip to content

Avoid finish retry, ensuring teardowns run only once - #3488

Merged
novemberborn merged 4 commits into
avajs:mainfrom
ump45nose:fix/3482-idempotent-finish-teardowns
Oct 31, 2026
Merged

Avoid finish retry, ensuring teardowns run only once#3488
novemberborn merged 4 commits into
avajs:mainfrom
ump45nose:fix/3482-idempotent-finish-teardowns

Conversation

@ump45nose

@ump45nose ump45nose commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

When a test returns a promise and times out (or is finished due to inactivity), finish() runs teardowns immediately. The original promise can still settle later and call finish() again, re-running every t.teardown() hook. Non-idempotent teardowns then throw (double server.close(), duplicate temp-dir removal, etc.).

Changes

  • Cache finishPromise so concurrent/late finish() calls share one completion
  • Drain this.teardowns with splice(0) before iterating so hooks cannot run twice

Fixes #3482

Test plan

  • npx tap test-tap/test.js — 146/146 pass (includes new timeout + inactivity regression cases)
  • npx tap — 1631 pass / 1 fail (pre-existing test-tap/reporters/tap.js snapshot drift on Node internal line numbers; unrelated to this change; same class of failure seen on open fix: skip file-less stack frames in extractSource #3486 CI)
  • npx test-ava test/test-timeouts/test.js test/idle-timeouts/test.js — 3/3 pass
  • Manual check: timeout path still invokes finish() twice, but teardown spy callCount === 1

AI assistance disclosure

Prepared with AI assistance; reproduction, patch, and test results were executed and reviewed on this machine.

@ump45nose ump45nose mentioned this pull request Sep 29, 2026
Closed
novemberborn
novemberborn requested changes Oct 8, 2026
View reviewed changes

@novemberborn novemberborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove the reporter changes.

Comment thread lib/test.js Resolved
@ump45nose

ump45nose commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Reporter changes are already out of this PR (current diff is only lib/test.js and test/teardowns.js). Re-requesting review.

AI assistance disclosure: I wrote this comment with help from Grok Bot.

novemberborn
novemberborn requested changes Oct 27, 2026
View reviewed changes

@novemberborn novemberborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR @ump45nose. The finishing once guard approach is right, but the implementation should take a different approach.

Comment thread lib/test.js Resolved
Comment thread lib/test.js Resolved
Comment thread lib/test.js Resolved
Comment thread test/teardowns.js Resolved
@ump45nose

ump45nose commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @novemberborn — addressed in f173acf:

  • Renamed the previous finish() body to private #doFinish()
  • Public async finish() is now the once-guard
  • Reused finished (default null) for the shared promise instead of finishPromise
  • Dropped the teardown splice(0)
  • Moved the regression tests into test-tap/test.js and deleted test/teardowns.js

pnpm exec tap test-tap/test.js → 155/155 pass.

@ump45nose
fix: run teardowns only once when finish is re-entered
8d0b77c 
Guard finish() so concurrent/late calls share one completion via the
finished promise (null default). Keep the previous body as private
#doFinish(). Regression coverage lives in test-tap/test.js.

Addresses review feedback on avajs#3488. Fixes avajs#3482.
@ump45nose
ump45nose force-pushed the fix/3482-idempotent-finish-teardowns branch from f173acf to 8d0b77c Compare October 28, 2026 06:42
@ump45nose

ump45nose commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main as a single commit (reporter snapshot commits dropped; tip is only lib/test.js + test-tap/test.js). pnpm exec tap test-tap/test.js → 155/155.

Ready for another review.

@ump45nose
fix: keep formatAsCodeBlock when making finish idempotent
362d104 
The earlier rebase dropped formatAsCodeBlock from compareWithSnapshot,
which broke code-block snapshot report generation.
@ump45nose

ump45nose commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: the earlier rebase accidentally dropped formatAsCodeBlock from compareWithSnapshot, which broke the code-block snapshot report tests. Restored in 362d104; the tip is again only the idempotent finish() / teardown change on top of current main.

@ump45nose
test: normalize #doFinish frames in TAP reporter logs
6252b1e 
Private #doFinish appears in V8 stacks on some platforms (Ubuntu CI)
while committed reporter logs expect Test.finish. Sanitize so the
idempotent-finish refactor does not churn OS-specific stacks.
@ump45nose
test: expect #doFinish in TAP logs; drop reporter sanitizer
bdabc1e 
Private #doFinish shows up in V8 stack frames after the review rename.
Update the checked-in reporter logs instead of rewriting frames in the
reporter (per review: no reporter changes).

Signed-off-by: Vincent <1729065730@qq.com>
@ump45nose

ump45nose commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@novemberborn All five review threads are addressed on the current tip (bdabc1e): once-guard via #doFinish() / async finish() with finished holding the promise, teardowns splice removed, and regression tests under test-tap/test.js. Ready for another look when you have a moment.

(AI-assisted reply.)

novemberborn
novemberborn approved these changes Oct 31, 2026
View reviewed changes

@novemberborn novemberborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lovely, thanks for the follow-up @ump45nose.

@novemberborn novemberborn changed the title fix: run teardowns only once when finish is re-entered Avoid finish retry, ensuring teardowns run only once Oct 31, 2026
@novemberborn
novemberborn merged commit 3eca603 into avajs:main Oct 31, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@novemberborn novemberborn novemberborn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

(High Severity Concurrency Bug): Double Teardown (t.teardown()) Invocation on Test Timeout or Inactivity

2 participants

@ump45nose @novemberborn