Repository navigation
Conversation
…ring connect awaited the WebSocket handshake, getVM and getIsolate with no timeout, so an app that accepted the connection but stopped responding (a sleeping device, an app paused in the debugger) made the tool wait until the MCP client gave up, which can take up to 30 minutes. The handshake now fails after 30 seconds with a TimeoutException that says what to check, and leaves the connector disconnected so the next connect starts fresh. A handshake that times out is abandoned: if its service connects later it is disposed instead of being published. disconnect also stops waiting for an unresponsive VM service to close after 5 seconds, so the previous app can't block a reconnect. Fixes leancodepl#131
uveshm003
requested review from
jakubfijalkowski and
mateusz-pietras
as code owners
October 23, 2026 18:34
👷 Deploy request for marionette-landing pending review.Visit the deploys page to approve it
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unbounded MCP setup requests, overlapping-attempt cleanup, and CLI timeout compatibility remain unresolved.
3 open issues
What changed in this PR
Addresses #131 by bounding VM service connection waits, extending the earlier stream-subscription timeout from #127.
Changes:
- Adds a configurable handshake timeout, defaulting to 30 seconds, with actionable errors.
- Bounds disconnect cleanup and handles WebSockets that connect after an attempt times out.
- Adds fake-server regression tests and an Unreleased changelog entry.
| File | Description |
|---|---|
packages/marionette_mcp/test/vm_service/connect_timeout_test.dart |
Tests responsive connections, timeouts, retries, and reconnects. |
packages/marionette_mcp/lib/src/vm_service/vm_service_connector.dart |
Adds handshake deadlines and bounded connection cleanup. |
CHANGELOG.md |
Documents the connection timeout behavior. |
Review effort: Balanced
Comment on lines
+225
to
+227
| final isolateId = await handshake().timeout( | ||
| timeout, | ||
| onTimeout: () { |
Comment on lines
+244
to
+246
| if (identical(_service, service)) { | ||
| await disconnect(); | ||
| } |
| Future<void> connect(String uri) async { | ||
| Future<void> connect( | ||
| String uri, { | ||
| Duration timeout = defaultConnectTimeout, |
Member
|
Hello, thank you for the contribution. We will look into the provided solution as well as review issues raised by copilot and follow up shortly. |
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.
Fixes #131
Problem
VmServiceConnector.connect()awaitedvmServiceConnectUri,getVMandgetIsolatewith no timeout. OnlystreamListenhad one (500 ms, from #127). When an app accepts the connection but then stops answering, for example on a sleeping device or with the app paused in the debugger,connectwaits until the MCP client gives up. That can take up to 30 minutes, which matches the 1 m 43 s and 9 min hangs in the issue's timeline.disconnect(), whichconnectcalls first when it is already connected, also awaitedVmService.dispose()with no limit.Change
connect(uri, {timeout})puts a timeout on the whole handshake, 30 s by default (VmServiceConnector.defaultConnectTimeout). When the timeout fires, it throws aTimeoutExceptionthat names the URI and what to check, and leaves the connector disconnected so the nextconnectstarts fresh. Theconnecttool already returns that as anisErrorresult:Failed to connect to app: … Timed out connecting to the VM service at ws://… after 30s. Make sure the app is still running, is not paused in the debugger, and its device is awake, then call connect again.disconnect()clears its state first, then waits at most 5 s for the old service to close, so an unresponsive app that was connected before can't block a reconnect.connectin its own--timeout(5 s by default), so its behaviour is unchanged.I left the per-call timeouts on the other tools (the second half of the issue's suggestion) out of this PR on purpose.
take_screenshots,scroll_toandhot_restartcan legitimately take a while, so choosing those limits seemed like a maintainer decision. I'm happy to follow up if you want them.Tests
New file
packages/marionette_mcp/test/vm_service/connect_timeout_test.dart. The tests use a local fake VM service built onHttpServerandWebSocketTransformer:I also ran these tests against the unfixed
vm_service_connector.dartfrommain. The three timeout tests hung until the test runner's 8 s timeout, which reproduces the bug; the other three passed. The last test passes onmaintoo, because the fake closes its socket cleanly, so it guards the newdisconnectbehaviour rather than reproducing a hang.Checks run locally (Dart 3.12.2, not the Flutter 3.47.6 pinned in
.fvmrc):dart testinpackages/marionette_mcp: 235 passeddart testinpackages/marionette_cli: 103 passeddart analyze --fatal-infos lib bin testinpackages/marionette_mcp: no issuesdart analyze --fatal-infos lib bininpackages/marionette_cli: no issuesdart format --set-exit-if-changed: cleanI haven't tested this against a real sleeping device. The hang is reproduced with the fake VM service above.
I added a CHANGELOG entry under Unreleased.