Skip to content

fix(appnet): tryDirectPingDial shortcut bypassed MinHops constraint#2751

Merged
0pcom merged 1 commit into
skycoin:developfrom
0pcom:fix/appnet-minhops-bypass
May 20, 2026
Merged

fix(appnet): tryDirectPingDial shortcut bypassed MinHops constraint#2751
0pcom merged 1 commit into
skycoin:developfrom
0pcom:fix/appnet-minhops-bypass

Conversation

@0pcom
Copy link
Copy Markdown
Collaborator

@0pcom 0pcom commented May 20, 2026

Summary

Follow-up to #2749. Independently diagnosed by Gamma at 23:12Z post-merge of #2749 with same root cause and same proposed fix; I had already committed mine locally — first one to push wins, they can close their duplicate.

#2749 plumbed `MinHops` through `visor.PingConfig` → `rpcgrpc.PingConf` → `appnet.PingContextWithMinHops` → `router.DialOptions.MinHops`, and `router.DialRoutes` correctly suppresses its direct-transport fast-path downgrade when `opts.MinHops > 1`. But there's an earlier shortcut at `pkg/app/appnet/skywire_networker.go:376` (PingContextWithOpts) that bypasses `router.DialRoutes` entirely:

```go
if directConn, ok := r.tryDirectPingDial(addr, opts); ok {
return &SkywireConn{
Conn: directConn,
freePort: freePort,
}, nil // <-- nrg=nil, RouteHopDetails() empty
}
conn, err := r.r.PingRoute(ctx, pk, ..., opts)
```

`tryDirectPingDial` dials via `appDirectMux` directly, returning a `SkywireConn` with no `NoiseRouteGroup` attached. Consequences:

  1. Even with `opts.MinHops >= 2`, when the destination has a direct transport reachable via `appDirectMux`, the dial succeeds via the shortcut and MinHops is silently defeated.
  2. The returned `SkywireConn` has `nrg=nil`, so `RouteHopDetails()` returns nil — visible as the `hops_count=0` Gamma reported in both `MuxRouteEstablished` and the post-fix(visor/router): plumb mux-bw --min-hops through DialPing → router #2749 N=8 run.

Gamma's empirical confirmation:

  • N=1 `--min-hops 2`: 282ms setup, hops_count=0
  • N=1 no min-hops: 466ms setup, hops_count=0
  • N=8 `--min-hops 2`: ~100ms each, hops_count=0

Setup times that fast on supposed multi-hop routes are also a tell — actual multi-hop setup is 1-10s.

Fix

```go
if opts.MinHops <= 1 {
if directConn, ok := r.tryDirectPingDial(addr, opts); ok {
return &SkywireConn{...}, nil
}
}
// Falls through to r.r.PingRoute, which returns a real nrg.
```

No proto change, no new API, no behavior change for callers that don't set MinHops.

Effect post-merge

`mux-bw --routes N --min-hops 2` actually routes through intermediates end-to-end:

  • `tryDirectPingDial` skipped
  • `r.r.PingRoute` → `router.DialRoutes` with `opts.MinHops=2`
  • returned conn is a `*router.NoiseRouteGroup` wrapped in `SkywireConn{nrg: ...}`
  • `SetForwardHops` was called by `router_dial.go:186` after route setup; `nrg.forwardHops` is populated
  • `SkywireConn.RouteHopDetails()` returns the hops
  • `visor.GetPingRouteDetailsAt(ref)` returns the hops
  • `MuxRouteEstablished.hops` is populated, `hops_count > 0`

Gamma's hops_count=0 observation should disappear, and the operator's hypothesis test can finally use the actual chosen intermediates to verify route diversity from the wire.

This also invalidates the 1.78x throughput claim from Gamma's 23:05Z run — those routes were still going direct via tryDirectPingDial. The Beta↔Gamma ~1.30 Mbps ceiling we both observed is more likely a NIC/upstream cap of a single direct stcpr, not the bandwidth-via-intermediates result we thought.

Test plan

  • `go build ./...` clean
  • `golangci-lint run` clean (0 issues)
  • Live: `mux-bw --routes 1 --min-hops 2 ` → setup time >1s, `hops_count > 0` in `route_established` event
  • Live: `mux-bw --routes 8 --min-hops 2 ` → 8 routes with distinct hop paths visible in the per-route summary

skycoin#2749 plumbed MinHops through visor.PingConfig → rpcgrpc.PingConf
→ appnet.PingContextWithMinHops → router.DialOptions.MinHops, and
router.DialRoutes correctly suppresses its direct-transport
fast-path downgrade when opts.MinHops > 1. But there's an earlier
shortcut in pkg/app/appnet/skywire_networker.go:359
(PingContextWithOpts) that bypasses router.DialRoutes entirely:

  if directConn, ok := r.tryDirectPingDial(addr, opts); ok {
      return &SkywireConn{
          Conn:     directConn,
          freePort: freePort,
      }, nil  // <-- nrg=nil, RouteHopDetails() empty
  }
  conn, err := r.r.PingRoute(ctx, pk, ..., opts)

tryDirectPingDial dials the destination's appDirectMux directly,
returning a SkywireConn with no NoiseRouteGroup attached. So:

1. Even with opts.MinHops >= 2, when the destination has a direct
   transport reachable via the appDirectMux, the dial succeeds
   via the shortcut and the MinHops constraint is silently
   defeated — same class of bug Gamma diagnosed pre-skycoin#2749, just
   one layer up.

2. The returned SkywireConn has nrg=nil. That's why
   MuxRouteEstablished.hops is empty in Gamma's post-skycoin#2749 run
   even though aggregate throughput appeared to scale: routes
   were still going direct, MuxBandwidthDone reflected the
   smux-stream parallelism over one transport, and consumers
   inspecting the chosen path saw nothing because
   SkywireConn.RouteHopDetails() falls through nrg.

FIX: skip tryDirectPingDial when opts.MinHops > 1.

  if opts.MinHops <= 1 {
      if directConn, ok := r.tryDirectPingDial(addr, opts); ok {
          ...
      }
  }
  // Falls through to r.r.PingRoute, which returns a real nrg.

EFFECT POST-MERGE

`mux-bw --routes N --min-hops 2` actually routes through
intermediates end-to-end:
  - tryDirectPingDial skipped
  - r.r.PingRoute → router.DialRoutes with opts.MinHops=2
  - returned conn is a *router.NoiseRouteGroup wrapped in
    SkywireConn{nrg: ...}
  - SetForwardHops was called by router_dial.go:186 after route
    setup, so nrg.forwardHops is populated
  - SkywireConn.RouteHopDetails() returns the hops
  - visor.GetPingRouteDetailsAt(ref) returns the hops
  - MuxRouteEstablished.hops is populated

Gamma's hops_count=0 observation should disappear post-merge, and
the operator's hypothesis test can finally use the actual chosen
intermediates to verify route diversity from the wire.

Same single-line guard (opts.MinHops <= 1) — no proto change, no
new API, no behavior change for callers that don't set MinHops.

Build / golangci-lint clean.
@0pcom 0pcom merged commit 6b585c3 into skycoin:develop May 20, 2026
4 checks passed
0pcom added a commit that referenced this pull request May 21, 2026
… + MinHops (#2757)

Beta's #2756 MuxRouteFailure event surfaced the smoking gun on
mux-bw --routes N --min-hops 2: one route would establish but its
pump loop immediately failed with

  no ping connection for <pk>#0, call DialPing first

while MuxRouteEstablished named route_index = 2 for the same
route. The lookup PingRouteRef.Index was 0 even though the
pump goroutine had RouteIndex = 2.

ROOT CAUSE
The rpcgrpc PingConf → visor PingConfig adapter in
pkg/visor/init_apps.go's visorPingAdapter forwards RouteIndex
correctly for DialPing (line 238) and PingOnce (line 249), but
PingOnceWithEcho's adapter (lines 316-322) was missing the
field — same for MinHops. Aux-route pumps therefore degraded to
a primary-route (Index=0) lookup, finding nothing because
DialPing had registered the conn at the matching aux Index.

FIX
Add `RouteIndex: conf.RouteIndex` and `MinHops: conf.MinHops` to
the visorPingAdapter.PingOnceWithEcho conversion. Three-line
adapter parity fix.

DMSG adapter (DmsgPingOnceWithEcho) intentionally untouched —
v.dmsgPing.conns is keyed by PK alone, not by PingRouteRef, so
no DMSG path consumes RouteIndex.

TEST
TestPingAdapter_PingOnceWithEcho_ForwardsRouteIndex pins the
adapter contract: with no ping connection registered, the visor's
PingOnceWithEcho returns

  no ping connection for %s#%d, call DialPing first

The "%d" portion is conf.RouteIndex post-adapter. The test calls
the adapter with RouteIndex in {0, 1, 2, 7} and asserts the error
message contains the matching `#<idx>,` — so a regression that
drops RouteIndex (or stuffs in MinHops by accident) surfaces in
the error string directly. No mock VisorAPI, no fixtures.

EMPIRICAL CHAIN
This is the third bug in the mux-bw --min-hops measurement chain
to surface via the wire-event observability landed in #2746#2749#2751#2752#2750#2753#2754#2756. The pattern
holds: each event-surface fix lights up the next bug downstream.
The operator's "mux > direct" hypothesis test should now finally
be measurable end-to-end once #2756 (route_failure event) +
this PR auto-deploy.
0pcom added a commit that referenced this pull request May 21, 2026
* fix(visor/ping): narrow ping.mu/dmsgPing.mu critical section to map lookup

DOMINANT BOTTLENECK for mux-bw bandwidth measurements. The visor's
Ping/PingOnce/PingOnceWithEcho (and dmsg twins) held v.ping.mu (a
single visor-global *sync.Mutex) for the ENTIRE wire roundtrip:

  v.ping.mu.Lock()
  defer v.ping.mu.Unlock()
  pingEntry, ok := v.ping.conns[ref]
  // ... ~287ms of wire I/O at 2-hop with 32 KB payloads ...

mux-bw's N pump goroutines all call PingOnceWithEcho on DIFFERENT
PingRouteRefs. They each look up their OWN conn via the map; the
wire I/O is independent. But the global mutex serialized them
through one ~287ms slot each. So:

  - Aggregate throughput across N routes didn't scale with N
  - Per-route avg pinned at ~351 kbps even though single-call peak
    was ~1.7 Mbps (1 RTT × 32 KB)
  - --probe-rtt latency probes during a loaded pump measured
    "probe-mutex-wait + network RTT" instead of network RTT,
    swamping the queueing-delay signal at short hop counts
  - Bidirectional simultaneous mux-bw measurements showed
    mutual-starvation that LOOKED like shared-link contention
    but was actually mutex contention on each side's ping state

ROOT CAUSE

The mutex's actual job is to protect v.ping.conns (the map) from
concurrent insert (DialPing) and delete (StopPingRoute). The
wire I/O on the chosen conn does NOT need the map mutex held —
each mux-bw pump goroutine owns its own conn via its RouteIndex,
no aliasing.

FIX

Shrink the critical section to just the map lookup:

  v.ping.mu.Lock()
  pingEntry, ok := v.ping.conns[ref]
  v.ping.mu.Unlock()
  if !ok { ... }
  // ... wire I/O on pingEntry.conn WITHOUT holding the mutex ...

Applied to Ping, PingOnce, PingOnceWithEcho, DmsgPing,
DmsgPingOnce, DmsgPingOnceWithEcho. BandwidthTest already had the
correct narrow scope.

CONCURRENT-CLOSE SEMANTICS

Pre-fix: StopPing concurrent with PingOnceWithEcho serialized via
the mutex — they took turns, no race. Post-fix: StopPing can close
the conn while PingOnceWithEcho is doing wire I/O. The Read/Write
on the closed conn returns ErrClosed cleanly. mux-bw's pump loop
already handles read/write errors by exiting the pump goroutine;
the resulting failure is surfaced via Beta's MuxRouteFailure event
(#2756) so the operator sees the cause instead of an indefinite
block.

The same-PingRouteRef-from-multiple-goroutines case (always
undefined behavior on the underlying net.Conn) is unchanged —
callers must serialize themselves. mux-bw enforces one
goroutine per RouteIndex natively.

TESTS

pkg/visor/ping_mu_concurrency_test.go:
  - TestPingOnceWithEcho_DoesNotSerializeAcrossRouteIndexes:
    200 concurrent calls with distinct PingRouteRefs (no registered
    conns) complete in << 1s. A regression that re-introduces
    wire-I/O-under-lock would either time out or take orders of
    magnitude longer.
  - TestPingMu_NotHeldDuringConnAbsentCallpath: after
    PingOnceWithEcho returns, the mutex must be immediately
    acquirable from another goroutine. Catches the defer-on-entry
    pattern directly.

EMPIRICAL PREDICTION

Once this auto-deploys, the operator's "mux > direct" hypothesis
becomes testable WITHOUT --hops-via intermediate pinning. Per-
route avg should rise from ~351 kbps toward the single-call peak
of ~1.7 Mbps, and N=2..8 disjoint mux should aggregate roughly
linearly (modulo per-intermediate quality variance) instead of
flat-lining at single-route throughput.

CHAIN

The mux-bw measurement loop has now closed:
  #2745 per-route teardown
  #2746 disjoint-intermediate routing
  #2749 plumb --min-hops through DialPing
  #2750 stop poisoning dst breaker from intermediate failures
  #2751 tryDirectPingDial gate on MinHops
  #2752 honor caller SetupTimeout
  #2756 MuxRouteFailure pump-phase event
  #2757 PingOnceWithEcho adapter forwards RouteIndex
  this PR: ping.mu doesn't serialize parallel pumps

If the mux > direct hypothesis is real, it should be visible
in measurements after this lands.

* fix(visor/ping): errcheck on discarded PingOnceWithEcho returns in test

golangci-lint errcheck flagged both test sites that discard the
4-tuple return of v.PingOnceWithEcho via _, _, _, _. The discards
are intentional — the concurrency test asserts on wall-clock
serialization behavior, not on per-call success; the mutex-release
test asserts that the lock is acquirable post-return regardless of
whether the call itself succeeded or returned ErrNoPingConnection.

Add //nolint:errcheck comments explaining the intent. No behavior
change; CI lint pass.
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.

1 participant