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:
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.
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.
DefaultNetworkis resolved in the facade but not propagated to the embedded corenetworkSpecs()resolves the effective default network correctly (RouterConfig.DefaultNetwork, otherwise netId=2, otherwise first entry), andRouterstores that value indefaultNetwork.However,
newRouter()constructs the core with:while
EmbeddedRouterinternally sets:This creates two different notions of the default network when the configured default is not the first entry.
Example:
The facade treats
i2pas default, while core-level operations usecorp.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
specsto 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
mergedListenermultiplexes one listener per bound network, butfeed()forwards the first sub-listenerAccept()error directly into the shared channel.That means this state:
can still cause:
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:3. Authenticated and unauthenticated datagram listener classes are no longer enforced
Before the multi-network change, the public APIs had an important semantic split:
After the refactor, both constructors call the same
listenPacket()path without validating protocol class.This allows mismatched combinations such as:
For protocol 20, decoding stores the claimed source in
UnauthPacket.ClaimedSourceand leaves the authenticatedsource Addrempty. If exposed throughPacketConn.ReadFrom(), callers can receive a zero source address.Conversely, routing protocol 19 through
UnauthPacketConndiscards the authenticated-source semantics of that protocol.Expected fix
Restore constructor-level protocol-class validation:
Keep network selection orthogonal to protocol authentication semantics.
4.
replyScratchcan race withReleaseSensitive()during shutdownThe PR shortened the
lifecycleMu.RLock()critical section inGarlicReceiver.HandleGarlicFrom()so expensive downstream work can run after releasing the lifecycle lock.That is reasonable, but some paths now do this pattern:
while
ReleaseSensitive()can concurrently acquire the write lock and execute:sync.Poolsupports concurrentGet/Puton the same pool, but reassigning the pool variable concurrently withPutis not safe.Expected fix
Make scratch-pool lifetime independent from the receiver field mutation. For example:
*sync.Pooland capture the pointer before unlocking; orReleaseSensitive()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.Namedocuments[a-zA-Z0-9_]+, butvalidNetworkName()currently usesunicode.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.ymlruns pull-request-controlled code withactions/checkoutcredential persistence still enabled; setpersist-credentials: false.Validation
Please add targeted tests for at least:
specs[0];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.