The cleanup that could never run: a silent Web Push leak in Go

永远无法执行的清理逻辑：Go 语言中静默的 Web Push 内存泄漏

Summer Bug Smash: Smash Stories 🐛🛹 This is a submission for DEV’s Summer Bug Smash: Smash Stories powered by Sentry. Nothing reported this bug. No crash, no stack trace, no failing test, no alert. I found it by reading through the notifications path in agentrq, a Go backend for a human-in-the-loop task manager for AI agents, and noticing that a block of error handling could not execute. Summer Bug Smash：Bug 修复故事 🐛🛹 这是为 Sentry 赞助的 DEV “Summer Bug Smash” 活动提交的文章。没有任何监控工具报告过这个 Bug。没有崩溃、没有堆栈跟踪、没有失败的测试用例，也没有报警。我是在阅读 agentrq 的通知路径代码时发现它的——这是一个用于 AI 代理的人机协作任务管理器的 Go 后端——我注意到有一块错误处理代码根本无法执行。

The code was not missing a case. It handled the case, in the wrong branch, and had a comment explaining what it was doing. The code push.sendToUser loops over a user’s browser push subscriptions and delivers a notification to each one. Browsers hand out push endpoints that expire: the user clears site data, uninstalls the PWA, or the subscription lapses on its own. When that happens the push service starts answering 410 Gone or 404 Not Found, and the sender is supposed to drop the subscription. 代码并没有遗漏逻辑，它处理了这种情况，只是放错了分支，而且还有一段注释解释了它的意图。push.sendToUser 代码会遍历用户的浏览器推送订阅并向每个订阅发送通知。浏览器提供的推送端点会过期：用户清除站点数据、卸载 PWA，或者订阅本身失效。当这种情况发生时，推送服务会返回 410 Gone 或 404 Not Found，发送方理应删除该订阅。

Here is what the code did, abbreviated: 以下是该代码的简化版：

resp, err := webpush.SendNotification(...)
if err != nil {
    zlog.Warn().Err(err)...
    // Remove invalid/expired subscriptions (410 Gone)
    if resp != nil && resp.StatusCode == 410 {
        _ = c.repo.DeletePushSubscription(ctx, userID, sub.Endpoint)
    }
    continue
}
resp.Body.Close()

Read on its own, that block is fine. It knows about 410 Gone. It nil-checks the response before touching it. It calls the right repository method with the right arguments. Someone understood the problem well enough to write a comment naming it. 单看这段代码，它似乎没问题。它知道 410 Gone 的存在，在访问响应前进行了 nil 检查，并用正确的参数调用了正确的存储库方法。编写这段代码的人显然很清楚问题所在，甚至还写了注释说明。

Why it can never run

为什么它永远无法运行

webpush.SendNotification comes from SherClockHolmes/webpush-go (v1.4.0), and it ends with return client.Do(req). So the function inherits the http.Client.Do contract, which splits results in a way that matters here: webpush.SendNotification 来自 SherClockHolmes/webpush-go (v1.4.0)，其结尾是 return client.Do(req)。因此，该函数继承了 http.Client.Do 的契约，其结果处理方式在这里至关重要：

A transport-level failure, like a DNS error or a refused connection, returns (nil, err). err is non-nil, so we enter the branch, but resp is nil, so resp != nil && resp.StatusCode == 410 is false. 传输层故障（如 DNS 错误或连接被拒绝）会返回 (nil, err)。此时 err 不为 nil，因此我们进入了 if 分支，但 resp 为 nil，所以 resp != nil && resp.StatusCode == 410 的判断结果为 false。

A request that completes returns (resp, nil), and that includes a 410 Gone and a 404 Not Found. As far as net/http is concerned, receiving a 410 is a success: the round trip worked, the server answered. err is nil, so the entire if err != nil block is skipped. 完成的请求会返回 (resp, nil)，这包括 410 Gone 和 404 Not Found。对于 net/http 而言，收到 410 属于成功：往返通信正常，服务器已响应。此时 err 为 nil，因此整个 if err != nil 代码块被跳过。

The two conditions the code needs are in different universes. Whenever resp is non-nil, err is nil. Whenever err is non-nil, resp is nil. DeletePushSubscription was unreachable from the moment it was written. 代码所需的两个条件处于不同的“宇宙”中。当 resp 不为 nil 时，err 必为 nil；当 err 不为 nil 时，resp 必为 nil。DeletePushSubscription 从被写下的那一刻起就永远无法触达。

What I find interesting is where the mistake actually sits. Everything about Web Push in that block is correct: the right status code, the right RFC semantics, the right cleanup call. The wrong assumption is one layer down, about what err means in Go’s HTTP client. The author was right about the domain and wrong about the platform, and the compiler has no opinion about that. 我觉得有趣的是错误的根源所在。该代码块中关于 Web Push 的一切都是正确的：正确的状态码、正确的 RFC 语义、正确的清理调用。错误的假设在于更底层——关于 Go 的 HTTP 客户端中 err 的含义。作者在业务领域上是正确的，但在平台特性上理解有误，而编译器对此毫无察觉。

What it cost

代价是什么

RFC 8030 §7.3 says a push service returns 404 or 410 once a subscription no longer exists. Since those never triggered a delete, and since DeletePushSubscription is otherwise only called from the explicit user-initiated unsubscribe handlers, nothing in the system ever removed a dead subscription. RFC 8030 §7.3 规定，当订阅不再存在时，推送服务会返回 404 或 410。由于这些响应从未触发删除操作，且 DeletePushSubscription 仅在用户主动取消订阅时才会被调用，系统内没有任何机制能移除失效的订阅。

Two things followed. Stale rows accumulated in push_subscriptions with no upper bound. And every future task or message event kept re-attempting delivery to endpoints that were already gone, which is one wasted outbound HTTP request per event, per dead subscription, forever. 这导致了两个后果：push_subscriptions 表中积累了无限多的过期记录；并且未来每一个任务或消息事件都会不断尝试向已失效的端点发送通知，这意味着每个事件、每个失效订阅都会产生一次浪费的对外 HTTP 请求，且永无止境。

This is the part I keep thinking about. The cost is not a spike, it is a slope. Each dead subscription adds a small permanent tax on every event the system will ever process, and the tax compounds as more subscriptions die. There is no moment where it breaks, so there is no moment where anyone looks. 这是我一直在思考的部分。代价不是一次性的峰值，而是一条持续上升的斜率。每个失效的订阅都会对系统处理的每个事件增加一点永久性的“税”，随着失效订阅的增多，这种税会不断累积。系统不会在某个瞬间崩溃，所以也就没人会去关注它。

It is also invisible to error monitoring, and not by accident. A dead endpoint answering 410 is a completed HTTP request. There is no exception to capture, no non-2xx client error thrown in the app, no log line above warning level. A dashboard would show a healthy delivery path with a slowly rising request count, which is exactly what a growing product looks like. 它对错误监控系统也是隐形的，这并非偶然。失效端点返回 410 是一个“已完成”的 HTTP 请求。没有异常可以捕获，应用中没有抛出非 2xx 的客户端错误，也没有高于警告级别的日志。仪表盘只会显示一条健康的发送路径，伴随着缓慢上升的请求数——这看起来完全像是一个正在增长的产品。

The fix

修复方案

The cleanup moves off the error branch and onto the successful one, where the status code actually lives: 清理逻辑从错误分支移到了成功分支，因为状态码实际上是在那里返回的：

if err != nil {
    zlog.Warn().Err(err).Str("endpoint", sub.Endpoint).Msg("[push] failed to send notification")
    continue
}
resp.Body.Close()

// webpush.SendNotification only returns a non-nil error for transport-level 
// failures; a delivered-but-rejected request comes back here with err == nil and 
// the failure encoded in the status code. Per RFC 8030 §7.3 the push service 
// returns 404 Not Found or 410 Gone once a subscription no longer exists, so 
// prune it — otherwise it lingers in the DB and every future event re-attempts 
// delivery to a dead endpoint.
if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone {
    _ = c.repo.DeletePushSubscription(ctx, userID, sub.Endpoint)
}

I widened 410 to 404 because RFC 8030 treats both as gone, and the original comment had only mentioned one of them. The transport-error path keeps doing what it did, logging and continuing, since a network failure says nothing about whether the subscription is still valid. Retrying later is correct there. 我将 410 扩展到了 404，因为 RFC 8030 将两者都视为“已失效”，而原始注释只提到了其中一个。传输错误路径保持原样，继续记录日志并跳过，因为网络故障无法说明订阅是否仍然有效，稍后重试是正确的做法。

The comment is longer than the code. That was deliberate. The thing that made this bug survive is that the broken version looked reasonable, so I wanted the next person reading it to find the reasoning about err and status codes right there, rather than having to rediscover the client.Do contract. 注释比代码还长，这是故意的。这个 Bug 之所以能存活，是因为错误的版本看起来很合理。我希望下一个阅读代码的人能直接看到关于 err 和状态码的逻辑推理，而不必重新去研究 client.Do 的契约。

The tests, and the part I am glad I did the slow way

测试，以及我庆幸自己选择了“慢”方法的部分

This delivery path had no test coverage at all, which is the other reason the bug lasted. The easy version of the test mocks out SendNotification and asserts the delete. It would have passed against the broken code just as happily, because it never exercises the branch structure that was wrong. 这条发送路径完全没有测试覆盖，这也是 Bug 长期存在的原因。简单的测试方法是 Mock 掉 SendNotification 并断言删除操作。这种测试在错误的代码上也能顺利通过，因为它根本没有触及那个逻辑错误的分支结构。

So I stood up a real httptest.Server returning 410 and 404 and let the actual library make the actual round trip. That has a cost: webpush.SendNotification encrypts the payload before sending, so it needs real keys or it fails long before. 所以我搭建了一个真实的 httptest.Server 来返回 410 和 404，让真实的库进行真实的往返通信。这确实有代价：webpush.SendNotification 在发送前会加密负载，因此它需要真实的密钥，否则在发送前就会失败。