Skip to content

probe: cancel collection and release stalled connections - #1450

Open
IlyaStarov wants to merge 2 commits into
prometheus-community:masterfrom
IlyaStarov:fix-probe-cancellation
Open

IlyaStarov wants to merge 2 commits into
prometheus-community:masterfrom
IlyaStarov:fix-probe-cancellation

Conversation

@IlyaStarov

Copy link
Copy Markdown

Fixes #1449

Pass the /probe request context through both collectors while retaining the existing constructor for long-lived runtimes. Stop the legacy database loop once collection is canceled.

Use lib/pq's custom dialer interface to bound connection startup and stalled reads. Allow a short grace period for CancelRequest, then abort the TCP connection if it remains blocked; a normal FIN can leave the server in ClientWrite. No dependency, metric, or configuration changes.

Validation:

  • Unit tests with the race detector and go vet ./....
  • New cancellation regression fails on unmodified master.
  • Integration tests against official PostgreSQL 18.1.
  • The issue's six-probe reproduction: connection exhaustion before the fix, successful SELECT 1 afterward.
  • Plain/TLS probes, repeated /metrics scrapes, active SQL cancellation, lost replies, ClientWrite cleanup, recovery, and database discovery cancellation.

Propagate request cancellation through both collectors and connection setup.
Allow SQL cancellation before aborting connections waiting on lost replies.
Preserve the constructor and connection reuse for long-lived runtimes.
Cover cancellation during startup, SSL negotiation, queries and timeouts.

Signed-off-by: Staroverov Ilja <i.staroverov@ftdata.ru>

@AynSiddiqui AynSiddiqui left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested 6105ad6b7: build, vet, package tests, PostgreSQL 16.15 integration tests and both cancellation tests (-race -count=5) pass. The copied probe test fails on master (a39c07304).

Six canceled probes against a silent TCP peer leave 12 connections/+18 fds/+36 goroutines on master, versus 0/0/0 here; withholding SELECT version() replies gives the same result. Five healthy /metrics scrapes pass on both.

I also paused PostgreSQL 18.6 (docker pause) in separate plain, TLS (sslmode=require) and --auto-discover-databases runs. After one stalled /metrics request and five aborted probes, master retains 13 connections/+18 fds/+36 goroutines; here it's 1/+1/+3. Pprof and ss show that's the existing legacy /metrics Ping, with no probe handlers left, and it clears after unpause. Both versions recover.

I also tested #1449's selective SELECT version() reply drop on PostgreSQL 18.1, using iptables inside the database container's network namespace. Aborted probes are cleaned up here: 1 connection/+1 fd/+3 goroutines left (including a concurrent /metrics), versus 10/+14/+29 on master.

With --collection-timeout=2s and a 1s scrape-timeout header, the selective-drop probe waits for my 5s client timeout on both versions. Pausing an active pg_sleep(5) query reaches my 6s client timeout; dropping inbound PostgreSQL packets during it returns HTTP 200 at 5s on both. In startup runs, that inbound drop makes the head probe return at 2s. The legacy socket lifetime uses e.ctx, so its shorter collection deadline doesn't force-close it. I see the request-cancellation fix working; could this pre-existing legacy collection-timeout gap be a follow-up?

The lint job fails on unchecked errors at abortConnection and SetLinger, plus QF1003 in the test. Explicit _ = assignments and a switch stage would fix these. A BUGFIX changelog entry would also help.

I haven't tested a pause partway through database discovery.

Make ignored connection-abort and SetLinger errors explicit.
Use a tagged switch for the connection-timeout test cases.
Document canceled-probe connection cleanup in the changelog.

Signed-off-by: Staroverov Ilja <i.staroverov@ftdata.ru>
@IlyaStarov

Copy link
Copy Markdown
Author

Thanks for the thorough testing!

Fixed the three lint findings and added a BUGFIX changelog entry. Build, vet, golangci-lint, package tests with -race, and PostgreSQL 18.1 integration tests with -race pass.

I also checked cancellation during database discovery. After unpausing PostgreSQL, no probe connections remained.

Confirmed the separate legacy collection-timeout overrun on both the tested master snapshot and the original PR commit. Opened #1455 to track it separately.

@AynSiddiqui

Copy link
Copy Markdown

I re-ran golangci-lint and go test ./... at 3962ddb; both pass, and the BUGFIX changelog entry is in place. #1455 captures the legacy collection-timeout gap I found. This looks good from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/probe accumulates connections when database replies are lost

3 participants