Skip to content

fix: add AbortSignal-based server lifecycle cleanup#8217

Draft
eventualbuddha wants to merge 6 commits into
mainfrom
fix/detect-devices-listener-leak
Draft

fix: add AbortSignal-based server lifecycle cleanup#8217
eventualbuddha wants to merge 6 commits into
mainfrom
fix/detect-devices-listener-leak

Conversation

@eventualbuddha

@eventualbuddha eventualbuddha commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Overview

Generated with Claude Code

  • detectDevices adds attach/detach listeners to the global usb singleton but never removes them, causing MaxListenersExceededWarning in tests that call start() multiple times.
  • Updated detectDevices to return a cleanup function.
  • Added an optional signal: AbortSignal to each app's StartOptions. Aborting the signal shuts down everything start() created: removes USB listeners and closes the HTTP server.
  • Updated all four apps (central-scan, scan, admin, mark-scan) with consistent abort behavior.

Demo Video or Screenshot

Before: pnpm test:coverage in central-scan backend emits:

(node) MaxListenersExceededWarning: Possible EventEmitter memory leak detected.
11 attach listeners added to [EventEmitter].

After: no warning. Tests use AbortController for clean shutdown:

const shutdownController = new AbortController();
const server = start({ ..., signal: shutdownController.signal });
// ... test ...
shutdownController.abort(); // cleans up listeners + closes server

Testing Plan

  • cd apps/central-scan/backend && pnpm test:run — all 110 tests pass, no MaxListenersExceededWarning
  • cd apps/scan/backend && pnpm test:run src/server.test.ts — 5 tests pass
  • cd libs/backend && pnpm test:run src/detect_devices.test.ts — 2 tests pass (includes cleanup verification)

Checklist

  • I have prefixed my PR title with "VxDesign: ", "VxPollBook: ", or "HWTA: " if my change is specific to one of those products.
  • I have added logging where appropriate for any new user actions.
  • I have added the "user-facing-change" label to this PR, if relevant, to automate an announcement in #machine-product-updates.

detectDevices adds attach/detach listeners to the global USB event
emitter but never removes them, causing MaxListenersExceededWarning
in tests that start the server multiple times. Return a cleanup
function and wire it to the server close event.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the shared backend USB device detection helper to support listener cleanup, and wires that cleanup into each app server’s lifecycle to prevent MaxListenersExceededWarning when servers are started repeatedly in tests.

Changes:

  • Change detectDevices to return a cleanup function that unregisters usb attach/detach listeners.
  • Register that cleanup function on each backend server’s close event (central-scan, scan, admin, mark-scan).
  • Adjust central-scan backend server unit tests to return an EventEmitter from the mocked listen() so server.on('close', …) can be registered.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
libs/backend/src/detect_devices.ts Make USB listener registration reversible via returned cleanup function.
apps/scan/backend/src/server.ts Capture server from listen() and register cleanup on close.
apps/mark-scan/backend/src/server.ts Register device-listener cleanup on server close.
apps/central-scan/backend/src/server.ts Register device-listener cleanup on server close.
apps/central-scan/backend/src/server.test.ts Update listen() mocks to return an EventEmitter-like server.
apps/admin/backend/src/server.ts Register device-listener cleanup on server close.

Comment thread apps/central-scan/backend/src/server.test.ts
Comment thread apps/central-scan/backend/src/server.test.ts
Comment thread apps/central-scan/backend/src/server.test.ts
Comment thread apps/scan/backend/src/server.ts Outdated
eventualbuddha and others added 4 commits March 30, 2026 16:47
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace server.on('close', stopDetectingDevices) with an optional
AbortSignal on StartOptions. The caller owns the signal and triggers
cleanup by aborting. For scan (which doesn't expose the server),
the abort handler also closes the server.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The signal handler now calls both stopDetectingDevices() and
server.close() in all four apps. Tests use abort() as the sole
shutdown mechanism instead of calling server.close() separately.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@eventualbuddha eventualbuddha changed the title fix: clean up detectDevices listeners on server close fix: add AbortSignal-based server lifecycle cleanup Mar 31, 2026
The abort handler is exercised in central-scan via setup_app.ts but
not in the other apps' server tests. Mark as istanbul ignore to
avoid coverage threshold failures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comment on lines 9 to 10
detectDevices({ logger });
testDetectDevices(logger, expect);

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

detectDevices now returns a cleanup function, but this test ignores it. That leaves USB listeners attached across tests and can reintroduce MaxListenersExceededWarning (and cross-test interference) within this suite. Capture the returned cleanup function and call it at the end of the test (or in an afterEach).

Suggested change
detectDevices({ logger });
testDetectDevices(logger, expect);
const cleanup = detectDevices({ logger });
testDetectDevices(logger, expect);
cleanup();

Copilot uses AI. Check for mistakes.
Comment on lines +248 to +255
signal?.addEventListener(
'abort',
/* istanbul ignore next - @preserve */
() => {
stopDetectingDevices();
server.close();
},
{ once: true }

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

Cleanup only runs on AbortSignal abort, but this server is also closed directly in tests (e.g. server?.close() in apps/admin/backend/src/server.test.ts). Closing the server currently does not remove the USB listeners, so listeners will still accumulate across test cases. Consider registering stopDetectingDevices on server's 'close' event (once) and/or wrapping server.close so cleanup happens regardless of how shutdown is triggered. Also handle the case where signal is already aborted before the listener is attached (otherwise cleanup never runs).

Copilot uses AI. Check for mistakes.
Comment on lines +143 to +151
signal?.addEventListener(
'abort',
/* istanbul ignore next - @preserve */
() => {
stopDetectingDevices();
server.close();
},
{ once: true }
);

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

Cleanup only runs on AbortSignal abort, but this server is closed directly in multiple tests via server.close(). That shutdown path currently does not remove the USB listeners, so they will still accumulate across tests. Register stopDetectingDevices on the server's 'close' event (once) and/or ensure server.close() triggers the same cleanup logic. Also consider handling an already-aborted signal so cleanup runs immediately.

Copilot uses AI. Check for mistakes.
Comment on lines +162 to +168
signal?.addEventListener(
'abort',
() => {
stopDetectingDevices();
server.close();
},
{ once: true }

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

Cleanup only runs on AbortSignal abort, but this server is also closed directly in tests (e.g. server.close() in apps/central-scan/backend/src/auth.test.ts). Closing the server currently does not remove the USB listeners, so they will continue to accumulate across test cases. Register stopDetectingDevices on the server's 'close' event (once) and/or ensure server.close() triggers the same cleanup logic. Also handle an already-aborted signal so cleanup runs deterministically.

Copilot uses AI. Check for mistakes.
app: Application;
logger: BaseLogger;
workspace: Workspace;
signal: AbortSignal;

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

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

StartOptions.signal is declared as required, but start accepts Partial<StartOptions> and uses signal?.addEventListener, which implies it is optional (and other apps’ StartOptions make signal optional). Making this field required is inconsistent and can mislead callers; consider making signal optional here for consistency.

Suggested change
signal: AbortSignal;
signal?: AbortSignal;

Copilot uses AI. Check for mistakes.
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