test: clean up leaked driver fixture temp dir in residue test - #1170
Open
joelmitz wants to merge 1 commit into
Open
test: clean up leaked driver fixture temp dir in residue test#1170joelmitz wants to merge 1 commit into
joelmitz wants to merge 1 commit into
Conversation
The "a staged input leaves nothing behind, whether the call succeeds or fails" test creates two mkdtemp()'d directories under the "agmsg-sync-driver-residue-" prefix: `root` (holding the successful "ok-driver.sh" fixture) and `bad` (holding the failing fixture, used via withDriverEnvironment). Only `bad` was ever removed -- its withDriverEnvironment(t, bad, ...) call registers its own t.after cleanup, but `root`'s own t.after only restored the AGMSG_SYNC_ROSTER_DRIVER env var and never removed the directory itself. This leaked exactly one directory per test-file run, 100% of the time, regardless of pass/fail outcome. Confirmed empirically: 206 leftover directories matched exactly 206 real invocations of this test file during investigation of fujibee#1106, and after another 200 invocations the count moved to 406 -- always +1 per run, never more, never fewer. Content is always the same static, hardcoded fixture script with no dynamic data, so this is a test-hygiene leak rather than anything affecting message/data safety. Fix mirrors the existing pattern already used for `bad` elsewhere in this same file: register a t.after that removes `root` with the same "unsafe test root" tmpdir() guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QPHuasJVLc4z6CA47C2kN
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.
Summary
The
"a staged input leaves nothing behind, whether the call succeeds or fails"test intests/remote_sync_engine.test.mjscreates twomkdtemp()'d directories under theagmsg-sync-driver-residue-prefix within the same test run:root(holding the successfulok-driver.shfixture) andbad(holding the failing fixture, passed throughwithDriverEnvironment). Onlybadwas ever cleaned up — itswithDriverEnvironment(t, bad, ...)call registers its ownt.aftercleanup, butroot's ownt.afteronly restores theAGMSG_SYNC_ROSTER_DRIVERenv var and never removes the directory itself.This leaks exactly one directory per test-file run, 100% of the time, independent of pass/fail outcome. Content is always the same static, hardcoded fixture script (no dynamic/message data), so this is a test-hygiene leak rather than anything touching message safety.
How I found this
While investigating #1106 (unrelated non-zero-exit issue) I ran this test file's driver-input tests several hundred times on Windows and noticed
%TEMP%accumulatingagmsg-sync-driver-residue-*directories at exactly +1 per invocation. Traced it to the missing cleanup above; empirically confirmed the count matches the invocation count exactly across 206 and then 406 runs (never more, never fewer).Fix
Mirrors the existing pattern already used for
bada few lines below in the same test: register at.afterthat removesroot, guarded by the same"unsafe test root"tmpdir()check used elsewhere in this file.Test plan
node --test --test-name-pattern 'whole input, from the start|leaves nothing behind' tests/remote_sync_engine.test.mjs— stillpass 2 / fail 0, exit 0bats --print-output-on-failure --filter "driver-input" tests/test_remote_sync_driver_input.bats) repeatedly before and after the fix; before the fix the leftover-directory count grows by 1 each run, after the fix it does notScope
This PR is the code fix only. It intentionally does not delete the directories that have already accumulated on any given machine from before this fix — that's a separate operational/cleanup decision, out of scope here.