Remove unnecessary await on sync callTool overload#124
Merged
Conversation
4 tasks
The RequestContext<CallTool.Result>-returning overload of callTool is synchronous; the async work happens when accessing context.value. The spurious await produced a "no 'async' operations occur within 'await' expression" warning on every clean build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
obj-p
force-pushed
the
fix/remove-unnecessary-await-calltool
branch
from
4631e2d to
0f3d352
Compare
obj-p
added a commit
that referenced
this pull request
Apr 20, 2026
Issue #127 tracked two CLI integration test flakes that escalated into a 6-test cascade on PR #124's rerun. Root cause: CLIRunner.runProcess used withCheckedThrowingContinuation with no cancellation handler. When .timeLimit(.minutes(20)) fired, the test Task was cancelled and the DaemonTestLock unlock ran — but the subprocess kept running and the daemon stayed busy serving it. The next test acquired the released lock, tried to talk to the still-busy daemon, and hung. This ports PR #126's MCP-test playbook to CLI integration tests: diagnostics first, cancellation safety, source-located failures. CLIRunner (Tests/CLIIntegrationTests/CLIRunner.swift): - Per-subcommand timeout table: 60s default; start/run 240s; kill-daemon 10s. Override per call via the new `timeout:` parameter. - runProcess wrapped in withTaskCancellationHandler. State machine (OSAllocatedUnfairLock<RunState>) guards the continuation against double-resume across the four terminal paths (normal exit, timeout, cancellation, run() throw). On cancellation/timeout: SIGTERM, then SIGKILL after a 2s grace. - Stderr captured to a per-call temp file (mirrors MCPTestServer's pattern from PR #126: avoids both the CFRunLoop-retention bug of Pipe+readabilityHandler and the shared-stderr NSApplication startup hang of inheriting FileHandle.standardError). - On timeout: subprocess stderr + the daemon's serve.log are dumped via Issue.record so the next CI failure produces actionable diagnostics instead of just a step-kill. DaemonTestLock (Tests/CLIIntegrationTests/DaemonTestLock.swift): - Optional `context: String` parameter (defaults to #function) is written into serve.log as a "=== TEST: <ctx> @ <iso8601> ===" marker so CI dumps are greppable to the failing test's window. - serve.log is truncated once per process (didTruncateLog flag) so a failure dump shows just the failing run rather than accumulated history. Truncation runs INSIDE the lock so concurrent suites can't race. SnapshotCommandTests: - Added `cleanSlate()` (kill-daemon --timeout 2) parity with SwitchCommandTests. Without it, a wedged daemon from a prior test silently corrupts subsequent snapshots; with A1's subprocess kill in place, the wedged daemon would still persist between tests until the next cleanSlate. MCPServer.handlePreviewSwitch (Sources/PreviewsCLI/MCPServer.swift): - Added daemon-side bounds check on previewIndex in BOTH iOS and macOS paths before delegating to switchPreview. Returns isError=true with "Preview index N out of range (available: 0..<M)". The compile path already validated, but the observed `switch 99` hang was upstream of switchPreview entirely — this belt guarantees a fast structured failure regardless. SwitchCommandTests.switchOutOfRange now asserts on the exact substring so a regression that bypasses this check fails loud rather than silently passing on any non-zero exit. CI (.github/workflows/ci.yml): - "Dump daemon log on CLI test failure" step added after the CLI integration tests step in the build-and-test job (mirror of the iOS job's existing block). On step failure, dumps $PREVIEWSMCP_SOCKET_DIR /serve.log + lists the socket dir. - CLI step timeout cut from 60m → 15m. Per-test .timeLimit cut from 20m → 5m (10m for the two iOS-simulator tests). Warm-cache local wall- clock is ~3 min for 61 tests with the slowest individual test at 33s and slowest body times around 8s. The new bounds preserve substantial cold-cache headroom while ensuring that on a hang the per-test .timeLimit fires first with a source-located Testing error rather than a bare GHA step kill. Verification: - swift test --filter CLIIntegrationTests --skip snapshotIOS --skip iosCLIWorkflow → 61/61 green in 174s. - swift test --filter PreviewsCoreTests → 204/204 green. - Negative test: `CLIRunner.run("serve", timeout: .seconds(3))` failed at exactly 3.007s with the diagnostic dump and SIGTERM reaching the daemon — confirms the timeout + kill path end-to-end. Stage B (the underlying color-scheme bug) and the upstream switch hang are deliberately unfixed here — they require a diagnostic-rich reproduction to investigate without speculation. The infrastructure landed here makes that reproduction possible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
obj-p
added a commit
that referenced
this pull request
Apr 20, 2026
The iOS `iosCLIWorkflow` integration test has been flaky on main since at least PR #124 — the test's `run --platform ios` call would occasionally hang for the full 20-min step timeout after logging step [8/8] "Waiting for host app connection...". Turns out the underlying 10s timeout was silently broken. `acceptConnection(timeout:)` races two tasks in a throwing task group: one that awaits a DispatchSourceRead inside a `withCheckedThrowingContinuation`, and one that sleeps for the timeout and throws. When the timeout task throws, the task group cancels the first task — but `withCheckedThrowingContinuation` does not respond to task cancellation. The continuation stayed suspended; the dispatch source kept running in `.global()`; the task group waited forever for its cancelled child to actually terminate; the whole call hung. Fix: wrap the continuation in `withTaskCancellationHandler` so cancellation can reach the dispatch source. A `DispatchSourceBox` holder (@unchecked Sendable, NSLock-guarded — DispatchSourceRead is not Sendable under Swift 6) bridges the source's identity across the closure boundary so the cancel handler can call `source.cancel()`. That fires the source's existing `setCancelHandler`, which resumes the continuation with `socketAcceptTimeout`, which lets Task 1 terminate, which lets the group unwind normally. After this fix: if the host app fails to connect within 10s, `run` fails cleanly with `socketAcceptTimeout` in ~80s (compile + build + boot + install + launch + 10s) instead of hanging until the test .timeLimit fires. That's both the correct semantic and what makes PR #129's CLIRunner-timeout diagnostics actually useful for iOS regressions — previously, the 20-min step hang outran every timeout in the stack. Scope note: this bug predates PR #129 but surfaced because the new per-call CLIRunner timeout (240s) turned a 20-min silent hang into a 4-min diagnosed failure. Fixing it here rather than filing a follow-up per the project convention of fixing bugs encountered during testing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RequestContext<CallTool.Result>-returning overload ofcallToolis synchronous; the async work happens when accessingcontext.value.awaitincallToolStructuredproduced ano 'async' operations occur within 'await' expressionwarning on every clean build.Test plan
swift package clean && swift build— no warnings