Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions darepod/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2456,6 +2456,30 @@ func (s *Server) registerOOREventRoutes(router *serverconn.EventRouter) { //noli
"response envelope must be provided")
}

// This route is shared with live, in-memory unary
// callers of ListVTXOsByScripts. By the time a response
// reaches durable dispatch without the OOR metadata
// correlation prefix it is either a stale response from
// a prior process or a malformed metadata ID; in both
// cases we consume and ack it so ingress can advance
// rather than wedging on a response we cannot adapt.
if !oor.IsIncomingMetadataCorrelationID(
env.Rpc.CorrelationId,
) {

s.log.DebugS(context.Background(),
"Acking response without OOR "+
"correlation prefix",
slog.String(
"correlation_id",
env.Rpc.CorrelationId,
),
slog.String("service", env.Rpc.Service),
slog.String("method", env.Rpc.Method))

return nil, serverconn.ErrEnvelopeHandled
}
Comment on lines +2466 to +2481

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While handling stale ListVTXOsByScripts responses by returning serverconn.ErrEnvelopeHandled is correct, doing so silently can make troubleshooting difficult if a valid response is misclassified. Adding a debug log helps with observability.

Additionally, the exact same stalling issue (described in #543) likely exists for the ListOORRecipientEventsByScript route (lines 2529-2535). If a stale normal unary response for ListOORRecipientEventsByScript is pulled from the mailbox, oor.ParseIncomingResolveCorrelationID will fail because the correlation ID lacks the expected OOR prefix, returning an error and stalling the ingress loop. You should implement a similar prefix check (e.g., oor.IsIncomingResolveCorrelationID) for that route to safely discard stale non-OOR responses.

Suggested change
if !oor.IsIncomingMetadataCorrelationID(
env.Rpc.CorrelationId,
) {
return nil, serverconn.ErrEnvelopeHandled
}
if !oor.IsIncomingMetadataCorrelationID(
env.Rpc.CorrelationId,
) {
s.log.DebugS(context.Background(),
"Discarding stale non-OOR ListVTXOsByScripts response",
"correlation_id", env.Rpc.CorrelationId,
)
return nil, serverconn.ErrEnvelopeHandled
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both points addressed in e5dd7ef:

  • Debug log on discard — added a DebugS breadcrumb (correlation_id/service/method) before returning ErrEnvelopeHandled on this route.
  • ListOORRecipientEventsByScript has the same bug — good catch, it did. Applied the symmetric fix: new oor.IsIncomingResolveCorrelationID (with ParseIncomingResolveCorrelationID refactored to use it), a matching prefix guard + debug log on that route, and a TestIsIncomingResolveCorrelationID mirroring the metadata test.


sessionID, err := oor.ParseIncomingMetadataCorrelationID( //nolint:ll
env.Rpc.CorrelationId,
)
Expand Down Expand Up @@ -2520,6 +2544,29 @@ func (s *Server) registerOOREventRoutes(router *serverconn.EventRouter) { //noli
"response envelope must be provided")
}

// As with the ListVTXOsByScripts route above, this
// route is shared with live, in-memory unary callers.
// A response that reaches durable dispatch without the
// OOR resolve correlation prefix is a stale or
// malformed response we cannot adapt; consume and ack
// it so ingress advances instead of wedging.
if !oor.IsIncomingResolveCorrelationID(
env.Rpc.CorrelationId,
) {

s.log.DebugS(context.Background(),
"Acking response without OOR "+
"correlation prefix",
slog.String(
"correlation_id",
env.Rpc.CorrelationId,
),
slog.String("service", env.Rpc.Service),
slog.String("method", env.Rpc.Method))

return nil, serverconn.ErrEnvelopeHandled
}

sessionID, recipientEventID, err :=
oor.ParseIncomingResolveCorrelationID(
env.Rpc.CorrelationId,
Expand Down
12 changes: 9 additions & 3 deletions oor/incoming_adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,21 @@ func IncomingResolveCorrelationID(sessionID SessionID,
strconv.FormatUint(recipientEventID, 10)
}

// IsIncomingResolveCorrelationID returns true when correlationID belongs to a
// durable incoming-transfer resolution query.
func IsIncomingResolveCorrelationID(correlationID string) bool {
return len(correlationID) > len(incomingResolveCorrelationPrefix) &&
correlationID[:len(incomingResolveCorrelationPrefix)] ==
incomingResolveCorrelationPrefix
}

// ParseIncomingResolveCorrelationID decodes a durable incoming-transfer
// resolution query correlation ID back into the OOR session ID and recipient
// event ID.
func ParseIncomingResolveCorrelationID(correlationID string) (SessionID, uint64,
error) {

if len(correlationID) <= len(incomingResolveCorrelationPrefix) ||
correlationID[:len(incomingResolveCorrelationPrefix)] !=
incomingResolveCorrelationPrefix {
if !IsIncomingResolveCorrelationID(correlationID) {
return SessionID{}, 0, fmt.Errorf("unexpected incoming "+
"resolve correlation id: %q", correlationID)
}
Expand Down
32 changes: 32 additions & 0 deletions oor/incoming_adapter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package oor

import (
"testing"

"github.com/stretchr/testify/require"
)

// TestIsIncomingResolveCorrelationID verifies only durable incoming-transfer
// resolution query correlation IDs match the OOR resolve route prefix.
func TestIsIncomingResolveCorrelationID(t *testing.T) {
t.Parallel()

var sessionID SessionID
sessionID[0] = 1

require.True(
t,
IsIncomingResolveCorrelationID(
IncomingResolveCorrelationID(sessionID, 7),
),
)
require.False(t, IsIncomingResolveCorrelationID(""))
require.False(
t, IsIncomingResolveCorrelationID("00aa8bfb11f09881bbd2"),
)
require.False(
t, IsIncomingResolveCorrelationID(
incomingResolveCorrelationPrefix,
),
)
}
12 changes: 9 additions & 3 deletions oor/incoming_metadata_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,20 @@ func IncomingMetadataCorrelationID(sessionID SessionID) string {
chainhash.Hash(sessionID).String()
}

// IsIncomingMetadataCorrelationID returns true when correlationID belongs to a
// durable incoming metadata query.
func IsIncomingMetadataCorrelationID(correlationID string) bool {
return len(correlationID) > len(incomingMetadataCorrelationPrefix) &&
correlationID[:len(incomingMetadataCorrelationPrefix)] ==
incomingMetadataCorrelationPrefix
}

// ParseIncomingMetadataCorrelationID decodes a durable incoming metadata query
// correlation ID back into the OOR session ID.
func ParseIncomingMetadataCorrelationID(correlationID string) (SessionID,
error) {

if len(correlationID) <= len(incomingMetadataCorrelationPrefix) ||
correlationID[:len(incomingMetadataCorrelationPrefix)] !=
incomingMetadataCorrelationPrefix {
if !IsIncomingMetadataCorrelationID(correlationID) {
return SessionID{}, fmt.Errorf("unexpected incoming metadata "+
"correlation id: %q", correlationID)
}
Expand Down
25 changes: 25 additions & 0 deletions oor/incoming_metadata_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,31 @@ import (
"github.com/stretchr/testify/require"
)

// TestIsIncomingMetadataCorrelationID verifies only durable incoming metadata
// query correlation IDs match the OOR metadata route prefix.
func TestIsIncomingMetadataCorrelationID(t *testing.T) {
t.Parallel()

var sessionID SessionID
sessionID[0] = 1

require.True(
t,
IsIncomingMetadataCorrelationID(
IncomingMetadataCorrelationID(sessionID),
),
)
require.False(t, IsIncomingMetadataCorrelationID(""))
require.False(
t, IsIncomingMetadataCorrelationID("00aa8bfb11f09881bbd2"),
)
require.False(
t, IsIncomingMetadataCorrelationID(
incomingMetadataCorrelationPrefix,
),
)
}

// TestIncomingMetadataFromRPCOperatorKey verifies incoming metadata parsing
// preserves the per-VTXO operator key returned by the indexer.
func TestIncomingMetadataFromRPCOperatorKey(t *testing.T) {
Expand Down
71 changes: 68 additions & 3 deletions serverconn/connector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,6 @@ func TestIngress_ResponseDelivery(t *testing.T) {
)
}

// TestIngress_ResponseDispatchWithoutWaiter verifies that a KIND_RESPONSE
// envelope without an in-memory unary waiter falls back to the durable
// dispatcher map keyed by service and method.
// TestIngress_ResponseDispatchHeaderOnlyError verifies that routed response
// dispatch still reaches EventRouter handlers when the server encodes a gRPC
// error in headers and intentionally omits the response body.
Expand Down Expand Up @@ -414,6 +411,74 @@ func TestIngress_ResponseDispatchHeaderOnlyError(t *testing.T) {
}
}

// TestIngressAckHandledResponseWithoutActorDelivery verifies a routed response
// can be consumed by the route and acked without enqueueing an actor message.
func TestIngressAckHandledResponseWithoutActorDelivery(t *testing.T) {
t.Parallel()

mb := newInMemoryMailbox()
store := newMemCheckpointStore()
system := actor.NewActorSystem()

routeKey := actor.NewServiceKey[*helloStartedMsg, struct{}](
"greeting-actor",
)
router := NewEventRouter(system)
AddEnvelopeRoute(
router, EnvelopeRouteConfig[*helloStartedMsg, struct{}]{
Service: "test.Svc",
Method: "Unary",
NewEvent: func() proto.Message {
return &wrapperspb.StringValue{}
},
Key: routeKey,
Adapt: func(_ *mailboxpb.Envelope, _ proto.Message) (
*helloStartedMsg, error) {

return nil, ErrEnvelopeHandled
},
},
)

cfg := newTestConnectorConfig(mb, store)
cfg.Dispatchers = router.AsDispatcherMap()
cfg.RetryBaseDelay = 10 * time.Millisecond
cfg.RetryMaxDelay = 50 * time.Millisecond

connector := NewServerConnectionActor(cfg)

ctx, cancel := context.WithCancel(t.Context())
defer cancel()

require.NoError(t, connector.StartIngress(ctx))
defer connector.StopIngress()

body, err := anypb.New(wrapperspb.String("stale"))
require.NoError(t, err)

status := mb.send(&mailboxpb.Envelope{
ProtocolVersion: 1,
Sender: "server-1",
Recipient: "client-1",
Body: body,
Rpc: &mailboxpb.RpcMeta{
Kind: mailboxpb.RpcMeta_KIND_RESPONSE,
CorrelationId: "stale-corr",
Service: "test.Svc",
Method: "Unary",
ReplyTo: "server-1",
},
})
require.True(t, status.Ok, "send response failed: %s", status.Message)

require.Eventually(t, func() bool {
return mb.getAckedUpTo("client-1") > 0
}, 5*time.Second, 10*time.Millisecond)
}

// TestIngress_ResponseDispatchWithoutWaiter verifies that a KIND_RESPONSE
// envelope without an in-memory unary waiter falls back to the durable
// dispatcher map keyed by service and method.
func TestIngress_ResponseDispatchWithoutWaiter(t *testing.T) {
t.Parallel()

Expand Down
13 changes: 12 additions & 1 deletion serverconn/event_router.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package serverconn

import (
"context"
"errors"
"fmt"
"sync"

Expand All @@ -11,6 +12,11 @@ import (
"google.golang.org/protobuf/proto"
)

// ErrEnvelopeHandled lets an envelope route acknowledge an envelope without
// delivering an actor message. This is useful for shared RPC methods where a
// stale response can be identified as unrelated to the durable route.
var ErrEnvelopeHandled = errors.New("serverconn: envelope handled")

// InboundActorMessage is the type-constraint for actor messages that arrive
// from the server. It combines actor.Message (for dispatch via the actor
// system's Receptionist) with InboundServerMessage (for proto
Expand Down Expand Up @@ -145,7 +151,8 @@ type EnvelopeRouteConfig[M actor.Message, R any] struct {
Key actor.ServiceKey[M, R]

// Adapt converts the deserialized proto and envelope metadata into the
// actor message type M.
// actor message type M. Return ErrEnvelopeHandled when the envelope was
// intentionally consumed without actor delivery.
Adapt func(*mailboxpb.Envelope, proto.Message) (M, error)
}

Expand Down Expand Up @@ -207,6 +214,10 @@ func AddEnvelopeRoute[M actor.Message, R any](r *EventRouter,

actorMsg, err := cfg.Adapt(env, event)
if err != nil {
if errors.Is(err, ErrEnvelopeHandled) {
return nil
}

return fmt.Errorf("adapt %s/%s event: %w", cfg.Service,
cfg.Method, err)
}
Expand Down
56 changes: 54 additions & 2 deletions serverconn/event_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import (
mailboxrpc "github.com/lightninglabs/darepo-client/mailbox/rpc"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/anypb"
"google.golang.org/protobuf/types/known/wrapperspb"
)

// TestAddEnvelopeRoute_RejectsNilBodyWithoutEncodedError verifies that
// TestAddEnvelopeRouteRejectsNilBodyWithoutEncodedError verifies that
// AddEnvelopeRoute still fails closed on malformed routed responses that
// omit both the proto body and the encoded gRPC status headers.
func TestAddEnvelopeRoute_RejectsNilBodyWithoutEncodedError(t *testing.T) {
func TestAddEnvelopeRouteRejectsNilBodyWithoutEncodedError(t *testing.T) {
t.Parallel()

router := NewEventRouter(actor.NewActorSystem())
Expand Down Expand Up @@ -60,3 +61,54 @@ func TestAddEnvelopeRoute_RejectsNilBodyWithoutEncodedError(t *testing.T) {
)
require.False(t, adaptCalled)
}

// TestAddEnvelopeRouteCanMarkEnvelopeHandled verifies that routes can consume
// an envelope without forwarding a message to an actor mailbox.
func TestAddEnvelopeRouteCanMarkEnvelopeHandled(t *testing.T) {
t.Parallel()

router := NewEventRouter(actor.NewActorSystem())
routeKey := actor.NewServiceKey[*helloStartedMsg, struct{}](
"test-route",
)

adaptCalled := false

AddEnvelopeRoute(
router, EnvelopeRouteConfig[*helloStartedMsg, struct{}]{
Service: "test.Svc",
Method: "Unary",
NewEvent: func() proto.Message {
return &wrapperspb.StringValue{}
},
Key: routeKey,
Adapt: func(_ *mailboxpb.Envelope, _ proto.Message) (
*helloStartedMsg, error) {

adaptCalled = true

return nil, ErrEnvelopeHandled
},
},
)

dispatcher := router.AsDispatcherMap()[mailboxrpc.ServiceMethod{
Service: "test.Svc",
Method: "Unary",
}]

body, err := anypb.New(wrapperspb.String("stale"))
require.NoError(t, err)

err = dispatcher(t.Context(), &mailboxpb.Envelope{
Body: body,
Rpc: &mailboxpb.RpcMeta{
Kind: mailboxpb.RpcMeta_KIND_RESPONSE,
CorrelationId: "corr-1",
Service: "test.Svc",
Method: "Unary",
},
})
require.NoError(t, err)
require.True(t, adaptCalled)
}
Loading