fix(mcp): keep tablepro-mcp reading a non-blocking stdin and drain every pipe without O_NONBLOCK - #3137
Merged
Conversation
…ery pipe without O_NONBLOCK
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.
Follow-ups to #3133 (
PipeReader/DescriptorRead), from its adversarial review. That review found the cause proven and the fix sound, plus five minor findings. I checked each one against the code, and all five held.What was wrong
PipeReaderTests.dispatchedCallbackAfterDrainReadsNothingcalled the capturedreadabilityHandleron an empty pipe while its writer was still open. APipeReaderthat reads before it checks for a consumer blocks inread(2)there, and.timeLimitcannot interrupt a synchronous body that is blocked. The review's mutant of that shape was still blocked after 3s.drainStopsAtTheLimitWithoutWaitingOnTheWriterandDescriptorReadTests.returnsWhatHasArrivedcheck the same property, "does not wait on an open writer", so a regression there waits forever too.PipeReaderis new, so its tests cannot run against the old code.PreConnectHookRunnerTestsonly fails on a busy machine. Per the review it lost 0 messages in 15,360 quiet runs and about 600 in 3,200 with 12 CPU burners. RevertingProcessNativeDumpRunnerto fix(export): give a dump the TLS options its own client tool takes #3049's shape turned no test red.BridgeStdinended the MCP session onEAGAIN. On a non-blocking stdin, the first empty read throwsEAGAIN, and the catch logged "Reading stdin failed" and finished the stream. Before fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 it was worse. v0.75.0'sBridgeStdin, compiled standalone and fed a non-blocking pipe, aborts with exit 134:PipeReadertreats the same errno as "nothing has arrived yet".CLIToolVersionProbe.readAvailablestill setO_NONBLOCKon its pipe to drain it. fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 replaced this loop in the dump runner withhasInputWithoutWaiting, and the probe held the last copy. It is not a crash path, because no handler races it. It came in with fix(export): give a dump the TLS options its own client tool takes #3049, so it never shipped.///lines that narrate mechanism and measurements, and leftDescriptorReadandPipeReaderwith implicit access control.stderrLock's doc also went stale: it said the lock was separate becausestateLock"must never wait on a pipe", but after fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133stderrLockno longer covers a read.What changed
DescriptorRead:nextBytes(from:upTo:)is a read that waits for input whether or not the descriptor is blocking. OnEAGAINit waits inpoll(2)(POLLIN, no timeout,EINTRretried) and reads again. An empty result means end of file, and any other error is thrown.bufferedBytes(from:upTo:)returns what the descriptor holds now, up to a limit, and never waits on a writer. This is the loopPipeReader.stop(drainingUpTo:)had, moved here so the version probe can share it.internal, with no doc comments.BridgeStdinreads throughnextBytes. It ends the session only at end of file or on an error other thanEAGAIN. The blocking read it had before could not be cancelled either, so waiting inpollwith no timeout costs nothing new.CLIToolVersionProbe:readAvailableis gone, and the probe callsbufferedBytes(upTo: outputCap). No code in the app setsO_NONBLOCKon a pipe to drain it any more. The twoO_NONBLOCKwrites left are the SSH tunnel sockets.PipeReader:stop(drainingUpTo:)hands the drained bytes to the consumer as one chunk frombufferedBytes. The class is nowinternal, with no doc comments.ProcessNativeDumpRunner:stderrLockis folded intostateLock, which removes the stale doc along with the lock. The append is a memory copy, and no path holdsstateLockwhile it calls into the reader, so the lock order stays acyclic: the reader's lock, thenstateLock. The runner takes its stderrPipeas an init parameter defaulting toPipe(), which is the seam the new test uses.SupervisedProcessRunner.finishand the fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 tests lose their doc comments. The measurements they carried are in fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133's body.Audited, left as they are
GitProcessRunner.draindoes a blockingread(upToCount:)to end of file on its own queue, bounded by the runner's timeout. It sets noO_NONBLOCKand has no handler.AgentCLIProcess.runcallsreadToEnd()in its termination handler, andstreamLinesusesbytes.lines. Neither setsO_NONBLOCKor races a handler.MCPStdioMessageTransportreads withstdin.bytes. Only its tests construct it. The bridge usesBridgeStdin.Tests, and the edit that turns each one red
Each mutant run below used a build of this branch with one edit applied, but ran an earlier revision of these tests: in the logged runs the CI-ordering and dump tests still end with a
bufferedBytesread, where the committed ones close the writer and read withavailableBytes. Three of the edits were rerun against the committed tests in #3142 (the #3049 runner, the read before the consumer check, andbufferedByteswithout itspollcheck), and each is red there within 10s, with nothing hung.ProcessNativeDumpRunnerTests/lateStderrCallbackLeavesThePipeAlone(new)ProcessNativeDumpRunner.swiftput back to #3049'savailableDatahandler andO_NONBLOCKdrain, keeping the pipe parameterO_NONBLOCK → 4, and the late callback took the test's bytesPipeReaderTests/dispatchedCallbackAfterDrainReadsNothing(restructured)readArrivedBytesreads before it checks for a consumer, the review's mutantBridgeStdinTests/nonBlockingStdinReadsUntilEndOfFile(new)nextBytesreduced to oneavailableBytescall, which is #3133'sBridgeStdinEAGAINerror loggedDescriptorReadTests/nextBytesWaitsOnANonBlockingPipe(new)EAGAINthrownDescriptorReadTests/bufferedBytesTakeWhatThePipeHolds(new), plus the CI-ordering, dump andstopWaitsForDeliveryInProgresstestsbufferedBytesdrops itspollcheck and reads until the limit or end of fileThe dump runner test injects a pipe and keeps a
dupof its write end open, the way a surviving grandchild would. The tool waits on a gate file, so the test can capture thereadabilityHandlerbefore the tool exits. Once the result is in, the test writeslateand calls the captured handler. It then checks three things: the descriptor is still blocking, the handler is cleared, andlateis still in the pipe. Nothing in it depends on load.BridgeStdinTests/unreadableStdinEndsTheSessionfeedsBridgeStdina directory. The read fails withEISDIR, and the session still ends with one error logged.Two test helpers:
HeldOpenWriterruns a call that must not wait on an open writer. If the call has not returned within 10s, it closes the writer so the call can return, and the test fails instead of hanging.BackgroundPipeWriterwrites from a thread withF_SETNOSIGPIPEset. The first mutant run showed why this is needed: a test that failed early freed its pipe, the writer thread's next write raisedSIGPIPE, and the test host died, taking every test running beside it down as "crashed with signal pipe". The four fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 tests that wrote from a detached thread use it too.Every final read in these tests now runs after the last writer is closed, so it returns data or end of file and cannot block.
CHANGELOG
tablepro-mcpcrashing when its standard input was non-blocking." v0.75.0 has this crash, as the standalone build in point 3 shows.Verification
verify.sh teston DescriptorReadTests, PipeReaderTests, ProcessNativeDumpRunnerTests, BridgeStdinTests, CLIToolVersionProbeTests, PreConnectHookRunnerTests, SupervisedProcessRunnerTests, MCPBridgeIntegrationTests, MCPStdioMessageTransportTests, TunnelCommandManagerTests, CloudSQLProxyManagerTests and CloudflareTunnelManagerTests: the result bundle holds 87 cases, 85 passed.CLIToolVersionProbeTestscases, and they were environmental. The probe logged "did not answer --version within 3s" before it read anything, andsyspolicyd's XProtect was still scanning the freshly written test script after the deadline, at a load average of 36 to 105 from other builds on the machine.CLIToolVersionProbeTestson its own at a lower load: 4/4 passed.CLIToolVersionProbe.swiftandDescriptorRead.swift, compiled into a standalone program, returned the banner in 4ms, the banner from a tool that leaves(sleep 30) &holding stdout in 315ms, and nil for a failing tool.DescriptorRead.swiftcompiles in themcp-servertarget (tablepro-mcp) as well as the app.swiftlint lint --stricton all 13 changed Swift files: 0 violations.check-test-suite-attributes.py,check-log-privacy.pyandaudit-refactor-health.sh --checkall pass.CI
macOS Tests run 36184824105 at 255515d passed: Detect changes, Package Tests, Build for testing, Unit tests, UI tests 0/3, 1/3 and 2/3, and the macOS Tests Gate. Every touched suite ran in the Unit tests job and passed:
Repo Hygiene and Docs passed. The iOS Tests Gate passed;
Run iOS Testsitself was skipped by its change detection.Not covered
AgentCLIProcess.runreads its combined output only after the process exits, so a tool that writes more than a pipe holds (64 KiB) would block on the write and never exit. Its callers run--version,auth status,status,loginandlogout, whose output is small, and this is a different shape from the one fixed here.ProcessSupervisedRunnerstill reads stderr to end of file, as fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 left it on purpose.TableProUITestsautomation. Local XCUITest is blocked on this machine, and CI runs the UI suites.