Skip to content

Commit c5cc6d1

Browse files
pgodwinclaude
andcommitted
fix(router): late AARP/LLAP claims never join the routing table
Runtime.Start attaches and seeds each [Router].members port synchronously right after StartAll returns, but a real EtherTalk/LToUDP/TashTalk port's node-address claim (AARP/LLAP) finishes in a background goroutine and Start never waits for it. When the claim lands after Attach already ran with NetworkMin()==0, router.Attach's own nonzero guard skips installing the directly-connected route and seedZone's zero-range guard skips the ZIT too — and nothing ever retried either install. The port still announces its claimed range correctly over RTMP and answers same-network traffic fine (Inbound's same-network fast path needs no routing-table entry), but any service reply that must round-trip through router.Reply->Route (ZIP's ATP zone queries, AFP's ASP session reads) does RoutingTable.GetByNetwork and gets a silent, permanent nil — the reply is dropped with no error, forever. This reproduced as: Chooser showing only the AFP server's own zone via GetLocalZones (expected) but never all zones via GetZoneList, and AFP connections stalling on ASP GetStatus with zero replies. Poll briefly (bounded, cancelled on Stop) for a late claim after Attach and re-run the same route/zone install once it lands; both installs are idempotent so this is a no-op on the fast path where the claim already beat Attach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 3923e13 commit c5cc6d1

2 files changed

Lines changed: 156 additions & 0 deletions

File tree

compose/runtime/runtime.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ package runtime
2222
import (
2323
"context"
2424
"fmt"
25+
"sync"
26+
"time"
2527

2628
"github.com/ObsoleteMadness/ClassicStack/compose/registry"
2729
"github.com/ObsoleteMadness/ClassicStack/compose/supervisor"
@@ -161,6 +163,9 @@ type Runtime struct {
161163
transports *transportWiring // retained IPX/NetBEUI mini-routers + MacIP egress; drives runtime port attach + egress lifecycle
162164
comps map[string]component.Component // built components by name, for compose-edge lookups (diagnostics wiring)
163165
log log.Logger
166+
167+
claimWatchStop chan struct{} // closed by Stop to cancel any still-polling late-claim watchers (§ late-claim fix)
168+
claimWatchWG sync.WaitGroup // Stop waits on this so no watcher touches the router after Stop returns
164169
}
165170

166171
// Load builds a config.Model from a Store + Codec. A missing store file yields the
@@ -534,6 +539,7 @@ func (r *Runtime) Start(ctx context.Context) error {
534539
eg.Start()
535540
}
536541
if r.rtr != nil {
542+
r.claimWatchStop = make(chan struct{})
537543
for _, p := range r.members {
538544
if err := r.rtr.Attach(p); err != nil {
539545
if r.log != nil {
@@ -543,6 +549,26 @@ func (r *Runtime) Start(ctx context.Context) error {
543549
continue
544550
}
545551
seedZone(r.rtr, p)
552+
if p.NetworkMin() == 0 {
553+
// A real AARP/LLAP claim (EtherTalk/LToUDP/TashTalk) finishes in a
554+
// background goroutine well after Start returns (runport/aarp never
555+
// blocks Start on the probe burst) — Attach ran above with
556+
// NetworkMin()==0, so its own directly-connected route was skipped
557+
// (router.go's `if nmin != 0 && nmax != 0` guard) and seedZone's own
558+
// zero-range guard skipped the ZIT too. Nothing else ever retries
559+
// either install: the port later announces its claimed range fine
560+
// over RTMP and answers same-network traffic fine (Inbound's
561+
// same-network fast path needs no routing-table entry), but every
562+
// service reply that must round-trip through router.Reply→Route
563+
// (ZIP's ATP zone queries, AFP's ASP session) does
564+
// RoutingTable.GetByNetwork and gets a silent, permanent nil. Poll
565+
// briefly for the claim to land and (re)run the same install once it
566+
// does — SetPortRange/AddNetworksToZone are both idempotent against
567+
// an already-correct entry, so this is a no-op on the fast path where
568+
// the claim beat Attach.
569+
r.claimWatchWG.Add(1)
570+
go r.awaitLateClaim(p, r.claimWatchStop)
571+
}
546572
}
547573
}
548574
// Begin the telemetry stats flush once the stack is up: it polls every Statful
@@ -552,10 +578,50 @@ func (r *Runtime) Start(ctx context.Context) error {
552578
return nil
553579
}
554580

581+
// claimWatchInterval is the poll period awaitLateClaim uses while waiting for a
582+
// member port's AARP/LLAP claim to land.
583+
const claimWatchInterval = 100 * time.Millisecond
584+
585+
// claimWatchAttempts bounds how long awaitLateClaim polls before giving up (30 ×
586+
// 100ms = 3s — generous over AARP's normal probe-burst duration; a port that has not
587+
// claimed by then logs a warning and is left for its own retry/conflict logic).
588+
const claimWatchAttempts = 30
589+
590+
// awaitLateClaim polls p for its AARP/LLAP claim to land, then installs its
591+
// directly-connected route + seed zone (see the Start comment for why this install
592+
// can be skipped at Attach time). Runs until the claim lands, claimWatchAttempts is
593+
// exhausted, or stop is closed by Runtime.Stop. r.claimWatchWG.Done is deferred so
594+
// Stop can wait out any watcher still polling before it detaches ports.
595+
func (r *Runtime) awaitLateClaim(p router.RoutedPort, stop chan struct{}) {
596+
defer r.claimWatchWG.Done()
597+
for range claimWatchAttempts {
598+
select {
599+
case <-stop:
600+
return
601+
case <-time.After(claimWatchInterval):
602+
}
603+
if p.NetworkMin() == 0 {
604+
continue
605+
}
606+
r.rtr.RoutingTable().SetPortRange(p, p.NetworkMin(), p.NetworkMax())
607+
seedZone(r.rtr, p)
608+
return
609+
}
610+
if r.log != nil {
611+
r.log.Log1(log.Warn, "router member never claimed an address; routing table has no directly-connected entry for it",
612+
log.Str("member", p.Name()))
613+
}
614+
}
615+
555616
// Stop detaches the router members (reversing Start's attach) and then brings the
556617
// whole stack down in reverse dependency order. Detach is best-effort — a member
557618
// already withdrawn (e.g. by an individual Stop) must not block shutdown.
558619
func (r *Runtime) Stop(ctx context.Context) error {
620+
if r.claimWatchStop != nil {
621+
close(r.claimWatchStop)
622+
r.claimWatchWG.Wait()
623+
r.claimWatchStop = nil
624+
}
559625
if r.log != nil && r.log.Enabled(log.Info) {
560626
r.log.Log0(log.Info, "shutdown: stopping telemetry stats flush")
561627
}

compose/runtime/runtime_test.go

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"sort"
77
"sync"
88
"testing"
9+
"time"
910

1011
"github.com/ObsoleteMadness/ClassicStack/compose/registry"
1112
"github.com/ObsoleteMadness/ClassicStack/core/bus"
@@ -435,6 +436,95 @@ func (p *fakeSeedPort) NetworkMin() uint16 { return p.nmin }
435436
func (p *fakeSeedPort) NetworkMax() uint16 { return p.nmax }
436437
func (p *fakeSeedPort) SeedZone() string { return p.zone }
437438

439+
// lateClaimPort simulates a real AARP-based EtherTalk port: NetworkMin/Max are 0 when
440+
// Start returns (matching runport/aarp's async claimLoop, which probes over the wire in
441+
// a background goroutine and calls SetAddress only once a node address is accepted —
442+
// Start itself never blocks on it). The range becomes available `delay` after Start.
443+
type lateClaimPort struct {
444+
fakeRoutedPort
445+
mu sync.Mutex
446+
nmin, nmax uint16
447+
zone string
448+
delay time.Duration
449+
}
450+
451+
func (p *lateClaimPort) Start(ctx context.Context) error {
452+
go func() {
453+
time.Sleep(p.delay)
454+
p.mu.Lock()
455+
p.nmin, p.nmax = 3, 5
456+
p.mu.Unlock()
457+
}()
458+
return p.fakeRoutedPort.Start(ctx)
459+
}
460+
func (p *lateClaimPort) NetworkMin() uint16 { p.mu.Lock(); defer p.mu.Unlock(); return p.nmin }
461+
func (p *lateClaimPort) NetworkMax() uint16 { p.mu.Lock(); defer p.mu.Unlock(); return p.nmax }
462+
func (p *lateClaimPort) SeedZone() string { return p.zone }
463+
464+
// TestStart_LateClaimingMemberNeverJoinsRoutingTable is the regression guard for the
465+
// dead-ZIP/ASP-reply bug: Runtime.Start attaches + seeds each router member SYNCHRONOUSLY
466+
// right after StartAll returns (runtime.go's Start loop), but a real EtherTalk port's AARP
467+
// claim finishes in a background goroutine — Start does not wait for it. When the claim
468+
// lands after Attach already ran with NetworkMin()==0, router.Attach's own
469+
// `if nmin != 0 && nmax != 0 { SetPortRange }` guard skips installing the directly-connected
470+
// route, and nothing ever retries it: the port later announces its range correctly over RTMP
471+
// and answers same-network unicast fine, but any reply that must round-trip through
472+
// router.Reply→Route (ZIP's ATP zone queries, AFP's ASP session reads) does
473+
// `RoutingTable.GetByNetwork(net)` and gets a permanent nil — the reply is dropped with no
474+
// error, forever, even though the port is otherwise live. This proves the race exists today.
475+
func TestStart_LateClaimingMemberNeverJoinsRoutingTable(t *testing.T) {
476+
m := config.NewModel()
477+
m.Router = config.RouterSection{Members: []string{"et0"}}
478+
port := &lateClaimPort{
479+
fakeRoutedPort: fakeRoutedPort{name: "et0"},
480+
zone: "EtherTalk Network",
481+
delay: 20 * time.Millisecond,
482+
}
483+
src := fakeSource{
484+
router.Name: func(*registry.BuildContext) (component.Component, error) {
485+
return router.New(log.New(router.Name)), nil
486+
},
487+
"et0": func(*registry.BuildContext) (component.Component, error) { return port, nil },
488+
}
489+
rt, err := Build(Options{Model: m, source: src})
490+
if err != nil {
491+
t.Fatalf("Build = %v", err)
492+
}
493+
if err := rt.Start(context.Background()); err != nil {
494+
t.Fatalf("Start = %v", err)
495+
}
496+
t.Cleanup(func() { rt.Stop(context.Background()) })
497+
498+
// Give the AARP-simulating goroutine time to land its claim (well past `delay`) and
499+
// the runtime's late-claim watcher (which polls every claimWatchInterval) a full
500+
// cycle to notice and install the route.
501+
time.Sleep(250 * time.Millisecond)
502+
503+
if got := port.NetworkMin(); got != 3 {
504+
t.Fatalf("port never claimed its range in this test setup: NetworkMin=%d", got)
505+
}
506+
507+
rtr := rt.router()
508+
// GetByNetwork's second return is a "marked bad" flag, not a found/ok flag (every
509+
// caller in router.go checks entry == nil instead) — a fresh entry is state-good,
510+
// so that bool is false here even on success.
511+
if entry, _ := rtr.RoutingTable().GetByNetwork(3); entry == nil {
512+
t.Errorf("REGRESSION: network 3 has no routing-table entry even after the port claimed" +
513+
" range 3-5 — router.Reply()'s Route() call will silently drop any service reply" +
514+
" addressed to this network forever, because Attach ran while NetworkMin() was still 0")
515+
}
516+
found := false
517+
for _, z := range rtr.Zones().Zones() {
518+
if string(z) == "EtherTalk Network" {
519+
found = true
520+
}
521+
}
522+
if !found {
523+
t.Errorf("REGRESSION: seed zone never installed into the ZIT — seedZone() also ran while" +
524+
" NetworkMin() was still 0 and its own `if nmin == 0 { return }` guard skipped it")
525+
}
526+
}
527+
438528
// TestStart_SeedsMemberZoneIntoZIT is the regression guard for the empty-Chooser bug:
439529
// when a seed member port attaches, the runtime must install its network range into the
440530
// router's Zone Information Table under the port's seed zone, so a self-contained seed

0 commit comments

Comments
 (0)