Skip to content

fix: address post-merge multi-network correctness regressions from #12 #13

Description

@gosunuts

Context

PR #12 introduced multi-network support and has already been merged. The overall direction is sound, but a post-merge review found several correctness and lifecycle issues that are still present on main.

This issue tracks the fixes together because they all sit at the new multi-network abstraction boundary.

1. DefaultNetwork is resolved in the facade but not propagated to the embedded core

networkSpecs() resolves the effective default network correctly (RouterConfig.DefaultNetwork, otherwise netId=2, otherwise first entry), and Router stores that value in defaultNetwork.

However, newRouter() constructs the core with:

node.NewEmbeddedRouterNetworks(ctx, specs)

while EmbeddedRouter internally sets:

def: specs[0].Name

This creates two different notions of the default network when the configured default is not the first entry.

Example:

Networks = []{
    {Name: "corp", NetworkID: 77},
    {Name: "i2p", NetworkID: 2},
}
DefaultNetwork = "i2p"

The facade treats i2p as default, while core-level operations use corp.

Affected behavior includes router-level operations such as:

  • Router.Hash()
  • Router.WaitReady()
  • Node().Default()
  • ExportLocalRouterInfo("")
  • ImportRouterInfo("", ...)

Expected fix

Propagate the resolved default network into the embedded core explicitly. Prefer preserving configuration order rather than reordering specs to make the default first.

For example, evolve the constructor/API so the core receives the resolved default name and validates it.


2. One failed sub-listener terminates a healthy merged multi-network listener

mergedListener multiplexes one listener per bound network, but feed() forwards the first sub-listener Accept() error directly into the shared channel.

That means this state:

corp listener -> failed
i2p listener  -> healthy

can still cause:

merged.Accept() -> error
http.Server.Serve() -> exits

A single network failure therefore takes down service on every network, defeating the availability benefit of a merged listener.

Expected fix

A failed sub-listener should retire independently while healthy sub-listeners continue accepting connections.

Only surface a terminal listener error once no sub-listener remains active.

Also fix mergedListener.Close() while touching this code:

  • do not discard sub-listener close errors;
  • drain and close already-buffered accepted connections;
  • keep shutdown idempotent.

3. Authenticated and unauthenticated datagram listener classes are no longer enforced

Before the multi-network change, the public APIs had an important semantic split:

PacketConn
  protocol 17 / 19
  authenticated source

UnauthPacketConn
  protocol 18 / 20
  unauthenticated / claimed source

After the refactor, both constructors call the same listenPacket() path without validating protocol class.

This allows mismatched combinations such as:

ListenPacket("corp-datagram3", ...)
ListenUnauthPacket("corp-datagram2", ...)

For protocol 20, decoding stores the claimed source in UnauthPacket.ClaimedSource and leaves the authenticated source Addr empty. If exposed through PacketConn.ReadFrom(), callers can receive a zero source address.

Conversely, routing protocol 19 through UnauthPacketConn discards the authenticated-source semantics of that protocol.

Expected fix

Restore constructor-level protocol-class validation:

ListenPacket / PacketConn
  -> protocols 17, 19 only

ListenUnauthPacket / UnauthPacketConn
  -> protocols 18, 20 only

Keep network selection orthogonal to protocol authentication semantics.


4. replyScratch can race with ReleaseSensitive() during shutdown

The PR shortened the lifecycleMu.RLock() critical section in GarlicReceiver.HandleGarlicFrom() so expensive downstream work can run after releasing the lifecycle lock.

That is reasonable, but some paths now do this pattern:

scratch := r.getReplyScratch()
unlock()
...
r.replyScratch.Put(scratch)

while ReleaseSensitive() can concurrently acquire the write lock and execute:

r.replyScratch = sync.Pool{}

sync.Pool supports concurrent Get/Put on the same pool, but reassigning the pool variable concurrently with Put is not safe.

Expected fix

Make scratch-pool lifetime independent from the receiver field mutation. For example:

  • store *sync.Pool and capture the pointer before unlocking; or
  • keep the pool return inside lifecycle synchronization; or
  • otherwise guarantee ReleaseSensitive() cannot replace/reset the pool while a handler still owns a scratch buffer from it.

The fix should preserve the reason the lock was shortened: downstream processing should not need to hold the receiver lifecycle read lock longer than necessary.


Smaller follow-ups from the same review

These are not the main blockers, but are worth fixing in the same cleanup if convenient:

  • NetworkConfig.Name documents [a-zA-Z0-9_]+, but validNetworkName() currently uses unicode.IsDigit, accepting non-ASCII digits.
  • validateBootstrap() concatenates normal and priority reseed URLs but reports all failures as .ReseedURLs[...]; priority failures should identify .PriorityReseedURLs[...] with the correct index.
  • .github/workflows/live-roundtrip.yml runs pull-request-controlled code with actions/checkout credential persistence still enabled; set persist-credentials: false.

Validation

Please add targeted tests for at least:

  • configured default network different from specs[0];
  • merged listener continuing to serve after one sub-listener fails;
  • datagram constructor rejection for protocol/authentication-class mismatches;
  • shutdown/race coverage for replyScratch (run under -race).

Non-goal

Do not collapse the new multi-network model back into one shared controller or one shared NetDB. The architecture introduced by #12 is useful; this issue is about making its ownership and lifecycle semantics correct.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions