holepunch: initialize timeout before registering notifiee

[Tristan-Wilson]

Description

waitForPublicAddr constructed a holePuncher, whose constructor registered the network notifiee, and only then assigned directDialTimeout to the returned object.

An inbound relayed connection could therefore enter DirectConnect and read the timeout concurrently with that assignment. Besides being a data race, a callback in this startup window could observe the zero value and create an immediately expired dial context.

This change passes the configured timeout into the constructor and initializes it before Network.Notify publishes the hole puncher. It also adds a deterministic regression test that inspects the configured value at the registration boundary.

No exported API or lifecycle behavior changes.

Reproduction

This was found while running a private downstream project's end-to-end DCUtR test under the race detector. It was initially observed with go-libp2p v0.48.0 and reproduced on current master before this change:

WARNING: DATA RACE
Read at ...:
  holepunch.(*holePuncher).directConnect()
      p2p/protocol/holepunch/holepuncher.go:115

Previous write at ...:
  holepunch.(*Service).waitForPublicAddr()
      p2p/protocol/holepunch/svc.go:152

The new unit test provides a deterministic form of the same ordering check. Without the fix it observes:

directDialTimeout at registration = 0s, want 137ms

Testing

go test -race ./p2p/protocol/holepunch \
  -run '^(TestHolePuncherFullyInitializedBeforeNetworkNotify|TestWaitForPublicAddr_NoDeadlockOnCancel)$' \
  -count=100
go vet ./p2p/protocol/holepunch

The private downstream DCUtR reproduction also passed 20 consecutive race-detector runs after the fix, followed by its package-level and full repository race suites.

AI assistance disclosure

This PR was developed with substantial OpenAI Codex assistance. Codex analyzed the race, authored the minimal code change and regression test, ran the validation described above, and drafted this description. I reviewed the result and am submitting it.

[sukunrt]

Thanks for the fix. The fix is fine, but the test seems too tailored to the internals. Can you see if there's a less intrusive way to test this? Maybe we can run lots of hosts in parallel and try to trigger the race?

Otherwise It's fine to fix this without a test.

[Tristan-Wilson]

Thanks for the fix. The fix is fine, but the test seems too tailored to the internals. Can you see if there's a less intrusive way to test this? Maybe we can run lots of hosts in parallel and try to trigger the race?

Otherwise It's fine to fix this without a test.

Thanks for the feedback, I'm traveling and should be able to get to it the week after next.

[sukunrt]

That's fine, I can do that.