Let the bridge client outlast the bridge's own request timeout - #245
Open
philipp-winterle wants to merge 1 commit into
Open
philipp-winterle wants to merge 1 commit into
philipp-winterle wants to merge 1 commit into
Conversation
The UI operation bridge gives each operation REQUEST_TIMEOUT_MS (30s) to complete, but every request from the client side used a fixed 2s socket timeout. Any operation that took longer than two seconds - a large events.get-response-body, or a UI that was briefly busy - failed with a bare ETIMEDOUT while the bridge was still waiting happily, so the server's 30s budget was unreachable and its error never arrived. The timeout now depends on what the request is waiting for: discovery requests keep failing fast at 2s, so a dead socket path doesn't hold up the paths behind it, while /api/execute waits just past the bridge's own limit, which lets the bridge's error - naming the operation - through. socketRequest is exported so the test can drive one known socket. Going through apiRequest would walk its candidate paths, and on macOS the last of those is the real server on the developer's machine.
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
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.
The problem
Two constants in this repo contradict each other on the same call path.
src/api/ui-operation-bridge.tsgives the UI 30 seconds to answer an operation:src/api/bridge-client.tsgave up on that same request after 2 seconds:timeout: 2000So the bridge's budget was unreachable. Any operation that took longer than two
seconds -
events.get-response-bodyon a large body, or a UI that was brieflybusy - failed on the client with a bare
ETIMEDOUTwhile the bridge was stillwaiting perfectly happily, and the bridge's own error message (which names the
operation) could never reach the caller.
Reproduced by holding a
/api/executeresponse back for three seconds:The change
The timeout now depends on what the request is waiting for.
/api/executewaits just pastREQUEST_TIMEOUT_MS; everything else keeps theold 2 seconds, which matters because
apiRequestwalks a list of candidatesocket paths and a dead one has to fail fast to leave time for the paths behind
it.
REQUEST_TIMEOUT_MSis exported so the two numbers can't drift apart again.socketRequestis exported as well, purely so the new test can drive one knownsocket. Going through
apiRequestwould walk its candidate paths, and the lastof those on macOS is the real HTTP Toolkit on the developer's own machine - the
first draft of this test silently executed its operation against my running
instance.
Out of scope, but worth flagging
apiRequestretries the next socket path when one fails, including on timeout.With the longer execute timeout, a genuinely unresponsive UI on macOS now costs
~35s on the primary socket and then another ~35s on the Darwin fallback before
the caller sees anything. That shape predates this change, but the numbers get
bigger. Happy to follow up with a "don't retry other paths after a timeout"
change if you want it.
Test plan
test/integration/bridge-client.spec.ts: an operation the UI answersafter 3 seconds now succeeds (fails with
Request timed outbefore thischange), plus two guards that the execute timeout stays above
REQUEST_TIMEOUT_MSand the discovery timeout stays well below it.test:unit: 23 passing.test:integration: 65 passing, 1 pending, 16 failing.mainhere fails 16too, and 62 pass, so the three added tests are the whole difference. The 16 on
mainare all environment-dependent - Docker CLI interception, Electron,Chrome, fresh and existing terminal, and the end-to-end server API test -
none of which this machine can run. I captured that list on
mainonly andcompared failure counts rather than names, because rerunning the suite here
launches real browsers and terminal windows.
tsc --noEmitclean.