Repository navigation
fix(apple): reuse runner HTTP connections - #4151
Conversation
|
@muvaf is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
|
I reviewed 39f7f65. The connection reuse change needs a live run before merge, and one open thread still applies. The change rewrites the live daemon-to-runner transport: connection persistence, idle expiry, and shutdown closing every connection (RunnerTests+Transport.swift#L134). This is a device-facing change, and the PR shows no run on a real session. The daemon's pool talking to the runner is unproven, and the latency win is unmeasured. In this repo it can only show on the CoreDevice tunnel, because simulator and macOS runners use loopback and usbmux still sends Connection: close. Please attach a run at this head with the rebuilt runner: (1) an iOS simulator session with 20+ mixed commands (snapshot, tap, uptime) and runner log or lsof/netstat output showing one daemon-to-runner TCP connection reused; (2) in the same daemon, a session close and reopen, or a forced runner relaunch, followed by a successful next command with no 'other side closed' or fetch failed error; (3) before/after per-command latency on a physical device over the CoreDevice tunnel, or measured on the external client this targets. The open inline thread on closing every connection in finish() still applies: #4151 (comment) Not blocking, take or leave: a request that fails before any response byte on a reused connection should be retried once on a fresh connection, because today it surfaces as 'other side closed' and a mutation is not replayed (runner-error-classification.ts#L785), so the runner-kill-then-command run in (2) should cover it; and RUNNER_PROTOCOL.md could say that network routes reuse connections through the global fetch pool while usbmux stays one request per connection (RUNNER_PROTOCOL.md#L11). The only failing check is Vercel, which needs team authorization for a fork deploy and looks unrelated to this diff. No repository CI has reported at this head, so there is no green signal yet. Before merge, the live run above is needed. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
I read 5c87457 and found no code problems. The findings from my earlier review are fixed, and nothing from it remains. Two inline threads no longer apply, so you can resolve them. The shutdown thread is fixed: The Vercel failure comes from fork deploy authorization. This change touches only I did not run the Swift tests, and I could not reproduce the simulator, USB and tunnel runs or the latency numbers. They are reported only in the PR body. The shutdown drain assumes Network.framework always calls a send's |
|
Thanks @muvaf! |
Summary
Reuse HTTP/1.1 runner connections with per-request authentication, framing checks, ordered pipelining, and bounded idle time. Drain every coalesced shutdown response before closing other connections. Clarify network pooling versus usbmux behavior. Six files changed.
Validation
Head:
5c87457fdc9a0f7cb00a3d038efbffc00f1d949d. Baseline:f7879cf2d.39f7f657bwithprematureEOFand passes with the fix.keep-aliveversusclose.Sequential measurements include timing variation. An initial baseline attempt hit an XCTest accessibility timeout and was excluded; the complete rerun passed.
Limrun forwarded navigation and close/reopen passed. Forced restart returns HTTP 409 on both versions, a separate integration issue.
Affected receipt
run-2TeQ2R,exactCommit: true: native-dupes, xctest-selection, packaged-runner-swift passed. Repository CI remains pending; Vercel requires fork authorization.