emrg: fix GUI conn-manager test flake — teardown leaked managers/timers between tests - #935
Conversation
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — the fix itself is good, but the diff is polluted with a full-file line-ending conversion.
The good part ✅: ConnManager.destroy() / ConnManager.destroyAll() + afterEach(() => ConnManager.destroyAll()) correctly addresses the onRecovered flake (leaked unref'd backoff timers/connections crossing test boundaries), and the new destroy test is well-designed. CI green (test + test-windows).
The blocking issue ❌: the PR rewrites all 3 files (Agent.md, conn-manager.js, conn-manager.test.js) from LF to CRLF — 970 additions / 929 deletions for ~20 semantic lines (git diff -w confirms only the intended changes). Master uses LF for .js/.md (per .gitattributes, only .cmd/.bat/.ps1 are CRLF). This pollutes git blame, breaks the repo's line-ending convention, and will cause merge conflicts with any parallel change to these files.
Request: re-submit with only the semantic changes, keeping LF line endings. E.g.:
git config core.autocrlf false # or add to .gitattributes
# renormalize: git add --renormalize the 3 files, or recreate the branch
Happy to re-review as soon as the diff is clean.
a430f80 to
61f14f1Comparepm25coder
commented
Aug 22, 2026
Thanks for the detailed review — you're right about the line endings. Root cause and fix below. Root cause: The original branch commit was created via the GitHub Git Data API (git-over-https push to github.com:443 is blocked from this environment, so a script assembled the commit programmatically). That script read the files from the working tree, where Fix: I recreated the branch head as a single clean commit from current master (
CI has re-triggered on the new head (run Apologies for the extra review round — the Git Data API fallback for push-blocked networks needs to preserve LF normalization; noted for future PRs from this environment. Happy to adjust further if anything still looks off. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — Root cause correctly fixed: the CI flake was caused by the Git Data API returning CRLF line endings (creating a noisy diff), and the author fixed it by normalizing line endings on the client side. Diff is clean (Agent.md +2/-2, conn-manager.js +24, conn-manager.test.js +17), no line-ending pollution, CI run 32556680129 fully green (test + test-windows). Previous ❌ review was against the old polluted head and is superseded.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle. Verified head 61f14f1: diff is exactly the semantic changes (Agent.md +2/−2, conn-manager.js +24, conn-manager.test.js +17; no line-ending pollution), CI run 32556680129 green (test + test-windows), mergeable CLEAN. Teardown via ConnManager.destroyAll() + afterEach correctly closes the leaked-timers flake; destroy test is well-designed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle. Independent fresh-eyes review of head 61f14f1:
- Root cause correctly addressed: unref'd per-connection backoff timers leaked across tests, firing during later tests and creating stray connections/recovery (the onRecovered cancelledByParent flake).
static _instancesregistry +destroy()(clears_singleRetries,_disconnects, closes all conns, clears hooks, unregisters) +destroyAll()in test afterEach is a complete teardown net;[..._instances]copy-iteration is correct since destroy() mutates the set during iteration.- New P6 test directly proves: backoff timer cleared, all() empty after destroy, and no stray connection 250ms later.
- Agent.md counts stay balanced (19→20 conn-manager, 130→129 renderer smoke, total 259).
- CI: test + test-windows both pass (run 32556680129); mergeStateStatus CLEAN.
Uh oh!
There was an error while loading. Please reload this page.
…doc-count conflict: pytest 1008 + GUI 20/129 from #935)
Summary
Fixes an intermittent GUI test failure (
P2 onRecovered→cancelledByParent) inemrg/gui/test/conn-manager.test.js.Root cause
conn-manager.test.jscreatesConnManagerinstances inline and never tears them down. Several tests schedule single-connection backoff retries via_scheduleSingleRetry(an unref'dsetTimeout). When a test finishes with a retry timer still pending, the timer fires during a later test, spawning stray mock connections (ensureDaemon/open→ newMockWs). The stray connections steal the sharedcurrentMockWsslot, so the running test's mock driver answers the wrong connection,recoverAllnever completes, and the test hangs →cancelledByParentwith "Promise resolution is still pending but the event loop has already resolved".Reproduction: failed 3/3 full-file runs (incl. standalone full-suite), while the
onRecoveredtest passes 1/1 in isolation (--test-name-pattern=onRecovered) — confirming cross-test interference.Fix
conn-manager.js: adddestroy()(clears pending backoff timers, disconnect records, closes all connections, clears hooks) + static instance registry withdestroyAll()for unified teardown.conn-manager.test.js: callConnManager.destroyAll()inafterEach; add a regression test verifyingdestroy()clears a pending backoff timer and no stray connection is created afterwards.Verification
cd emrg/gui && npm test) — 3 consecutive full runs, all clean: 259 tests / 251 pass / 0 fail / 0 cancelled / 8 skipped (before: 249 pass + 1 cancelled).uv run pytest tests/ -q→ 938 passed / 65 skipped.tests/test_doc_counts.py4/4 passed (Agent.md GUI breakdown synced 258 → 259).from emrg.client.app import run_client+emrg --helpOK.