[tools] gopls/internal/cmd: fix flaky MCP logging test
1 view
Skip to first unread message
Madeline Kalil (Gerrit)
Mar 17, 2026, 5:07:14 PM (2 days ago) Mar 17
to goph...@pubsubhelper.golang.org, Alan Donovan, Hongxiang Jiang, Go LUCI, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Mar 18, 2026, 2:25:22 PM (yesterday) Mar 18
to goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com
Madeline Kalil abandoned this change.
Abandoned
Madeline Kalil abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Mar 18, 2026, 2:57:08 PM (yesterday) Mar 18
to goph...@pubsubhelper.golang.org, Hongxiang Jiang, Go LUCI, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
3:25 PM (3 hours ago) 3:25 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #5 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
3:25 PM (3 hours ago) 3:25 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #6 to this change.
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
3:26 PM (3 hours ago) 3:26 PM
to goph...@pubsubhelper.golang.org, Alan Donovan, Hongxiang Jiang, Go LUCI, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
3:33 PM (3 hours ago) 3:33 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #7 to this change.
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
3:37 PM (3 hours ago) 3:37 PM
to Madeline Kalil, goph...@pubsubhelper.golang.org, Go LUCI, Hongxiang Jiang, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang and Madeline Kalil
Alan Donovan added 2 comments![]( "Open in Gerrit")
Commit Message
Line 9, Patchset 6:
This test flake occurs when shutting down the MCP server. If gopls takes too long to write the logs, it may get shut down prematurely (i.e. while it's still writing to the log file), causing the "gopls: server is closing: EOF" error.Alan Donovan . unresolved
How did you figure this out? Were you able to reproduce the problem locally?
File gopls/internal/cmd/mcp_test.go
Line 160, Patchset 6:
// The "initialized" notification is sent asynchronously; allow some timeAlan Donovan . unresolved
This suggests the problem is one of timing (the log is written asynchronously) rather than one of functionality (the log is never completely written). How did you determine that? (I was unable to reproduce the failures locally.)
The previous test asserted that, after Close returns, the log is written. The new test asserts only that the log is written 31s after Close. That's not nothing--it proves that it gets there eventually--but it does mean programs built atop ClientSession can't make the obvious assumptions about the log.
We should definitely report this as an upstream bug in MCP.
Related details
Attention is currently required from:
- Hongxiang Jiang
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
4:01 PM (3 hours ago) 4:01 PM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang and Madeline Kalil
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #8 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Hongxiang Jiang
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
4:03 PM (3 hours ago) 4:03 PM
to goph...@pubsubhelper.golang.org, Go LUCI, Alan Donovan, Hongxiang Jiang, golang-co...@googlegroups.com
Attention needed from Alan Donovan and Hongxiang Jiang
Madeline Kalil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
2 comments
Commit Message
Line 9, Patchset 6:
This test flake occurs when shutting down the MCP server. If gopls takes too long to write the logs, it may get shut down prematurely (i.e. while it's still writing to the log file), causing the "gopls: server is closing: EOF" error.Alan Donovan . unresolved
How did you figure this out? Were you able to reproduce the problem locally?
Madeline Kalil
My first change was to just wrap mcpSession.Close() in a defer, since the Logging test is the only one that didn't already do this.
Running locally with only this change, this logging test consistently failed due to a different issue: it did not read the expected "initialized" log. (failed with "logs do not contain expected initialized"). By the time the test checks the log, I observed that it had written a large portion of the normal MCP server startup logs, but the specific "initialized" (which comes last) was omitted. The logs that are written before "initialized" include the gopls MCP server instructions (gopls/internal/mcp/instructions.md). I guess that's why it takes a non-trivial amount of time, and we can't immediately check the logs after connecting to the server.
I'll paste the logs in a different comment
File gopls/internal/cmd/mcp_test.go
Line 160, Patchset 6:
// The "initialized" notification is sent asynchronously; allow some timeAlan Donovan . unresolved
This suggests the problem is one of timing (the log is written asynchronously) rather than one of functionality (the log is never completely written). How did you determine that? (I was unable to reproduce the failures locally.)
The previous test asserted that, after Close returns, the log is written. The new test asserts only that the log is written 31s after Close. That's not nothing--it proves that it gets there eventually--but it does mean programs built atop ClientSession can't make the obvious assumptions about the log.
We should definitely report this as an upstream bug in MCP.
Madeline Kalil
Sorry, asynchronously is perhaps the wrong term to use here. I mean that after we call client.Connect, the logs do not appear instantaneously (there is a non-trivial amount to write), so we cannot check the log file immediately.
What do you mean that the log is written after Close? We check the log before calling Close.
Here's the log output from a successful test run:
```
Logs:
2026/03/19 15:53:39 Listening for MCP messages on stdin...
read: {"jsonrpc":"2.0","id":1,"method":"initialize","params":{"clientInfo":{"name":"client","version":"v0.0.1"},"protocolVersion":"2025-06-18","capabilities":{"roots":{"listChanged":true}}}}
write: {"jsonrpc":"2.0","id":1,"result":{"capabilities":{"logging":{},"tools":{"listChanged":true}},"instructions":"# The gopls MCP server\n\nThese instructions describe how to efficiently work in the Go programming language using the gopls MCP server. You can load this file directly into a session where the gopls MCP server is connected.\n\n## Detecting a Go workspace\n\nAt the start of every session, you MUST use the `go_workspace` tool to learn about the Go workspace. ONLY if you are in a Go workspace, you MUST run `go_vulncheck` immediately afterwards to identify any existing security risks. The rest of these instructions apply whenever that tool indicates that the user is in a Go workspace.\n\n## Go programming workflows\n\nThese guidelines MUST be followed whenever working in a Go workspace. There are two workflows described below: the 'Read Workflow' must be followed when the user asks a question about a Go workspace. The 'Edit Workflow' must be followed when the user edits a Go workspace.\n\nYou may re-do parts of each workflow as necessary to recover from errors. However, you must not skip any steps.\n\n### Read workflow\n\nThe goal of the read workflow is to understand the codebase.\n\n1. **Understand the workspace layout**: Start by using `go_workspace` to understand the overall structure of the workspace, such as whether it's a module, a workspace, or a GOPATH project.\n\n2. **Find relevant symbols**: If you're looking for a specific type, function, or variable, use `go_search`. This is a fuzzy search that will help you locate symbols even if you don't know the exact name or location.\n EXAMPLE: search for the 'Server' type: `go_search({\"query\":\"server\"})`\n\n3. **Understand a file and its intra-package dependencies**: When you have a file path and want to understand its contents and how it connects to other files *in the same package*, use `go_file_context`. This tool will show you a summary of the declarations from other files in the same package that are used by the current file. `go_file_context` MUST be used immediately after reading any Go file for the first time, and MAY be re-used if dependencies have changed.\n EXAMPLE: to understand `server.go`'s dependencies on other files in its package: `go_file_context({\"file\":\"/path/to/server.go\"})`\n\n4. **Understand a package's public API**: When you need to understand what a package provides to external code (i.e., its public API), use `go_package_api`. This is especially useful for understanding third-party dependencies or other packages in the same monorepo.\n EXAMPLE: to see the API of the `storage` package: `go_package_api({\"packagePaths\":[\"example.com/internal/storage\"]})`\n\n### Editing workflow\n\nThe editing workflow is iterative. You should cycle through these steps until the task is complete.\n\n1. **Read first**: Before making any edits, follow the Read Workflow to understand the user's request and the relevant code.\n\n2. **Find references**: Before modifying the definition of any symbol, use the `go_symbol_references` tool to find all references to that identifier. This is critical for understanding the impact of your change. Read the files containing references to evaluate if any further edits are required.\n EXAMPLE: `go_symbol_references({\"file\":\"/path/to/server.go\",\"symbol\":\"Server.Run\"})`\n\n3. **Make edits**: Make the required edits, including edits to references you identified in the previous step. Don't proceed to the next step until all planned edits are complete.\n\n4. **Check for errors**: After every code modification, you MUST call the `go_diagnostics` tool. Pass the paths of the files you have edited. This tool will report any build or analysis errors.\n EXAMPLE: `go_diagnostics({\"files\":[\"/path/to/server.go\"]})`\n\n5. **Fix errors**: If `go_diagnostics` reports any errors, fix them. The tool may provide suggested quick fixes in the form of diffs. You should review these diffs and apply them if they are correct. Once you've applied a fix, re-run `go_diagnostics` to confirm that the issue is resolved. It is OK to ignore 'hint' or 'info' diagnostics if they are not relevant to the current task. Note that Go diagnostic messages may contain a summary of the source code, which may not match its exact text.\n\n6. **Check for vulnerabilities**: If your edits involved adding or updating dependencies in the go.mod file, you MUST run a vulnerability check on the entire workspace. This ensures that the new dependencies do not introduce any security risks. This step should be performed after all build errors are resolved. EXAMPLE: `go_vulncheck({\"pattern\":\"./...\"})`\n\n7. **Run tests**: Once `go_diagnostics` reports no errors (and ONLY once there are no errors), run the tests for the packages you have changed. You can do this with `go test [packagePath...]`. Don't run `go test ./...` unless the user explicitly requests it, as doing so may slow down the iteration loop.\n\n","protocolVersion":"2025-06-18","serverInfo":{"name":"gopls","version":"v1.0.0"}}}
read: {"jsonrpc":"2.0","method":"notifications/initialized","params":{}}
write: {"jsonrpc":"2.0","id":1,"method":"roots/list","params":{}}
read: {"jsonrpc":"2.0","id":1,"result":{"roots":[]}}
write: {"jsonrpc":"2.0","method":"notifications/tools/list_changed","params":{}}
```
In the failing test runs, it cuts off after "serverInfo" and doesn't reach the read of initialized.
I'll open a bug with MCP
Related details
Attention is currently required from:
- Alan Donovan
- Hongxiang Jiang
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages