Fix Harness process lingering after tests when diagnostics are enabled#159
Merged
Conversation
…is enabled
With diagnostics enabled, instrumented() collapsed every thenable through
.then(), turning nano-spawn Subprocess objects (Promise & AsyncIterable &
{ nodeChildProcess }) into plain promises. Teardown's stopSubprocess()
then found no nodeChildProcess to kill, so background adb logcat /
app-launch processes survived and kept the harness process alive after
tests finished. Track spans as a side effect and return the original
thenable untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this?
This PR fixes a hang where the Harness process kept running after all tests finished whenever diagnostics were enabled (via the
diagnosticsconfig option or theRN_HARNESS_DIAGNOSTICSenv var). Runs with diagnostics off exited cleanly; the same runs with diagnostics on lingered until killed manually or by a CI timeout.How does it work?
When diagnostics are enabled, platform modules like adb and simctl are wrapped with the
instrumented()proxy to record timing spans. The wrapper treated every thenable result as an ordinary promise and returnedresult.then(...)— but calling.then()on a thenable always produces a brand-new plainPromise, discarding any extra shape the original object had.That's fatal for nano-spawn's
Subprocessreturn value (used byadb.startLogcatandsimctl.launchAppProcess), which is a promise plusnodeChildProcessandSymbol.asyncIterator. With those stripped, teardown'sstopSubprocess()foundnodeChildProcess === undefined, the kill attempt threw into an intentionally silent catch, and the backgroundadb logcat/ app-launch OS processes survived — keeping Node's event loop alive after the run finished. Log streaming viafor awaitover the subprocess broke the same way.The wrapper now records span completion as a side-effect
.then()and always returns the original thenable untouched, so subprocess handles (and any other hybrid thenables) pass through instrumentation with their shape intact. A regression test covers the hybrid-thenable case, and a version plan is included.Why is this useful?
adb logcatand app-launch subprocesses are reliably killed during teardown, so log streaming and crash detection behave identically with and without diagnostics.