fix(repo-refresh): cancel manager-owned timers + run tests in CI#217
Merged
Conversation
… tests in CI Addresses two non-blocking follow-ups from #213 review. Test isolation: createRepoRefreshManager() now tracks every setTimeout it schedules (debounced save, runFetch 60s timeout/SIGKILL grace, waiter timeouts) in a Set and exposes shutdown() to cancel them all. Earlier reliance on timer.unref() was enough to let the process exit, but node:test's resource tracker still observed the pending handle and intermittently reported tests 12-16 as cancelled (per maintainer's report). Tests now register shutdown() via t.after() through a makeMgr(t, opts) helper — no more leak. CI: workflows/ci.yml previously ran only `node bin/cli.js version|help` and `node -c` syntax checks on five files, so "CI green" meant no test ever ran. Adds `node --test test/*.test.js` so future regressions are caught. wsl-windows.test.js skipped on non-win32: the assertion under test relies on backslash path joining which only happens when path defaults to win32. The companion "returns empty on non-Windows" test still validates the cross-platform branch.
Following up on the CI failure for the first attempt at this fix. macOS
runners on node:test reported tests 12–16 as cancelledByParent with the
error "Promise resolution is still pending but the event loop has already
resolved", even though all timers were now cleared.
Root cause: test 11 ("initOnStartup garbage-collects orphan perProject
entries") triggers a fetch for /repos/alive via initOnStartup but never
resolves the mocked execFile call. shutdown() cancelled the 60s
timeoutTimer but left the inflight Promise hanging — stricter node:test
hosts (macOS CI) flag this at test teardown, while my local machine
tolerated it.
Track each runFetch finalize callback in `activeFinalizers` and resolve
every outstanding fetch with a synthetic "cancelled (manager shutdown)"
error state when shutdown() runs. Tests now pass cleanly on the same
runners that failed before.
…into api tests CI second run still failed tests 12–16 with "Promise resolution is still pending but the event loop has already resolved". Root cause is more fundamental than a missing cleanup hook: The four manager timers (runFetch timeoutTimer/killTimer, waiter `to`, debounced saveTimer) were all .unref()'d. unref'd timers are not counted as keeping the event loop alive — so when a test does `await mgr.waitForRefreshOrTimeout(..., 30)`, the only handle keeping the loop alive is the unref'd 30ms timer. node:test sees the loop drain to idle while the test's returned Promise is still pending and flags it. Now that shutdown() exists, .unref() is redundant for graceful host exit. Drop all four .unref() calls so the timers properly keep the event loop alive while in flight; hosts call shutdown() to release them. Also thread t.after(shutdown) through repo-refresh-api.test.js's makeDeps(t, ...) helper — that file ran before repo-refresh.test.js in the CI test order and was leaving timers across the file boundary.
vakovalskii
approved these changes
May 16, 2026
Owner
vakovalskii
left a comment
vakovalskii
left a comment
There was a problem hiding this comment.
🎯 Both follow-ups from #213 review addressed exactly:
Timer leak fixed:
createRepoRefreshManager()now tracks everysetTimeoutin a Set and exposesshutdown()to cancel them all- Tests wire
shutdown()viat.after()through amakeMgr(t, opts)helper — no leak survives a test boundary - Local verification:
node --test test/repo-refresh.test.js→ 16/16 pass, 0 cancelled (was 11 pass + 5 cancelled before)
CI now actually runs tests:
node --test test/*.test.jsadded to.github/workflows/ci.yml- Full suite simulation locally: 79 pass / 0 fail / 2 skipped (cross-platform skips, fine)
Bonus: the pre-existing wsl-windows.test.js failure that everyone learned to ignore is now properly skipped on non-win32 (matches the actual implementation — POSIX path.join on macOS/Linux). 👏
Thanks for the fast turnaround — exactly the kind of incremental hardening that pays off.
This was referenced May 16, 2026
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
3 participants
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
Addresses both non-blocking follow-ups from the #213 review.
createRepoRefreshManager()now tracks everysetTimeoutit schedules (debounced save,runFetch60s timeout + SIGKILL grace,waitForRefreshOrTimeoutwaiter) in a singleSet, and exposesshutdown()to cancel them all. Earlier reliance ontimer.unref()was enough to let the process exit, butnode:test's resource tracker still observed the pending handle and intermittently reported tests 12–16 as `cancelled`. Tests now wireshutdown()viat.after()through amakeMgr(t, opts)helper — no leak survives a test boundary..github/workflows/ci.ymlpreviously ran onlynode bin/cli.js version|helpandnode -csyntax checks on five files, so "CI green 6/6" meant no test ever executed. Addsnode --test test/*.test.jsso future regressions are caught.wsl-windows.test.jsskipped on non-win32. The asserting subtest relies on backslash path joining which only happens whenpathdefaults to win32; on linux/darwin the implementation uses POSIXpath.joinand the mockedexistsSyncset never matches. The companion "returns empty on non-Windows" subtest still validates the cross-platform branch.Test plan
Follow-up to #213.