Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Oct 21, 2025

When starting a test run, delete all old Emscripten files in the temp directory to help avoid runaway temp file leaks filling up a CI system hard drive.

Do it before starting a run, so that developers can still access test files in debug logs.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 21, 2025

If this specific to the parallel test runner or should it happen on startup in general?

We do have EMTEST_DETECT_TEMPFILE_LEAKS which has a mechansim to detect leaks, so I would hope they do not happen much in practice.

It looks like we enable EMTEST_DETECT_TEMPFILE_LEAKS as the main CI, but not for browser tests.

@juj
Copy link
Collaborator Author

juj commented Oct 21, 2025

If this specific to the parallel test runner or should it happen on startup in general?

This should run in both singlethreaded and parallel runner startup.

We do have EMTEST_DETECT_TEMPFILE_LEAKS which has a mechansim to detect leaks, so I would hope they do not happen much in practice.

Yeah, I remember creating that a long time ago to combat temp file leaks. Added that env. var. now to my CI bots, but wanted to ensure that even if these leaks do happen, that they won't be able to take my CI bots down.

I see on all my CI bots that there are a bunch of emtest_ directories that have crept up over time. Some of those are definitely caused by test harness runs aborting mid-flight (e.g. due to CI disconnect, which aborts in buildbot).

@sbc100
Copy link
Collaborator

sbc100 commented Oct 21, 2025

If this specific to the parallel test runner or should it happen on startup in general?

This should run in both singlethreaded and parallel runner startup.

We do have EMTEST_DETECT_TEMPFILE_LEAKS which has a mechansim to detect leaks, so I would hope they do not happen much in practice.

Yeah, I remember creating that a long time ago to combat temp file leaks. Added that env. var. now to my CI bots, but wanted to ensure that even if these leaks do happen, that they won't be able to take my CI bots down.

I see on all my CI bots that there are a bunch of emtest_ directories that have crept up over time. Some of those are definitely caused by test harness runs aborting mid-flight (e.g. due to CI disconnect, which aborts in buildbot).

Yes, I think we explictly do not clean stuff up when the run is interrupted, since this is good for debugging.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 21, 2025

Looking into this I noticed we were creating a lot more temp directories than we needed to: #25615

@juj
Copy link
Collaborator Author

juj commented Oct 21, 2025

Yes, I think we explictly do not clean stuff up when the run is interrupted, since this is good for debugging.

Agreed. That is why the idea here was to do the cleanup at startup, and not at end (or at atexit)

juj added 2 commits October 22, 2025 02:39
… directory to help avoid runaway temp file leaks filling a CI system temp directory.
@juj juj force-pushed the cleanup_emscripten_temp branch from 175321b to d5c6e1a Compare October 21, 2025 23:39
@sbc100
Copy link
Collaborator

sbc100 commented Oct 21, 2025

Yes, I think we explictly do not clean stuff up when the run is interrupted, since this is good for debugging.

Agreed. That is why the idea here was to do the cleanup at startup, and not at end (or at atexit)

Sounds good to me.

I would feel more comfortable if it was just the emtest_* that we cleanup, since that is obviously stuff created by the test harness itself. Are you times when the compiler itself is not cleaning up its stuff too?

@juj
Copy link
Collaborator Author

juj commented Oct 21, 2025

Are you times when the compiler itself is not cleaning up its stuff too?

The majority of the items are from emtest_, but I did see some emscripten_ there as well, which is why I added that pattern there as well.

I think combining this with running with EMTEST_DETECT_TEMPFILE_LEAKS should ensure that there won't be leaks in that area on emscripten_ either.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 22, 2025

I found some more usages of tempfile that we can completely remove : #25617

try:
utils.delete_dir(os.path.join(shared.TEMP_DIR, entry))
except Exception:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using force_delete_dir here... then I think you don't need the try/catch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

force_delete_dir() raises an exception if the deletion failed. The thinking here was that it wouldn't be catastrophic if that happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough

@juj juj enabled auto-merge (squash) October 22, 2025 00:19
@juj juj merged commit c81dbdd into emscripten-core:main Oct 22, 2025
34 checks passed
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.

2 participants