Skip to content

fix(apple): reuse runner HTTP connections - #4151

Merged
thymikee merged 2 commits into
callstack:mainfrom
muvaf:feat/ios-runner-http-keepalive
Nov 2, 2026
Merged

thymikee merged 2 commits into
callstack:mainfrom
muvaf:feat/ios-runner-http-keepalive

Conversation

@muvaf

@muvaf muvaf commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

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.

  • iOS/macOS builds passed. Native transport/framing tests: 24/24 locally and on Limrun. The new 32-waiter shutdown regression fails on 39f7f657b with prematureEOF and passes with the fix.
  • Local simulator: 24 mixed commands with HTTP tree snapshots, stable pooled connections, close/reopen, and forced-restart recovery passed.
  • Physical iPhone 14 Pro, iOS 27.0.1, Xcode 27.1: 24 mixed daemon commands over USB and close/reopen passed. Forced termination required unlocking the phone; subsequent runner startup and snapshot passed.
  • Shared HTTP transport over the wired CoreDevice tunnel: 60 measured commands plus six warm-up requests used 2 TCP connections, versus 66 on baseline. Responses advertised keep-alive versus close.
Phone median ms, 20 samples each Baseline Updated
uptime 8.2 3.7
snapshot 701 692
tap 2,126 2,028

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.

View guided diff

@vercel

vercel Bot commented Nov 1, 2026

Copy link
Copy Markdown

@muvaf is attempting to deploy a commit to the Callstack Team on Vercel.

A member of the Team first needs to authorize it.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread apple/runner/RUNNER_PROTOCOL.md Resolved
@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member

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: finish() now runs after every coalesced write completes, and the 32-waiter regression test covers it (thread). The docs thread asks to drop the pooling-versus-usbmux sentence from RUNNER_PROTOCOL.md, but that file already documents daemon-side routing, and the sentence states a wire-visible fact (usbmux sends Connection: close) that was requested in the earlier review (thread).

The Vercel failure comes from fork deploy authorization. This change touches only apple/runner, so it is unrelated. No repository CI has reported at this head yet, and it must pass before merge. I see no conflicts.

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 .contentProcessed completion, even with an error when the peer already cancelled. I believe it does, but I did not test it.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 1, 2026
@thymikee
thymikee merged commit 1b5af38 into callstack:main Nov 2, 2026
11 of 12 checks passed
@thymikee

thymikee commented Nov 2, 2026

Copy link
Copy Markdown
Member

Thanks @muvaf!

@muvaf
muvaf deleted the feat/ios-runner-http-keepalive branch November 2, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants