copy of https://github.com/OutlineFoundation/tunnel-server/pull/291

[ohnorobo]

Copying closed #291 so I can take another crack at it

[ohnorobo]

@greptileai

[greptile-apps]

Greptile Summary

This PR re-opens and completes the fix from PR #291, addressing a stale-NAT-deletion race in PacketServe where a finishing handler goroutine could evict a newer association that happened to reuse the same client-address key.

Key changes:

• natmap.DelIfMatches — replaces the unconditional nm.Del with a pointer-equality check, so a goroutine finishing for association A1 cannot accidentally evict A2 that was created for the same key after A1 closed.
• association.enqueue + sync.Mutex — guards the gap between checking doneCh and writing to readCh, ensuring that Close() draining the queue and a concurrent enqueue cannot both act on the same packet (guaranteeing pkt.done() is called exactly once).
• association.Close drains readCh — prevents buffer leaks when an association is torn down while packets are still queued.
• Removed dead error check — the orphaned if err != nil block (which shadowed the outer ReadFrom error) is removed.
• Test coverage — three new tests validate the drain-on-close path, post-close enqueue behaviour, and the end-to-end NAT cleanup lifecycle; natTestMetrics is made thread-safe.

Confidence Score: 4/5

PR is safe to merge; the race condition from PR #291 is correctly resolved and well-tested.

All three pillars of the fix are mechanically correct: DelIfMatches enforces pointer equality so a finishing goroutine can never evict a newer association; the enqueue mutex makes the doneCh-check → readCh-write step atomic with Close()'s drain loop, guaranteeing pkt.done() is called exactly once per packet; and new tests cover the drain-on-close, post-close enqueue, and end-to-end NAT cleanup paths. Score withheld from 5 only because the concurrency pattern is subtle enough to merit a second set of eyes before merge.

service/udp.go — specifically the Close() / enqueue / outer-select interaction; no issues found but the logic is non-trivial.

Important Files Changed

Filename	Overview
service/udp.go	Fixes NAT eviction race via DelIfMatches, adds enqueue with mutex to guard the doneCh/readCh window, and drains readCh in Close() to prevent buffer leaks. Logic is sound.
service/udp_test.go	Adds three targeted tests: Close drains queued packets, enqueue after Close returns closed=true, and PacketServe removes closed associations from NAT. natTestMetrics is made thread-safe with a mutex.

Sequence Diagram

sequenceDiagram
    participant ML as Main Loop (goroutine per pkt)
    participant NM as natmap
    participant A as association
    participant HG as Handler Goroutine

    Note over ML,HG: Happy path – packet arrives for known live assoc
    ML->>NM: Get(clientAddrKey)
    NM-->>ML: assoc (existing)
    ML->>A: enqueue(pkt) [under mu.Lock]
    A-->>ML: queued=true
    HG->>A: Read() → receives pkt, calls pkt.done()

    Note over ML,HG: Race-safe teardown (DelIfMatches fix)
    HG->>A: assocHandle returns → assoc.Close()
    A->>A: close(doneCh)
    A->>A: mu.Lock → drain readCh → mu.Unlock
    HG->>NM: DelIfMatches(key, assoc) [pointer check]
    HG->>HG: metrics.RemoveNATEntry()

    Note over ML,HG: Next packet from same client after assoc expired
    ML->>NM: Get(clientAddrKey)
    NM-->>ML: nil (already removed)
    ML->>ML: create new assoc A2
    ML->>NM: Add(clientAddrKey, A2)
    ML->>HG: go assocHandle(ctx, A2)

    Note over ML,HG: Concurrent pkt sees closed assoc – enqueue detects it
    ML->>A: enqueue(pkt) [under mu.Lock]
    A->>A: select {case <-doneCh} → closed!
    A-->>ML: queued=false, closed=true
    ML->>NM: DelIfMatches(key, assoc) [safe, won't evict A2]
    ML->>ML: pkt.done()

Loading

_{Reviews (2): Last reviewed commit: "fix stale NAT deletion race via DelIfMat..." | Re-trigger Greptile}

[ohnorobo]

@greptileai