test: Harden browser contract-test adapter against unanswered-command hangs#1399
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
kinyoklion
changed the title
The adapter's send() waited forever for the browser entity to answer a command over WebSocket. If the browser ever missed one reply, that REST request -- and therefore the harness -- hung indefinitely (even status queries returned 000), with no recovery, wedging the whole service. - Add a timeout to send() (ADAPTER_REQUEST_TIMEOUT_MS, default 30s) so a stalled command fails that single request with HTTP 500 instead of hanging everything; the harness fails that one test and continues. - Reject any in-flight commands when the entity's WebSocket closes, so no REST request is left waiting on a dead socket. - Add an error handler on the REST listen() so a silent bind failure (e.g. port still held) is logged instead of disappearing.
kinyoklion
changed the title
kinyoklion
force-pushed
the
rlamb/harden-contract-test-adapter
branch
from
ebc7c5d to
84ba13b
Compare
kinyoklion
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 84ba13b. Configure here.
Addresses review feedback: the ws.on('close') handler rejected every entry
in a waiters map that was shared across all connections, so a late close on
an old connection could reject waiters belonging to a newer (e.g.
reconnecting) connection -> spurious 500s on that connection's in-flight
REST requests.
Move the waiters map inside the connection handler so each connection has
its own. send(), the message handler, the timeout, and the close handler are
all already per-connection, and the REST server is rebuilt per connection
using that connection's send(); a per-connection map isolates them cleanly.
2 participants
Summary
The browser SDK contract-test adapter (
startAdapter.ts) forwarded each command to the browser entity over WebSocket and waited forever for a reply. If the browser ever missed one reply, that REST request — and therefore the harness — hung indefinitely (even capability/status queries returned000), with no recovery path. This permanently wedged the service, which is especially painful when reusing one service across many runs (e.g. flake hunting).This PR makes the adapter resilient:
send()(ADAPTER_REQUEST_TIMEOUT_MS, default 30s). A stalled command now fails that single request with HTTP 500 so the harness fails that one test and continues, instead of wedging the whole chain.listen()so a silent bind failure (e.g. the port still being held → "WS up but REST never bound") is logged instead of disappearing.Changes are confined to test tooling; no SDK runtime code is touched.
Verification
5 consecutive full suites against one reused adapter/browser stayed green —
ec=0, fail=0, adapter responsive at ~1 ms throughout. The timeout path turns a previously-permanent hang into a single failed request the harness can recover from.Notes
getCapabilities. That was removed:getCapabilitiesis not a safe "new run" signal because multiple harnesses can drive one entity concurrently, so it could wipe another run's active clients. The lingering-final-client cleanup needs a different, run-aware approach and is out of scope here.packages/sdk/browser/example-fdv2/src/app.tshas unrelated local debugging tweaks and is intentionally not part of this branch.Test coverage
This tooling package has no unit-test harness; validation was integration-based (above).
Note
Low Risk
Only internal contract-test adapter tooling is modified; no production SDK or auth/data paths are affected.
Overview
The contract-test adapter (
startAdapter) no longer waits indefinitely for browser-entity WebSocket replies.send()now uses a configurable timeout (requestTimeoutMs/ADAPTER_REQUEST_TIMEOUT_MS, default 30s) and rejects with a clear error;waitersare scoped per WebSocket and include timers plus reject on socket close, so a dropped connection does not leave unrelated REST calls stuck.Async REST routes are wrapped with a
guardthat maps failures (timeout, closed socket, etc.) to HTTP 500 instead of unhandled rejections that could hang the harness. The RESTlisten()server also gets anerrorhandler so bind failures (e.g.EADDRINUSE) are logged.Changes are limited to internal contract-test tooling; SDK runtime is unchanged.
Reviewed by Cursor Bugbot for commit d62dc65. Bugbot is set up for automated code reviews on this repo. Configure here.