Skip to content

fix(ios): leave a breadcrumb if a listener dies after starting - #669

Open
jkmassel wants to merge 1 commit into
jkmassel/dependency-fetch-cancelledfrom
jkmassel/silent-listener-failure
Open

jkmassel wants to merge 1 commit into
jkmassel/dependency-fetch-cancelledfrom
jkmassel/silent-listener-failure

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Note — stacked on #651, not trunk. One commit, one file, +19/−1.

Supersedes #663, which GitHub auto-closed as merged when the stack below it was reordered and this branch's commit briefly became reachable from its own base. Same branch, same change.

What?

HTTPServer.start nils stateUpdateHandler on .ready and nothing replaces it, so a listener that failed after a successful bind left no trace anywhere — while the server went on reporting a port and token that addressed a socket nobody was listening on.

Replace the handler instead of clearing it, and log a post-start .failed or .waiting. That's the whole change. No API change, no behaviour change.

Why?

Mostly so an assumption becomes falsifiable.

We assume this never fires. There's no issue behind it, no report, no Sentry. The one hypothesis that was actually tested — TN2277 socket reclamation while suspended — was disproven on device: iPhone 15 Pro, 30.5 minutes suspended across four cycles with memory pressure from five foregrounded apps, listener still accepting TCP. The other candidates fail the bind instead, before .ready, which start(...) already handles by throwing. Lockdown Mode was root-caused to CORS and Origin: file:// sanitization, not a listener kill.

So this isn't a fix for an observed failure. It's the difference between "we assume it doesn't happen" and "we'd find out if we were wrong," for about ten lines.

Deliberately not doing

Not guaranteed. A state arriving in the gap between .ready being delivered and this assignment is still missed — .ready is delivered on the listener's queue and consumed off it, so for that stretch the start race's handler is still installed and whatever it yields goes into an AsyncStream nothing reads any more. Closing that needs a re-read of the live state on the listener's queue plus one-shot reporting to stop the two observers doubling up. That's real complexity to never miss an event that has never happened; best-effort is the right trade for a diagnostic.

Nothing acts on it. In particular the advertised endpoint is not withdrawn, because falling back to the WebView's default path is wrong in both modes:

  • Normally it succeeds. tokenAuthMiddleware supplies the Authorization header and allowUniversalAccessFromFileURLs exempts the file:// page from CORS, so a direct POST /wp/v2/media lands — silently skipping a host MediaProcessor and putting GutenbergKit back on the network a MediaUploader host explicitly took it off ("with GutenbergKit out of the network entirely", MediaHandlers.swift:185-189). Nobody notices, because it worked.
  • Under Lockdown Mode it fails, at the CORS check — and this loopback server's own permissive CORS is the only route that still works there.

Left alone, the page keeps addressing the dead port and nativeMediaUploadMiddleware surfaces a fetch_error without retrying. That's already the right outcome: an error the user can act on.

Endpoint withdrawal does have a real trigger — a host calling stopMediaHandling() deliberately — and that lives in #625, further down this stack, where it belongs.

.cancelled is not logged. stop() and deinit are its only causes, so it's the ordinary teardown of every server and would be pure noise.

How?

let port = p.rawValue
listener.stateUpdateHandler = { state in
    switch state {
    case .failed(let error): // log error
    case .waiting(let error): // log warning
    default: break
    }
}

Captures only the port, so it holds neither the server nor the listener and cannot form a cycle with either.

Testing Instructions

There is nothing to reproduce by hand — that's the point of the PR — and nothing user-facing changes. The existing suites cover that replacing the handler doesn't disturb the start race:

  • swift test — 587 GutenbergKitTests + 396 GutenbergKitHTTPTests
  • iOS Simulator xcodebuild test — 597 + 396
  • make lint-swift clean

No new tests. A log line with no observable behaviour doesn't warrant one, and asserting that a handler was installed would pin the implementation rather than anything real.

Related

@github-actions github-actions Bot added the [Type] Bug An existing feature does not function as intended label Sep 15, 2026
@jkmassel jkmassel added [Type] Code Quality Issues or PRs that relate to code quality iOS labels Sep 15, 2026
@jkmassel jkmassel self-assigned this Sep 15, 2026
@wpmobilebot

wpmobilebot commented Sep 15, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/669")

Built from d14165b

@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch from 41650f3 to bfe5cf7 Compare September 15, 2026 22:39
@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch from bfe5cf7 to 5a88648 Compare September 16, 2026 19:41
@jkmassel
jkmassel added this pull request to stack #690 September 17, 2026 18:33
`HTTPServer.start` nils `stateUpdateHandler` on `.ready` — load-bearing for
the start race, since it stops yielding into the `AsyncStream` the start
timeout races — and nothing replaced it. If a listener ever failed after a
successful bind it left no trace anywhere, while the server went on reporting
a `port` and `token` that addressed a socket nobody was listening on.

Replace the handler instead of clearing it, and log a post-start `.failed` or
`.waiting`. That is the whole change.

We assume this never fires. No listener here has been observed dying after a
successful bind, there is no issue or report behind it, and the one hypothesis
that was tested — TN2277 socket reclamation while suspended — was disproven on
device: 30 minutes suspended across four cycles, still accepting TCP. The
other candidates fail the *bind* instead, before `.ready`, which `start(...)`
already handles by throwing. This exists so the assumption is falsifiable
rather than invisible.

Deliberately best-effort, and deliberately log-only:

- A state arriving in the gap between `.ready` and this assignment is still
  missed. Closing that needs a re-read of the live state on the listener's
  queue plus one-shot reporting to keep the two observers from doubling up —
  real complexity to never miss an event that has never happened.
- Nothing acts on it. Withdrawing the advertised endpoint would be worse in
  both modes: normally the page's fallback to a direct `POST /wp/v2/media`
  *succeeds*, silently skipping a host `MediaProcessor` and putting
  GutenbergKit back on the network a `MediaUploader` host explicitly took it
  off; under Lockdown Mode it fails at the CORS check, where this loopback
  server's permissive CORS is the only route that still works. Left alone, the
  page keeps addressing the dead port and `nativeMediaUploadMiddleware`
  surfaces a `fetch_error` without retrying — already the right outcome.
- `.cancelled` is not logged. `stop()` and `deinit` are its only causes, so it
  is the ordinary teardown of every server.

The handler captures only the port, so it holds neither the server nor the
listener and cannot form a cycle with either. No API change.
@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch from 5a88648 to d14165b Compare September 18, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants