Fix webhooks and reactions 404 issues (#21) #25
Conversation
* Fix webhook fast 404 when getting deleted webhook message * encode emojis
📝 WalkthroughWalkthroughCompute and reuse a request route early in lib/discord.go (including special normalization for a reactions path), detect "Unknown Webhook" by inspecting 404 response bodies in lib/queue.go, re-wrap consumed response bodies in lib/http.go, and add tests validating request/response forwarding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant D as doDiscordReq
participant U as GetMetricsPath
participant A as Discord API
participant M as Metrics
C->>D: doDiscordReq(path, ...)
activate D
D->>U: GetMetricsPath(path)
U-->>D: route
rect rgba(200,230,255,0.18)
note over D: If reactions path\n=> split/unescape/escape emoji segment
end
D->>A: HTTP request
A-->>D: Response / Error
D->>M: histogram.Observe(route, status, latency)
D-->>C: Return response / error
deactivate D
sequenceDiagram
autonumber
participant Q as Queue loop
participant H as Discord HTTP
participant W as isUnknownWebhook
participant R as Caller
Q->>H: Send request for item
H-->>Q: Response (status, body)
alt status == 404 and not interaction URL
Q->>Q: Read resp.Body -> bodyBytes
Q->>Q: Replace resp.Body with new reader from bodyBytes
Q->>W: isUnknownWebhook(bodyBytes)?
alt true
Q->>Q: set ret404 = true (fail-fast webhook)
else false
Q->>Q: handle as regular 404
end
Q->>R: send resp via item.doneChan
else non-404
Q->>R: normal flow, send resp via item.doneChan
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex deep review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/queue.go (1)
279-282: Consider using more robust JSON parsingThe current implementation uses simple byte matching to detect the error code, which could potentially match unrelated content containing the same string pattern.
-func isUnknownWebhook(_body io.ReadCloser) bool { - body, _ := ioutil.ReadAll(_body) - _body.Close() - return bytes.Contains(body, []byte("\"code\": 10015")) -} +func isUnknownWebhook(_body io.ReadCloser) bool { + body, _ := ioutil.ReadAll(_body) + _body.Close() + + var errorResponse struct { + Code int `json:"code"` + } + + if err := json.Unmarshal(body, &errorResponse); err != nil { + return false + } + + return errorResponse.Code == 10015 +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/discord.go(2 hunks)lib/queue.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/discord.go (1)
lib/bucketpath.go (1)
GetMetricsPath(41-65)
🔇 Additional comments (5)
lib/discord.go (3)
13-13: LGTM - Import added for URL handlingThe
net/urlimport is necessary for the URL path processing added below.
218-226: Proper emoji encoding handling in reaction pathsThe special case handling for reaction endpoints correctly addresses emoji encoding issues. The logic properly checks if the 8th segment needs escaping and applies URL encoding when necessary.
The implementation correctly:
- Identifies the specific reaction path pattern
- Extracts the emoji segment (segs[7])
- Only re-escapes if the segment wasn't already escaped
- Reassembles the path properly
218-218: Approve change; manual verification required for reaction path handlinglib/discord.go:218 (route := GetMetricsPath(path)) is architecturally fine, but the provided verification script produced no output — manually confirm GetMetricsPath normalizes reaction routes (e.g. /reactions and /reactions/:id) or re-run: rg -n "GetMetricsPath" --type go && rg -n "reactions" --type go.
lib/queue.go (2)
4-8: LGTM - Required imports for response body processingThe added imports (
bytes,io,io/ioutil) are necessary for the newisUnknownWebhookfunction implementation.
360-367: Effective webhook 404 detection and fail-fast implementationThe logic correctly identifies Unknown Webhook errors by inspecting the response body content and sets up fail-fast behavior to avoid repeated requests to deleted webhooks. The condition properly excludes interaction URLs which may have different 404 semantics.
The implementation correctly:
- Uses the new
isUnknownWebhookhelper to detect the specific error- Excludes interaction URLs which shouldn't trigger this behavior
- Sets the
ret404flag to enable fail-fast responses- Provides appropriate logging for debugging
|
@codex fix comments, lets check for a leading slash and increment the index if its present, lets rewind the reader and move the doneChan send below this if |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/discord.go (1)
219-238: Emoji still not encoded for /@me; broaden route match and re-encode safelyThe condition only matches
/channels/!/messages/!/reactions/!/!, missing the common/@mevariant, so many reaction requests remain unescaped. Also, prefer decode-then-encode and skip the@mesegment explicitly to avoid partial/double encodes.Apply:
- route := GetMetricsPath(path) - if route == "/channels/!/messages/!/reactions/!/!" { + route := GetMetricsPath(path) + if strings.HasPrefix(route, "/channels/!/messages/!/reactions/!/") { segs := strings.Split(path, "/") emojiIdx := -1 for idx, seg := range segs { if seg == "reactions" { emojiIdx = idx + 1 break } } - if emojiIdx >= 0 && emojiIdx < len(segs) { - emojiSegment := segs[emojiIdx] - if emojiSegment != "" { - if unescaped, err := url.PathUnescape(emojiSegment); err == nil && emojiSegment == unescaped { - segs[emojiIdx] = url.PathEscape(emojiSegment) - path = strings.Join(segs, "/") - } - } - } + if emojiIdx >= 0 && emojiIdx < len(segs) { + emojiSegment := segs[emojiIdx] + // Do not touch the user sentinel + if emojiSegment != "" && emojiSegment != "@me" { + if decoded, err := url.PathUnescape(emojiSegment); err == nil { + segs[emojiIdx] = url.PathEscape(decoded) + } else { + segs[emojiIdx] = url.PathEscape(emojiSegment) + } + path = strings.Join(segs, "/") + } + } }
🧹 Nitpick comments (3)
lib/discord.go (1)
296-306: Body rewind LGTM; switch to io.ReadAll (ioutil is deprecated)Functionality is correct and unblocks downstream reads. Minor modernization: prefer
io.ReadAll.- respBody, err = ioutil.ReadAll(discordResp.Body) + respBody, err = io.ReadAll(discordResp.Body)Also applies to: 314-316
lib/queue.go (2)
279-285: Make “Unknown Webhook” detection robust (parse JSON, not substring)Substring match is brittle to whitespace/ordering. Parse the body’s
codeinstead.-import ( +import ( "bytes" "context" "errors" "io" "io/ioutil" "net/http" "strconv" "strings" "sync" "sync/atomic" "time" "github.com/Clever/leakybucket" "github.com/Clever/leakybucket/memory" "github.com/sirupsen/logrus" + "encoding/json" ) @@ -func isUnknownWebhook(body []byte) bool { - if len(body) == 0 { - return false - } - return bytes.Contains(body, []byte("\"code\": 10015")) -} +func isUnknownWebhook(body []byte) bool { + if len(body) == 0 { + return false + } + var v struct{ Code int `json:"code"` } + if err := json.Unmarshal(body, &v); err != nil { + return false + } + return v.Code == 10015 +}
361-372: Use URL.Path and fix SplitN to correctly identify interactions
isInteractioncurrently usesSplitN(..., 1)(no-op) and you passURL.String()(includes scheme/host). This can misclassify, skipping the 404 fast‑path.-func isInteraction(url string) bool { - parts := strings.Split(strings.SplitN(url, "?", 1)[0], "/") +func isInteraction(path string) bool { + parts := strings.Split(strings.SplitN(path, "?", 2)[0], "/") for _, p := range parts { if len(p) > 128 { return true } } return false } @@ - shouldCheckWebhook := resp.StatusCode == 404 && !isInteraction(item.Req.URL.String()) + shouldCheckWebhook := resp.StatusCode == 404 && !isInteraction(item.Req.URL.Path)Also applies to: 286-294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/discord.go(3 hunks)lib/queue.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/discord.go (2)
lib/bucketpath.go (1)
GetMetricsPath(41-65)lib/http.go (1)
CopyResponseToResponseWriter(21-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
lib/queue.go (1)
361-372: LGTM: body buffering and signaling orderReading, rewinding, and moving
doneChanafter detection fixes the prior race and enables fast 404s. Nice.Please confirm this path triggers for both:
- PUT/DELETE /channels/{cid}/messages/{mid}/reactions/{emoji}/@me
- GET /webhooks/{wid}/{wtoken}/messages/{mid}
Also applies to: 382-382
|
@codex I'm worried that the change to body reading breaks something, since we are assigning request.Body directly. Write a few unit tests to verify |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/http.go (1)
24-39: Close original body before reassigning; switch to io package to avoid leaks/deprecationNot closing the original resp.Body before replacing it leaks the upstream connection. Also, ioutil is deprecated. Close the original body after reading and use io equivalents.
import ( - "bytes" - "io/ioutil" + "bytes" + "io" "net/http" "strings" ) @@ func CopyResponseToResponseWriter(resp *http.Response, respWriter *http.ResponseWriter) error { writer := *respWriter - body, err := ioutil.ReadAll(resp.Body) + origBody := resp.Body + body, err := io.ReadAll(origBody) if err != nil { writer.WriteHeader(500) _, _ = writer.Write([]byte(err.Error())) + _ = origBody.Close() return err } + _ = origBody.Close() @@ - _, err = writer.Write(body) + _, err = writer.Write(body) if err != nil { return err } - resp.Body = ioutil.NopCloser(bytes.NewReader(body)) + resp.Body = io.NopCloser(bytes.NewReader(body)) return nil }
🧹 Nitpick comments (3)
lib/http.go (1)
10-20: Preserve multi-value headers and skip hop-by-hop fieldscopyHeader lower-cases keys and collapses multi-value headers to the last value. Prefer preserving all values and excluding hop-by-hop headers (Connection, TE, Trailer). Also, if you intend to drop Date from upstream, don’t re-add it.
func copyHeader(dst, src http.Header) { - dst["Date"] = nil - dst["Content-Type"] = nil - for k, vv := range src { - for _, v := range vv { - if k != "Content-Length" { - dst[strings.ToLower(k)] = []string{v} - } - } - } + dst.Del("Date") + dst.Del("Content-Type") + dst.Del("Content-Length") + for k, vv := range src { + if strings.EqualFold(k, "Content-Length") || + strings.EqualFold(k, "Connection") || + strings.EqualFold(k, "Transfer-Encoding") || + strings.EqualFold(k, "Trailer") { + continue + } + // Preserve all values; let net/http canonicalize on write. + dst[k] = append([]string(nil), vv...) + } }lib/process_request_test.go (2)
71-139: Response body preservation test looks good; add header assertionsThe test verifies body/status well. Add checks that response headers are preserved to the client and on the returned resp.
if recorder.Code != http.StatusNotFound { t.Fatalf("unexpected status written to client: got %d want %d", recorder.Code, http.StatusNotFound) } + if got := recorder.Header().Get("Content-Type"); got != "application/json" { + t.Fatalf("unexpected Content-Type header to client: got %q want %q", got, "application/json") + } if recorder.Body.String() != upstreamBody { t.Fatalf("unexpected body written to client: got %q want %q", recorder.Body.String(), upstreamBody) } @@ if string(bodyBytes) != upstreamBody { t.Fatalf("expected preserved response body %q, got %q", upstreamBody, string(bodyBytes)) } + if got := resp.Header.Get("Content-Type"); got != "application/json" { + t.Fatalf("unexpected Content-Type header on returned resp: got %q want %q", got, "application/json") + }
74-78: Add emoji/reactions and 404-body-path tests to lock fixesTo fully cover PR objectives, add:
- A test that PUT /channels/{id}/messages/{mid}/reactions/{emoji}/@me encodes tricky emojis (#️⃣, *️⃣) correctly.
- A queue/worker test that distinguishes 404 Unknown Webhook vs other 404s using the body, ensuring the re-read path works.
I can draft these tests targeting the updated route normalization and 404-body check. Want me to open a follow-up PR with them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/discord.go(2 hunks)lib/http.go(2 hunks)lib/process_request_test.go(1 hunks)lib/queue.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/queue.go
- lib/discord.go
🧰 Additional context used
🧬 Code graph analysis (1)
lib/process_request_test.go (3)
lib/logger.go (1)
SetLogger(30-33)lib/queue.go (1)
QueueItem(21-26)lib/discord.go (1)
ProcessRequest(264-297)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
lib/process_request_test.go (1)
27-69: Test for request forwarding is solidGood coverage ensuring body and Content-Type are forwarded; cleanup of the global client is correct.
| route := GetMetricsPath(path) | ||
| if route == "/channels/!/messages/!/reactions/!/!" { | ||
| segs := strings.Split(path, "/") | ||
| emojiIdx := 7 |
There was a problem hiding this comment.
If the path doesn't have an API Version set (e.g. /api/channels/...), this doesn't work because it's missing one slash.
I'd suggest getting the 2nd last index
Fix webhook fast 404 when getting deleted webhook message
encode emojis
Fixes #18
Fixes #19