diff --git a/.claude/rules/architecture.md b/.claude/rules/architecture.md index cdfb561..3c1b186 100644 --- a/.claude/rules/architecture.md +++ b/.claude/rules/architecture.md @@ -54,3 +54,28 @@ To add or change a collection's strategy: 3. If the master needs format conversion (JP2/TIFF), route through `signedConverterURL` instead of decoding in-process — see [`converter.md`](converter.md). 4. Validate end-to-end with `./Sources/Testing/CollectionTester/test-collection.sh ""`. + +## HTTP client lifecycle + +All outbound HTTP goes through `NetworkRequestManager` +(`Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift`), which uses Alamofire. Two rules, +both there to keep the Lambda process alive: + +- **DigitalNZ requests** use Alamofire's global `AF` session. +- **Everything else** (HTML scrapes, HEAD probes, ranged GET probes) uses one of three + process-lifetime `static let` `Session`s on `NetworkRequestManager`, differing only in request + timeout and whether they send `Range: bytes=0-0`. + +Nothing may create a `Session` (or a bare `URLSession`) per call. On Linux, releasing one runs +`URLSession._MultiHandle.deinit` in swift-corelibs-foundation, which calls `curl_multi_cleanup`; +that synchronously re-enters the registered `CURLMOPT_TIMERFUNCTION`, and for a zero timeout +`updateTimeoutTimer(to: .immediate)` does `queue.async { nonisolatedSelf... }`, taking a strong +reference to the object being deinitialized. The Swift runtime then aborts (or segfaults on the +dangling reference), and Lambda reports `Runtime.ExitError` with a 500 even though the handler had +already produced its answer. It is a race, so it only fires under scheduling pressure, which a +512 MB Lambda has plenty of. + +To reproduce or re-verify, run the Lambda's local server in a Linux container under CPU pressure +and hammer `POST /invoke`; a per-call-session build fails several requests in 40, a shared-session +build fails none. `Tests/NZImageApiLambdaTests/NetworkRequestManagerSessionTests.swift` pins the +shared-session invariant and each session's configuration. diff --git a/CLAUDE.md b/CLAUDE.md index cef8156..52454ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,6 +100,14 @@ Secrets live in gitignored `.env`, `samconfig.toml`, and `.consumer-secrets/` - `template.yaml` deliberately sets **no** `ReservedConcurrentExecutions` on either function (the account's Lambda concurrency quota is already only 10, fully unreserved) — don't describe concurrency as reserved/limited per-function. +- **Never let a `URLSession` (or an Alamofire `Session`, which owns one) be deallocated in the + Lambda.** On Linux, `URLSession.deinit` tears down swift-corelibs-foundation's libcurl + `_MultiHandle`, whose own `deinit` re-enters curl's timer callback and takes a strong reference + to the object being deinitialized. The Swift runtime aborts the process, so a request that has + already done all its work returns a 500 (`Runtime.ExitError`). This caused an intermittent ~12-25% + failure rate on `/image` until 2026-08-24. `NetworkRequestManager` now holds process-lifetime + static `Session`s (`browserSession`, `shortTimeoutBrowserSession`, `rangeProbeSession`); add a new + static one there rather than constructing `Session(configuration:)` per call. - Routing a new collection through the converter requires adding its host to `ALLOWED_HOSTS` and redeploying the converter; pure `URLProcessor` strategy changes don't. - Compiled Swift binaries (e.g. running `CollectionLister` or `NZImageApiLambda` after diff --git a/Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift b/Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift index ae5a249..dd7359d 100644 --- a/Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift +++ b/Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift @@ -45,6 +45,67 @@ final class NetworkRequestManager: ValidatedRequestManager { return .success(()) } + /// The browser User-Agent every non-DigitalNZ request presents. Several sources (Recollect + /// vanity domains, Te Papa media) 403 a request that looks like a bot. + static let browserUserAgent = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36" + + /// Process-lifetime `Session`s, deliberately never released. + /// + /// These were previously created per call. On Linux, an Alamofire `Session` owns a + /// `URLSession`, whose `deinit` tears down swift-corelibs-foundation's libcurl + /// `URLSession._MultiHandle`. That teardown is unsound: `_MultiHandle.deinit` calls + /// `curl_multi_remove_handle`/`curl_multi_cleanup`, which synchronously re-enter the + /// registered `CURLMOPT_TIMERFUNCTION` callback; for a zero timeout that lands in + /// `updateTimeoutTimer(to: .immediate)`, which does `queue.async { nonisolatedSelf... }` and so + /// takes a *strong* reference to the object currently being deinitialized. The Swift runtime + /// then aborts the whole process with "Object ... of class _MultiHandle deallocated with + /// non-zero retain count". In a Lambda that abort surfaces as `Runtime.ExitError` and a 500, + /// after the request has already done all of its useful work. + /// + /// (Source: swift-corelibs-foundation, swift-6.3-RELEASE, + /// `Sources/FoundationNetworking/URLSession/libcurl/MultiHandle.swift`.) + /// + /// It is a race, so it only fires some of the time, and it fires far more often when CPU is + /// scarce, which is exactly a 512 MB Lambda. A session that is never deallocated never runs + /// that teardown, so keeping these alive for the life of the process removes the crash + /// entirely. Alamofire's own `AF` default session (used by `makeRequest`) is already a + /// process-lifetime global for the same reason. + /// + /// Do **not** reintroduce a per-call `Session(configuration:)` here. + static let browserSession = makeSession( + additionalHeaders: ["User-Agent": browserUserAgent], + requestTimeout: nil + ) + + /// As `browserSession`, but with the short timeout used by the redirect-following status probes. + static let shortTimeoutBrowserSession = makeSession( + additionalHeaders: ["User-Agent": browserUserAgent], + requestTimeout: 15 + ) + + /// As `shortTimeoutBrowserSession`, plus the `Range: bytes=0-0` header that keeps the probing + /// GETs to a single byte. + static let rangeProbeSession = makeSession( + additionalHeaders: ["User-Agent": browserUserAgent, "Range": "bytes=0-0"], + requestTimeout: 15 + ) + + private static func makeSession( + additionalHeaders: [String: String], + requestTimeout: TimeInterval? + ) + -> Session + { + let configuration = URLSessionConfiguration.default + configuration.httpAdditionalHeaders = additionalHeaders + + if let requestTimeout { + configuration.timeoutIntervalForRequest = requestTimeout + } + + return Session(configuration: configuration) + } + func makeRequest( endpoint: String, apiKey: String? = nil, @@ -75,13 +136,6 @@ final class NetworkRequestManager: ValidatedRequestManager { } func fetchHTML(endpoint: String) async throws -> String { - let userAgent = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36" - - let configuration = URLSessionConfiguration.default - configuration.httpAdditionalHeaders = ["User-Agent": userAgent] - - let session = Session(configuration: configuration) - // Also set the User-Agent as a per-REQUEST header (not only on the session config) so it is // carried onto the redirected URLRequest when URLSession auto-follows a redirect. Some // Recollect instances 301/302 the harvested *.recollect.co.nz landing host to a council vanity @@ -89,8 +143,8 @@ final class NetworkRequestManager: ValidatedRequestManager { // request lacking a browser UA. Session-level httpAdditionalHeaders are NOT reliably reapplied // to the cross-host redirect (URLSession returns the 403 error page, so an og:image scrape sees // no image and falls back), whereas request headers ARE copied across the redirect. - let headers: HTTPHeaders = ["User-Agent": userAgent] - let response = await session.request(endpoint, headers: headers).serializingString().response + let headers: HTTPHeaders = ["User-Agent": Self.browserUserAgent] + let response = await Self.browserSession.request(endpoint, headers: headers).serializingString().response switch response.result { case let .success(value): @@ -107,14 +161,10 @@ final class NetworkRequestManager: ValidatedRequestManager { /// safe at request time even for very large assets. Uses a browser User-Agent and a /// short timeout. func headStatusFollowingRedirects(endpoint: String) async -> Int { - let configuration = URLSessionConfiguration.default - configuration.httpAdditionalHeaders = [ - "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36" - ] - configuration.timeoutIntervalForRequest = 15 - - let session = Session(configuration: configuration) - let response = await session.request(endpoint, method: .head).serializingData().response + let response = await Self.shortTimeoutBrowserSession + .request(endpoint, method: .head) + .serializingData() + .response return response.response?.statusCode ?? 0 } @@ -127,15 +177,10 @@ final class NetworkRequestManager: ValidatedRequestManager { /// downloads the full image at request time (the endpoint must honour Range — Te Papa/S3 does). /// Browser User-Agent + short timeout. func rangeStatusFollowingRedirects(endpoint: String) async -> Int { - let configuration = URLSessionConfiguration.default - configuration.httpAdditionalHeaders = [ - "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36", - "Range": "bytes=0-0", - ] - configuration.timeoutIntervalForRequest = 15 - - let session = Session(configuration: configuration) - let response = await session.request(endpoint, method: .get).serializingData().response + let response = await Self.rangeProbeSession + .request(endpoint, method: .get) + .serializingData() + .response return response.response?.statusCode ?? 0 } @@ -146,15 +191,10 @@ final class NetworkRequestManager: ValidatedRequestManager { /// for GET only), but also surfaces the MIME type so a caller can branch on the resolved /// original's actual format (e.g. `image/jpeg` vs `image/tiff`) without downloading the body. func rangeContentType(endpoint: String) async -> (status: Int, contentType: String?) { - let configuration = URLSessionConfiguration.default - configuration.httpAdditionalHeaders = [ - "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36", - "Range": "bytes=0-0", - ] - configuration.timeoutIntervalForRequest = 15 - - let session = Session(configuration: configuration) - let response = await session.request(endpoint, method: .get).serializingData().response + let response = await Self.rangeProbeSession + .request(endpoint, method: .get) + .serializingData() + .response let status = response.response?.statusCode ?? 0 let contentType = response.response?.value(forHTTPHeaderField: "Content-Type") @@ -162,13 +202,10 @@ final class NetworkRequestManager: ValidatedRequestManager { } func headRequest(endpoint: String) async throws -> (contentType: String, contentLength: Int64) { - let configuration = URLSessionConfiguration.default - configuration.httpAdditionalHeaders = [ - "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36" - ] - - let session = Session(configuration: configuration) - let response = await session.request(endpoint, method: .head).serializingData().response + let response = await Self.browserSession + .request(endpoint, method: .head) + .serializingData() + .response guard let httpResponse = response.response else { throw NetworkRequestManagerError( diff --git a/Tests/NZImageApiLambdaTests/NetworkRequestManagerSessionTests.swift b/Tests/NZImageApiLambdaTests/NetworkRequestManagerSessionTests.swift new file mode 100644 index 0000000..33d00a4 --- /dev/null +++ b/Tests/NZImageApiLambdaTests/NetworkRequestManagerSessionTests.swift @@ -0,0 +1,154 @@ +// +// NetworkRequestManagerSessionTests.swift +// +// Regression coverage for the intermittent Lambda crash where a request completed all of its +// work and then the process aborted with: +// +// Object 0x... of class _MultiHandle deallocated with non-zero retain count 2. +// +// NetworkRequestManager used to build a fresh Alamofire `Session` on every call. On Linux that +// `Session` owns a `URLSession`, and releasing it runs swift-corelibs-foundation's +// `URLSession._MultiHandle.deinit`, which re-enters libcurl's timer callback and takes a strong +// reference to the object being deinitialized. The Swift runtime turns that into a fatal error, +// which API Gateway surfaced as a 500 on roughly one request in four. +// +// These tests pin the fix: the sessions are process-lifetime shared instances (so nothing is ever +// deallocated), and each still carries the request configuration its callers depend on. +// + +import Alamofire +import Foundation +import XCTest +@testable import NZImageApiLambda + +final class NetworkRequestManagerSessionTests: XCTestCase { + // MARK: No per-call sessions + + // The invariant this file exists to protect is "the Lambda target never constructs an HTTP + // session outside the shared static factory". That cannot be observed at runtime -- the + // statics are the same object however they are reached -- so it is checked against the source + // itself, which is what a regression would actually change. + + func testTheLambdaTargetConstructsSessionsOnlyInTheSharedFactory() throws { + var offenders: [String] = [] + + for file in try Self.lambdaSourceFiles() { + let source = try String(contentsOf: file, encoding: .utf8) + + for (offset, line) in Self.codeLines(of: source) { + guard line.contains("Session(configuration") || line.contains("URLSession(") else { continue } + + // The one legitimate construction site. + let isSharedFactory = file.lastPathComponent == "NetworkRequestManager.swift" + && line.contains("return Session(configuration: configuration)") + + if !isSharedFactory { + offenders.append("\(file.lastPathComponent):\(offset + 1): \(line.trimmingCharacters(in: .whitespaces))") + } + } + } + + XCTAssertEqual( + offenders, + [], + """ + A session is being constructed outside NetworkRequestManager.makeSession. On Linux, \ + releasing a URLSession runs _MultiHandle.deinit, which aborts the process and turns a \ + completed request into a 500. Add a process-lifetime static session instead. + """ + ) + } + + func testTheSharedFactoryProducesExactlyTheThreeExpectedSessions() throws { + let source = try String(contentsOf: Self.networkRequestManagerSource(), encoding: .utf8) + let constructions = Self.codeLines(of: source).filter { $0.line.contains("Session(configuration") } + let factoryCalls = Self.codeLines(of: source).filter { $0.line.contains("makeSession(") } + + XCTAssertEqual(constructions.count, 1, "Exactly one Session(configuration:) call is expected.") + // The three static properties, plus the factory's own declaration line. + XCTAssertEqual(factoryCalls.count, 4, "Expected three shared sessions built by one factory.") + } + + // MARK: Configuration preserved + + func testBrowserSessionSendsBrowserUserAgentAndNoRequestTimeoutOverride() { + let headers = Self.additionalHeaders(of: NetworkRequestManager.browserSession) + + XCTAssertEqual(headers["User-Agent"], NetworkRequestManager.browserUserAgent) + XCTAssertNil(headers["Range"]) + // Untouched, so it keeps URLSessionConfiguration.default's 60s. + XCTAssertEqual(NetworkRequestManager.browserSession.session.configuration.timeoutIntervalForRequest, 60) + } + + func testShortTimeoutSessionKeepsTheFifteenSecondProbeTimeout() { + let headers = Self.additionalHeaders(of: NetworkRequestManager.shortTimeoutBrowserSession) + + XCTAssertEqual(headers["User-Agent"], NetworkRequestManager.browserUserAgent) + XCTAssertNil(headers["Range"]) + XCTAssertEqual( + NetworkRequestManager.shortTimeoutBrowserSession.session.configuration.timeoutIntervalForRequest, + 15 + ) + } + + func testRangeProbeSessionKeepsTheSingleByteRangeHeader() { + // Without this header the "does the high-res original exist" probes would download the + // whole asset instead of one byte. + let headers = Self.additionalHeaders(of: NetworkRequestManager.rangeProbeSession) + + XCTAssertEqual(headers["User-Agent"], NetworkRequestManager.browserUserAgent) + XCTAssertEqual(headers["Range"], "bytes=0-0") + XCTAssertEqual(NetworkRequestManager.rangeProbeSession.session.configuration.timeoutIntervalForRequest, 15) + } + + func testBrowserUserAgentLooksLikeABrowser() { + // Several sources 403 anything that does not present a browser UA. + XCTAssertTrue(NetworkRequestManager.browserUserAgent.hasPrefix("Mozilla/5.0")) + } + + // MARK: Helpers + + /// The repository root, derived from this file's own path. + private static func repositoryRoot() -> URL { + URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() // NZImageApiLambdaTests + .deletingLastPathComponent() // Tests + .deletingLastPathComponent() // repository root + } + + private static func networkRequestManagerSource() -> URL { + repositoryRoot() + .appendingPathComponent("Sources/NZImageApiLambda/Helpers/NetworkRequestManager.swift") + } + + private static func lambdaSourceFiles() throws -> [URL] { + let root = repositoryRoot().appendingPathComponent("Sources/NZImageApiLambda") + + guard let enumerator = FileManager.default.enumerator(at: root, includingPropertiesForKeys: nil) else { + throw NSError(domain: "NetworkRequestManagerSessionTests", code: 1) + } + + return enumerator.compactMap { $0 as? URL }.filter { $0.pathExtension == "swift" } + } + + /// Source lines with comment-only lines removed, so the doc comments that *mention* + /// `Session(configuration:)` do not register as constructions. + private static func codeLines(of source: String) -> [(offset: Int, line: String)] { + source + .components(separatedBy: "\n") + .enumerated() + .filter { !$0.element.trimmingCharacters(in: .whitespaces).hasPrefix("//") } + .map { (offset: $0.offset, line: $0.element) } + } + + private static func additionalHeaders(of session: Session) -> [String: String] { + var headers: [String: String] = [:] + + for (key, value) in session.session.configuration.httpAdditionalHeaders ?? [:] { + guard let key = key as? String, let value = value as? String else { continue } + headers[key] = value + } + + return headers + } +}