From 3230e08c77d51c4383aa9677f75689456af64833 Mon Sep 17 00:00:00 2001 From: Andrey Mikhaylov Date: Fri, 28 Aug 2026 08:48:18 +0100 Subject: [PATCH] Build on the Swift 6.2 toolchains the README promises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `swift build -c release` failed on every Swift 6.2 toolchain — Xcode 26.0 through 26.3 — with three `#SendingRisksDataRace` errors in ServerInference.swift. Nobody outside Xcode 26.4+ has been able to build since the image-support merge in 0.5.0, against a README that promises Swift 6.2 or newer. Reported as issue 161. CI never caught it: `runs-on: macos-26` resolves to the newest Xcode on the image, currently 26.6. Reproduced against a real Swift 6.2.4 compiler. `ServerModelSession` is an actor, so its `visionRuntime` and `model` are self-isolated, and both were captured by non-escaping closures handed to the nonisolated helpers `ServerRequestImages.encode` and `.encodeAll`. Swift 6.2's region isolation cannot prove those closures stay in the actor's domain and calls the capture a send; 6.3 accepts the same code. Nothing escapes: the helpers are synchronous, the closures are non-escaping, and every call runs on the actor. The closures are what has to go, so the decisions become values. `ServerRequestImages.source(for:)` returns which read an encode does, and `plannedEntries` takes the dictionary snapshot, the plan pass and the id-to-plan pairing together — `[UUID: URL]` has no order, so pairing ids and plans from two walks files one image's features under another's id, and when both project to the same soft-token count nothing downstream notices. Along the way: - A truncated upload now answers 400 `invalid_image` instead of 500. Admission reads dimensions with stream verification off and the encode re-plans with it on, so a truncated image is admitted and fails the second read; that failure reached the generic handler as a server error, which official clients retry and 4xx they do not. - The plans are released before the render walks the whole history rather than after, so up to 32 ImageIO descriptors are not held across it. - Two 6.2-only test-target breaks: `channel.getOption` resolving to the `EventLoopFuture` overload, and `Attachment.record(_ image: NSImage, as:)`, which the Testing library only gained in 6.3. - A `build-oldest-supported-toolchain` CI job selecting Xcode 26.3, so this cannot regress silently again. Build only: running the suite twice would double CI time. Verified: release build on Swift 6.2.4 and 6.3.3; 128 tests green; both rewritten test cases mutation-checked by reverting the fix and watching them fail; and five real multimodal requests A/B'd against main — four byte-identical in reply and token counts, the fifth the intended 500 → 400. --- .github/workflows/ci.yml | 24 +++ .../AboutPanelPresentation.swift | 2 +- .../Core/ServerInference.swift | 133 ++++++++----- .../Support/TranscriptFrameRenderer.swift | 16 ++ .../ServerInferenceTests.swift | 175 ++++++++++++------ .../ServerIngressHardeningTests.swift | 5 +- 6 files changed, 241 insertions(+), 114 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3bf08b70..7c30cee0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,11 +20,35 @@ concurrency: cancel-in-progress: true jobs: + # The runner's default Xcode is the newest on the image, so this job alone + # never exercised the oldest toolchain the README supports. Three Swift 6.2 + # build breaks shipped in 0.5.0 behind that gap. + build-oldest-supported-toolchain: + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false + runs-on: macos-26 + steps: + - uses: actions/checkout@v4 + - name: Select the oldest supported Xcode + run: | + app=$(ls -d /Applications/Xcode_26.3*.app 2>/dev/null | head -1) + if [ -z "$app" ]; then + echo "Xcode 26.3 is not on this runner image; available:" >&2 + ls -d /Applications/Xcode*.app >&2 + exit 1 + fi + sudo xcode-select -s "$app" + - name: Report the toolchain + run: swift --version + - name: Build release products + run: swift build -c release + test: if: github.event_name != 'pull_request' || github.event.pull_request.draft == false runs-on: macos-26 steps: - uses: actions/checkout@v4 + - name: Report the toolchain + run: swift --version - name: Build release products run: swift build -c release - name: Run serial tests diff --git a/Sources/TurboFieldfareApp/MacPresentation/AboutPanelPresentation.swift b/Sources/TurboFieldfareApp/MacPresentation/AboutPanelPresentation.swift index f62f499c..d816df34 100644 --- a/Sources/TurboFieldfareApp/MacPresentation/AboutPanelPresentation.swift +++ b/Sources/TurboFieldfareApp/MacPresentation/AboutPanelPresentation.swift @@ -17,7 +17,7 @@ public enum AboutPanelPresentation { // Most users build from a clone, where there is no Info.plist to read a // version from, so this constant is what they see. Scripts/check_app_version.rb // fails CI when it falls behind the newest published release. - public static let fallbackShortVersion = "0.7.0" + public static let fallbackShortVersion = "0.7.1" private static let licenseURL = URL( string: "https://github.com/drumih/turbo-fieldfare/blob/main/LICENSE")! diff --git a/Sources/TurboFieldfareServer/Core/ServerInference.swift b/Sources/TurboFieldfareServer/Core/ServerInference.swift index 5dda235f..fb947d19 100644 --- a/Sources/TurboFieldfareServer/Core/ServerInference.swift +++ b/Sources/TurboFieldfareServer/Core/ServerInference.swift @@ -435,44 +435,65 @@ enum ServerRequestImages { return planned } - /// Encodes an image from the plan its count was taken from. Only an image + /// What an image's encode reads through. + enum Source { + /// The plan the image's count was taken from, still open. + case plan(VisionImagePlan) + /// The file again, for an image the open bound released. + case reopened(URL) + } + + /// An image is encoded from the plan its count was taken from. Only one /// whose plan the open bound released is read a second time. - static func encode( - _ image: Planned, - fromPlan: (VisionImagePlan) throws -> Features, - byReopening: (URL) throws -> Features - ) throws -> Features { - guard let plan = image.plan else { return try byReopening(image.url) } - return try fromPlan(plan) + /// + /// Returned rather than dispatched through caller-supplied closures: + /// Swift 6.2's region isolation rejects an actor-isolated closure that + /// captures a session's `VisionRuntime` and `Model` and is handed to a + /// nonisolated callee, which broke the build on Xcode 26.0-26.3. + static func source(for image: Planned) -> Source { + guard let plan = image.plan else { return .reopened(image.url) } + return .plan(plan) } - /// Every image a full prefill needs, each read through one open file. + /// Every image of a request planned, each still paired with the id its + /// features are filed under. + /// + /// The pairing is the reason this is not two statements at the call site: + /// `[UUID: URL]` has no order, so the ids and the plans have to be taken + /// from one snapshot. Pairing them from two walks of the dictionary files + /// one image's features under another's id, and when both project to the + /// same soft-token count nothing downstream can notice. /// - /// A full prefill lays its spans out from the encoded features rather than - /// from a count, so it asked for no plan of its own and let - /// `encodeImage(at:)` make one per image internally — the third copy of the - /// pattern, and the one that also encoded its way through the images ahead - /// of one that could not be read at all. - static func encodeAll( + /// Planning every image before the caller encodes any is the other half: + /// the spans are laid out from the same open files the encodes read, and a + /// request whose last image cannot be read at all is refused before the + /// tower has run on the ones ahead of it. + static func plannedEntries( _ imageFiles: [UUID: URL], with preprocessor: Gemma4ImagePreprocessor, maximumOpenPlans: Int = ServerRequestImages.maximumOpenPlans, - checkCancellation: () throws -> Void = {}, - encode: (Planned) throws -> Features - ) throws -> [UUID: Features] { + checkCancellation: () throws -> Void = {} + ) throws -> [(id: UUID, image: Planned)] { let entries = Array(imageFiles) let planned = try plans( for: entries.map { $0.value }, with: preprocessor, maximumOpenPlans: maximumOpenPlans, checkCancellation: checkCancellation) - var features: [UUID: Features] = [:] - features.reserveCapacity(entries.count) - for (entry, image) in zip(entries, planned) { - try checkCancellation() - features[entry.key] = try encode(image) - } - return features + return zip(entries, planned).map { (id: $0.key, image: $1) } + } + + /// A file the caller supplied that cannot be read is a bad request, not a + /// server fault, and it stays one wherever the read fails. Admission reads + /// dimensions with stream verification off; the encode re-plans with it on, + /// so a truncated upload is admitted and only fails the second read. + /// Without this that second failure reached the generic handler as a 500, + /// which official clients retry and 4xx they do not. + static func requestError(forUnreadable error: any Error) -> any Error { + if error is ServerRequestError || error is CancellationError { return error } + return ServerRequestError.invalid( + message: "image could not be read: \(error)", + param: "messages", code: "invalid_image") } } @@ -682,22 +703,20 @@ public actor ServerModelSession: ServerInferenceBackend { private func encodeTurnImage( _ image: ServerRequestImages.Planned, visionRuntime: VisionRuntime ) throws -> VisionFeatures { - try ServerRequestImages.encode( - image, - fromPlan: { - try visionRuntime.encodeImage( - plan: $0, - languageModel: model, - residencyPolicy: visionResidencyPolicy, - checkCancellation: { try Task.checkCancellation() }) - }, - byReopening: { - try visionRuntime.encodeImage( - at: $0, - languageModel: model, - residencyPolicy: visionResidencyPolicy, - checkCancellation: { try Task.checkCancellation() }) - }) + switch ServerRequestImages.source(for: image) { + case .plan(let plan): + return try visionRuntime.encodeImage( + plan: plan, + languageModel: model, + residencyPolicy: visionResidencyPolicy, + checkCancellation: { try Task.checkCancellation() }) + case .reopened(let url): + return try visionRuntime.encodeImage( + at: url, + languageModel: model, + residencyPolicy: visionResidencyPolicy, + checkCancellation: { try Task.checkCancellation() }) + } } /// Projected token count per image from headers alone; no pixel is decoded @@ -717,12 +736,8 @@ public actor ServerModelSession: ServerInferenceBackend { do { let geometry = try preprocessor.admissionGeometry(fileURL: url) counts.append(geometry.softTokenCount) - } catch let error as ServerRequestError { - throw error } catch { - throw ServerRequestError.invalid( - message: "image could not be read: \(error)", - param: "messages", code: "invalid_image") + throw ServerRequestImages.requestError(forUnreadable: error) } } return counts @@ -805,11 +820,27 @@ public actor ServerModelSession: ServerInferenceBackend { param: "messages", code: "vision_unavailable") } do { - let features = try ServerRequestImages.encodeAll( - request.imageFiles, - with: imagePreprocessor(visionRuntime), - checkCancellation: { try Task.checkCancellation() }, - encode: { try encodeTurnImage($0, visionRuntime: visionRuntime) }) + var features: [UUID: VisionFeatures] = [:] + // Scoped, so the plans — up to `maximumOpenPlans` live ImageIO + // descriptors — are released before the render walks the whole + // history, rather than held across it. + do { + let planned: [(id: UUID, image: ServerRequestImages.Planned)] + do { + planned = try ServerRequestImages.plannedEntries( + request.imageFiles, + with: imagePreprocessor(visionRuntime), + checkCancellation: { try Task.checkCancellation() }) + } catch { + throw ServerRequestImages.requestError(forUnreadable: error) + } + features.reserveCapacity(planned.count) + for entry in planned { + try Task.checkCancellation() + features[entry.id] = try encodeTurnImage( + entry.image, visionRuntime: visionRuntime) + } + } return try MultimodalPromptRenderer.render( messages: messages, featuresByID: features, diff --git a/Tests/TurboFieldfareApp/MacPresentation/Support/TranscriptFrameRenderer.swift b/Tests/TurboFieldfareApp/MacPresentation/Support/TranscriptFrameRenderer.swift index 985600a5..2484c303 100644 --- a/Tests/TurboFieldfareApp/MacPresentation/Support/TranscriptFrameRenderer.swift +++ b/Tests/TurboFieldfareApp/MacPresentation/Support/TranscriptFrameRenderer.swift @@ -88,7 +88,23 @@ enum TranscriptFrameRenderer { /// `TURBO_FIELDFARE_FRAME_DIR` is set, also writes it there so frames can /// be reviewed without passing `--attachments-path`. static func record(_ image: NSImage, named name: String) throws { + #if compiler(>=6.3) Attachment.record(image, named: name, as: .png) + #else + // `NSImage` gained its `Attachable` conformance in the Testing library + // shipped with Swift 6.3, and this suite has to build on the 6.2 + // toolchains the README supports. The bytes are the same; what is lost + // is the lazy serialization, so a 6.2 run pays a PNG encode per frame + // even without `--attachments-path`. Isolating a lazy wrapper is not + // available either: a struct holding an `NSImage` cannot conform + // without the conformance crossing into main-actor code. + if let rep = image.representations.first as? NSBitmapImageRep, + let data = rep.representation(using: .png, properties: [:]) { + Attachment.record(data, named: name) + } else { + Issue.record("frame \(name) has no PNG representation to attach") + } + #endif guard let directory = ProcessInfo.processInfo .environment["TURBO_FIELDFARE_FRAME_DIR"], !directory.isEmpty else { return diff --git a/Tests/TurboFieldfareServer/ServerInferenceTests.swift b/Tests/TurboFieldfareServer/ServerInferenceTests.swift index 61f4acc9..b874f0ae 100644 --- a/Tests/TurboFieldfareServer/ServerInferenceTests.swift +++ b/Tests/TurboFieldfareServer/ServerInferenceTests.swift @@ -41,15 +41,27 @@ struct ServerRequestImagesTests { private static let rewrittenWidth = 128 private static let rewrittenHeight = 64 - /// What an image's span was laid out for, beside what its encode read. - private struct EncodedImage { - let planned: Int - let encoded: Int + /// The soft-token count an image's encode would read, taken the way + /// `encodeTurnImage` takes it. + private static func encodedCount( + of image: ServerRequestImages.Planned, + with preprocessor: Gemma4ImagePreprocessor + ) throws -> Int { + switch ServerRequestImages.source(for: image) { + case .plan(let plan): + return plan.geometry.softTokenCount + case .reopened(let url): + return try preprocessor.plan(fileURL: url).geometry.softTokenCount + } } /// The full-prefill path: every image of the request encoded from the plan /// its count came from, so a file rewritten after the request was planned /// cannot move the count out from under the span already laid out for it. + /// + /// The assertion is against a fresh read of the same file, not against + /// `Planned.softTokenCount` — that field is copied from the plan's own + /// geometry, so comparing the two would hold however the code behaved. @Test func aFullPrefillEncodesEveryImageFromThePlanItsCountCameFrom() throws { let device = try #require(MTLCreateSystemDefaultDevice()) let preprocessor = Gemma4ImagePreprocessor(device: device) @@ -63,44 +75,55 @@ struct ServerRequestImagesTests { width: Self.plannedWidth, height: Self.plannedHeight, to: url) imageFiles[UUID()] = url } + let planned = try ServerRequestImages.plannedEntries( + imageFiles, with: preprocessor) + + // Every staged file replaced after the request was planned, standing in + // for one rewritten while the request was in flight. + for url in imageFiles.values { + try Self.writeSolidImage( + width: Self.rewrittenWidth, height: Self.rewrittenHeight, to: url) + } + + #expect(planned.count == imageFiles.count) + for entry in planned { + let rereadCount = try preprocessor + .plan(fileURL: entry.image.url).geometry.softTokenCount + try #require( + rereadCount != entry.image.softTokenCount, + "the rewrite has to move the projected count or a second read is invisible") + let encoded = try Self.encodedCount(of: entry.image, with: preprocessor) + #expect(encoded != rereadCount, + "\(entry.image.url.lastPathComponent) was read again at encode time: the encode saw the rewritten file's \(rereadCount) tokens") + #expect(encoded == entry.image.softTokenCount) + } + } + + /// The features of one image must not be filed under another's id. The + /// dictionary has no order, so the ids and the plans have to come from one + /// snapshot; taking them from two walks pairs them wrongly, and when both + /// images project to the same count nothing downstream can notice. + @Test func everyPlanIsPairedWithItsOwnImageID() throws { + let device = try #require(MTLCreateSystemDefaultDevice()) + let preprocessor = Gemma4ImagePreprocessor(device: device) + let directory = try Self.makeStagingDirectory() + defer { try? FileManager.default.removeItem(at: directory) } + + var imageFiles: [UUID: URL] = [:] + for index in 0..<8 { + let url = directory.appendingPathComponent("staged-\(index).png") + try Self.writeSolidImage( + width: Self.plannedWidth, height: Self.plannedHeight, to: url) + imageFiles[UUID()] = url + } - // Rewritten from inside the first encode, which is the only point that - // sits between the counts and the rest of the encodes. It stands in for - // a staged file replaced while the request was in flight. - var rewritten = false - let encoded = try ServerRequestImages.encodeAll( - imageFiles, - with: preprocessor, - encode: { image -> EncodedImage in - if !rewritten { - rewritten = true - for url in imageFiles.values { - try Self.writeSolidImage( - width: Self.rewrittenWidth, - height: Self.rewrittenHeight, - to: url) - } - } - let tokens = try ServerRequestImages.encode( - image, - fromPlan: { $0.geometry.softTokenCount }, - byReopening: { - try preprocessor.plan(fileURL: $0).geometry.softTokenCount - }) - return EncodedImage(planned: image.softTokenCount, encoded: tokens) - }) - - #expect(encoded.count == imageFiles.count) - let plannedCounts = Set(encoded.values.map(\.planned)) - let reread = try ServerRequestImages.plans( - for: Array(imageFiles.values), with: preprocessor) - let rereadCounts = Set(reread.map(\.softTokenCount)) - try #require( - plannedCounts.isDisjoint(with: rereadCounts), - "the rewrite has to move the projected count or a second read is invisible") - for (id, image) in encoded { - #expect(image.encoded == image.planned, - "image \(id) was read again at encode time: its span was laid out for \(image.planned) tokens and the encode read \(image.encoded)") + let planned = try ServerRequestImages.plannedEntries( + imageFiles, with: preprocessor) + #expect(planned.count == imageFiles.count) + #expect(Set(planned.map(\.id)) == Set(imageFiles.keys)) + for entry in planned { + #expect(entry.image.url == imageFiles[entry.id], + "\(entry.id) was paired with \(entry.image.url.lastPathComponent)") } } @@ -125,15 +148,19 @@ struct ServerRequestImagesTests { try Self.writeSolidImage( width: Self.rewrittenWidth, height: Self.rewrittenHeight, to: url) } - let encoded = try planned.map { image in - try ServerRequestImages.encode( - image, - fromPlan: { $0.geometry.softTokenCount }, - byReopening: { - try preprocessor.plan(fileURL: $0).geometry.softTokenCount - }) + + guard case .plan = ServerRequestImages.source(for: planned[0]) else { + Issue.record("the image inside the bound gave its plan up") + return + } + guard case .reopened = ServerRequestImages.source(for: planned[1]) else { + Issue.record("the image past the bound kept a plan it had to release") + return } + let encoded = try planned.map { + try Self.encodedCount(of: $0, with: preprocessor) + } #expect(encoded[0] == planned[0].softTokenCount, "the image inside the bound kept its plan, so its encode reads \(planned[0].softTokenCount) tokens rather than \(encoded[0])") #expect(encoded[1] != planned[1].softTokenCount, @@ -141,8 +168,11 @@ struct ServerRequestImagesTests { } /// An unreadable image is the request's problem whichever position it sits - /// in, and the tower is the expensive part: planning every image first - /// refuses the request before any of the others is encoded. + /// in, and the tower is the expensive part: planning is one complete pass + /// that either yields a plan for every image or throws, so the caller has + /// nothing to encode from until every image has been read. The broken image + /// is last, which is the position that used to run the tower on the two + /// ahead of it before failing. @Test func anUnreadableImageIsRefusedBeforeAnyOtherImageIsEncoded() throws { let device = try #require(MTLCreateSystemDefaultDevice()) let preprocessor = Gemma4ImagePreprocessor(device: device) @@ -161,23 +191,46 @@ struct ServerRequestImagesTests { .write(to: broken) imageFiles[UUID()] = broken - var encodes = 0 + var planned: [(id: UUID, image: ServerRequestImages.Planned)]? var failure: (any Error)? do { - _ = try ServerRequestImages.encodeAll( - imageFiles, - with: preprocessor, - encode: { image -> Int in - encodes += 1 - return image.softTokenCount - }) + planned = try ServerRequestImages.plannedEntries( + imageFiles, with: preprocessor) } catch { failure = error } - #expect(failure != nil, "an image that cannot be read has to fail the request") - #expect(encodes == 0, - "the request was refused only after \(encodes) of its images had been encoded") + #expect(planned == nil, + "planning handed the caller \(planned?.count ?? 0) images to encode from a request it could not read in full") + } + + /// An image the client sent that cannot be read is a bad request. The + /// encode re-plans with stream verification on, so a truncated upload is + /// admitted and fails there; reaching the generic handler made it a 500, + /// which official clients retry and 4xx they do not. + @Test func anUnreadableImageAtEncodeTimeIsAClientError() { + struct ReadFailure: Error {} + + let mapped = ServerRequestImages.requestError(forUnreadable: ReadFailure()) + guard case .invalid(_, _, let code) = mapped as? ServerRequestError else { + Issue.record("an unreadable image mapped to \(mapped), not a request error") + return + } + #expect(code == "invalid_image") + + // A refusal the caller already classified, and an abandoned request, + // both keep their own meaning. + let alreadyClassified = ServerRequestError.invalid( + message: "image support is unavailable", + param: "messages", code: "vision_unavailable") + let preserved = ServerRequestImages.requestError(forUnreadable: alreadyClassified) + guard case .invalid(_, _, let preservedCode) = preserved as? ServerRequestError else { + Issue.record("a classified refusal was rewritten to \(preserved)") + return + } + #expect(preservedCode == "vision_unavailable") + #expect(ServerRequestImages.requestError( + forUnreadable: CancellationError()) is CancellationError) } private static func makeStagingDirectory() throws -> URL { diff --git a/Tests/TurboFieldfareServer/ServerIngressHardeningTests.swift b/Tests/TurboFieldfareServer/ServerIngressHardeningTests.swift index 42ff7111..82bb5b59 100644 --- a/Tests/TurboFieldfareServer/ServerIngressHardeningTests.swift +++ b/Tests/TurboFieldfareServer/ServerIngressHardeningTests.swift @@ -342,7 +342,10 @@ private actor EchoBackend: ServerInferenceBackend { let server = TurboFieldfareHTTPServer( modelID: "test-model", queueLimit: 1, backend: EchoBackend()) let channel = try await server.start(port: 0) - let backlog = try await channel.getOption(ChannelOptions.backlog) + // Annotated: without it Swift 6.2 picks the `EventLoopFuture`-returning + // overload and the comparison fails to type-check, while 6.3 picks the + // async one. + let backlog: Int32 = try await channel.getOption(ChannelOptions.backlog) #expect(backlog >= Int32(TurboFieldfareHTTPServer.maximumConnections)) try await server.shutdown() }