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

A developer discovered a silent bug in the Go backend of agentrq, a human-in-the-loop task manager for AI agents, where HTTP 410 Gone responses from expired Web Push subscriptions were never handled due to a misunderstanding of Go's http.Client.Do contract. The bug caused stale subscriptions to accumulate and waste resources indefinitely, with no errors or alerts to signal the problem.

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 https://github.com/agentrq/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. The code was not missing a case. It handled the case, in the wrong branch, and had a comment explaining what it was doing. 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. 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. 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: 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. 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. 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. 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. RFC 8030 §7.3 https://datatracker.ietf.org/doc/html/rfc8030 section-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. 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. 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. 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. 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. 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. 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 any HTTP happens. The tests generate them. // newTestSubscriptionKeys returns a valid P-256 ECDH public key p256dh and a // 16-byte auth secret, base64url-encoded exactly as a browser PushSubscription // provides them. Real keys are required because webpush.SendNotification encrypts // the payload before sending, so bogus keys would fail before any HTTP round-trip. func newTestSubscriptionKeys t testing.T p256dh, auth string { t.Helper priv, err := ecdh.P256 .GenerateKey rand.Reader // ... return base64.RawURLEncoding.EncodeToString priv.PublicKey .Bytes , base64.RawURLEncoding.EncodeToString authBytes } TestSendToUser PrunesExpiredSubscription asserts DeletePushSubscription userID, endpoint is called for both statuses. TestSendToUser KeepsSubscriptionOnSuccess asserts a 201 does not delete, which is the assertion that would catch someone later "simplifying" the condition into something that prunes healthy subscriptions. go test -race ./internal/... is green, gofmt and go vet clean. agentrq/agentrq 252 https://github.com/agentrq/agentrq/pull/252 , merged on July 9. It closes 251 https://github.com/agentrq/agentrq/issues/251 , the issue I opened with the diagnosis before writing the fix. Two files, 99 lines added, 4 removed, and most of the additions are tests. A comment is a claim, and a claim can be false. // Remove invalid/expired subscriptions 410 Gone was not a description of what the code did. It was a description of what someone meant it to do, and it made the block read as solved to everyone who came after. The other thing is about where to look. This bug was not in unfamiliar or clever code. It was at a seam, in the handful of lines where application logic meets a library's error convention. The domain reasoning above the seam was right and the assumption below it was wrong, and that combination produces code that reviews well, tests green, and quietly does nothing.