Skip to content

fix(mcp): keep tablepro-mcp reading a non-blocking stdin and drain every pipe without O_NONBLOCK - #3137

Merged
datlechin merged 1 commit into
mainfrom
fix/pipe-reader-followups
Sep 26, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/pipe-reader-followups

Conversation

@datlechin

@datlechin datlechin commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

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

  1. A regression in the CI-ordering test would hang instead of failing. PipeReaderTests.dispatchedCallbackAfterDrainReadsNothing called the captured readabilityHandler on an empty pipe while its writer was still open. A PipeReader that reads before it checks for a consumer blocks in read(2) there, and .timeLimit cannot interrupt a synchronous body that is blocked. The review's mutant of that shape was still blocked after 3s. drainStopsAtTheLimitWithoutWaitingOnTheWriter and DescriptorReadTests.returnsWhatHasArrived check the same property, "does not wait on an open writer", so a regression there waits forever too.
  2. Nothing turned red at the crash site without machine load. PipeReader is new, so its tests cannot run against the old code. PreConnectHookRunnerTests only 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. Reverting ProcessNativeDumpRunner to fix(export): give a dump the TLS options its own client tool takes #3049's shape turned no test red.
  3. BridgeStdin ended the MCP session on EAGAIN. On a non-blocking stdin, the first empty read throws EAGAIN, 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's BridgeStdin, compiled standalone and fed a non-blocking pipe, aborts with exit 134:
    *** Terminating app due to uncaught exception 'NSFileHandleOperationException',
        reason: '*** -[NSConcreteFileHandle availableData]: No such process'
    
    PipeReader treats the same errno as "nothing has arrived yet".
  4. CLIToolVersionProbe.readAvailable still set O_NONBLOCK on 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 with hasInputWithoutWaiting, 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.
  5. Style. fix(connections): read every subprocess pipe through one reader that cannot raise or read after it stops #3133 added 42 /// lines that narrate mechanism and measurements, and left DescriptorRead and PipeReader with implicit access control. stderrLock's doc also went stale: it said the lock was separate because stateLock "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 #3133 stderrLock no longer covers a read.

What changed

  • DescriptorRead:
    • nextBytes(from:upTo:) is a read that waits for input whether or not the descriptor is blocking. On EAGAIN it waits in poll(2) (POLLIN, no timeout, EINTR retried) 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 loop PipeReader.stop(drainingUpTo:) had, moved here so the version probe can share it.
    • The type is now internal, with no doc comments.
  • BridgeStdin reads through nextBytes. It ends the session only at end of file or on an error other than EAGAIN. The blocking read it had before could not be cancelled either, so waiting in poll with no timeout costs nothing new.
  • CLIToolVersionProbe: readAvailable is gone, and the probe calls bufferedBytes(upTo: outputCap). No code in the app sets O_NONBLOCK on a pipe to drain it any more. The two O_NONBLOCK writes left are the SSH tunnel sockets.
  • PipeReader: stop(drainingUpTo:) hands the drained bytes to the consumer as one chunk from bufferedBytes. The class is now internal, with no doc comments.
  • ProcessNativeDumpRunner: stderrLock is folded into stateLock, which removes the stale doc along with the lock. The append is a memory copy, and no path holds stateLock while it calls into the reader, so the lock order stays acyclic: the reader's lock, then stateLock. The runner takes its stderr Pipe as an init parameter defaulting to Pipe(), which is the seam the new test uses.
  • SupervisedProcessRunner.finish and 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.drain does a blocking read(upToCount:) to end of file on its own queue, bounded by the runner's timeout. It sets no O_NONBLOCK and has no handler.
  • AgentCLIProcess.run calls readToEnd() in its termination handler, and streamLines uses bytes.lines. Neither sets O_NONBLOCK or races a handler.
  • MCPStdioMessageTransport reads with stdin.bytes. Only its tests construct it. The bridge uses BridgeStdin.

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 bufferedBytes read, where the committed ones close the writer and read with availableBytes. Three of the edits were rerun against the committed tests in #3142 (the #3049 runner, the read before the consumer check, and bufferedBytes without its poll check), and each is red there within 10s, with nothing hung.

Test Edit Result
ProcessNativeDumpRunnerTests/lateStderrCallbackLeavesThePipeAlone (new) ProcessNativeDumpRunner.swift put back to #3049's availableData handler and O_NONBLOCK drain, keeping the pipe parameter red in 40ms: O_NONBLOCK → 4, and the late callback took the test's bytes
PipeReaderTests/dispatchedCallbackAfterDrainReadsNothing (restructured) readArrivedBytes reads before it checks for a consumer, the review's mutant red in 21ms, where the old version of the test blocked
BridgeStdinTests/nonBlockingStdinReadsUntilEndOfFile (new) nextBytes reduced to one availableBytes call, which is #3133's BridgeStdin red in 21ms: no lines, and one EAGAIN error logged
DescriptorReadTests/nextBytesWaitsOnANonBlockingPipe (new) same red in 2ms: EAGAIN thrown
DescriptorReadTests/bufferedBytesTakeWhatThePipeHolds (new), plus the CI-ordering, dump and stopWaitsForDeliveryInProgress tests bufferedBytes drops its poll check and reads until the limit or end of file each red at the 10s deadline, and none hung

The dump runner test injects a pipe and keeps a dup of its write end open, the way a surviving grandchild would. The tool waits on a gate file, so the test can capture the readabilityHandler before the tool exits. Once the result is in, the test writes late and calls the captured handler. It then checks three things: the descriptor is still blocking, the handler is cleared, and late is still in the pipe. Nothing in it depends on load.

BridgeStdinTests/unreadableStdinEndsTheSession feeds BridgeStdin a directory. The read fails with EISDIR, and the session still ends with one error logged.

Two test helpers:

  • HeldOpenWriter runs 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.
  • BackgroundPipeWriter writes from a thread with F_SETNOSIGPIPE set. The first mutant run showed why this is needed: a test that failed early freed its pipe, the writer thread's next write raised SIGPIPE, 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

  • New Fixed entry: "tablepro-mcp crashing when its standard input was non-blocking." v0.75.0 has this crash, as the standalone build in point 3 shows.
  • The fix(export): give a dump the TLS options its own client tool takes #3049 entry, "Backup failure reported as an exit code alone when the tool wrote its message and exited at once.", names the bug rather than the drain. It is still what the dump runner fixes, so it stays as written. Nothing unreleased gets an entry.

Verification

  • verify.sh test on DescriptorReadTests, PipeReaderTests, ProcessNativeDumpRunnerTests, BridgeStdinTests, CLIToolVersionProbeTests, PreConnectHookRunnerTests, SupervisedProcessRunnerTests, MCPBridgeIntegrationTests, MCPStdioMessageTransportTests, TunnelCommandManagerTests, CloudSQLProxyManagerTests and CloudflareTunnelManagerTests: the result bundle holds 87 cases, 85 passed.
    • The two failures were CLIToolVersionProbeTests cases, and they were environmental. The probe logged "did not answer --version within 3s" before it read anything, and syspolicyd'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.
    • CLIToolVersionProbeTests on its own at a lower load: 4/4 passed.
    • The real CLIToolVersionProbe.swift and DescriptorRead.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.swift compiles in the mcp-server target (tablepro-mcp) as well as the app.
  • swiftlint lint --strict on all 13 changed Swift files: 0 violations. check-test-suite-attributes.py, check-log-privacy.py and audit-refactor-health.sh --check all 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:

Suite Cases
DescriptorReadTests 8/8
PipeReaderTests 6/6
ProcessNativeDumpRunnerTests 4/4
BridgeStdinTests 2/2
CLIToolVersionProbeTests 4/4
PreConnectHookRunnerTests 1/1
SupervisedProcessRunnerTests 8/8
MCPBridgeIntegrationTests 18/18

Repo Hygiene and Docs passed. The iOS Tests Gate passed; Run iOS Tests itself was skipped by its change detection.

Not covered

  • AgentCLIProcess.run reads 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, login and logout, whose output is small, and this is a different shape from the one fixed here.
  • ProcessSupervisedRunner still 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.
  • No UI flow changed, so there is no TableProUITests automation. Local XCUITest is blocked on this machine, and CI runs the UI suites.

@datlechin
datlechin marked this pull request as ready for review September 26, 2026 00:10
@datlechin
datlechin merged commit dc96c00 into main Sep 26, 2026
13 checks passed
@datlechin
datlechin deleted the fix/pipe-reader-followups branch September 26, 2026 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant