serverconn: ack stale unary responses

[bhandras]

Summary

Fixes #543 by letting mailbox ingress move past stale ListVTXOsByScripts responses that are not correlated to a durable OOR incoming-metadata query.

A fresh local install can start mailbox ingress from cursor 0 while reusing the same client mailbox identity, especially in lnd mode where the identity derives from the same wallet key. If the remote mailbox still contains old unary responses, serverconn has no in-memory waiter for them and falls back to durable route dispatch by service/method.

The OOR metadata route handles arkrpc.IndexerService/ListVTXOsByScripts, but durable OOR metadata responses are the only responses on that route that should be delivered to the OOR actor. Those responses use the oor-incoming-metadata: correlation prefix. Stale normal unary responses do not, so treating them as OOR metadata was making adaptation fail and preventing cursor/ack advancement.

Changes

• Add serverconn.ErrEnvelopeHandled so an envelope route can consume an envelope without delivering an actor message.
• Add oor.IsIncomingMetadataCorrelationID to distinguish durable OOR metadata responses from ordinary ListVTXOsByScripts responses.
• Have the OOR metadata route mark non-OOR-prefixed responses handled so normal checkpoint/ack processing can advance.
• Add regression coverage for handled routed responses and OOR metadata correlation prefix matching.

Validation

• make fmt-changed
• make unit pkg=./serverconn
• make unit pkg=./oor
• make unit pkg=./darepod
• make lint-changed-local
• make commitmsg-lint range="origin/main..HEAD"
• git diff --check

[bhandras]

@claude review

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to safely acknowledge and discard stale or unrelated envelopes without delivering actor messages, preventing the ingress loop from stalling. It adds a new ErrEnvelopeHandled sentinel error, a helper function IsIncomingMetadataCorrelationID to identify durable incoming metadata queries, and corresponding unit tests. The reviewer recommends adding a debug log when discarding stale responses to improve observability, and points out that a similar stalling issue likely exists for the ListOORRecipientEventsByScript route, which should also be addressed.

[gemini-code-assist]

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
	}

[bhandras]

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.

[claude]

Claude finished @bhandras's task in 2m 14s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze correctness and design
• Post review findings

---

The approach is clean and well-scoped. The ErrEnvelopeHandled sentinel is the right abstraction — it lets a route consume an envelope without needing to reach into ingress internals. I traced the ack path end-to-end: when Adapt returns ErrEnvelopeHandled, the dispatcher returns nil, dispatchBatch falls through to the lastCommitted = env.EventSeq update at ingress.go:352, and the ack watermark advances correctly. The mechanism works.

One issue that should be addressed before merging:

ListOORRecipientEventsByScript has the same stalling bug

The Gemini reviewer is correct. The ListOORRecipientEventsByScript route at darepod/server.go:2529-2534 has the identical vulnerability:

sessionID, recipientEventID, err :=
    oor.ParseIncomingResolveCorrelationID(
        env.Rpc.CorrelationId,
    )
if err != nil {
    return nil, err  // propagates to dispatchBatch → stalls ingress
}

A stale normal ListOORRecipientEventsByScript response (without the oor-incoming-resolve: prefix) causes ParseIncomingResolveCorrelationID to fail with an error, which propagates to dispatchBatch at ingress.go:293-295 and stalls the ingress loop — exactly the same failure mode fixed for ListVTXOsByScripts.

The fix is symmetric: add IsIncomingResolveCorrelationID to oor/incoming_adapter.go using the same pattern as IsIncomingMetadataCorrelationID, then guard the route:

// oor/incoming_adapter.go
func IsIncomingResolveCorrelationID(correlationID string) bool {
    return len(correlationID) > len(incomingResolveCorrelationPrefix) &&
        correlationID[:len(incomingResolveCorrelationPrefix)] ==
            incomingResolveCorrelationPrefix
}

And in darepod/server.go before calling ParseIncomingResolveCorrelationID:

if !oor.IsIncomingResolveCorrelationID(env.Rpc.CorrelationId) {
    return nil, serverconn.ErrEnvelopeHandled
}

And refactor ParseIncomingResolveCorrelationID to use the new helper (same as ParseIncomingMetadataCorrelationID was refactored here).

Fix this →

---

Minor observations (non-blocking):

• Debug log on discard: The Gemini suggestion to add a debug log when silently discarding a stale response is reasonable for observability. A one-liner like s.log.DebugS(ctx, "Discarding stale non-OOR ListVTXOsByScripts response", "correlation_id", env.Rpc.CorrelationId) before the return nil, ErrEnvelopeHandled would help during troubleshooting without affecting behavior.

• Rename test fix is correct: Renaming TestAddEnvelopeRoute_RejectsNilBodyWithoutEncodedError → TestAddEnvelopeRouteRejectsNilBodyWithoutEncodedError fixes the underscore rule from CLAUDE.md critical rule multi: clean up unnecessary stuff and use instance logging in favor of global logger #7.

• IsIncomingMetadataCorrelationID uses > len(prefix) not >= len(prefix): This is intentional (and tested) — a correlation ID equal to just the prefix with no session suffix is rejected. Correct.

---

Summary: The ErrEnvelopeHandled mechanism and the ListVTXOsByScripts guard are correct and well-tested. The ListOORRecipientEventsByScript route needs the same treatment before this can ship — it's the same bug in the same code.
|

[bhandras]

@claude addressed both your finding and Gemini's in e5dd7ef5 (force-pushed). Changes since the last review:

1. ListOORRecipientEventsByScript stalling bug (your blocker) — fixed symmetrically with the ListVTXOsByScripts route:

   • Added oor.IsIncomingResolveCorrelationID in oor/incoming_adapter.go (same > len(prefix) pattern as IsIncomingMetadataCorrelationID).
   • Refactored ParseIncomingResolveCorrelationID to use it.
   • Guarded the route in darepod/server.go (~line 2547): non-oor-incoming-resolve:-prefixed responses now return serverconn.ErrEnvelopeHandled instead of erroring out of ParseIncomingResolveCorrelationID and stalling ingress.
   • Added TestIsIncomingResolveCorrelationID mirroring TestIsIncomingMetadataCorrelationID.

2. Debug log on discard (both reviewers) — both OOR routes now emit a DebugS breadcrumb (correlation_id/service/method) before returning ErrEnvelopeHandled, so a misclassification is traceable.

Validation: make fmt-changed, go build ./darepod/ ./oor/, make lint-changed-local (0 issues), make unit pkg=./oor, make unit pkg=./darepod — all green.

Please re-review.

[claude]

Claude finished @bhandras's task in 1m 35s —— View job

---

Code Review (re-review)

• Gather context
• Read changed files
• Verify IsIncomingResolveCorrelationID implementation
• Verify route guard in darepod/server.go
• Verify tests
• Verify debug logs
• Post review findings

---

All previous concerns are resolved. The fixes are symmetric, correct, and well-tested.

oor/incoming_adapter.go — IsIncomingResolveCorrelationID uses the exact same > len(prefix) guard as IsIncomingMetadataCorrelationID (rejects an ID equal to exactly the bare prefix). ParseIncomingResolveCorrelationID is correctly refactored to delegate to it, eliminating the direct prefix comparison.

darepod/server.go — Both routes are now guarded symmetrically:

• ListVTXOsByScripts (line ~2466): prefix guard → DebugS with correlation_id/service/method → ErrEnvelopeHandled
• ListOORRecipientEventsByScript (line ~2553): same pattern with IsIncomingResolveCorrelationID

Both nil-check env.Rpc before accessing CorrelationId, and the prefix guard comes after, so the ordering is safe. context.Background() in the log calls is the right choice since Adapt closures don't receive a context.

Tests — TestIsIncomingResolveCorrelationID mirrors TestIsIncomingMetadataCorrelationID exactly (valid ID, empty string, arbitrary string, bare-prefix string). TestIngressAckHandledResponseWithoutActorDelivery closes the integration loop: stale routed response → route returns ErrEnvelopeHandled → ingress ack watermark advances without actor delivery. TestAddEnvelopeRouteCanMarkEnvelopeHandled covers the event_router.go mechanic directly.

This PR is ready to merge.