Skip to content

Fix the flaky thread count assertion in the listen keepalive test - #542

Merged
koic merged 1 commit into
modelcontextprotocol:mainfrom
koic:fix_flaky_listen_keepalive_thread_assertion
Sep 4, 2026
Merged

Fix the flaky thread count assertion in the listen keepalive test#542
koic merged 1 commit into
modelcontextprotocol:mainfrom
koic:fix_flaky_listen_keepalive_thread_assertion

Conversation

@koic

@koic koic commented Sep 3, 2026

Copy link
Copy Markdown
Member

Motivation and Context

"listen keepalive is not started when the interval is nil" fails intermittently on CI. The most recent run failed on Ruby 3.2 alone while 3.1 and head passed, and a re-run of the same commit went green.

The assertion samples a process-wide count:

before = Thread.list.size
open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true })
assert_equal before, Thread.list.size, "a nil interval must not spawn a keepalive thread"

Thread.list covers the whole process, so the count moves for reasons this test has no interest in. The observed failure reported 54 against 51: three threads had gone away between the samples, not appeared. This file starts threads in sixteen places, and the SSE stream and reaper threads they leave behind finish whenever they finish, so whether the count holds still depends on the test order and on the scheduler rather than on the behavior under test.

The test itself is asking the right question. setup builds its transport with listen_keepalive_interval: nil precisely so opening a listen stream spawns no timer, and the point is to hold that guarantee. Only the measurement is wrong, so this compares the set of threads instead: a thread that disappears no longer registers, and a thread that appears still does. Minitest runs these tests serially and open_listen_stream drives the SSE body inline rather than on a thread, so a keepalive timer is the only thread that can appear across the two samples.

No library code changes.

How Has This Been Tested?

The rewritten assertion was checked for sensitivity before being kept: a temporary probe pointed open_listen_stream at a transport built with listen_keepalive_interval: 15 and asserted the set difference was not empty. It passed, confirming the assertion still catches a spawned keepalive thread rather than merely never failing. The probe was removed afterwards.

The failure did not reproduce locally beforehand, across six seeds of the file on its own and six full-suite runs, which matches a timing-sensitive flake that needs a slower machine.

Breaking Changes

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

## Motivation and Context

"listen keepalive is not started when the interval is nil" fails intermittently on CI. The most recent run failed
on Ruby 3.2 alone while 3.1 and head passed, and a re-run of the same commit went green.

The assertion samples a process-wide count:

```ruby
before = Thread.list.size
open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true })
assert_equal before, Thread.list.size, "a nil interval must not spawn a keepalive thread"
```

`Thread.list` covers the whole process, so the count moves for reasons this test has no interest in.
The observed failure reported 54 against 51: three threads had *gone away* between the samples, not appeared.
This file starts threads in sixteen places, and the SSE stream and reaper threads they leave behind finish whenever
they finish, so whether the count holds still depends on the test order and on the scheduler rather than on
the behavior under test.

The test itself is asking the right question. `setup` builds its transport with `listen_keepalive_interval: nil`
precisely so opening a listen stream spawns no timer, and the point is to hold that guarantee. Only the measurement is wrong,
so this compares the set of threads instead: a thread that disappears no longer registers, and a thread that appears still does.
Minitest runs these tests serially and `open_listen_stream` drives the SSE body inline rather than on a thread,
so a keepalive timer is the only thread that can appear across the two samples.

No library code changes.

## How Has This Been Tested?

The rewritten assertion was checked for sensitivity before being kept: a temporary probe pointed `open_listen_stream` at
a transport built with `listen_keepalive_interval: 15` and asserted the set difference was *not* empty. It passed,
confirming the assertion still catches a spawned keepalive thread rather than merely never failing. The probe was
removed afterwards.

The failure did not reproduce locally beforehand, across six seeds of the file on its own and six full-suite runs,
which matches a timing-sensitive flake that needs a slower machine.

## Breaking Changes

None.
@koic
koic merged commit f706cc8 into modelcontextprotocol:main Sep 4, 2026
11 checks passed
@koic
koic deleted the fix_flaky_listen_keepalive_thread_assertion branch September 4, 2026 17:23
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.

2 participants