Skip to content

test: Memory leak fix in act runner#5025

Closed
pswitchy wants to merge 2 commits into
dequelabs:developfrom
pswitchy:task/act-runner
Closed

test: Memory leak fix in act runner#5025
pswitchy wants to merge 2 commits into
dequelabs:developfrom
pswitchy:task/act-runner

Conversation

@pswitchy
Copy link
Copy Markdown

@pswitchy pswitchy commented Mar 4, 2026

I resolved the MaxListenersExceededWarning by identifying that process.on('exit') listeners were being leaked in each test file. By switching from driver.close() to driver.quit(), I ensured that the ChromeDriver child process is fully terminated rather than just closing the browser window.

1. Robust Teardown Logic

To prevent EADDRINUSE errors and handle cases where the driver might crash, I wrapped the teardown in a try/catch/finally block. This guarantees the server closes even if the driver fails to quit.

after(async () => {
  try {
    // I switched to .quit() to kill the process and clear listeners
    if (driver) await driver.quit();
  } catch (err) {
    console.error('Error quitting driver:', err.message);
  } finally {
    // I use finally to ensure the server always releases the port
    if (server) await new Promise(r => server.close(r));
  }
});

2. My CI Stress Test Configuration

To prove the stability of my fix, I modified the GitHub Actions workflow. I added a strategy matrix to force the suite to run 10 times concurrently on every PR.

test_act:
    runs-on: ubuntu-24.04
    strategy:
      fail-fast: false # I disabled fail-fast so all 10 runs must complete
      matrix:
        run_number: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
    steps:
      - name: Run ACT test suite
        run: npm run test:act

Closes:
#5018

@pswitchy pswitchy requested a review from a team as a code owner March 4, 2026 10:41
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 4, 2026

CLA assistant check
All committers have signed the CLA.

@pswitchy
Copy link
Copy Markdown
Author

pswitchy commented Mar 4, 2026

@straker could you review this?

@straker
Copy link
Copy Markdown
Contributor

straker commented Mar 5, 2026

Thanks for pr. Once the tests runs the 10x and we've shown it doesn't error, we'll need you to remove the 10x runner before we merge.

@straker straker changed the title Memory leak fix in act runner test: Memory leak fix in act runner Mar 5, 2026
@pswitchy
Copy link
Copy Markdown
Author

pswitchy commented Mar 5, 2026

Thanks for pr. Once the tests runs the 10x and we've shown it doesn't error, we'll need you to remove the 10x runner before we merge.

chrome is crashing in these tests, do you need me to do this right now, or do i try to fix the issue first?

Comment on lines +134 to +137
strategy:
fail-fast: false
matrix:
run_number: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
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.

This needs to be removed prior to merging.

await driver.close();
await new Promise(r => server.close(r));
try {
if (driver) await driver.quit();
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.

This is a good fix, but this PR is incomplete. test/aria-practices/apg.spec.js has the same problem. Could you please update it too?

Comment thread test/get-webdriver.js
Comment on lines +8 to +13
const options = new chrome.Options().addArguments([
'--headless',
'--no-sandbox',
'--disable-extensions',
'--disable-dev-shm-usage'
]);
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.

These are reasonable additions, but adding them seems unrelated to this PR.

@straker
Copy link
Copy Markdown
Contributor

straker commented Mar 5, 2026

@pswitchy So testing this out further I don't think this fix by itself fixes the problem. Even after adding @stephenmathieson additions to add the options it still fails. I'm going to close this pr and take on figuring out what to do. My guess is something similar to axe-core-npm and pin the chrome/driver version, but that will be a bit more work in this repo.

@straker straker closed this Mar 5, 2026
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.

4 participants