From dc7b858f78c5c6839384ce71b79f9fdabdb30f5f Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Sun, 21 Jun 2026 21:43:13 -0400 Subject: [PATCH] Fix long delay replies answering later command --- internal/core/confirm.go | 22 +++++++++++--- internal/mesh/exchange.go | 17 +++++++++++ internal/mesh/exchange_test.go | 52 ++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/internal/core/confirm.go b/internal/core/confirm.go index 6d0779b..5675090 100644 --- a/internal/core/confirm.go +++ b/internal/core/confirm.go @@ -182,8 +182,8 @@ func (s *Handlers) wsConfirm(w http.ResponseWriter, r *http.Request) { // Optionally fetch and store the repeater's location (owner consented). Each // coordinate is a separate query so progress (and retries) are visible. if rep.StoreLocation { - fetchCoord := func(label, cmd string) (float64, bool) { - reply, err := ex.Command(ctx, cmd, func(attempt, max int) { + fetchCoord := func(label, cmd string, accept func(text string) bool) (float64, bool) { + reply, err := ex.CommandAccept(ctx, cmd, accept, func(attempt, max int) { if attempt == 1 { _ = bridge.Status("info", "Fetching "+label+"…") } else { @@ -195,8 +195,22 @@ func (s *Handlers) wsConfirm(w http.ResponseWriter, r *http.Request) { } return parseLocationFloat(reply) } - lat, okLat := fetchCoord("latitude", "get lat") - lon, okLon := fetchCoord("longitude", "get lon") + lat, okLat := fetchCoord("latitude", "get lat", nil) + // A slow latitude fetch is retried, which makes the repeater re-run "get + // lat" and emit duplicate replies; one can straggle in during the "get + // lon" wait and be misread as the longitude (storing lat,lat). Since the + // two coordinates differ, reject a longitude reply whose value equals the + // latitude we just read and keep waiting for the genuine reply. + lon, okLon := fetchCoord("longitude", "get lon", func(text string) bool { + f, ok := parseLocationFloat(text) + if ok && okLat && f == lat { + if debug { + _ = bridge.Status("debug", "ignored a stale 'get lat' reply while awaiting longitude") + } + return false + } + return true + }) if okLat && okLon { if err := s.Store.SetRepeaterLocation(ctx, id, lat, lon); err != nil { _ = bridge.Status("error", "could not store location: "+err.Error()) diff --git a/internal/mesh/exchange.go b/internal/mesh/exchange.go index 411baa0..7f31781 100644 --- a/internal/mesh/exchange.go +++ b/internal/mesh/exchange.go @@ -170,6 +170,20 @@ func (e *Exchanger) directThreshold() int { return (e.tries + 1) / 2 } // path has been learned (via Login), early attempts use direct routing and // later attempts fall back to flood. func (e *Exchanger) Command(ctx context.Context, command string, onAttempt func(attempt, max int)) (string, error) { + return e.CommandAccept(ctx, command, nil, onAttempt) +} + +// CommandAccept is Command with a caller-supplied acceptance test. A decoded +// reply is only taken as the answer when accept(text) returns true; otherwise +// the reply is ignored and the exchange keeps waiting (and retrying). +// +// This lets a caller reject a stale reply that decrypts fine but belongs to an +// earlier command. Because a retried request makes the repeater re-run the +// command, a slow exchange can leave duplicate replies in flight; without a +// request identifier in the reply, the only way to tell such a straggler from +// the genuine reply is a caller filter that knows what a valid answer looks +// like. accept may be nil to take the first decodable reply (like Command). +func (e *Exchanger) CommandAccept(ctx context.Context, command string, accept func(text string) bool, onAttempt func(attempt, max int)) (string, error) { var reply string err := e.exchange(ctx, func(ts time.Time, attempt int) ([]byte, error) { @@ -187,6 +201,9 @@ func (e *Exchanger) Command(ctx context.Context, command string, onAttempt func( if err != nil { return false } + if accept != nil && !accept(text) { + return false + } reply = text return true }, diff --git a/internal/mesh/exchange_test.go b/internal/mesh/exchange_test.go index f410ac5..4af75a0 100644 --- a/internal/mesh/exchange_test.go +++ b/internal/mesh/exchange_test.go @@ -146,6 +146,58 @@ func TestExchangerUsesDirectAfterLogin(t *testing.T) { } } +// seqModem replies to each send with the next text in replies (repeating the +// last once exhausted), simulating a repeater that emits a stale duplicate +// before the genuine reply arrives. +type seqModem struct { + ex *Exchanger + repeater meshcore.LocalIdentity + server meshcore.Identity + replies []string + sends int +} + +func (f *seqModem) SendData(_ []byte) error { + text := f.replies[len(f.replies)-1] + if f.sends < len(f.replies) { + text = f.replies[f.sends] + } + f.sends++ + shared, _ := f.repeater.SharedSecret(f.server) + plain := meshcore.BuildTextPlaintext(time.Unix(1_700_000_000, 0), 1<<2, []byte(text)) + tm, _ := meshcore.NewTextMessage(f.repeater, f.server, plain, shared) + payload, _ := tm.ToBytes() + pkt := &meshcore.Packet{Header: meshcore.MakeHeader(meshcore.RouteTypeFlood, meshcore.PayloadTypeTxtMsg, 0), Payload: payload} + raw, _ := pkt.ToBytes() + f.ex.HandleData(raw) + return nil +} + +// TestCommandAcceptRejectsStaleReply verifies the acceptance filter skips a +// reply it rejects and keeps waiting for one it accepts — the guard that keeps +// a straggling "get lat" duplicate from being misread as the longitude. +func TestCommandAcceptRejectsStaleReply(t *testing.T) { + server := mustIdentity(t) + repeater := mustIdentity(t) + // First two sends echo the stale latitude; the third gives the longitude. + fm := &seqModem{repeater: repeater, server: server.Identity, replies: []string{"> 37.7749", "> 37.7749", "> -122.4194"}} + ex := NewExchanger(fm, server, repeater.Identity, 0, time.Millisecond, 4) + fm.ex = ex + + reply, err := ex.CommandAccept(context.Background(), "get lon", func(text string) bool { + return text != "> 37.7749" // reject the stale latitude duplicate + }, nil) + if err != nil { + t.Fatalf("CommandAccept: %v", err) + } + if reply != "> -122.4194" { + t.Errorf("reply = %q, want %q", reply, "> -122.4194") + } + if fm.sends != 3 { + t.Errorf("sends = %d, want 3 (two rejected, one accepted)", fm.sends) + } +} + func TestExchangerCancelled(t *testing.T) { server := mustIdentity(t) repeater := mustIdentity(t)