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

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.

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.

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.

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:

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.

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.

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 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.

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.

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. 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.

The PR

agentrq/agentrq#252, merged on July 9. It closes #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.

What I took from it

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.