Skip to content

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

Merged
jkmassel merged 0 commit into
fix/release-media-handling-on-teardownfrom
jkmassel/silent-listener-failure
Sep 15, 2026
Merged

jkmassel merged 0 commit into
fix/release-media-handling-on-teardownfrom
jkmassel/silent-listener-failure

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Note — stacked on test/media-mock-cleanup, not trunk. One commit, one file, +37/−1.

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:149-153). 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 #649, 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 — 585 GutenbergKitTests + 396 GutenbergKitHTTPTests
  • iOS Simulator xcodebuild test — 589 + 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 the iOS label Sep 15, 2026
@jkmassel jkmassel self-assigned this Sep 15, 2026
@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch from a8f1040 to 355c9ad Compare September 15, 2026 20:11
@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/663")

Built from 37d598e

@jkmassel jkmassel changed the title fix(ios): report a media upload server that stops listening fix(ios): leave a breadcrumb if a listener dies after starting Sep 15, 2026
@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch 3 times, most recently from 8bbdac5 to 37d598e Compare September 15, 2026 20:37
@jkmassel
jkmassel changed the base branch from test/media-mock-cleanup to fix/release-media-handling-on-teardown September 15, 2026 20:37
@jkmassel
jkmassel force-pushed the jkmassel/silent-listener-failure branch from 37d598e to 41650f3 Compare September 15, 2026 22:03
@jkmassel
jkmassel merged commit 41650f3 into fix/release-media-handling-on-teardown Sep 15, 2026
0 of 2 checks passed
@jkmassel
jkmassel force-pushed the fix/release-media-handling-on-teardown branch from 50eefa7 to 7ee1945 Compare September 15, 2026 22:03
@jkmassel
jkmassel deleted the jkmassel/silent-listener-failure branch September 15, 2026 22:03
@jkmassel
jkmassel restored the jkmassel/silent-listener-failure branch September 15, 2026 22:05
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants