Fix superfluous WriteHeader in bot connect digest handler - #2383
Fix superfluous WriteHeader in bot connect digest handler#2383lieut-data wants to merge 1 commit into
Conversation
5feffa1 to
4766f17
Compare
4766f17 to
8a4f6f4
Compare
📝 WalkthroughWalkthroughThe bot connect handler now returns after invoking the selected digest sender without explicitly writing a success status. End-to-end coverage verifies successful responses, first-request digest delivery, and suppression of an immediate duplicate digest. ChangesBot connect behaviour
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api_bot_test.go`:
- Around line 101-105: Update the second-connect verification around
GetPostsSince to query from digestPost.CreateAt minus one millisecond, then
exclude digestPost.Id when checking for duplicates. Ensure the assertion fails
if any other post appears, including one created in the same millisecond as
digestPost.
- Around line 76-105: The connect tests currently cover only successful
responses and do not exercise the changed error path. Add a sender-failure test
around the bot connect handler using a response-writer spy, force the digest
send to fail, and assert that HandleError is not followed by a second
WriteHeader; retain the existing successful-connect assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cbdaac0-53a3-4eee-907d-1368ddeb4c80
📒 Files selected for processing (2)
server/api/bot.goserver/api_bot_test.go
💤 Files with no reviewable changes (1)
- server/api/bot.go
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is a bug fix: it removes a superfluous w.WriteHeader(http.StatusOK) in BotHandler.connect() that can run after the digest sender has already written an error via HandleError, producing Hub http: superfluous response.WriteHeader call noise. Supported lines come from the Version Support table in README on master. Each supported Playbooks release still has the same double-write pattern in server/api/bot.go, and the fix applies cleanly there — so cherry-picks are recommended for all currently supported release branches (master itself is the PR target).
| MM version | Playbooks branch | Cherry Pick? | Reason |
|---|---|---|---|
| master | master | No | PR already targets master |
| v11.10 | release-2.11 | Yes | Bug present; fix applies cleanly |
| v11.9 | release-2.10 | Yes | Bug present; fix applies cleanly |
| v11.8 | release-2.9 | Yes | Bug present; fix applies cleanly |
| v11.7 ESR | release-2.9 | Yes | Bug present; fix applies cleanly |
| v10.11 ESR | release-2.4 | Yes | Bug present; fix applies cleanly |
Sent by Cursor Automation: Playbooks cherry pick recommender
|
|
||
| // GetPostsSince is exclusive (server queries UpdateAt > since), so digestPost | ||
| // itself won't reappear here. | ||
| sincePosts, _, err := e.ServerClient.GetPostsSince(context.Background(), dmChannel.Id, digestPost.CreateAt, false) |
There was a problem hiding this comment.
Because we're using a global variables here and the subtests are dependent, we should require.NotNil on digestPost.
Having the protection inside for first subtest does not protect the second one, and the second subtest will panic.
2 participants
Summary
Fix
http: superfluous response.WriteHeader callas seen on Hub ERROR logs --BotHandler.connect()(GET /bot/connect) always wrotew.WriteHeader(http.StatusOK)at the end, even when the digest-sending closure it just invoked had already written an error response viaHandleError.Now we rely (and test!) on
net/http's implicit 200 on success -- note that I opted not to add all the mock infrastructure just to verify that we don't emit the superfluous headers themselves. Instead, just verifying the end-user visible semantics.Checklist
Gated by experimental feature flagPlanned cherry-picks:release-X.Y