fix(mcp): avoid task-group teardown abort in runTransport retry backoff#517
Open
pnascimento9596 wants to merge 1 commit into
Open
fix(mcp): avoid task-group teardown abort in runTransport retry backoff#517pnascimento9596 wants to merge 1 commit into
pnascimento9596 wants to merge 1 commit into
Conversation
Collaborator
|
Thanks for contributing! This looks like a great fix |
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
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.
What changed
Replaced the three-child throwing task-group race in
MCPService.run()with an explicit-task race that:MCPServiceProxyTaskGroupPolicy, terminal-record persistence, the post-race cancellation check, kill-signal semantics, andrunTransport()retry/backoff behavior are unchanged.Added deterministic tests for every child winning with either a value or error, loser cancellation and completion before return, and outer cancellation draining all three children. The SIGABRT is a Swift 6.2 release-runtime interaction and is not unit-reproduced; the tests pin the replacement control-flow structure identified by the issue's crash forensics.
The contract ledger also reconciles three executable IDs that were already missing on current
main, which was required for exact ledger verification after adding this suite.Why
Issue #513 maps the residual
swift_task_deallocabort to the retry-backoff sleep inMCPService.runTransport()while the surrounding task group is being cancelled. This mirrors the crash class reported in #139 and fixed at the initialization site in #140 by commit9f881efb.Flagged decision for maintainer review: this restructures the second race site away from a value-returning throwing task group, following #140's explicit-task settlement and drain pattern. Please scrutinize the preserved first-completion priority, especially value versus error ordering.
Validation
Passed:
make dev-test FILTER=MCPServiceProxyRaceTests(3 tests)make dev-test FILTER=MCPProxyTerminalRecordTests(7 tests)make dev-test-listpython3 Scripts/test_suite_optimizer.py verify-ledger --ledger Scripts/Fixtures/test-suite-contract-ledger.tsv(3175 IDs)make dev-format-checkmake dev-lintmake guardrailsmake dev-swift-build PRODUCT=repoprompt-mcpcommitandpushLocal full-root status:
make dev-test-impactedcorrectly required the full root suite because the ledger is a broad boundary. Three coordinated full-root attempts did not complete green due unrelated, nondeterministic codemap/worktree failures. The repeated failure wasPersistentAgentModeMCPReadFileConnectionTests/testPairAgentOwnedNoRangeReadSelectsNonEmptyWorktreeLogicalFile; it passed immediately when rerun alone. One attempt also failed two other unrelated codemap tests. The issue #513 focused suites stayed green.Not run:
Fixes #513