fix: shutdown controller after tests

[mhofman]

closes: #5447

Description

After investigating #5447, I realized that the culprit was the xsnap child process holding onto the kernel bundles through an obscure chain of retainers.

Security Considerations

None

Documentation Considerations

Any new tests creating a controller should add a teardown hook to shutdown the controller.

Testing Considerations

Ran previously exploding terminate test locally with v8 heap snapshots, and confirmed no leaks were occurring.

[turadg]

run-protocol changes LGTM

[mhofman]

I don't know if this change is related, but it looks like Swingset tests now only take 2/3 minutes!

[warner]

Swingset changes look good.. one question about the E() in some non-swingset tests though

[warner]

why the E(controller) ?

[mhofman]

That test setup used that form and doesn't await the result of buildVatController, so I kept it intact 🤷‍♂️

[warner]

ah, ok. I happened to ask MarkM about that pattern today, from some unrelated code, and he said it was a wart that E() allowed the proxy-trap -returned function to be observed that way (E.get would not, and the tildot syntax would probably treat x = target~.method; x() quite differently than x = target~.method(), so he discouraged me from considering using it.

If controller doesn't last more than a few lines, I'd be willing to rewrite the code to do const controller = await ... But yeah, I'm also ok with matching the existing pattern and calling it a day.

[warner]

same

[warner]

is this just hygiene, or is it possible for processes to .on('exit') multiple times?

[mhofman]

It's not possible for 'exit' to be triggered multiple times (however the base EventEmitter logic doesn't know that), and hygiene dictates that events should be removed once no longer used.

[warner]

Wow, good catch. I remember adding explicit c.shutdown()s to the replay tests that explicitly exercise successive launches of the same state, and to tests that deliberately launch xsnap workers, but I didn't think about the environment variables that can cause the entire test suite to use xsnap. I have a vague memory that the lack of .shutdown made the parent process hang (because Node is waiting for all sockets to close before allowing the process to exit?). Did it look like all none of the AVA processes were ever exiting or something?

Did the memory being held seem appropriate for a live controller to be holding? I know there's some amount of memory needed just for the kernel to do its thing, and having a zillion of them in a single AVA process could be a problem, but it sounded like you were seeing old messages being retained too? That would sound like a different problem that might still be an issue.

[mhofman]

xsnap processes were lingering around, and through the pipes from them the whole kernel stayed in memory, including bundles etc. After doing a shutdown, it seems like almost nothing was surviving from one test to another.

The most heap data was the bundles, but there were other/shorter retaining paths to them, which I had to clean up (the stackString logic in SES in particular) before I realized the retaining path was really the xsnap childProcess, at which point I wondered why that was actually there, and realized we needed to explicitely shutdown the controllers.

[warner]

I bet I believe(d) that AVA runs each test file in a separate process, and once all those test cases have finished, the process exits. I bet I also believe(d) that when the process exits, all the child xsnap processes would exit. But 1: I haven't actually tested either belief (I should add some console.log(getpid())s and see if they're distinct), and 2: I suspect AVA is doing something clever with threads that winds up running more than one test file in a single process (based on some GC weirdness we keep seeing), and 3: I can easily imagine that turning into a whole lot of dead kernels being kept in RAM until the larger/aggregate process exits.

If AVA were really enthusiastic about threads, maybe every test it runs would contribute a dead kernel and the VM footprint would grow hugely before finally exiting for real. But only in CI where we use the environment variable to switch on XS on all tests.