to main - #263
Merged
Merged
Conversation
docs: add repository AGENTS.md guidelines
client.listener/client.socket.listener was pulled off its owner into a bare variable and invoked standalone, so async-stream-emitter's listener() ran with no receiver. In production, ConnectionData never exposes client.listener directly, so the client.socket?.listener branch was live on every incoming socket connection, and the detached call threw "Cannot read properties of undefined (reading 'stream')" inside handleDisconnect (via sendPulse -> addSocket -> handleConnections), crash-looping the webjamsocket Heroku app and taking down joshandmariamusic.com (503) and the gigs/images feed on web-jam.com/music. Call listener() as a method of its owning object (client or client.socket) so `this` stays bound, preserving the existing client.listener -> client.socket.listener fallback and early-return.
Fake client stubs elsewhere in this file use a plain arrow/function for `listener`, which doesn't care about its receiver, so they didn't catch the detached-call bug. This test asserts `this` inside `socket.listener(...)` is bound to the socket object, matching a real async-stream-emitter receiver. Confirmed it fails against the pre-fix code (capturedThis undefined) and passes against the fix (capturedThis === socketOwner).
Add scripts/smoke-prod-socket.mjs and wire it into CircleCI. Unlike the AgController unit tests, this boots the ACTUAL compiled server (NODE_ENV=production, matching Heroku -- agServerUtils.routing only skips resetData() in production, so dev/test-mode boot takes a different path than the one that crashed), connects a real socketcluster-client, holds the connection briefly, disconnects cleanly, and asserts the process is still alive and the HTTP server still answers. It's black-box: it doesn't know which bug crashes the boot path, so it also guards future regressions here, not just this one. Verified locally against a throwaway local MongoDB (via docker, discarded after use -- no WebJamApps database touched): - unfixed code: FAILS, reproducing the exact production stack trace (async-stream-emitter TypeError: Cannot read properties of undefined (reading 'stream') at AgController.handleDisconnect -> sendPulse -> addSocket -> handleConnections) - fixed code: PASSES Reuses MONGO_DB_URI or TEST_DB (whichever CircleCI already provides for the unit suite) rather than adding a new secret; fails fast with a clear message if neither is set instead of hanging. socketcluster-client is already a runtime "dependencies" entry (not added here), so it survives the existing `npm install --omit=dev` prod-parity step.
The step name's unquoted colon-space ("...(regression: detached-listener...")
was read by YAML as a nested key/value separator, so the whole config failed
to parse and the pipeline never started -- meaning the smoke gate added in
the previous commit had never actually run in CI. Quoting the scalar fixes
the parse; command/name padding already matched the surrounding steps.
Validated locally before pushing:
- python3 -c "import yaml,sys; yaml.safe_load(open('.circleci/config.yml'))" -> parses cleanly
- circleci config validate .circleci/config.yml -> "Config file at .circleci/config.yml is valid."
…crash Fix detached-listener crash-loop (production outage) + regression test + CI smoke gate
…'localhost' (v3.0.15) CircleCI build 504 on dev failed: the server logged [Active] (listening) and never crashed -- no async-stream-emitter TypeError, and the script's own exit-vs-timeout distinction correctly reported "no connection succeeded" rather than "server process exited". The AgController fix from #262 is not implicated; this was a defect in the test harness itself. Root cause, verified empirically (not just restating the hypothesis): ran the actual CI base image (cimg/node:24.18-browsers) and confirmed Node 24's *default* dns.lookup('localhost') resolves ::1 (IPv6) first in that image. src/index.ts calls httpServer.listen(SOCKETCLUSTER_PORT) with no host, so which family actually ends up reachable depends on the environment. Dialling by the hostname 'localhost' left address selection to Node's resolver/DNS ordering, which is exactly the kind of thing that differs between a local Docker Desktop sandbox (where both loopback families happened to work in my repro) and CircleCI's actual container networking (where the real job observed zero successful connections in 20s despite a healthy listener -- the classic signature of a family that's present but not actually routable, which a Docker Desktop bridge network does not reproduce). Fix: probe both 127.0.0.1 and ::1 with a short, bounded raw-TCP connect BEFORE ever handing a hostname to socketcluster-client, and dial whichever one actually answers. This sidesteps DNS/dual-stack ambiguity entirely and is correct regardless of which family (if any) is broken in a given environment -- no more guessing. Also: capture the client's 'error' listener continuously so any future handshake-level failure carries a concrete reason in the final message instead of a bare "didn't connect", and log the raw dns.lookup ordering up front for visibility in CI logs. Timeout budget is unchanged (20s total), not raised -- the fix is WHICH address is dialed and WHY a failure happened, not how long we wait. Verified locally against cimg/node:24.18-browsers with a throwaway docker MongoDB (both discarded after use): - fixed AgController + fixed harness -> smoke-prod-socket OK - reverted AgController (temporarily, not committed) + fixed harness -> smoke-prod-socket FAILS with the exact production stack trace (TypeError: Cannot read properties of undefined (reading 'stream') at AsyncStreamEmitter.listener -> AgController.handleDisconnect -> sendPulse -> addSocket -> handleConnections), proving the gate still genuinely catches the regression.
Fix smoke-prod-socket harness: dial a probed-reachable loopback, not 'localhost' (v3.0.15)
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.
No description provided.