diff --git a/Package.resolved b/Package.resolved index e35107a..39c9452 100644 --- a/Package.resolved +++ b/Package.resolved @@ -149,8 +149,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/huggingface/swift-jinja.git", "state" : { - "revision" : "0aeefadec459ce8e11a333769950fb86183aca43", - "version" : "2.3.5" + "revision" : "4588064a20f3fc093c95f2f7d3359999bf30cae5", + "version" : "2.5.1" } }, { diff --git a/Sources/SwiftLM/Server.swift b/Sources/SwiftLM/Server.swift index 1f41f43..b8dfa24 100644 --- a/Sources/SwiftLM/Server.swift +++ b/Sources/SwiftLM/Server.swift @@ -182,8 +182,15 @@ private struct TransformersTokenizerBridge: MLXLMCommon.Tokenizer, Sendable { additionalContext: [String: any Sendable]? ) throws -> [Int] { do { + // Issue #168: JSON `null` in tool schemas decodes to NSNull via AnyCodable. + // swift-jinja's Value.init(any:) has no NSNull case, so a single null anywhere + // in a tool spec aborted the whole render with a misleading "Optional" + // conversion error that was then mislabeled as a broken template. Strip nulls + // (and unwrap nested optionals) before handing anything to the template engine. return try upstream.applyChatTemplate( - messages: messages, tools: tools, additionalContext: additionalContext) + messages: messages.map { $0.mapValuesDeep(sanitizeForJinja) }, + tools: tools?.map { $0.mapValuesDeep(sanitizeForJinja) }, + additionalContext: additionalContext.map { $0.mapValuesDeep(sanitizeForJinja) }) } catch Tokenizers.TokenizerError.missingChatTemplate { throw MLXLMCommon.TokenizerError.missingChatTemplate } catch { @@ -196,6 +203,42 @@ private struct TransformersTokenizerBridge: MLXLMCommon.Tokenizer, Sendable { } } +/// Returns `nil` when the value must be dropped (JSON `null` / NSNull), otherwise a +/// structure with every nested null removed. See `TransformersTokenizerBridge.applyChatTemplate`. +func sanitizeForJinja(_ value: any Sendable) -> (any Sendable)? { + if value is NSNull { return nil } + let mirror = Mirror(reflecting: value) + if mirror.displayStyle == .optional { + guard let child = mirror.children.first else { return nil } + return sanitizeForJinja(child.value as any Sendable) + } + if let dict = value as? [String: Any] { + var out: [String: any Sendable] = [:] + for (key, val) in dict { + if let cleaned = sanitizeForJinja(val as any Sendable) { + out[key] = cleaned + } + } + return out + } + if let arr = value as? [Any] { + return arr.compactMap { sanitizeForJinja($0 as any Sendable) } + } + return value +} + +extension Dictionary where Key == String, Value == any Sendable { + func mapValuesDeep(_ transform: (any Sendable) -> (any Sendable)?) -> [String: any Sendable] { + var out: [String: any Sendable] = [:] + for (key, val) in self { + if let cleaned = transform(val) { + out[key] = cleaned + } + } + return out + } +} + // ── CLI ────────────────────────────────────────────────────────────────────── final class ProgressTracker { @@ -1115,6 +1158,37 @@ struct MLXServer: AsyncParsableCommand { // supply. Neither is a reason to refuse to start. } + // Issue #168: render once more with a minimal non-empty tools array. The probe + // above uses `tools: nil`, so a checkpoint whose template (or tool payload) + // breaks only under the tools branch loaded clean and then failed every real + // agentic request. Warn rather than abort: a tools-broken model still serves + // plain chat and /v1/completions. + do { + let probeTokenizer = await container.tokenizer + let probeTool: [String: any Sendable] = [ + "type": "function", + "function": [ + "name": "probe", + "description": "startup chat-template tools probe", + "parameters": [ + "type": "object", + "properties": [ + "query": ["type": "string", "default": NSNull() as any Sendable] + ], + ] as [String: any Sendable], + ] as [String: any Sendable], + ] + _ = try probeTokenizer.applyChatTemplate( + messages: [["role": "user", "content": "ping"]], + tools: [probeTool], + additionalContext: ["add_generation_prompt": true] + ) + } catch let error as MalformedChatTemplate { + print("[SwiftLM] ⚠️ chat-template tools probe failed (plain chat may still work): \(error.description)") + } catch { + // Same lenient pass as above: no template, or context the probe lacks. + } + print("[SwiftLM] Model loaded. Starting HTTP server on \(host):\(port)") // ── Capture CLI defaults into a shared config ── @@ -1777,6 +1851,7 @@ func handleChatCompletion( // ── Acquire slot (concurrency limiter) ── await semaphore.wait() + let slot = GenerationSlot(semaphore: semaphore) await stats.requestStarted() let genStart = Date() @@ -1849,6 +1924,15 @@ func handleChatCompletion( fflush(stdout) let prefillStart = Date() + let modelId = config.modelId + + // ── Generation start (deferred for streaming) ── + // For streaming responses this closure runs inside the SSE consumer task + // AFTER the response headers are on the wire, so a long prefill cannot + // leave the client staring at a silent connection (client idle-timeout → + // ECONNRESET → retry with an ever-grown payload). Non-streaming awaits it + // inline, exactly as before. (Body indentation intentionally unchanged.) + let startGeneration: () async throws -> (AsyncStream, (() async -> Void)?) = { // ── DFlash block-diffusion speculative decoding ── // When --dflash is enabled and both DFlash draft model and target model conform // to DFlashTargetModel, we use DFlashRuntime.generate instead of the standard path. @@ -1892,24 +1976,7 @@ func handleChatCompletion( } } - let modelId = config.modelId - if isStream { - return handleChatStreaming( - stream: genStream, modelId: modelId, stopSequences: stopSequences, - includeUsage: includeUsage, promptTokenCount: promptTokenCount, - enableThinking: enableThinking, thinkingPreOpened: thinkingPreOpened, - jsonMode: jsonMode, semaphore: semaphore, - stats: stats, genStart: genStart, prefillStart: prefillStart, - emitPrefillProgress: false, onPrefillDone: nil - ) - } else { - return try await handleChatNonStreaming( - stream: genStream, modelId: modelId, stopSequences: stopSequences, - promptTokenCount: promptTokenCount, enableThinking: enableThinking, - thinkingPreOpened: thinkingPreOpened, jsonMode: jsonMode, semaphore: semaphore, - stats: stats, genStart: genStart, prefillStart: prefillStart, onPrefillDone: nil - ) - } + return (genStream, nil) } // ── Cache-aware generation (standard path) ── @@ -2017,23 +2084,24 @@ func handleChatCompletion( } return (stream, onPrefillDone) } - - let modelId = config.modelId + return (stream, onPrefillDone) + } // end startGeneration if isStream { return handleChatStreaming( - stream: stream, modelId: modelId, stopSequences: stopSequences, + startGeneration: startGeneration, modelId: modelId, stopSequences: stopSequences, includeUsage: includeUsage, promptTokenCount: promptTokenCount, enableThinking: enableThinking, thinkingPreOpened: thinkingPreOpened, - jsonMode: jsonMode, semaphore: semaphore, + jsonMode: jsonMode, slot: slot, stats: stats, genStart: genStart, prefillStart: prefillStart, - emitPrefillProgress: emitPrefillProgress, onPrefillDone: onPrefillDone + emitPrefillProgress: emitPrefillProgress ) } else { + let (stream, onPrefillDone) = try await startGeneration() return try await handleChatNonStreaming( stream: stream, modelId: modelId, stopSequences: stopSequences, promptTokenCount: promptTokenCount, enableThinking: enableThinking, - thinkingPreOpened: thinkingPreOpened, jsonMode: jsonMode, semaphore: semaphore, + thinkingPreOpened: thinkingPreOpened, jsonMode: jsonMode, slot: slot, stats: stats, genStart: genStart, prefillStart: prefillStart, onPrefillDone: onPrefillDone ) } @@ -2175,7 +2243,7 @@ actor PrefillState { } func handleChatStreaming( - stream: AsyncStream, + startGeneration: @escaping () async throws -> (AsyncStream, (() async -> Void)?), modelId: String, stopSequences: [String], includeUsage: Bool, @@ -2183,12 +2251,11 @@ func handleChatStreaming( enableThinking: Bool = false, thinkingPreOpened: Bool = false, jsonMode: Bool = false, - semaphore: AsyncSemaphore, + slot: GenerationSlot, stats: ServerStats, genStart: Date, prefillStart: Date, - emitPrefillProgress: Bool, - onPrefillDone: (() async -> Void)? = nil + emitPrefillProgress: Bool ) -> Response { let (sseStream, cont) = AsyncStream.makeStream() @@ -2197,7 +2264,9 @@ func handleChatStreaming( // We capture the hook in a local variable so that concurrent requests // cannot clobber each other's hook via the global. The global is still // written here because LLMModel.prepare() reads it, but the semaphore - // ensures only one generation runs at a time. + // ensures only one generation runs at a time. Installed BEFORE the + // response is returned: startGeneration (prefill) runs inside the + // consumer task below, so the hook is live for the whole prefill. var heartbeatTask: Task? = nil activePrefillProgressHook = nil if emitPrefillProgress { @@ -2224,7 +2293,12 @@ func handleChatStreaming( } } - Task { + // First byte on the wire before any model work: proves connection + // liveness so a long prefill cannot trip the client's idle timeout + // (e.g. Bun fetch's 10s) into an ECONNRESET-and-retry loop. + cont.yield(": connected\r\n\r\n") + + let consumerTask: Task = Task { var hasToolCalls = false var toolCallIndex = 0 var completionTokenCount = 0 @@ -2241,14 +2315,31 @@ func handleChatStreaming( // arriving in a later chunk merges with the previous one. var emittedTextCount = 0 var heldStopTail = "" - // Unconditional cleanup: guarantees heartbeat is cancelled on ALL exit paths - // (normal completion, client disconnect, or task cancellation during prefill). + // Unconditional cleanup: guarantees heartbeat is cancelled and the + // generation slot is returned on ALL exit paths (normal completion, + // startGeneration failure, client disconnect, or task cancellation). defer { heartbeatTask?.cancel() heartbeatTask = nil activePrefillProgressHook = nil + slot.release() } - + + // Start generation only now — the response headers are already on the + // wire, so even a multi-second prefill leaves an idle-timeout-free + // connection (heartbeat chunks flow if the client opted in). + let generation: (stream: AsyncStream, onPrefillDone: (() async -> Void)?) + do { + generation = try await startGeneration() + } catch { + _ = cont.yield(sseErrorChunk(error)) + _ = cont.yield("data: [DONE]\r\n\r\n") + cont.finish() + return + } + let stream = generation.stream + let onPrefillDone = generation.onPrefillDone + // ── JSON mode streaming: buffer early tokens to strip hallucinated prefixes ── var jsonBuffering = jsonMode var jsonBuffer = "" @@ -2480,8 +2571,11 @@ func handleChatStreaming( cont.finish() let duration = Date().timeIntervalSince(genStart) await stats.requestFinished(tokens: completionTokenCount, duration: duration) - await semaphore.signal() } + // If the client disconnects, Hummingbird tears down the response body, + // terminating sseStream — cancel the consumer so the generation loop stops + // and the defer above returns the slot instead of finishing a dead download. + cont.onTermination = { _ in consumerTask.cancel() } return Response( status: .ok, headers: sseHeaders(), @@ -2499,7 +2593,7 @@ func handleChatNonStreaming( enableThinking: Bool = false, thinkingPreOpened: Bool = false, jsonMode: Bool = false, - semaphore: AsyncSemaphore, + slot: GenerationSlot, stats: ServerStats, genStart: Date, prefillStart: Date, @@ -2549,7 +2643,7 @@ func handleChatNonStreaming( print("srv slot done: id 0 | gen_tokens=\(completionTokenCount) | OS_RAM=\(String(format: "%.1f", postMemSnap.os))GB | MEM_DEMAND=\(String(format: "%.1f", postMemSnap.demand))GB | GPU_MEM=\(String(format: "%.1f", postMemSnap.gpu))GB") let duration = Date().timeIntervalSince(genStart) await stats.requestFinished(tokens: completionTokenCount, duration: duration) - await semaphore.signal() + slot.release() // ── Apply stop sequences to final text ── var finishReason: String @@ -2702,6 +2796,7 @@ func handleTextCompletion( } await semaphore.wait() + let slot = GenerationSlot(semaphore: semaphore) await stats.requestStarted() let genStart = Date() @@ -2711,19 +2806,23 @@ func handleTextCompletion( // ── Get actual prompt token count before generate() to avoid data race ── let promptTokenCount = lmInput.text.tokens.size - let stream = try await container.generate(input: lmInput, parameters: params) let modelId = config.modelId if isStream { + // Deferred: container.generate runs prefill; inside the consumer task it + // happens AFTER the response headers are on the wire (same rationale as + // the chat path — no silent-prefill idle timeout). return handleTextStreaming( - stream: stream, modelId: modelId, stopSequences: stopSequences, - promptTokenCount: promptTokenCount, semaphore: semaphore, stats: stats, + startGeneration: { try await container.generate(input: lmInput, parameters: params) }, + modelId: modelId, stopSequences: stopSequences, + promptTokenCount: promptTokenCount, slot: slot, stats: stats, genStart: genStart, emitPrefillProgress: emitPrefillProgress ) } else { + let stream = try await container.generate(input: lmInput, parameters: params) return try await handleTextNonStreaming( stream: stream, modelId: modelId, stopSequences: stopSequences, - promptTokenCount: promptTokenCount, semaphore: semaphore, stats: stats, genStart: genStart + promptTokenCount: promptTokenCount, slot: slot, stats: stats, genStart: genStart ) } } @@ -2731,11 +2830,11 @@ func handleTextCompletion( // ── Text Streaming ─────────────────────────────────────────────────────────── func handleTextStreaming( - stream: AsyncStream, + startGeneration: @escaping () async throws -> AsyncStream, modelId: String, stopSequences: [String], promptTokenCount: Int, - semaphore: AsyncSemaphore, + slot: GenerationSlot, stats: ServerStats, genStart: Date, emitPrefillProgress: Bool @@ -2764,7 +2863,9 @@ func handleTextStreaming( } } } - Task { + // First byte before any model work — same liveness guarantee as the chat path. + cont.yield(": connected\r\n\r\n") + let consumerTask: Task = Task { var completionTokenCount = 0 var fullText = "" var stopped = false @@ -2773,12 +2874,22 @@ func handleTextStreaming( // a stop sequence (#133). Local to this loop; the chat path has its own pair. var emittedTextCount = 0 var heldStopTail = "" - // Unconditional cleanup: guarantees heartbeat is cancelled on ALL exit paths - // (normal completion, client disconnect, or task cancellation during prefill). + // Unconditional cleanup: cancels the heartbeat and returns the generation + // slot on ALL exit paths (completion, startGeneration failure, disconnect). defer { heartbeatTask?.cancel() heartbeatTask = nil activePrefillProgressHook = nil + slot.release() + } + let stream: AsyncStream + do { + stream = try await startGeneration() + } catch { + _ = cont.yield(sseErrorChunk(error)) + _ = cont.yield("data: [DONE]\n\n") + cont.finish() + return } for await generation in stream { if stopped { break } @@ -2857,8 +2968,8 @@ func handleTextStreaming( cont.finish() let duration = Date().timeIntervalSince(genStart) await stats.requestFinished(tokens: completionTokenCount, duration: duration) - await semaphore.signal() } + cont.onTermination = { _ in consumerTask.cancel() } return Response( status: .ok, headers: sseHeaders(), @@ -2873,7 +2984,7 @@ func handleTextNonStreaming( modelId: String, stopSequences: [String], promptTokenCount: Int, - semaphore: AsyncSemaphore, + slot: GenerationSlot, stats: ServerStats, genStart: Date ) async throws -> Response { @@ -2894,7 +3005,7 @@ func handleTextNonStreaming( } let duration = Date().timeIntervalSince(genStart) await stats.requestFinished(tokens: completionTokenCount, duration: duration) - await semaphore.signal() + slot.release() var finishReason = "stop" if let (trimmedText, _) = checkStopSequences(fullText, stopSequences: stopSequences) { @@ -2958,6 +3069,41 @@ actor AsyncSemaphore { } } +/// Holds one slot acquired from ``AsyncSemaphore`` and guarantees it is +/// released exactly once — whichever of an explicit `release()` or `deinit` +/// fires first. +/// +/// Without this, any `throw` after `semaphore.wait()` (e.g. `container.prepare` +/// or `container.perform` failing) skipped every `signal()` call site and +/// permanently leaked the slot; with the default `--parallel 1` a single failed +/// request wedged the server for all subsequent generations until restart. +final class GenerationSlot: @unchecked Sendable { + private let semaphore: AsyncSemaphore + private let lock = NSLock() + private var released = false + + init(semaphore: AsyncSemaphore) { + self.semaphore = semaphore + } + + func release() { + lock.lock() + let first = !released + released = true + lock.unlock() + guard first else { return } + // Bind locally: a closure that touches `self.semaphore` would capture + // `self`, which the runtime rejects (dangling ref) when this is called + // from `deinit`. + let sem = semaphore + Task { await sem.signal() } + } + + deinit { + release() + } +} + // ── CORS Middleware ─────────────────────────────────────────────────────────── struct CORSMiddleware: RouterMiddleware { @@ -3125,6 +3271,23 @@ func sseHeaders() -> HTTPFields { ]) } +/// Build an OpenAI-style SSE `error` event for a failure after the stream's +/// headers are already sent (the client sees HTTP 200). The message is +/// JSON-encoded, so quotes, backslashes and newlines in it stay valid JSON. +func sseErrorChunk(_ error: Error) -> String { + let payload: [String: Any] = ["error": [ + "message": String(describing: error), + "type": "server_error", + "code": "internal_error", + ]] + guard let data = try? JSONSerialization.data(withJSONObject: payload), + let json = String(data: data, encoding: .utf8) + else { + return "data: {\"error\":{\"message\":\"internal error\",\"type\":\"server_error\",\"code\":\"internal_error\"}}\r\n\r\n" + } + return "data: \(json)\r\n\r\n" +} + /// Build a chat.completion.chunk SSE event. /// - reasoningContent: if non-nil, added to delta as "reasoning_content" (llama-server thinking style) /// - content: if non-nil, added to delta as "content" (standard response text) @@ -3161,8 +3324,7 @@ func sseChunk(modelId: String, reasoningContent: String?, content: String?, fini /// Prefill-progress heartbeat chunk — emitted every 2s while the server is processing the prompt /// when explicitly enabled via `X-SwiftLM-Prefill-Progress: true`. -/// It is sent as a named SSE event (`event: prefill_progress`) to avoid breaking strict -/// OpenAI-compatible clients (e.g. OpenCode), which reject unknown `data:` objects. +/// It is sent as a named SSE event (`event: prefill_progress`). /// Format mirrors llama-server's slot_update event: /// n_past : tokens evaluated so far (real value from chunked prefill, or 0 for single-chunk) /// n_prompt_tokens : total prompt token count @@ -3170,6 +3332,9 @@ func sseChunk(modelId: String, reasoningContent: String?, content: String?, fini /// elapsed_seconds : wall-clock time since the request started /// Note: `model` is intentionally omitted — clients can correlate from preceding stream chunks. /// Note: `on` is accepted as a truthy header value for parity with common reverse proxy conventions. +/// Issue #168: `choices: []` is present so strict OpenAI chunk validators (opencode's +/// ChatCompletionChunk union) accept the payload even when they parse every `data:` line +/// regardless of `event:`. The named event alone was not enough. func ssePrefillChunk(nPast: Int = 0, promptTokens: Int, elapsedSeconds: Int) -> String { let fraction = promptTokens > 0 ? Double(nPast) / Double(promptTokens) : 0.0 let chunk: [String: Any] = [ @@ -3177,7 +3342,8 @@ func ssePrefillChunk(nPast: Int = 0, promptTokens: Int, elapsedSeconds: Int) -> "n_past": nPast, "n_prompt_tokens": promptTokens, "fraction": fraction, - "elapsed_seconds": elapsedSeconds + "elapsed_seconds": elapsedSeconds, + "choices": [Any]() ] let data = try! JSONSerialization.data(withJSONObject: chunk) return "event: prefill_progress\r\ndata: \(String(data: data, encoding: .utf8)!)\r\n\r\n" diff --git a/tests/SwiftLMTests/GenerationSlotTests.swift b/tests/SwiftLMTests/GenerationSlotTests.swift new file mode 100644 index 0000000..39a6e02 --- /dev/null +++ b/tests/SwiftLMTests/GenerationSlotTests.swift @@ -0,0 +1,120 @@ +import XCTest +import Foundation +@testable import SwiftLM + +/// GenerationSlot guarantees the AsyncSemaphore slot acquired after `wait()` +/// is returned exactly once — on explicit `release()` or `deinit` (whichever +/// comes first). Without it, any throw between `wait()` and the success-path +/// `signal()` calls permanently leaked the slot; with the default +/// `--parallel 1` a single failed request wedged the server until restart. +final class GenerationSlotTests: XCTestCase { + + /// Race `wait()` against a timeout: returns true if the wait completed. + private func waitCompletes(_ sem: AsyncSemaphore, timeoutNs: UInt64 = 1_000_000_000) async -> Bool { + await withTaskGroup(of: Bool.self) { group in + group.addTask { + await sem.wait() + await sem.signal() // immediately give it back; only completion matters + return true + } + group.addTask { + try? await Task.sleep(nanoseconds: timeoutNs) + return false + } + let first = await group.next() ?? false + group.cancelAll() + return first + } + } + + /// Simulates the error path: slot created after `wait()`, then dropped + /// without an explicit release (scope unwinds on throw). The deinit must + /// reclaim the slot so the next request can proceed. + func testDeinitReclaimsSlotWithoutExplicitRelease() async { + let sem = AsyncSemaphore(limit: 1) + await sem.wait() + do { + let slot = GenerationSlot(semaphore: sem) + _ = slot + } // deinit → release + + let ok = await waitCompletes(sem) + XCTAssertTrue(ok, "slot was not reclaimed by deinit — server would wedge after one error at --parallel 1") + } + + /// Explicit release on the success path, then deinit when the frame unwinds: + /// the once-flag must prevent a second signal (which would over-admit and + /// break the parallel limit when other requests are queued). + func testExplicitReleaseThenDeinitDoesNotDoubleSignal() async { + let sem = AsyncSemaphore(limit: 1) + await sem.wait() + + let slot = GenerationSlot(semaphore: sem) + slot.release() // success-path release + // slot deinits at end of scope → second release attempt, must no-op + + var bAdmitted = false + let bTask = Task { + await sem.wait() + bAdmitted = true + // hold the slot until the test finishes asserting + try? await Task.sleep(nanoseconds: 2_000_000_000) + await sem.signal() + } + // Let B queue and be admitted by the single legitimate release. + try? await Task.sleep(nanoseconds: 200_000_000) + XCTAssertTrue(bAdmitted, "legitimate release should admit exactly one waiter") + + // If double-signalling had happened, the semaphore would have counted + // an extra free slot and this second wait would complete while B still + // holds the only slot. + var cAdmitted = false + let cTask = Task { + await sem.wait() + cAdmitted = true + await sem.signal() + } + try? await Task.sleep(nanoseconds: 300_000_000) + XCTAssertFalse(cAdmitted, "double-release over-admitted a second concurrent request (parallel limit broken)") + + await bTask.value + await cTask.value + _ = slot + } + + /// Multiple explicit releases (e.g. a success path plus a deferred cleanup + /// that both run) must not signal twice either: exactly one waiter is + /// admitted while the holder still holds the slot. + func testRepeatedReleaseIsIdempotent() async { + let sem = AsyncSemaphore(limit: 1) + await sem.wait() + let slot = GenerationSlot(semaphore: sem) + slot.release() + slot.release() + slot.release() + + var bAdmitted = false + let bTask = Task { + await sem.wait() + bAdmitted = true + // hold the slot so any over-admission would surface as C below + try? await Task.sleep(nanoseconds: 2_000_000_000) + await sem.signal() + } + try? await Task.sleep(nanoseconds: 200_000_000) + XCTAssertTrue(bAdmitted, "single slot must be reclaimed exactly once") + + var cAdmitted = false + let cTask = Task { + await sem.wait() + cAdmitted = true + await sem.signal() + } + try? await Task.sleep(nanoseconds: 300_000_000) + XCTAssertFalse(cAdmitted, "extra signals leaked additional slots beyond the limit") + + await bTask.value + await cTask.value + withExtendedLifetime(slot) {} // deinit runs here; flag must make it a no-op + } +} diff --git a/tests/SwiftLMTests/JinjaSanitizerTests.swift b/tests/SwiftLMTests/JinjaSanitizerTests.swift new file mode 100644 index 0000000..3fe38ca --- /dev/null +++ b/tests/SwiftLMTests/JinjaSanitizerTests.swift @@ -0,0 +1,83 @@ +import XCTest +import Foundation +@testable import SwiftLM + +/// Issue #168: JSON `null` (NSNull) in tool schemas used to abort Jinja.Value conversion +/// with a misleading "Optional" error, mislabeled as a broken chat template. +final class JinjaSanitizerTests: XCTestCase { + + func testDropsTopLevelNSNull() { + XCTAssertNil(sanitizeForJinja(NSNull())) + } + + func testDropsNestedObjectNulls() throws { + let tool: [String: any Sendable] = [ + "type": "function", + "function": [ + "name": "probe", + "parameters": [ + "type": "object", + "properties": [ + "query": [ + "type": "string", + "default": NSNull() as any Sendable, + ] as [String: any Sendable], + ] as [String: any Sendable], + ] as [String: any Sendable], + ] as [String: any Sendable], + ] + + let cleaned = try XCTUnwrap(sanitizeForJinja(tool) as? [String: any Sendable]) + let fn = try XCTUnwrap(cleaned["function"] as? [String: any Sendable]) + let params = try XCTUnwrap(fn["parameters"] as? [String: any Sendable]) + let props = try XCTUnwrap(params["properties"] as? [String: any Sendable]) + let query = try XCTUnwrap(props["query"] as? [String: any Sendable]) + + XCTAssertNil(query["default"], "null default must be stripped") + XCTAssertEqual(query["type"] as? String, "string") + XCTAssertEqual(params["type"] as? String, "object") + } + + func testDropsNullArrayElements() throws { + let value: [String: any Sendable] = [ + "enum": [1, NSNull(), 2] as [any Sendable] + ] + let cleaned = try XCTUnwrap(sanitizeForJinja(value) as? [String: any Sendable]) + let enumValues = try XCTUnwrap(cleaned["enum"] as? [Any]) + XCTAssertEqual(enumValues.count, 2) + XCTAssertFalse(enumValues.contains { $0 is NSNull }) + } + + func testPreservesScalarsAndStructure() throws { + let value: [String: any Sendable] = [ + "type": "string", + "minimum": 0, + "required": ["command"] as [any Sendable], + "flag": true, + ] + let cleaned = try XCTUnwrap(sanitizeForJinja(value) as? [String: any Sendable]) + XCTAssertEqual(cleaned["type"] as? String, "string") + XCTAssertEqual(cleaned["minimum"] as? Int, 0) + XCTAssertEqual(cleaned["flag"] as? Bool, true) + XCTAssertEqual((cleaned["required"] as? [Any])?.count, 1) + } + + func testUnwrapsNestedOptional() throws { + let wrapped: any Sendable = Optional.some("hi") + let cleaned = sanitizeForJinja(wrapped) + XCTAssertEqual(cleaned as? String, "hi") + + let empty: any Sendable = Optional.none + XCTAssertNil(sanitizeForJinja(empty)) + } + + func testMapValuesDeepOnToolDict() throws { + let dict: [String: any Sendable] = [ + "keep": "x", + "drop": NSNull(), + ] + let cleaned = dict.mapValuesDeep(sanitizeForJinja) + XCTAssertEqual(cleaned["keep"] as? String, "x") + XCTAssertNil(cleaned["drop"]) + } +} diff --git a/tests/SwiftLMTests/ServerSSETests.swift b/tests/SwiftLMTests/ServerSSETests.swift index cb05374..6db5ee7 100644 --- a/tests/SwiftLMTests/ServerSSETests.swift +++ b/tests/SwiftLMTests/ServerSSETests.swift @@ -46,7 +46,9 @@ final class ServerSSETests: XCTestCase { XCTAssertEqual(json["n_prompt_tokens"] as? Int, 128) XCTAssertEqual(json["elapsed_seconds"] as? Int, 4) XCTAssertNil(json["object"]) - XCTAssertNil(json["choices"]) + // Issue #168: empty choices keeps strict OpenAI chunk validators (opencode) happy + // even when they validate every data: line regardless of event name. + XCTAssertEqual((json["choices"] as? [Any])?.count, 0) } // MARK: - 1b: Zero-token boundary (no divide-by-zero crash) @@ -93,7 +95,24 @@ final class ServerSSETests: XCTestCase { XCTAssertNil(json["id"], "prefill chunk must not carry an id field") XCTAssertNil(json["object"], "prefill chunk must not carry an object field") XCTAssertNil(json["model"], "prefill chunk must not carry a model field") - XCTAssertNil(json["choices"], "prefill chunk must not carry a choices field") + // Issue #168: choices must be present but empty — opencode's chunk union + // requires `choices` (or `error`); omitting it fails type validation. + XCTAssertEqual((json["choices"] as? [Any])?.count, 0, + "prefill chunk must carry an empty choices array") + } + + // MARK: - Issue #168: empty choices is what strict validators require + + func testPrefillChunk_ChoicesIsEmptyArrayForStrictValidators() throws { + let chunk = ssePrefillChunk(nPast: 1, promptTokens: 4, elapsedSeconds: 1) + let prefix = "event: prefill_progress\r\ndata: " + let suffix = "\r\n\r\n" + let payload = String(chunk.dropFirst(prefix.count).dropLast(suffix.count)) + let data = try XCTUnwrap(payload.data(using: .utf8)) + let json = try XCTUnwrap(JSONSerialization.jsonObject(with: data) as? [String: Any]) + + let choices = try XCTUnwrap(json["choices"] as? [Any], "choices must be present") + XCTAssertTrue(choices.isEmpty) } // MARK: - 1e: PrefillState.finish() is idempotent (Issue #2 guard) @@ -120,4 +139,27 @@ final class ServerSSETests: XCTestCase { // is irrelevant to correctness. We capture the current contract here. // If a post-done guard is added later, add XCTAssertNotEqual(await state.nPast, 999). } + + // MARK: - Error event stays valid JSON for any message text + + private struct MessyError: Error, CustomStringConvertible { + let description = "bad \"quote\", back\\slash\nnew line" + } + + func testErrorChunkEncodesMessageAsValidJSON() throws { + let chunk = sseErrorChunk(MessyError()) + + let prefix = "data: " + let suffix = "\r\n\r\n" + XCTAssertTrue(chunk.hasPrefix(prefix)) + XCTAssertTrue(chunk.hasSuffix(suffix)) + + let payload = String(chunk.dropFirst(prefix.count).dropLast(suffix.count)) + let obj = try XCTUnwrap( + JSONSerialization.jsonObject(with: Data(payload.utf8)) as? [String: Any]) + let err = try XCTUnwrap(obj["error"] as? [String: Any]) + XCTAssertEqual(err["message"] as? String, MessyError().description) + XCTAssertEqual(err["type"] as? String, "server_error") + XCTAssertEqual(err["code"] as? String, "internal_error") + } } diff --git a/tests/test-opencode.sh b/tests/test-opencode.sh index 6d7e904..bb41471 100755 --- a/tests/test-opencode.sh +++ b/tests/test-opencode.sh @@ -222,6 +222,81 @@ else fail "opencode-shaped request failed: $AGENT_OUT" fi +# ── Test 3: tools + heartbeat combined (Issue #168) ──────────────── +# Test 1 covers heartbeat without tools; Test 2 covers tools without the heartbeat +# header. Issue #168 reported both gaps at once: a tools-bearing request with +# X-SwiftLM-Prefill-Progress enabled failed on the null-bearing tool schema (Jinja +# NSNull conversion) *and* would have hit opencode's strict validation of the +# prefill_progress payload. Exercise the intersection here. +log "Test 3: tools + prefill-progress heartbeat (Issue #168)" + +cat << 'PYEOF' > /tmp/opencode_tools_heartbeat_test.py +import json, os, sys +import openai + +client = openai.OpenAI(base_url=os.environ["OPENAI_BASE_URL"], api_key="sk-test", max_retries=0) + +# A tool schema shaped like opencode's zod/effect output — includes JSON nulls +# (`default: null`) that previously crashed swift-jinja Value.init(any:) with +# "Cannot convert value of type Optional to Jinja Value" (#168). +TOOLS = [ + {"type": "function", "function": { + "name": "bash", + "description": "Execute a shell command", + "parameters": {"type": "object", + "properties": { + "command": {"type": "string"}, + "timeout": {"type": "integer", "default": None}, + }, + "required": ["command"]}}}, +] +MESSAGES = [ + {"role": "system", "content": "You are a coding agent."}, + {"role": "user", "content": "Say hi."}, +] + +try: + stream = client.chat.completions.create( + model=os.environ["MODEL"], messages=MESSAGES, tools=TOOLS, + stream=True, max_tokens=64, temperature=0, + stream_options={"include_usage": True}, + # Enables the named `event: prefill_progress` heartbeat payloads. + extra_headers={"X-SwiftLM-Prefill-Progress": "true"}, + ) +except Exception as e: + print(f"Error: request rejected: {e}") + sys.exit(1) + +chunks = 0 +finish = None +try: + for chunk in stream: + chunks += 1 + for choice in chunk.choices: + if choice.finish_reason: + finish = choice.finish_reason +except Exception as e: + print(f"Error: SSE stream failed to parse (heartbeat or tools payload rejected): {e}") + sys.exit(1) + +if chunks == 0: + print("Error: stream produced no chunks") + sys.exit(1) + +print(f"Success: {chunks} chunks, finish_reason={finish}") +PYEOF + +set +e +HB_OUT=$("$VENV_DIR/bin/python" /tmp/opencode_tools_heartbeat_test.py 2>&1) +HB_EXIT=$? +set -e + +if [ $HB_EXIT -eq 0 ]; then + pass "tools + heartbeat stream accepted — $HB_OUT" +else + fail "tools + heartbeat stream rejected: $HB_OUT" +fi + # ── Results ────────────────────────────────────────────────────────── echo "" log "═══════════════════════════════════════" diff --git a/tests/test-server.sh b/tests/test-server.sh index 7a50ed6..110bfe9 100755 --- a/tests/test-server.sh +++ b/tests/test-server.sh @@ -1126,6 +1126,82 @@ else fi +# ── Test 37: /v1/models + /health respond during in-flight generation ──────── +# The main server runs with the default --parallel 1, so a generation request +# holds the only slot. Control-plane endpoints must NOT queue behind it. +log "Test 37: control-plane endpoints respond during in-flight generation" + +INFLIGHT_PROBE_OK=true +curl -sf -N -X POST "$URL/v1/chat/completions" \ + -H "Content-Type: application/json" \ + -d "{\"model\":\"$MODEL\",\"stream\":true,\"max_tokens\":150,\"messages\":[{\"role\":\"user\",\"content\":\"Count from one to one hundred, one number per line.\"}]}" \ + --max-time 90 \ + -o /tmp/mlx_inflight_stream.txt & +INFLIGHT_PID=$! + +sleep 1.0 + +curl -sf --max-time 3 "$URL/v1/models" -o /tmp/mlx_inflight_models.json || INFLIGHT_PROBE_OK=false +curl -sf --max-time 3 "$URL/health" -o /tmp/mlx_inflight_health.json || INFLIGHT_PROBE_OK=false + +wait "$INFLIGHT_PID" || INFLIGHT_PROBE_OK=false + +if [ "$INFLIGHT_PROBE_OK" = true ] \ + && jq -e '.data | length > 0' /tmp/mlx_inflight_models.json >/dev/null 2>&1 \ + && grep -q "data: \[DONE\]" /tmp/mlx_inflight_stream.txt 2>/dev/null; then + pass "In-flight probe: /v1/models + /health answered within 3s while generation held the slot" +else + fail "In-flight probe: endpoint blocked/timed out, or in-flight stream did not complete" +fi +rm -f /tmp/mlx_inflight_models.json /tmp/mlx_inflight_health.json /tmp/mlx_inflight_stream.txt + + +# ── Test 38: streaming TTFB — headers + ": connected" before model prefill ─── +# Regression test for the silent-prefill stall: the handler used to run the +# whole prefill inside container.perform before returning headers, so clients +# saw no bytes for the entire prefill (Bun fetch 10s idle → ECONNRESET/retry). +# A large prompt makes prefill multi-second; TTFB must stay well under that. +# ~2k tokens. Gemma-4 prefill allocates O(n²) attention in one Metal buffer +# (~149 bytes × n² for gemma-4-e2b) and the CI runner caps a single buffer at +# 3.5 GB, i.e. ~4.9k tokens max. 7k tokens crashed the server with a 7.4 GB +# malloc; 2k tokens needs ~0.6 GB and still gives a measurable prefill. +log "Test 38: streaming TTFB — ': connected' arrives before model prefill" + +python3 -c "print(' '.join(f'{i:06d}' for i in range(300)))" > /tmp/mlx_ttfb_prompt.txt +jq -nc --arg m "$MODEL" --rawfile p /tmp/mlx_ttfb_prompt.txt \ + '{model:$m, stream:true, max_tokens:5, messages:[{role:"user",content:("Summarize this list in one word:\n"+$p)}]}' \ + > /tmp/mlx_ttfb_body.json + +TTFB=$(curl -sf -N -X POST "$URL/v1/chat/completions" \ + -H "Content-Type: application/json" \ + --data-binary @/tmp/mlx_ttfb_body.json \ + --max-time 120 \ + -o /tmp/mlx_ttfb_stream.txt \ + -w '%{time_starttransfer}' 2>/dev/null || true) + +TTFB_OK=false +if [ -n "$TTFB" ] && awk "BEGIN{exit !($TTFB < 2.0)}" 2>/dev/null; then + TTFB_OK=true +fi + +CONNECTED_OK=false +if head -5 /tmp/mlx_ttfb_stream.txt 2>/dev/null | grep -q ": connected"; then + CONNECTED_OK=true +fi + +DONE_OK=false +if grep -q "data: \[DONE\]" /tmp/mlx_ttfb_stream.txt 2>/dev/null; then + DONE_OK=true +fi + +if [ "$TTFB_OK" = true ] && [ "$CONNECTED_OK" = true ] && [ "$DONE_OK" = true ]; then + pass "Early TTFB: first byte in ${TTFB}s (< 2s, prefill deferred), ': connected' sent, stream completed" +else + fail "Early TTFB: ttfb='${TTFB}' (<2s: $TTFB_OK), ': connected': $CONNECTED_OK, [DONE]: $DONE_OK" +fi +rm -f /tmp/mlx_ttfb_prompt.txt /tmp/mlx_ttfb_body.json /tmp/mlx_ttfb_stream.txt + + # ── Results ────────────────────────────────────────────────────────── echo "" log "═══════════════════════════════════════"