feat: webhook 限频:每rid 2min一次#90
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些高层次的反馈:
- 建议在 Redis 的 SetNX 调用中使用现有的请求/业务 context,而不是 context.Background(),这样调用链上的取消/超时也能作用到限流检查上。
- 目前如果 Redis 限流检查出错,你会记录一条 warning,但仍然继续发送 webhook;也许可以把“失败时放行 (fail-open)”和“失败时阻断 (fail-closed)”的行为显式化(比如通过提前返回或添加注释),让后续读代码的人明确知道这是刻意的行为。
- 现在 2 分钟的限流时间是内联写死的;可以考虑把它提取成一个具名常量(放在 WebhookNotifyThrottlePrefix 附近),这样以后调整和理解限流策略会更容易。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using the existing request/logic context instead of context.Background() for the Redis SetNX call so that cancellation/timeouts on the calling path also apply to the throttle check.
- Right now if the Redis throttle check errors you log a warning but still proceed to send the webhook; it may be worth making the fail-open vs fail-closed behavior explicit (e.g., by returning early or adding a comment) so future readers know this is intentional.
- The 2-minute throttle duration is currently inlined; extracting it to a named constant (near WebhookNotifyThrottlePrefix) would make the throttling policy easier to adjust and understand.
## Individual Comments
### Comment 1
<location path="internal/logic/version.go" line_range="521-525" />
<code_context>
}
+ throttleKey := misc.WebhookNotifyThrottlePrefix + ":" + resourceId
+ ok, err := l.rdb.SetNX(context.Background(), throttleKey, 1, 2*time.Minute).Result()
+ if err != nil {
+ l.logger.Warn("Failed to check webhook throttle", zap.Error(err))
+ }
+ if !ok {
+ return
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Redis errors currently suppress webhook notifications instead of just disabling throttling
Because `SetNX` returns `ok=false` on error, the current `if !ok { return }` will drop notifications whenever Redis is down or transiently failing. If throttling is meant to be best-effort, consider only returning when Redis succeeded and the key already existed (e.g., `if err == nil && !ok { return }`), so Redis failures disable throttling rather than suppressing webhooks entirely.
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据反馈改进后续的评审质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Consider using the existing request/logic context instead of context.Background() for the Redis SetNX call so that cancellation/timeouts on the calling path also apply to the throttle check.
- Right now if the Redis throttle check errors you log a warning but still proceed to send the webhook; it may be worth making the fail-open vs fail-closed behavior explicit (e.g., by returning early or adding a comment) so future readers know this is intentional.
- The 2-minute throttle duration is currently inlined; extracting it to a named constant (near WebhookNotifyThrottlePrefix) would make the throttling policy easier to adjust and understand.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using the existing request/logic context instead of context.Background() for the Redis SetNX call so that cancellation/timeouts on the calling path also apply to the throttle check.
- Right now if the Redis throttle check errors you log a warning but still proceed to send the webhook; it may be worth making the fail-open vs fail-closed behavior explicit (e.g., by returning early or adding a comment) so future readers know this is intentional.
- The 2-minute throttle duration is currently inlined; extracting it to a named constant (near WebhookNotifyThrottlePrefix) would make the throttling policy easier to adjust and understand.
## Individual Comments
### Comment 1
<location path="internal/logic/version.go" line_range="521-525" />
<code_context>
}
+ throttleKey := misc.WebhookNotifyThrottlePrefix + ":" + resourceId
+ ok, err := l.rdb.SetNX(context.Background(), throttleKey, 1, 2*time.Minute).Result()
+ if err != nil {
+ l.logger.Warn("Failed to check webhook throttle", zap.Error(err))
+ }
+ if !ok {
+ return
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Redis errors currently suppress webhook notifications instead of just disabling throttling
Because `SetNX` returns `ok=false` on error, the current `if !ok { return }` will drop notifications whenever Redis is down or transiently failing. If throttling is meant to be best-effort, consider only returning when Redis succeeded and the key already existed (e.g., `if err == nil && !ok { return }`), so Redis failures disable throttling rather than suppressing webhooks entirely.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ok, err := l.rdb.SetNX(context.Background(), throttleKey, 1, 2*time.Minute).Result() | ||
| if err != nil { | ||
| l.logger.Warn("Failed to check webhook throttle", zap.Error(err)) | ||
| } | ||
| if !ok { |
There was a problem hiding this comment.
issue (bug_risk): 目前 Redis 错误会直接抑制 webhook 通知,而不是仅仅禁用限流
由于 SetNX 在出错时会返回 ok=false,当前的 if !ok { return } 会导致在 Redis 宕机或短暂故障时,通知被直接丢弃。如果限流只是尽力而为(best-effort),可以考虑只在 Redis 调用成功且键已存在时才 return(例如:if err == nil && !ok { return }),这样当 Redis 失败时,会禁用限流,而不是完全阻止发送 webhook 通知。
Original comment in English
issue (bug_risk): Redis errors currently suppress webhook notifications instead of just disabling throttling
Because SetNX returns ok=false on error, the current if !ok { return } will drop notifications whenever Redis is down or transiently failing. If throttling is meant to be best-effort, consider only returning when Redis succeeded and the key already existed (e.g., if err == nil && !ok { return }), so Redis failures disable throttling rather than suppressing webhooks entirely.
Summary by Sourcery
为 webhook 通知添加节流机制,以限制每个资源 ID 的发送频率。
Enhancements:
Original summary in English
Summary by Sourcery
Add throttling to webhook notifications to limit how frequently they are sent per resource ID.
Enhancements: