Skip to content

mcp: extend the write deadline on every write to a keep-alive stream - #1293

Open
jmrplens wants to merge 2 commits into
modelcontextprotocol:mainfrom
jmrplens:jmrp-extend-the-write-deadline-on-every-write
Open

jmrplens wants to merge 2 commits into
modelcontextprotocol:mainfrom
jmrplens:jmrp-extend-the-write-deadline-on-every-write

Conversation

@jmrplens

Copy link
Copy Markdown
Contributor

Fixes #1262.

http.Server.WriteTimeout is the ordinary slow-write guard, and it applies to the whole response. A subscriptions/listen stream is a response the transport means to hold open indefinitely, so the deadline passes while the stream sits idle and healthy. Nothing notices, because nothing is being written, and the first real notification after it fails and takes the stream with it.

Only the transport knows a response became a stream, so only the transport can move the deadline. As agreed in #1262, every write to a stream that runs a keep-alive (#1232) now pushes the deadline out by twice the keep-alive interval, through http.ResponseController.SetWriteDeadline:

  • Extended rather than cleared, so WriteTimeout keeps meaning what it was set for, and a peer that stopped reading still fails within a bounded time.
  • Twice the interval rather than once, so a keep-alive that lands a little late does not race the deadline it exists to defer.
  • Scoped to streams with a keep-alive, which today means subscriptions/listen, because the interval is what the extension is measured in. A stream with no periodic write has nothing to measure it by.
  • Best effort: a writer that manages its own deadlines reports ErrNotSupported, and a server with no WriteTimeout has no deadline to move, so the result is ignored.

TestStreamWriteDeadline_SurvivesServerWriteTimeout holds a listen stream open behind an httptest server whose WriteTimeout is three keep-alive intervals, for five times that timeout. Against main it fails every time: no keep-alive arrives after the 75 ms deadline, because the first write past it fails and ends the stream.

http.Server.WriteTimeout is the ordinary slow-write guard and applies to
the whole response. A stream is a response the transport intends to hold
open indefinitely, so the deadline passes while the stream sits idle and
healthy: nothing notices, because nothing is being written, and the first
real notification fails instead and takes the stream with it.

Only the transport knows a response became a stream, so only the
transport can move the deadline. Every write to a stream that runs a
keep-alive now pushes it out by twice the interval. Extended rather than
cleared, so WriteTimeout keeps meaning what it was set for and a peer
that stopped reading still fails within a bounded time; twice rather than
once, so a keep-alive landing a little late does not race the deadline it
exists to defer.

Scoped to streams with a keep-alive, which today means subscriptions/listen,
because the interval is what the extension is measured in and a stream
with no periodic write has nothing to measure.

Fixes modelcontextprotocol#1262.
jmrplens added a commit to jmrplens/gitlab-mcp-server that referenced this pull request Sep 25, 2026
The top of the stack: what the whole-stack review still held open after
two fix rounds, one CI defect the stack itself ran into, the register
brought up to date, and every generated artifact the layers below moved,
regenerated once here.

**The review's last two findings.**

- The test that holds the individual tool Descriptions assembled at run
time (the switches in deploykeys and deploytokens, the map in pages)
checked only the tool names they spell. The rule `cmd/audit_action_ids`
applies to a constant Description also refuses a dotted ID that resolves
nowhere or only as an alias, so a misspelt ID in one of those switches
passed every gate; the review reproduced it by changing
`access.deploy_key_list_project` to `list_projekt`. The test now offers
a dotted token as an ID on the same condition as the rule, one of its
halves being one a canonical ID uses, leaves a `.git` tail alone, and
reports an alias with the canonical ID it stands for. The planted
misspelling fails it on both catalog classes. The limits paragraph in
the cmd utilities page said "those are held by a test" of two holes when
the test holds one, and says which now.
- The comment on the temporary-file refusal of a download said the file
it names is `output_path` unless the path was relative or its leaf a
link. `canonicalDownloadOutputPath` resolves every existing component
through symlinks, so a linked directory above the leaf renames it too,
the macOS temp root among them. The comment and a test row say so.

**Lint from the tree, not from a restored analysis.** The golangci-lint
job restored `~/.cache/golangci-lint` with a prefix key, and a pull
request reads its own ref's cache first. After a rebase that cache holds
the analysis of the branch's previous tree, and on 2026-09-24 it failed
a cascaded layer of this stack twice on an unused
`//nolint:contextcheck` in a file the stack never touched, until that
ref's caches were deleted. A reused result can hide a finding as easily
as it invents one. The analysis cache is no longer restored, so a lint
result depends on the tree alone; setup-go's build and module caches
stay, since their keys are content hashes. On a 4-CPU host a cold
analysis of main took 101 s against 29 s warm on the identical tree; in
CI the restored cache never matched the tree anyway, and over twenty
runs on 2026-09-24 the make step took 110 to 300 s while the
cross-platform matrix that sets the wall clock took 800 to 1150 s. The
static analysis page says the cache is not kept, and why.

**The register.** `modelcontextprotocol/go-sdk#1232` merged on
2026-09-21, so the write-deadline fix issue 1262 asked for was opened as
[modelcontextprotocol/go-sdk#1293](modelcontextprotocol/go-sdk#1293);
row 8, its section and the re-verification paragraph name it. Row 12's
section says what `modelcontextprotocol/go-sdk#1267` answers a POST
whose every call was cancelled (204, since the 20th) and that it was
rebased after `modelcontextprotocol/go-sdk#1232` conflicted in
`deliverLocked`. The client-go paragraph named two gitlab.com reviewers
with pronouns nobody here was told, and names them by handle and role
instead.

**Regenerated.** The tool snapshots (the served-prose rewrite of issue
910: tool names outside a See also clause replaced by canonical IDs),
the token footprint, the llms files, the testing reference and the
README stats. Over this tree every `check-*` target the
generated-artifacts job runs passes, as do the whole unit suite with
`-shuffle=on` and snapshot parity checked, and golangci-lint over every
build tag.

**Recorded.** The request inventory, which CI records during the
coverage job and found 47 rows short: the layers below added tests that
drive group access tokens, SAML group links, group security settings and
the other refusals the permission hints describe. Recorded again over
this tree it holds 1604 rows over 956 distinct paths in 177 packages,
every one of the 1082 catalog actions owned by a package that issued a
request, and R-PATH passes over it. And the e2e coverage, from both
Docker runs against this tree on truenas: the CE suite ran 1024 tests
with 9 skipped and none failed, the licensed EE suite 1282 with 8
skipped and none failed, and action coverage holds where it stood (CE L1
821 of 869, L2 721, L3 705; EE L1 1016 of 1089, L2 917, L3 901).

Closes #945.
jmrplens added a commit to jmrplens/gitlab-mcp-server that referenced this pull request Sep 29, 2026
I read every go-sdk issue and pull request
`docs/development/upstream-bugs.md` links, and this records what moved.
Nothing there asks anything of us, and go-sdk has cut no release since
v1.8.0, so every merge recorded is still unreleased; the maintainer's
estimate of the next one, about two months after v1.8.0, is now cited.

- **Row 11,
[modelcontextprotocol/go-sdk#1266](modelcontextprotocol/go-sdk#1266
A maintainer updated the branch from `main` on 2026-09-29 and all nine
checks passed; `main` gained two commits after it, and I updated it
again the same day with GitHub's Update branch, which kept the
maintainer's merge commit, and the checks passed again. It has had no
review since 2026-09-15.
- **Rows 8 and 12.** Both were six commits behind `main`; I updated them
from `main` on 2026-09-29 and their checks passed.
[modelcontextprotocol/go-sdk#1293](modelcontextprotocol/go-sdk#1293)
has had no review, and the one review thread on
[modelcontextprotocol/go-sdk#1267](modelcontextprotocol/go-sdk#1267)
still waits on the reviewer.
- **Row 68.**
[modelcontextprotocol/go-sdk#1299](modelcontextprotocol/go-sdk#1299)
was triaged P2 on 2026-09-28.
[modelcontextprotocol/go-sdk#1300](modelcontextprotocol/go-sdk#1300),
which another contributor opened, now conflicts with `main` and its
checks have never run. The dates of my comment and review, and of the
author's follow-up, are corrected to UTC.
- **Row 48.** The case is named under "Known limitations" of
[modelcontextprotocol/go-sdk#1275](modelcontextprotocol/go-sdk#1275),
which another contributor opened and a maintainer merged on 2026-09-18,
so the summary row and the entry say so, with the maintainer's position
on that pull request. Its discussion also showed the entry's transport
claim too wide: v1.8.0's stateful HTTP handler refuses every 2026-07-28
request but `server/discover`, so only stdio reaches the case.

## knowledge-graph

- **Rows 75 and 76,
[gitlab-org/orbit/knowledge-graph!2650](https://gitlab.com/gitlab-org/orbit/knowledge-graph/-/merge_requests/2650)
and
[gitlab-org/orbit/knowledge-graph!2651](https://gitlab.com/gitlab-org/orbit/knowledge-graph/-/merge_requests/2651).**
Both descriptions now carry `[skip pinned-version-check]`, which the
project's check asks for when a change does not affect the shape, so the
merged-results pipeline a maintainer runs will not refuse either for
want of a `query_dsl` bump. The marker sits at the top of each
description's collapsed details block, where the project's description
lint does not count it and the 2700 characters a pipeline reads still
hold it. Each merge request also offers the bump instead: row 75's a
patch bump, so clients that cache the schema by version fetch the new
text, and row 76's should the maintainers read the declared default that
branch changes as part of the contract. Both Files lists now name the
test pins.

## Checks

- `npx markdownlint-cli2 docs/development/upstream-bugs.md` passes.
- `go run ./cmd/format_md_tables/ --check` passes.
- No em dashes in the added lines.

This branch has not been deployed

No deployments
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.

mcp: an SSE response keeps http.Server.WriteTimeout armed, so a long-lived stream dies before its first notification

1 participant