fix(metrics): make Conn/PacketConn Close idempotent

[thomasjib]

• sing-box closes a routed conn up to 3x on non-graceful teardown; guard Leave/goodput so the connections gauge doesn't drift and duration isn't double-counted
• adds a regression test covering Conn and PacketConn

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Made connection closing idempotent so repeated Close() calls no longer double-count session duration, byte transfer, or goodput metrics.
  • Ensured both stream and packet connections keep accurate active-connection and performance statistics when teardown triggers multiple close attempts.

• Tests

  • Added tests covering idempotent Close() behavior for both stream and packet connections, including verification that metrics counters return to zero and guarded metrics are recorded only once.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Connection metric closure

Layer / File(s)	Summary
One-time close metric guards tracker/metrics/conn.go, tracker/metrics/packet_conn.go	Conn and PacketConn use atomic close guards so duration, connection, and goodput metrics are recorded only on the first Close() call.
Repeated-close metric tests tracker/metrics/close_idempotency_test.go	Adds isolated metric-collection coverage for repeated closure of routed stream and packet connections, including active-connection and histogram sample-count assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: making Conn and PacketConn Close idempotent for metrics.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch worktree-metrics-close-proof

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🟡 Not ready to approve

Close() still forwards to the underlying connection on every call, so close behavior is not fully idempotent and can still error/side-effect if the wrapped conn isn’t itself idempotent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR aims to make tracked connection wrappers resilient to sing-box calling Close() multiple times during non-graceful teardown, preventing connection-count drift and double-counted duration/goodput.

Changes:

• Add a closed guard to Conn and PacketConn so metrics emission at close is recorded only once.
• Add a regression test ensuring repeated closes don’t cause the active-connection metric to drift.
• Update close-method documentation to explain the multi-close behavior and intent.

File summaries

File	Description
tracker/metrics/conn.go	Adds a closed flag to make close-time metric recording one-shot for net.Conn wrappers.
tracker/metrics/packet_conn.go	Adds a closed flag to make close-time metric recording one-shot for PacketConn wrappers.
tracker/metrics/close_idempotency_test.go	Introduces a regression test to validate close idempotency for both wrappers.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tracker/metrics/close_idempotency_test.go`:
- Around line 42-49: Strengthen assertCloseIdempotent by asserting activeConns
is 1 before any Close call and 0 immediately after the first close, then verify
the duration and goodput instruments each contain exactly one recording. Keep
the remaining repeated Close calls and final zero-gauge assertion to preserve
idempotency coverage.
- Around line 57-62: Update newMeter and the test setup so subtests do not
mutate the process-wide OTel provider via sdkotel.SetMeterProvider without
isolation. Reset the global provider after each subtest, or replace the
package-level tracker/metrics instrument handles with instruments from an
isolated provider before each subtest, ensuring package-level state cannot leak
between tests.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 036cfef0-8ab0-41f2-a142-b3d074070732

📥 Commits

Reviewing files that changed from the base of the PR and between e5f0e33 and 541e6dd.

📒 Files selected for processing (3)

• tracker/metrics/close_idempotency_test.go
• tracker/metrics/conn.go
• tracker/metrics/packet_conn.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tracker/metrics/close_idempotency_test.go (1)

26-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that every Close reaches the wrapped connection.

The metric assertions pass if the guard returns nil on later calls without invoking the underlying Close. Line 30’s net.Pipe and Line 81’s no-op fake provide no way to distinguish that regression. Wrap both underlying connections with a call counter and assert three underlying closes.

Proposed regression coverage

- _, server := net.Pipe()
+ _, pipeServer := net.Pipe()
+ server := &countingConn{Conn: pipeServer}
  conn := tr.RoutedConnection(ctx, server, adapter.InboundContext{}, nil, nil).(*Conn)
  primeGoodput(&conn.rxBytes, &conn.startTime)
- assertCloseIdempotent(t, reader, conn)
+ assertCloseIdempotent(t, reader, conn, &server.closeCalls)

- conn := tr.RoutedPacketConnection(ctx, fakePacketConn{}, adapter.InboundContext{}, nil, nil).(*PacketConn)
+ underlying := &fakePacketConn{}
+ conn := tr.RoutedPacketConnection(ctx, underlying, adapter.InboundContext{}, nil, nil).(*PacketConn)
  primeGoodput(&conn.rxBytes, &conn.startTime)
- assertCloseIdempotent(t, reader, conn)
+ assertCloseIdempotent(t, reader, conn, &underlying.closeCalls)

-func assertCloseIdempotent(t *testing.T, reader *sdkmetric.ManualReader, conn io.Closer) {
+func assertCloseIdempotent(t *testing.T, reader *sdkmetric.ManualReader, conn io.Closer, closeCalls *atomic.Int64) {
  ...
  assertClosedOnce(t, reader)
+ require.Equal(t, int64(3), closeCalls.Load())
 }

-type fakePacketConn struct{ N.PacketConn }
+type countingConn struct {
+ net.Conn
+ closeCalls atomic.Int64
+}
+func (c *countingConn) Close() error {
+ c.closeCalls.Add(1)
+ return c.Conn.Close()
+}
+
+type fakePacketConn struct {
+ N.PacketConn
+ closeCalls atomic.Int64
+}
-func (fakePacketConn) Close() error { return nil }
+func (c *fakePacketConn) Close() error {
+ c.closeCalls.Add(1)
+ return nil
+}

Also applies to: 78-81

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tracker/metrics/close_idempotency_test.go` around lines 26 - 42, Update the
Conn and PacketConn idempotency tests around assertCloseIdempotent to wrap each
underlying connection with a close-call counter, then assert the counter reaches
three after the repeated Close calls. Replace the current net.Pipe and
fakePacketConn inputs with counter-aware wrappers while preserving the existing
metric assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tracker/metrics/close_idempotency_test.go`:
- Around line 26-42: Update the Conn and PacketConn idempotency tests around
assertCloseIdempotent to wrap each underlying connection with a close-call
counter, then assert the counter reaches three after the repeated Close calls.
Replace the current net.Pipe and fakePacketConn inputs with counter-aware
wrappers while preserving the existing metric assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8853a816-176d-4f4c-b94c-34eef53b446f

📥 Commits

Reviewing files that changed from the base of the PR and between 541e6dd and 9f10817.

📒 Files selected for processing (3)

• tracker/metrics/close_idempotency_test.go
• tracker/metrics/conn.go
• tracker/metrics/packet_conn.go

🚧 Files skipped from review as they are similar to previous changes (2)

• tracker/metrics/packet_conn.go
• tracker/metrics/conn.go