fix(notifications): stop the event queue busy-looping on a slow sink (!2963) · Merge requests · GitLab.org / container-registry

What does this MR do?

The bufferer goroutine of eventQueue in notifications/sinks.go never blocks while it holds events that the sender has not taken yet. The select of its main loop (sinks.go#L299-324) has an empty default: case, so when the sender is still busy writing the previous event and no new event arrives, the select returns at once and the loop starts over. While a notification endpoint is slow or unreachable, the backoff sink keeps the sender busy through every retry, and the registry keeps a full CPU core busy for each endpoint in that state. The loop that drains the queue on Close (sinks.go#L334-344) has the same empty default: and spins for up to queuepurgetimeout.

Neither loop has anything else to do while it waits, so this MR removes both default: cases and lets each select block until one of its channels is ready. Both empty cases came with 4d0e4639 (fix: make eventqueue sink interruptable, 2024-09-25).

With a registry built from master, one notification endpoint on a port nothing listens on, and one blob pushed and pulled three times, the registry process used 5020 ms of CPU time over the next 5 seconds; built from this branch, 0 ms. With a second endpoint like the first, three samples of 5 seconds read 9630 to 9990 ms from master and 0 ms from this branch. The steps are in the issue.

It is two commits:

test(notifications): wait for the sender in the exact queue limit test. The respects exact queue size limit subtest writes maxQueueSize+1 events to a blocked sink and expects none of them to be dropped, which only holds once the sender has taken the first event, and nothing makes the subtest wait for that. It passes on master, but with the fix alone it failed in 3 to 15 of 50 runs, depending on the machine. blockableSink now reports when a Write starts waiting, and the subtest waits for the first one before it fills the queue. This commit passes on master as it is.
fix(notifications): stop the event queue busy-looping on a slow sink, with TestEventQueueBuffererWaitsForBusySink. With the sink blocked and one event waiting, it checks that every bufferer goroutine is parked on a channel, once during normal operation and once while Close drains the queue, and that both events reach the sink once the sink is released. It reads the goroutine state from runtime.Stack rather than measuring CPU time, so that the result does not depend on the load of the CI runner.

Nothing else changes: when a new event and the sender are both ready, the select still picks one of them at random, as it did before, and the purge timeout still bounds the drain on Close.

Closes #2987 (closed)

The new test, and how it fails on master

On master at 2f9d3333 with the test changes of this MR but without the change to sinks.go, on Go 1.26.8, trimmed to the assertion lines (Go 1.25.10 and 1.27.2 print the same):

=== RUN   TestEventQueueBuffererWaitsForBusySink/during_normal_operation
        Error:      Should be true
        Messages:   bufferer goroutine is "runnable" instead of waiting on a channel: [runnable]
        Error:      Condition never satisfied
=== RUN   TestEventQueueBuffererWaitsForBusySink/while_draining_on_close
        Error:      Should be true
        Messages:   bufferer goroutine is "runnable" instead of waiting on a channel: [runnable]
        Error:      Condition never satisfied
--- FAIL: TestEventQueueBuffererWaitsForBusySink (10.00s)
    --- FAIL: TestEventQueueBuffererWaitsForBusySink/during_normal_operation (5.00s)
    --- FAIL: TestEventQueueBuffererWaitsForBusySink/while_draining_on_close (5.00s)

On this branch both subtests pass, and 100 runs in a row pass. Putting back either default: case alone fails the matching subtest and only that one. Taking the first commit's wait out again, with the fix in place, makes the exact limit subtest fail again in some of 50 runs.

Checks I ran locally

go test -count=1 ./notifications/... on Go 1.25.10, 1.26.8 and 1.27.2 (the CI matrix): pass. Each commit on its own, with go vet, on Go 1.26.8: pass.
go test -race -count=1 ./notifications/...: pass. TestEventQueue, TestEventQueueMaxSize and the new test under -race with -count=20: pass.
TestEventQueueMaxSize/respects_exact_queue_size_limit with -count=50 on master, on the first commit and on this branch: pass.
The api:conformance suite (integration,api_conformance_test, no database), which runs every case again with webhook notifications enabled: pass.
The same suite with the metadata database, as the database:api-conformance job runs it, against the project's postgresql-ci:17 image: pass.
golangci-lint run with v2.13.1 and the project's .golangci.yml, on the whole repository for amd64 and on ./notifications/... for 386, arm and arm64: 0 issues. gofmt -l, go vet and go build ./...: clean.
commitlint with @commitlint/config-conventional in node:lts-alpine on both commits: 0 problems.
The registry built from master and from this branch, run through the steps in the issue: 5020 ms and 0 ms of CPU time; with a second unreachable endpoint, 9630 to 9990 ms and 0 ms.

Author checklist

Documentation/resources

Code review guidelines

Go Style guidelines

Feature flags

When documentation is required

Documentation workflow

Reviewer checklist

Ensure the commit and MR tittle are still accurate.
If the change contains a breaking change, verify the breaking change label.
If the change is considered high risk, verify the label high-risk-change
Identify if the change can be rolled back safely. (note: all other reasons for not being able to rollback will be sufficiently captured by major version changes).

fix(notifications): stop the event queue busy-looping on a slow sink

What does this MR do?

Author checklist

Reviewer checklist

Merge request reports