Conversation
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.
This branch has not been deployed
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
1 participant
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.
Fixes #1262.
http.Server.WriteTimeoutis the ordinary slow-write guard, and it applies to the whole response. Asubscriptions/listenstream 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:WriteTimeoutkeeps meaning what it was set for, and a peer that stopped reading still fails within a bounded time.subscriptions/listen, because the interval is what the extension is measured in. A stream with no periodic write has nothing to measure it by.ErrNotSupported, and a server with noWriteTimeouthas no deadline to move, so the result is ignored.TestStreamWriteDeadline_SurvivesServerWriteTimeoutholds a listen stream open behind anhttptestserver whoseWriteTimeoutis three keep-alive intervals, for five times that timeout. Againstmainit fails every time: no keep-alive arrives after the 75 ms deadline, because the first write past it fails and ends the stream.