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