Skip to content

mailbox+serverconn: durable response store and unified connector boundary - #116

Merged
Roasbeef merged 14 commits into
mainfrom
serverconn-mailbox
Feb 21, 2026
Merged

mailbox+serverconn: durable response store and unified connector boundary#116
Roasbeef merged 14 commits into
mainfrom
serverconn-mailbox

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

In this PR, we lay the groundwork for making the client's mailbox RPC
layer crash-safe by introducing a Store abstraction for response
persistence and expanding serverconn into the unified connector
boundary for all client↔server mailbox traffic.

mailbox/client: durable response store

We add a Store interface to mailbox/client that persists the pull
cursor and response payloads keyed by correlation ID. The client's main
run loop is then wired to use this store: on startup we load the
persisted cursor so pulls resume from where we left off, each inbound
response envelope is written to the store before notifying in-memory
waiters, and the cursor is persisted before acking with the remote
mailbox. AwaitRPC now does a two-tier lookup — first the in-memory
pending map, then the store — so responses survive a restart between
PutResponse and the caller's AwaitRPC.

A MemoryStore ships for tests and short-lived processes that don't
need crash safety. The Config.Store field defaults to it when unset,
so existing callers are unaffected.

serverconn: unified connector boundary

The serverconn package is expanded from its previous stub into the
single ingress/egress boundary for mailbox traffic. The core pieces:

Ack state machine (types.go): Four cursors — PullCursor,
DispatchCommittedTo, AckTarget, AckCommittedTo — enforce the
invariant that AckUpTo only advances after durable local dispatch.
TLV encode/decode lets the state persist via the durable actor
checkpoint mechanism.

TLV message types (actor.go): SendClientEventRequest wraps
outbound FSM events in anypb.Any for type-preserving TLV
serialization. SendRPCRequest marshals the full mailbox envelope for
the unary path. Both carry stable TLV type identifiers for the durable
actor codec. The actor struct gains a response registry, egress
handlers that build proper mailbox envelopes, and ingress lifecycle
management.

Ingress loop (ingress.go): A self-driven background goroutine
that long-polls the remote mailbox via Edge.Pull, routes
KIND_RESPONSE envelopes to the response registry for unary waiters,
routes KIND_REQUEST/KIND_EVENT envelopes through the dispatch table
to local durable actors, and advances the ack watermark only after
committed dispatch. Transient failures get exponential backoff w/
jitter.

Unary facade (unary_facade.go): Implements mailboxrpc.RPCClient
on top of the connector. SendRPC goes directly through Edge.Send
(no durable mailbox — caller retries on failure), while AwaitRPC
blocks on the response registry channel until the ingress loop delivers
a matching response.

Tests

21 tests across three files cover the ack state machine invariants,
full ingress loop integration against an in-memory mailbox edge
(dispatch, ack, checkpoint restart, shutdown), and the unary facade
round-trip including concurrent inflight isolation and context
cancellation. See each commit message for a detailed description w.r.t
the incremental changes.

In this commit, we introduce a Store abstraction that the mailbox RPC
client uses to persist response payloads and the pull cursor across
restarts. The interface defines five operations: LoadCursor, SaveCursor,
PutResponse, GetResponse, and DeleteResponse, with idempotency and
monotonicity contracts that match the cursor-based AckUpTo protocol.

A MemoryStore implementation is included for tests and short-lived
processes that don't need crash safety. The Config struct gains a Store
field that defaults to MemoryStore when unset, preserving backward
compatibility with existing callers.
With the Store interface in place, we now integrate it into the client's
main run loop to achieve crash-safe response handling under the
cursor-based AckUpTo protocol.

On startup the client loads the persisted cursor so it resumes pulling
from where it left off. Each inbound response envelope is written to the
Store before any in-memory waiter is notified, ensuring the payload
survives a crash between dispatch and consumption. The cursor is
persisted to the Store before acking with the remote mailbox, so a
restart replays only unacked envelopes rather than the entire stream.

AwaitRPC now peeks through the in-memory pending map into the Store,
and only deletes the response from both layers after successful proto
unmarshal. A best-effort retry loop handles transient delete failures
without blocking the caller. This two-tier lookup means responses are
available even if the process restarted between PutResponse and the
caller's AwaitRPC.

The malformed response body error is extracted to a package-level
format string so it can be matched in tests.
@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the reliability and functionality of the client-side mailbox communication. It introduces a durable storage mechanism for RPC responses, ensuring that client operations can gracefully recover from crashes without losing critical data. Concurrently, it refactors and expands the serverconn package to act as a unified, crash-safe gateway for all client-server interactions, managing both outgoing messages and incoming envelope processing with robust acknowledgment and dispatch logic. These changes collectively provide a more resilient and integrated communication layer for the client.

Highlights

  • Durable Response Store for Mailbox Client: Introduced a Store interface in mailbox/client to persist pull cursors and RPC response payloads, making the client's RPC layer crash-safe. The client now loads its cursor from the store on startup, writes inbound responses to the store before notifying waiters, and persists the cursor before acknowledging with the remote mailbox. A MemoryStore is provided for testing and non-durable use cases.
  • Unified Server Connection Connector (serverconn): Expanded the serverconn package into a comprehensive ingress/egress boundary for all client-server mailbox traffic. This includes a robust ack state machine, TLV message types for durable actor persistence, a self-driven ingress loop, and a unary facade for RPC client implementation.
  • Ack State Machine for At-Least-Once Delivery: Implemented a detailed AckState machine within serverconn that tracks PullCursor, DispatchCommittedTo, AckTarget, and AckCommittedTo. This ensures that remote acknowledgments (AckUpTo) only occur after envelopes have been durably committed to local actors, guaranteeing at-least-once delivery even across crashes.
  • Durable Actor Integration with TLV Serialization: Outbound FSM events (SendClientEventRequest) and unary RPC envelopes (SendRPCRequest) are now serialized using TLV (Type-Length-Value) encoding, wrapped in anypb.Any, and persisted by the durable actor runtime. This prevents message loss for egress traffic on client restarts.
  • Ingress Loop for Pulling and Dispatching: A background goroutine continuously long-polls the remote mailbox, routes KIND_RESPONSE envelopes to an in-memory response registry for unary RPC waiters, and dispatches KIND_REQUEST/KIND_EVENT envelopes to local durable actors via a configurable dispatch table. It also manages the ack watermark and incorporates exponential backoff with jitter for transient failures.
  • Unary RPC Facade: A UnaryFacade implements the mailboxrpc.RPCClient interface, allowing generated RPC stubs to use the connector. SendRPC directly sends envelopes via the mailbox edge, while AwaitRPC registers a waiter with the connector's response registry, blocking until the ingress loop delivers the matching response.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • mailbox/client/client.go
    • Added errMalformedResponseBody constant for clearer error messages.
    • Introduced ackTo field to track the highest acknowledged cursor.
    • Modified New function to load initial cursor from the new Store interface.
    • Updated AwaitRPC to first peek for responses in memory, then from the Store, and to delete responses from both after successful unmarshaling.
    • Modified applyDefaults to initialize cfg.Store with a NewMemoryStore if unset.
    • Integrated ackTo logic into the run loop for processing pending acknowledgments.
    • Refactored run loop's envelope handling to call handleEnvelope with context and handle potential errors, and updated cursor storage and acknowledgment logic.
    • Changed handleEnvelope signature to accept context.Context and return an error, and added logic to persist response payloads to the Store.
    • Removed redundant payloadCopy creation within handleEnvelope as it's now handled by the Store.
    • Renamed popPending to peekPending and added deletePending to separate peeking from deletion.
    • Added peekResponse to check for responses in both in-memory pending map and the Store.
    • Implemented deleteResponseBestEffort for resilient deletion of responses from the Store.
    • Added loadAckTo, setAckTo, and clearAckTo methods for managing the ackTo cursor.
    • Updated storeCursor to persist the cursor to the Store before updating the in-memory field.
  • mailbox/client/config.go
    • Added Store interface field to the Config struct, allowing for configurable persistence of response payloads and pull cursor state.
  • mailbox/client/doc.go
    • Updated package documentation to describe the new Store interface, its role in crash-safety, and the persistence of pull cursors and response payloads.
  • mailbox/client/store.go
    • Added Store interface defining methods for loading/saving cursors and putting/getting/deleting responses.
    • Implemented MemoryStore, an in-memory, non-crash-safe implementation of the Store interface for testing and short-lived processes.
  • serverconn/ack_state_test.go
    • Added unit tests for AckState.AdvanceDispatch to verify monotonic advancement of dispatch and ack targets.
    • Added unit tests for AckState.AdvanceAck to verify correct updates to AckCommittedTo and PullCursor.
    • Added unit tests for AckState.NeedsAck to confirm accurate reporting of pending acknowledgments.
    • Added TestAckState_FullCycle to verify a complete dispatch-ack workflow.
    • Added TestAckState_EncodeDecode and TestAckState_EncodeDecode_Zero to ensure TLV serialization and deserialization of AckState works correctly.
  • serverconn/actor.go
    • Added TLV type constants (SendClientEventRequestMsgType, SendRPCRequestMsgType, protoPayloadRecordType, envelopeRecordType) for message serialization.
    • Introduced rawServerMessage struct to wrap anypb.Any for lazy protobuf deserialization.
    • Modified ServerConnMsg interface to extend actor.TLVMessage for durable actor persistence.
    • Added TLVType, Encode, and Decode methods to SendClientEventRequest for TLV serialization.
    • Updated SendClientEventRequest to send messages via the mailbox edge, wrapping the proto message in anypb.Any.
    • Introduced SendRPCRequest message type for outbound unary RPC envelopes, including TLVType, Encode, and Decode methods.
    • Updated ServerConnectionActor struct to include ConnectorConfig, responseRegistry (with mutex), cancel function, and sync.WaitGroup for ingress loop management.
    • Modified NewServerConnectionActor to accept ConnectorConfig and initialize the response registry.
    • Added handleSendRPCRequest to process SendRPCRequest messages by sending the envelope via the mailbox edge.
    • Implemented RegisterWaiter to add a response waiter for a given correlation ID.
    • Implemented removeWaiter to clean up registered waiters.
    • Implemented deliverResponse to look up and deliver response envelopes to registered waiters.
    • Added StartIngress and StopIngress methods to manage the background ingress loop goroutine.
    • Added NewServerConnCodec to create a message codec with registered server connection message types.
  • serverconn/connector_test.go
    • Added newTestConnector helper function to create ServerConnectionActor instances with in-memory test dependencies.
    • Added sendResponseToMailbox and sendEventToMailbox helpers to inject envelopes into the in-memory mailbox for testing.
    • Implemented TestIngress_DispatchAndAck to verify the ingress loop's pull, dispatch, and ack functionality.
    • Implemented TestIngress_ResponseDelivery to confirm that KIND_RESPONSE envelopes are correctly delivered to registered waiters.
    • Implemented TestIngress_NoAckOnDispatchFailure to ensure the ack watermark does not advance on dispatch failures.
    • Implemented TestIngress_Shutdown_NoGoroutineLeak to verify clean termination of the ingress loop goroutine.
    • Implemented TestIngress_CheckpointSurvivesRestart to test the persistence and loading of the ack state checkpoint.
    • Added TestRetryDelay and TestRetryDelay_DefaultsOnZero to verify the exponential backoff logic with jitter.
  • serverconn/doc.go
    • Added comprehensive package documentation detailing the serverconn as a unified connector boundary, covering egress, ingress, ack watermark invariants, dispatch table, and unary facade.
  • serverconn/ingress.go
    • Added ingressLoop function, the core of the pull-dispatch-ack mechanism, including checkpoint loading, envelope pulling, dispatching, acking, and error handling with backoff.
    • Implemented pullBatch to fetch envelopes from the mailbox edge with long-polling.
    • Implemented dispatchBatch to route envelopes based on their RPC kind to either the response registry or local actors, handling partial failures.
    • Implemented ackRemote to send AckUpTo requests to the mailbox edge.
    • Implemented loadCheckpoint and saveCheckpoint for persisting and restoring AckState.
    • Implemented sleepBackoff for exponential backoff with jitter on transient failures.
    • Implemented retryDelay function for calculating backoff durations.
    • Defined statusError struct for wrapping mailbox status failures.
  • serverconn/log.go
    • Added logging setup for the serverconn package, including Subsystem constant, log variable, DisableLog, and UseLogger functions.
  • serverconn/testutil_test.go
    • Added inMemoryMailbox struct, a mock implementation of MailboxService for testing, including send, pull, ackUpTo, and getAckedUpTo methods.
    • Added fakeMailboxServiceClient to adapt inMemoryMailbox to the MailboxServiceClient interface.
    • Added memCheckpointStore struct, a mock in-memory implementation of actor.DeliveryStore for checkpointing, with SaveCheckpoint and LoadCheckpoint methods.
  • serverconn/types.go
    • Defined CorrelationID and IdempotencyKey types for clarity.
    • Defined ackStateType constant for checkpointing.
    • Defined TLV record type constants for AckState serialization.
    • Introduced AckState struct to track pull, dispatch, and ack cursors, with AdvanceDispatch, AdvanceAck, NeedsAck, Encode, and Decode methods.
    • Defined EnvelopeDispatcher functional type for routing inbound envelopes.
    • Defined ResponseWaiter struct for managing unary RPC response channels.
    • Introduced ConnectorConfig struct to consolidate all configuration parameters for the ServerConnectionActor.
    • Provided DefaultConnectorConfig with sensible defaults for polling and retry behavior.
  • serverconn/unary_facade.go
    • Defined errMalformedResponseBody constant.
    • Introduced UnaryFacade struct to implement mailboxrpc.RPCClient using the ServerConnectionActor.
    • Implemented NewUnaryFacade to create new instances.
    • Implemented SendRPC to construct and send RPC request envelopes via the mailbox edge, generating correlation and idempotency IDs.
    • Implemented AwaitRPC to register a waiter with the connector and block until a matching response is delivered or the context is cancelled.
    • Added randomID helper function for generating cryptographically random hex-encoded identifiers.
  • serverconn/unary_facade_test.go
    • Implemented TestUnaryFacade_SendRPC to verify RPC request envelope construction and sending.
    • Implemented TestUnaryFacade_SendRPC_ExplicitOptions to ensure custom correlation and idempotency IDs are respected.
    • Implemented TestUnaryFacade_AwaitRPC to test the full send-await round trip with response delivery.
    • Implemented TestUnaryFacade_AwaitRPC_CancelledContext to verify correct handling of context cancellation during AwaitRPC.
    • Implemented TestUnaryFacade_ConcurrentInflight to test concurrent RPC send/await operations.
    • Added TestUnaryFacade_RPCClientInterface for compile-time interface compliance check.
    • Implemented TestUnaryFacade_AwaitRPC_NilBody to verify error handling when a response envelope has a nil body.
Activity
  • No specific human activity (comments, reviews) was provided in the context for this pull request.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant changes to the mailbox client and server connection components, primarily focusing on crash-safe operation and improved RPC handling. Key changes include the addition of a Store interface and MemoryStore implementation for persisting pull cursors and response payloads, ensuring at-least-once delivery even across restarts. The Client struct in mailbox/client/client.go was refactored to utilize this new store, incorporating logic for loading/saving cursors and persisting/retrieving RPC responses. A new ackTo field and associated methods were added to manage the acknowledgment watermark. The AwaitRPC method was updated to use peekResponse and deleteResponseBestEffort for more robust response handling and cleanup. Concurrently, a new serverconn package was introduced, defining a ServerConnectionActor that acts as a unified connector for client-server mailbox traffic. This actor includes an ingress loop for pulling, dispatching, and acknowledging envelopes, and an egress path for sending FSM events and RPC requests. It uses a DeliveryStore for checkpointing its AckState to ensure durability. The ServerConnectionActor also manages an in-memory responseRegistry for delivering unary RPC responses. A UnaryFacade was added to implement the mailboxrpc.RPCClient interface, leveraging the ServerConnectionActor for sending and awaiting RPCs. The review comment highlights an inconsistency in logging, noting that serverconn/log.go initializes a btclog.Logger while other files in the package use log/slog calls, suggesting a conversion to log/slog for consistency and to resolve compilation errors.

Comment thread serverconn/log.go
Comment on lines +1 to +24
package serverconn

import "github.com/btcsuite/btclog/v2"

// Subsystem defines the logging code for this subsystem.
const Subsystem = "SVRC"

// log is a logger that is initialized with no output filters. This means the
// package will not perform any logging by default until the caller requests
// it.
var log = btclog.Disabled

// DisableLog disables all library log output. Logging output is disabled by
// default until UseLogger is called.
func DisableLog() {
UseLogger(btclog.Disabled)
}

// UseLogger uses a specified Logger to output package logging info. This
// should be used in preference to SetLogWriter if the caller is also using
// btclog.
func UseLogger(logger btclog.Logger) {
log = logger
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The serverconn package appears to have inconsistent logging setups. This file initializes a btclog.Logger, but other files in the package (ingress.go, actor.go, unary_facade.go) make calls assuming a log/slog logger (e.g., log.InfoS). This will lead to compilation errors because btclog.Logger does not have an InfoS method.

To ensure consistency and fix the compilation issue, this logging setup should be converted to use log/slog, which seems to be the intended logging library for new code in this repository.

package serverconn

import (
	"io"
	"log/slog"
)

// Subsystem defines the logging code for this subsystem.
const Subsystem = "SVRC"

// log is a logger that is initialized with no output filters. This means the
// package will not perform any logging by default until the caller requests
// it.
var log = slog.New(slog.NewTextHandler(io.Discard, nil))

// DisableLog disables all library log output. Logging output is disabled by
// default until UseLogger is called.
func DisableLog() {
	UseLogger(slog.New(slog.NewTextHandler(io.Discard, nil)))
}

// UseLogger uses a specified Logger to output package logging info.
func UseLogger(logger *slog.Logger) {
	log = logger
}

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34b21f7bc9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread serverconn/ingress.go Outdated
Comment on lines +123 to +125
if committedCursor > state.PullCursor {
state.AdvanceDispatch(committedCursor)
state.PullCursor = committedCursor

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Advance pull cursor past committed envelope on partial failure

dispatchBatch returns committedCursor as the last successfully processed event_seq, but on partial failure the loop stores that value directly in state.PullCursor. Since the pull cursor is the next expected sequence, resuming from committedCursor re-pulls the last already-dispatched envelope and can dispatch it twice before ack catches up, which violates the intended “don’t re-dispatch already committed envelopes” behavior and can duplicate downstream side effects.

Useful? React with 👍 / 👎.

Comment thread serverconn/ingress.go Outdated
Comment on lines +231 to +233
if !delivered {
log.DebugS(ctx,
"No waiter for response, dropping",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Buffer unmatched responses instead of dropping them

When deliverResponse cannot find a waiter, the response is explicitly dropped; the loop still advances/acks the cursor later, so that response is permanently lost. In the normal generated client flow (SendRPC then AwaitRPC), a fast response can be pulled before AwaitRPC registers its waiter, which leads to intermittent timeouts even though the server already replied successfully.

Useful? React with 👍 / 👎.

Comment thread serverconn/actor.go
return err
}

if _, err := stream.DecodeWithParsedTypes(r); err != nil {

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.

Could just store pure protos here and bypass the TLV round trip since we send protos in the end.

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.

Ah we require TLV serialization rn with the package. So let's do the composite generic type thing mentioned below.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 19, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 5m 22s —— View job


PR Review: mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand the PR
  • Read key source files
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is a well-structured PR with good test coverage. The ack state machine design is sound and the ResponseRegistry buffering to handle the fast-response-before-waiter race is correct. Below are the issues I found, ordered by severity.


1. Pull cursor off-by-one on partial dispatch failure (correctness bug)

serverconn/ingress.go:123-125

if committedCursor > state.PullCursor {
    state.AdvanceDispatch(committedCursor)
    state.PullCursor = committedCursor   // ← sets to event_seq, not event_seq+1

On the success path, dispatchBatch returns batchNextCursor = maxSeq + 1, so PullCursor correctly points one past the last processed sequence. On the partial failure path, dispatchBatch returns lastCommitted = env.EventSeq of the last successfully dispatched envelope — an inclusive position. Assigning this directly to PullCursor causes the next Pull call (with cursor = event_seq) to include that envelope again (event_seq >= cursor is the filter), re-dispatching it on the next loop iteration.

The comment on line 120 says "so we don't re-dispatch already committed envelopes" — but the implementation does exactly that.

At-most-once dispatch is rescued by EnqueueMessage's ON CONFLICT DO NOTHING dedup in the actor store (so no duplicate processing in practice), but the invariant stated in the code is violated, AckUpTo is called with an inclusive event_seq instead of an exclusive cursor (so GC on the remote side is delayed), and every backoff cycle re-does work unnecessarily.

Fix: use committedCursor + 1 (guarded on > 0) in the partial failure branch, consistent with how batchNextCursor is used on the success path:

if committedCursor > 0 {
    nextCursor := committedCursor + 1
    if nextCursor > state.PullCursor {
        state.AdvanceDispatch(nextCursor)
        state.PullCursor = nextCursor
        ...
    }
}

Fix this →


2. Misleading "No waiter for response, dropping" log message

serverconn/ingress.go:231-237

delivered := a.deliverResponse(corrID, env)
if !delivered {
    log.DebugS(ctx, "No waiter for response, dropping", ...)
}

DeliverResponse in response_registry.go returns false only when proto.Clone(env) fails (the type assertion). When there is no waiter, it buffers the response in pending and returns true. So this log fires on a clone failure — not a "no waiter" condition — and responses are actually not dropped when there is no waiter. The message will actively mislead during debugging.


3. Stale-waiter prune can permanently strand a blocked AwaitRPC

mailbox/conn/response_registry.go:137-153

pruneStaleLocked evicts waiters from the waiters map after waiterTTL (10 min default). However, goroutines blocked in AwaitRPC hold the channel reference directly; they are not notified of the eviction. If a slow response arrives after the eviction, DeliverResponse finds no waiter and buffers the response in pending — but AwaitRPC is still blocking on the channel and never receives it. The goroutine will hang until context cancellation, silently losing the delivery.

A lightweight mitigation: when pruning a waiter, close its channel so blocked goroutines wake up and return an appropriate error. Alternatively, DeliverResponse could detect that the evicted-but-channel-live case and still send on the channel.


4. handleSendRPCRequest returns the wrong response type

serverconn/actor.go:442

return fn.Ok[ServerConnResp](&SendClientEventResponse{Success: true})

handleSendRPCRequest returns a SendClientEventResponse, not a conceptually distinct SendRPCResponse. This works today because both implement ServerConnResp, but any future caller type-switching on the response (e.g., to extract a per-RPC field) would silently match the wrong branch.


5. Double-wrapping in handleSendClientEvent (Roasbeef's comment at actor.go:180)

During Encode, the proto is wrapped in anypb.Any for TLV storage. On replay, rawServerMessage.ToProto() unmarshals the Any back to the original proto. Then handleSendClientEvent wraps it in Any again for the wire envelope. This double-wrap round-trip works correctly but:

  • Serialises the proto twice on every retry path.
  • rawServerMessage.ToProto() silently returns nil on unmarshal failure (line 60-65 in actor.go), which propagates into anypb.New(nil) in handleSendClientEvent and returns an error only then — obscuring the actual failure site.

Storing the raw proto bytes directly in TLV (and unmarshalling once at use time) would remove the intermediate anypb.Any layer and make the error surface cleaner, as Roasbeef noted.


6. Gemini's log.go comment is incorrect

Gemini flagged serverconn/log.go as causing compilation errors because "btclog.Logger does not have an InfoS method". This is wrong — btclog/v2.Logger has full structured logging methods (InfoS, WarnS, DebugS, etc.). The existing logging setup is correct and consistent with the project's other packages.


Minor observations

  • dispatchBatch doesn't advance lastCommitted for "no dispatcher" skips (line 265 continue). On a successful batch that contains unknown service/method envelopes, batchNextCursor covers them via the post-loop max(). On partial failure after such skips, lastCommitted reflects only dispatched envelopes, which is correct semantically — just worth a comment.

  • AwaitRPC registers the waiter a second time (line 158) after SendRPC already pre-registered it. RegisterWaiter is idempotent and returns the same channel, so this is correct, but a comment explaining that this is intentional (to cover the case where AwaitRPC is called independently without a prior SendRPC) would reduce reader confusion.

  • dispatchBatch signature comment says it returns "the cursor through which all dispatches succeeded" — the word "cursor" implies the exclusive next-position, but on the error path it returns an inclusive event_seq. Aligning the terminology would prevent the confusion that led to bug lib: start adding lib helpers #1.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 19, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 7m 2s —— View job


PR Review: mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files
  • Analyze for bugs, correctness, and design issues
  • Post final review

This PR significantly restructures the mailbox client layer: mailbox/client is entirely removed and its responsibilities split between the new mailbox/conn primitives package and the expanded serverconn connector. The previous review's reported bugs have been addressed in the current code. Below is the analysis of what remains.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixedingress.go:127-129 now adds +1 and guards committedCursor > 0 with a clear comment
Misleading "No waiter for response, dropping" log FixedDeliverResponse now buffers; log only fires on proto.Clone failure
Stale-waiter hangs blocked AwaitRPC FixedpruneStaleLocked completes expired promises with ErrWaiterExpired so callers wake
handleSendRPCRequest returns wrong response type Fixed — now returns SendRPCResponse
Double-wrap anypb.Any on replay Open — Roasbeef acknowledged at actor.go:192; see below

1. rawServerMessage.ToProto() silently returns nil — obscures replay failures

serverconn/actor.go:59-65

func (m *rawServerMessage) ToProto() proto.Message {
    msg, err := m.anyMsg.UnmarshalNew()
    if err != nil {
        return nil   // ← error silently discarded
    }
    return msg
}

When UnmarshalNew fails (e.g., a proto type URL that was removed from the binary's registry), ToProto returns nil. This propagates to handleSendClientEvent:

protoMsg := req.Message.ToProto()       // nil if type unknown
body, err := anypb.New(protoMsg)        // returns "invalid nil message" error

The handler returns fmt.Errorf("wrap proto in Any: %w", err), which the durable actor retries — permanently, since the type URL won't suddenly appear in the registry. The actual root cause (stale type URL) is invisible in the error chain.

Adding a logged error before the return nil and surfacing the cause through the ToProto signature (or returning an explicit sentinel error type) would make these replay failures diagnosable.

Fix this →


2. handleSendClientEvent does unnecessary marshal work on replay path

serverconn/actor.go:374-403

On the replay path, req.MsgID and req.IdempotencyKey are both set by Decode from the persisted TLV record. However, the handler unconditionally:

  1. Calls rawServerMessage.ToProto() → unmarshals anypb.Any → proto
  2. Calls anypb.New(protoMsg) → re-wraps proto → anypb.Any
  3. Marshals the Any to bodyBytes (only needed for stable ID derivation)

The bodyBytes computation at step 3 is only used in:

if msgID == "" {
    msgID = mailboxconn.StableEventMsgID(bodyBytes)
}

Since both IDs are set on replay, bodyBytes is computed and immediately discarded. The double-wrap (steps 1–2) is also redundant on replay, though it's needed to produce body for the envelope. The fix Roasbeef noted — storing the raw proto bytes in TLV rather than anypb.Any — would eliminate this entirely. At minimum, the bodyBytes marshal can be guarded:

if msgID == "" || idempotencyKey == "" {
    // Only compute bodyBytes when IDs must be derived.
    bodyBytes, err = marshalAny(body)
    ...
}

3. Data race on a.cancel between StartIngress and StopIngress

serverconn/actor.go:497-511

func (a *ServerConnectionActor) StartIngress(ctx context.Context) {
    ingressCtx, cancel := context.WithCancel(ctx)
    a.cancel = cancel   // ← unsynchronized write
    a.wg.Add(1)
    go a.ingressLoop(ingressCtx)
}

func (a *ServerConnectionActor) StopIngress() {
    if a.cancel != nil {   // ← unsynchronized read
        a.cancel()
    }
    a.wg.Wait()
}

a.cancel is accessed without a mutex. Runtime.Start/Stop calls these sequentially so the race doesn't fire in the current integration, but adding protection now prevents a future caller from introducing it. A sync.Mutex or atomic.Pointer[context.CancelFunc] would suffice.

Fix this →


4. WrappedProto.Record() uses placeholder TLV type 0 — potential footgun

mailbox/conn/proto_record.go:13,41

var protoRecordType tlv.Type = 0   // placeholder

func (w *WrappedProto[T]) Record() tlv.Record {
    return tlv.MakeDynamicRecord(
        protoRecordType, w, sizeFunc, ...)
}

protoRecordType = 0 works because all callers go through tlv.NewRecordT[T] which overrides the type. But calling WrappedProto.Record() directly yields a TLV record with type 0, which collides with any other record at type 0 and is not caught at compile time.

A doc comment on Record() stating the precondition ("must be called via tlv.NewRecordT, not directly") would prevent this footgun.


5. AwaitRPC second RegisterWaiter call — correct but could use a comment

serverconn/unary_facade.go:159

SendRPC pre-registers a waiter at line 80 to close the race where a fast response arrives before AwaitRPC. AwaitRPC then calls RegisterWaiter again at line 159. Since RegisterWaiter is idempotent (returns the existing waiter if already present), this is correct. However, without a comment, a reader may wonder why both SendRPC and AwaitRPC register, or whether the second call clobbers the first.

Adding a comment — e.g., "idempotent re-registration: if SendRPC already reserved a waiter, this returns the same future" — would clarify the design intent.


6. Clarification on Gemini's btclog.Logger comment

Gemini's review flagged serverconn/log.go as a compilation error because "btclog.Logger does not have an InfoS method". This is incorrect — btclog/v2.Logger includes structured logging methods (InfoS, WarnS, DebugS, TraceS, etc.) as part of its interface. The logging setup is valid and consistent with other packages in the project. No action needed.


Minor observations

  • dispatchBatch lastCommitted starting at 0: Conflates "nothing dispatched" with "dispatched up to seq 0". The guard committedCursor > 0 && in ingressLoop handles this correctly, but a comment on dispatchBatch's return value contract ("0 means nothing was committed") would prevent future callers from misinterpreting seq 0 as a valid cursor.

  • AwaitRPC skips type URL validation: env.Body.Value is unmarshaled directly into resp without checking env.Body.TypeUrl. The DiscardUnknown option provides forward compatibility but a type mismatch would yield silently garbled data rather than an error. This is a documented trade-off in the facade design, but worth capturing in a comment.

  • DeliverResponse leaves completed waiters in r.waiters: By design — AwaitRPC's defer removeWaiter provides cleanup. The sync.Once-based actor.Promise ensures the second Complete(ErrWaiterCancelled) call from RemoveWaiter is a no-op, so the pattern is safe. Worth a short comment on DeliverResponse noting that cleanup is the caller's responsibility.

@Roasbeef
Roasbeef force-pushed the serverconn-mailbox branch 2 times, most recently from 155a0c6 to 744bf65 Compare February 19, 2026 04:51
@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 20, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 4m 38s —— View job


PR Review: mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is the third review pass. Most of the previously reported issues have been fixed. Below is the current state of the open issues, followed by new findings.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixedingress.go:127-129 adds +1 and guards committedCursor > 0
Misleading "No waiter, dropping" log Fixed — log only fires on proto.Clone failure
Stale-waiter hangs blocked AwaitRPC FixedpruneStaleLocked completes with ErrWaiterExpired
handleSendRPCRequest returns wrong response type Fixed — returns SendRPCResponse
rawServerMessage.ToProto() silently returns nil Still openactor.go:59-65
Unnecessary bodyBytes marshal on replay Still openactor.go:386-404
Data race on a.cancel Still openactor.go:497-511
WrappedProto.Record() placeholder type 0 missing precondition comment Still openproto_record.go:13
AwaitRPC second RegisterWaiter missing comment Still openunary_facade.go:159

1. NewRuntime does not validate cfg.Edge (new)

serverconn/runtime.go:29-44

NewRuntime validates cfg.Store and cfg.LocalMailboxID with clean errors, but skips cfg.Edge and cfg.RemoteMailboxID. A nil Edge defers to a nil-pointer panic on the first pullBatch call inside the goroutine launched by Start. A zero-length RemoteMailboxID silently addresses outbound envelopes to the empty string.

The validation block should include:

if cfg.Edge == nil {
    return nil, fmt.Errorf("connector edge is required")
}
if cfg.RemoteMailboxID == "" {
    return nil, fmt.Errorf("remote mailbox id is required")
}

Fix this →


2. Checkpoint load failure silently starts from cursor 0 (new)

serverconn/ingress.go:34-39

state, err := a.loadCheckpoint(ctx)
if err != nil {
    log.WarnS(ctx, "Failed to load checkpoint, starting fresh", err, ...)
}
// state is zero-value → PullCursor = 0

On a transient store error (connection blip, temporary I/O failure), the ingress loop silently falls back to pulling from cursor 0. In a long-running system this re-processes every envelope the remote mailbox still holds, triggering duplicate EnqueueMessage calls on every durable actor the mailbox has ever delivered to. The dedup logic in EnqueueMessage (ON CONFLICT DO NOTHING in production) makes re-dispatch safe but the thundering-herd re-pull is expensive.

A better policy is to retry the checkpoint load (with backoff) before giving up and falling back to 0. Alternatively, if a non-nil error means the store is unhealthy, returning from ingressLoop immediately (and letting the supervisor restart) is safer than continuing with stale state.


3. SendRPC discards RegisterWaiter return value without explanation (new)

serverconn/unary_facade.go:80

f.connector.RegisterWaiter(corrID)  // return value discarded

This is the pre-registration to close the fast-response race. Without a comment, the discarded return value looks like dead code — a reader may assume the waiter isn't registered and wonder why AwaitRPC registers again on line 159. Adding a short comment (e.g., "pre-register to close the window between Send and AwaitRPC; return value not needed here") eliminates the confusion.


4. rawServerMessage.ToProto() silent nil — still open

serverconn/actor.go:59-65

This was flagged in review #2 and remains unchanged. When anyMsg.UnmarshalNew() fails (e.g., a type URL removed from the registry after a binary upgrade), ToProto() returns nil without logging the failure. The nil propagates to anypb.New(nil) in handleSendClientEvent, which returns "proto: the passed message is nil" — obscuring the real root cause.

At minimum, log at error level before returning nil:

func (m *rawServerMessage) ToProto() proto.Message {
    msg, err := m.anyMsg.UnmarshalNew()
    if err != nil {
        log.ErrorS(context.Background(), "Failed to unmarshal Any",
            err,
            slog.String("type_url", m.anyMsg.GetTypeUrl()))
        return nil
    }
    return msg
}

Fix this →


5. Data race on a.cancel — still open

serverconn/actor.go:497-511

StartIngress writes a.cancel and StopIngress reads it without synchronization. Sequential Start/Stop calls in production are safe today, but the race detector will flag any concurrent access. A sync.Mutex (or promoting cancel to atomic.Pointer[context.CancelFunc]) would close the window.

Fix this →


Minor observations

  • Unnecessary bodyBytes on replay (actor.go:386-404): When both req.MsgID and req.IdempotencyKey are non-empty (replay path), body is still marshaled to bodyBytes which is then immediately discarded (both ID guards short-circuit). Gating the marshal on msgID == "" || idempotencyKey == "" would remove the redundant work.

  • WrappedProto.Record() placeholder type (proto_record.go:13): protoRecordType = 0 with no doc comment. Calling Record() directly yields type 0, which silently conflicts with any other type-0 record. A comment stating "must be called via tlv.NewRecordT, not directly" would prevent misuse.

  • AwaitRPC second RegisterWaiter (unary_facade.go:159): Correct (idempotent), but a one-line comment explaining this is an intentional re-registration (not a clobber) would help the next reader.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 20, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 4m 38s —— View job


PR Review: mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is the fourth review pass. Most of the previously-flagged issues have been addressed. Below is the current status of open items, followed by new findings.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixed
Misleading "No waiter, dropping" log Fixed
Stale-waiter hangs blocked AwaitRPC Fixed
handleSendRPCRequest returns wrong response type Fixed
rawServerMessage.ToProto() silently returns nil Fixed — now logs at WarnS (see note #1 below)
Unnecessary bodyBytes marshal on replay Fixed — guarded with if msgID == "" || idempotencyKey == ""
Data race on a.cancel Partially addressedstopOnce prevents double-cancel but field-level race remains (see #1 below)
WrappedProto.Record() missing precondition comment Fixed
AwaitRPC second RegisterWaiter missing comment Fixed
NewRuntime missing cfg.Edge/cfg.RemoteMailboxID validation Fixed
Checkpoint load failure silent fallback to cursor 0 FixedStartIngress now returns error and aborts
SendRPC discards RegisterWaiter return without comment Fixed

1. Data race on a.cancel — still present

serverconn/actor.go:524-525, 535-540

stopOnce was added to prevent double-cancel, but the write-to-read visibility of a.cancel between StartIngress and StopIngress is still unsynchronized:

// StartIngress (line 524-525):
ingressCtx, cancel := context.WithCancel(ctx)
a.cancel = cancel   // ← unsynchronized write

// StopIngress (line 535-540):
a.stopOnce.Do(func() {
    if a.cancel != nil {   // ← unsynchronized read
        a.cancel()
    }
})

sync.Once ensures the inner function runs once, but it does not provide a happens-before guarantee between StartIngress's write and StopIngress's read — that requires the caller to sequence them without concurrent overlap, which the type doesn't enforce. The race detector will flag this if both methods are called from separate goroutines close in time. An atomic.Pointer[context.CancelFunc] or a sync.Mutex on the cancel field would close the gap.

Fix this →


2. rawServerMessage.ToProto() logs at WarnS for an unrecoverable condition

serverconn/actor.go:63

When anyMsg.UnmarshalNew() fails due to an unknown type URL (e.g., after a binary upgrade removes a proto type), the actor retries handleSendClientEvent indefinitely since the error propagates as a retryable actor error. The log is at WarnS, which in most monitoring setups won't page on-call. Since no binary restart will resolve an unknown type URL without a code change, this is effectively an operator-action-required condition and should be logged at ErrorS.


3. ConnectorConfig.ActorSystem is dead code

serverconn/types.go:69-71

ConnectorConfig.ActorSystem is declared and documented but never read in any production code (actor.go, ingress.go, unary_facade.go, runtime.go). Dispatchers are pre-built closures with captured actor references, so the ActorSystem is not needed at runtime.

The DefaultConnectorConfig doc comment compounds this by saying:

"The caller must still set Edge, mailbox IDs, Store, Codec, and ActorSystem."

This is doubly misleading: ActorSystem is never used, and Codec is auto-filled in NewRuntime (cfg.Codec = NewServerConnCodec() when nil). The field should be removed if not needed, or the doc should clarify it is reserved for future use and currently unused.

Fix this →


4. Checkpoint save failure after successful ack silently resets failCount

serverconn/ingress.go:68-76

After a successful ackRemote, failCount is unconditionally reset to 0 even if the subsequent saveCheckpoint fails:

state.AdvanceAck()
if err := a.saveCheckpoint(ctx, state); err != nil {
    log.WarnS(ctx, "Failed to save checkpoint after ack", err)
    // failCount not incremented — reset happens below
}
failCount = 0   // ← resets even when checkpoint failed

If the checkpoint store is persistently unavailable (e.g., a disconnected DB), the loop resets failCount to 0 after every successful ack iteration and immediately pulls another batch at full speed, generating a checkpoint failure warning on every cycle. The backoff (sleepBackoff) is only applied on ackRemote or pullBatch failures, not on checkpoint failures. This could produce high-frequency log spam and unnecessary load while in a degraded state.

A straightforward mitigation: if saveCheckpoint returns an error after ack, increment failCount instead of resetting it, so the backoff also applies to persistent checkpoint failures.


5. AwaitRPC unmarshals without TypeUrl validation (carry-forward)

serverconn/unary_facade.go:190-194

err := (proto.UnmarshalOptions{
    DiscardUnknown: true,
}).Unmarshal(env.Body.Value, resp)

env.Body.Value is the raw bytes from anypb.Any. The TypeUrl field is never checked against the expected type. A server-side bug that returns the wrong proto type in the response body would produce silently garbled output rather than an error. Adding a log line with env.Body.TypeUrl on success (or a type-check assertion) would help diagnose response type mismatches.


Minor observations

  • dispatchBatch response clone failure advances lastCommitted: When proto.Clone(env) fails in DeliverResponse (line 145-148 of response_registry.go), deliverResponse returns false and the envelope is not buffered. The caller in dispatchBatch still advances lastCommitted for the response envelope (line 294-296 of ingress.go), meaning a subsequent ack will GC the envelope from the remote mailbox and the caller's AwaitRPC will time out. proto.Clone failing for a concrete *mailboxpb.Envelope is essentially unreachable, but a comment noting this assumption would make the invariant explicit.

  • AckUpTo cursor semantics confirmed correct: The proto definition reads // AckUpToRequest acknowledges all envelopes with event_seq < cursor (exclusive). The ack state machine passes batchNextCursor (exclusive) on the success path and committedCursor + 1 (exclusive) on the error path — both are correct. No issue.

Comment thread mailbox/conn/response_registry.go Outdated
}
}

return waiter.Ch

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.

Should this be an actor.Future[mailboxpb.Envelope] instead?

Comment thread mailbox/conn/response_registry.go Outdated

if waiter, ok := r.waiters[id]; ok {
select {
case waiter.Ch <- env:

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.

This would then complete the promise.

Comment thread serverconn/runtime.go Outdated
// later.
type Runtime struct {
connector *ServerConnectionActor
durable *actor.DurableActor[ServerConnMsg, ServerConnResp]

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.

Embed instead? Then an actual actor.

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.

So it can be registered directly.

Comment thread serverconn/actor.go Outdated
}
idempotencyBytes := []byte(idempotencyKey)

records := []tlv.Record{

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.

We can make a composite type wrapper here, which handles this boiler plate for us (wrap proto in TLV for now).

Comment thread serverconn/actor.go
return err
}

if _, err := stream.DecodeWithParsedTypes(r); err != nil {

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.

Ah we require TLV serialization rn with the package. So let's do the composite generic type thing mentioned below.

Comment thread mailbox/conn/ack_state.go
//
// Cursor never advances past non-durable local work. Repeated acks are safe
// and idempotent.
type AckState struct {

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.

Moved this here so it can be used across other instances. As part of the durable actor, you store this state for server/client conn and use that to remember what was sent vs not.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 3m 25s —— View job


PR Review: mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files (current state)
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is the fifth review pass. The codebase is in good shape — the previous round of fixes was thorough. Two genuine issues remain, along with a minor documentation inconsistency.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixed
Misleading "No waiter, dropping" log Fixed
Stale-waiter hangs blocked AwaitRPC FixedpruneStaleLocked completes with ErrWaiterExpired
handleSendRPCRequest returns wrong response type Fixed
rawServerMessage.ToProto() silently returns nil Fixed — now logs at ErrorS with type_url
Unnecessary bodyBytes marshal on replay Fixed — guarded with if msgID == "" || idempotencyKey == ""
Data race on a.cancel Still open — see #1 below
WrappedProto.Record() missing precondition comment Fixed
AwaitRPC second RegisterWaiter missing comment Fixed
NewRuntime missing Edge/RemoteMailboxID validation Fixed
Checkpoint load failure silent fallback to cursor 0 FixedStartIngress now returns error
SendRPC discards RegisterWaiter return without comment Fixed
ConnectorConfig.ActorSystem dead code Fixed — field removed
Checkpoint save failure after ack resets failCount Fixed — ack path now applies backoff

1. Data race on a.cancel — still open

serverconn/actor.go:524-525, 536-539

// StartIngress
ingressCtx, cancel := context.WithCancel(ctx)
a.cancel = cancel   // ← unsynchronized write

// StopIngress
a.stopOnce.Do(func() {
    if a.cancel != nil {   // ← unsynchronized read
        a.cancel()
    }
})

sync.Once ensures the inner function runs at most once, but it does not establish a happens-before relationship between the write to a.cancel in StartIngress and the read in StopIngress. If StopIngress is called from a different goroutine — e.g., from a signal handler while Start is still in progress — the race detector will flag this. Sequential single-goroutine Start/Stop calls are safe today, but the type does not enforce that constraint.

The minimal fix is atomic.Pointer[context.CancelFunc]:

// In struct:
cancel atomic.Pointer[context.CancelFunc]

// StartIngress:
a.cancel.Store(&cancel)

// StopIngress:
a.stopOnce.Do(func() {
    if fn := a.cancel.Load(); fn != nil {
        (*fn)()
    }
})

Fix this →


2. Checkpoint save failure on full-dispatch-success path does not apply backoff

serverconn/ingress.go:158-164

if err := a.saveCheckpoint(ctx, state); err != nil {
    log.WarnS(ctx, "Failed to save checkpoint after dispatch", err)
    // ← no backoff, no continue
}

failCount = 0   // ← always reset, even when checkpoint failed

The ack path (lines 70-84) was fixed in the previous round to apply backoff on checkpoint failure. But the full-dispatch-success path resets failCount unconditionally, even when saveCheckpoint returns an error. If the checkpoint store is persistently unavailable, the loop will spin at full speed — pulling batches, dispatching them, failing to checkpoint, and immediately looping again — while emitting a warning on every cycle. The partial-dispatch path avoids this because it calls sleepBackoff unconditionally for the dispatch error; but the full-success path has no such guard.

Making the behavior consistent with the ack path:

if err := a.saveCheckpoint(ctx, state); err != nil {
    log.WarnS(ctx, "Failed to save checkpoint after dispatch", err)
    a.sleepBackoff(ctx, &failCount)
    continue
}

failCount = 0

Fix this →


3. DefaultConnectorConfig doc comment repeats "mailbox IDs"

serverconn/types.go:91-92

// DefaultConnectorConfig returns a ConnectorConfig with sensible defaults for
// polling and retry behavior. The caller must still set Edge, mailbox IDs,
// Store, and mailbox IDs. Codec is optional — NewRuntime fills a default.

"mailbox IDs" appears twice; one instance should be removed.


4. AwaitRPC TypeUrl validation — carry-forward

serverconn/unary_facade.go:190-194

env.Body.Value is unmarshaled directly into the caller's response message without checking env.Body.TypeUrl. A server-side bug returning the wrong proto type in Body would produce silently garbled output. This was noted in review #4 as a documented trade-off; capturing it in a code comment would close the loop.


Minor observations

  • AdvanceAck dead branch: The if s.AckCommittedTo > s.PullCursor branch in ack_state.go:67-69 is unreachable under normal invariants (PullCursor is always advanced to at least DispatchCommittedTo = AckTarget before AdvanceAck is called). It is correct as defensive code, but a comment noting when this branch would fire (crash-recovery edge case) would help the next reader.

  • DeliverResponse sends non-cloned envelope to waiter: When a waiter exists, response_registry.go:136 completes the promise with the raw env pointer (not cloned). For ingress-path envelopes, gRPC allocates fresh protos per response so mutation is not a concern in practice — but a comment noting this assumption would make the invariant explicit.

This commit introduces the foundational types for the serverconn
connector, which will serve as the unified boundary for all mailbox
traffic between the client and the remote server.

The AckState struct models the four-cursor state machine that governs
the pull-dispatch-ack cycle: PullCursor tracks where to resume pulling,
DispatchCommittedTo records the last durably dispatched position,
AckTarget holds the cursor to send in the next AckUpTo call, and
AckCommittedTo tracks what the remote has actually acknowledged. The
state machine enforces the critical invariant that AckUpTo only advances
after durable local dispatch, preventing message loss on crash. TLV
encode/decode methods allow the state to be persisted via the durable
actor checkpoint mechanism.

Also included are the ConnectorConfig struct with all dependency
injection points (Edge client, dispatch table, delivery store, pull
tuning knobs), named types for CorrelationID and IdempotencyKey, the
EnvelopeDispatcher function type for ServiceKey-based routing, and the
ResponseWaiter struct for in-memory unary RPC correlation.
@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 5m 29s —— View job


PR Review (Pass 6): mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files (current state)
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is the sixth review pass. The PR is in excellent shape — all substantive issues from prior passes have been addressed. Two minor structural notes below.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixed
Misleading "No waiter, dropping" log Fixed
Stale-waiter hangs blocked AwaitRPC Fixed
handleSendRPCRequest returns wrong response type Fixed
rawServerMessage.ToProto() silently returns nil Fixed — logs at ErrorS with type_url
Unnecessary bodyBytes marshal on replay Fixed
Data race on a.cancel Fixed — see note below
WrappedProto.Record() missing precondition comment Fixed
AwaitRPC second RegisterWaiter missing comment Fixed
NewRuntime missing Edge/RemoteMailboxID validation Fixed
Checkpoint load failure silent fallback Fixed
SendRPC discards RegisterWaiter return without comment Fixed
ConnectorConfig.ActorSystem dead code Fixed
Checkpoint save failure after ack resets failCount Fixed
DefaultConnectorConfig doc comment repeats "mailbox IDs" Fixed
Checkpoint save failure on full-dispatch-success path Fixed

1. cancelCh send precedes wg.AddStopIngress can return before goroutine exits (minor)

serverconn/actor.go:527-531

ingressCtx, cancel := context.WithCancel(ctx)
a.cancelCh <- cancel   // (A) cancel placed in channel

a.wg.Add(1)            // (B) counter incremented after (A)
go a.ingressLoop(ingressCtx, state)

StopIngress receives the cancel function from cancelCh inside stopOnce.Do, calls it, then calls wg.Wait() outside the Do. There is a narrow window between (A) and (B): if StopIngress executes its cancelCh receive and wg.Wait() after (A) but before (B), wg.Wait() returns immediately (counter is still 0). wg.Add(1) then runs, the goroutine starts with an already-cancelled ingressCtx, exits on the first ctx.Done() check, and calls wg.Done() — but StopIngress has already returned, so its caller proceeds without the goroutine having actually finished.

In the current usage (Runtime.Start then Runtime.Stop — always sequential, never concurrent), this window is unreachable in practice. The fix is to swap the order so wg.Add(1) happens before the channel send, ensuring any concurrent StopIngress caller blocks in wg.Wait() until the goroutine (whenever it starts) finishes:

a.wg.Add(1)
a.cancelCh <- cancel
go a.ingressLoop(ingressCtx, state)

Fix this →


2. AdvanceAck defensive branch needs a comment (carry-forward)

mailbox/conn/ack_state.go:67-69

func (s *AckState) AdvanceAck() {
    s.AckCommittedTo = s.AckTarget

    if s.AckCommittedTo > s.PullCursor {
        s.PullCursor = s.AckCommittedTo   // never reached under normal invariants
    }
}

Under normal operation PullCursor is always advanced to at least AckTarget before AdvanceAck is called, so this branch is unreachable in the expected state machine. It exists as a crash-recovery safety net for the case where a partial checkpoint write leaves AckCommittedTo > PullCursor after restart. A short comment noting when this branch would fire would make the intent clear for the next reader.


Minor observations confirmed correct (no action needed)

  • cancelCh channel pattern for the data race: the channel-based transfer idiom (make(chan context.CancelFunc, 1)) is race-free and idiomatic. The channel provides the necessary happens-before guarantee between StartIngress's write and StopIngress's read; sync.Once prevents double-cancel. This is a clean solution to the issue flagged across multiple prior passes.

  • DeliverResponse non-cloned delivery to live waiters: env.EventSeq is the only field read after deliverResponse returns in the ingress loop. AwaitRPC reads env.Body.Value without mutating env. No actual race in the current code.

  • dispatchBatch lastCommitted not updated for skipped envelopes (nil Rpc, missing corrID, no dispatcher): on the success path batchNextCursor covers all skipped envelopes. On the error path they cause redundant re-processing on retry, but all skips are idempotent.

  • AwaitRPC TypeUrl validation skip: the code comment at unary_facade.go:189-192 explicitly documents this trade-off. No action needed.


The ServerConnectionActor is transformed from a stub relay into the
unified connector boundary described in the architecture plan.

Message types gain full TLV serialization so they can be persisted in
the durable actor mailbox. SendClientEventRequest wraps the outbound
proto in an anypb.Any to preserve type information across the
encode/decode boundary, using the global protobuf type registry for
reconstruction. SendRPCRequest serializes the entire mailbox Envelope
via proto.Marshal for the unary facade path. Both message types carry
stable TLV type identifiers (2000, 2001) for the durable actor codec.

The actor struct gains a response registry (correlation ID to waiter
channel map) for unary RPC response routing, protected by a dedicated
mutex separate from the actor's mailbox processing. Egress handlers
now build proper mailbox envelopes with protocol version, sender/
recipient addressing, and RPC metadata, sending them through the
configured Edge client. A MessageCodec factory registers both message
types for the durable actor runtime.

The ingress loop lifecycle is managed via StartIngress/StopIngress,
running as a background goroutine independent of the actor mailbox.
The ingress loop is the heart of the connector's inbound message
processing. It runs as a self-driven background goroutine that
continuously long-polls the remote mailbox via Edge.Pull and routes
received envelopes to local consumers.

The loop follows a strict three-phase cycle per iteration:

First, if a prior dispatch committed successfully, it acks the remote
mailbox up to the committed cursor via Edge.AckUpTo. This is done
before pulling new work so the remote can garbage-collect delivered
envelopes. The ack only advances after the checkpoint has been
persisted, maintaining the invariant that AckUpTo never races ahead of
durable local state.

Second, it pulls a batch of envelopes starting from the current cursor.
Empty long-poll returns reset the backoff counter and loop immediately
since the long-poll timeout already provides the delay.

Third, each envelope is dispatched based on its RPC metadata kind.
KIND_RESPONSE envelopes are delivered to the in-memory response registry
for unary RPC waiters. KIND_REQUEST and KIND_EVENT envelopes are routed
through the configured dispatch table to local durable actors via their
ServiceKey. If any dispatch fails mid-batch, the loop advances state
only through the last successfully committed position, avoiding
re-dispatch of already-committed envelopes on retry.

All checkpoint state is persisted through the DeliveryStore's checkpoint
mechanism using a stable actor ID derived from the local mailbox ID.
Transient failures trigger exponential backoff with jitter to prevent
busy-spinning, capped at the configured maximum delay.
The UnaryFacade provides the mailboxrpc.RPCClient interface on top of
the ServerConnectionActor, giving generated service stubs a synchronous
send-and-await API for unary RPCs.

SendRPC builds a complete mailbox envelope with correlation ID,
idempotency key, service/method addressing, and protocol version, then
sends it directly via Edge.Send rather than going through the durable
actor mailbox. This direct path keeps unary RPC latency low since the
caller already handles retries on failure and doesn't need durable
egress guarantees.

AwaitRPC registers a waiter in the actor's response registry, then
blocks on the waiter channel until the ingress loop delivers a matching
KIND_RESPONSE envelope or the caller's context is cancelled. On
receipt, it unmarshals the proto body from the envelope's Any value
into the caller-provided response message. The waiter is always cleaned
up on exit to prevent registry leaks.

Package documentation is included describing the connector's role as
the single ingress/egress boundary, the ack watermark invariants, and
the relationship between the dispatch table and the unary facade.
This commit adds comprehensive tests covering the three main layers of
the serverconn connector.

The ack state machine tests verify the cursor advancement invariants:
AdvanceDispatch sets the ack target monotonically, AdvanceAck updates
the committed cursor and pull position, NeedsAck correctly reflects
pending acks, and TLV round-trip encoding preserves all four cursor
fields including the zero-value edge case.

The connector integration tests exercise the full ingress loop against
an in-memory mailbox edge. They verify that dispatched envelopes are
acked, responses are delivered to registered waiters, failed dispatches
don't advance the ack cursor, shutdown drains cleanly without goroutine
leaks, and checkpoint state survives a simulated restart cycle. The
exponential backoff helper is tested for range bounds and zero-value
defaults.

The unary facade tests cover the SendRPC envelope construction path
(verifying correlation ID, idempotency key, and service/method metadata
in the sent envelope), the AwaitRPC round-trip through the response
registry including context cancellation, concurrent inflight request
isolation, and nil body handling.

Test infrastructure includes an in-memory mailbox with long-poll
support, a fake MailboxServiceClient adapter, and a minimal checkpoint
store that implements the full DeliveryStore interface with panicking
stubs for unused methods.
In this commit, we introduce the mailbox/conn package to house reusable
building blocks shared by client-side and server-side connector
runtimes.

The AckState struct and its TLV encode/decode are moved here wholesale
from serverconn/types.go, along with the CorrelationID and
IdempotencyKey named types. A ResponseRegistry provides in-memory
correlation waiter tracking with early-response buffering (a response
that arrives before its waiter is registered gets cloned and held until
the waiter appears) and configurable TTL-based stale cleanup. Envelope
identity helpers (StableEventMsgID, StableEventIdempotencyKey) derive
deterministic SHA-256-based identifiers from a serialized payload so
durable retries of the same semantic event produce the same message and
idempotency keys for remote dedupe.

The test suite includes both table-driven unit tests and property-based
randomized checks (via pgregory.net/rapid) for the ack state invariants
and response registry interleaving behavior.
…dentity

In this commit, we wire the serverconn package to use the shared
mailbox/conn primitives and fix two correctness gaps identified during
review.

The type aliases for CorrelationID, IdempotencyKey, AckState, and
ResponseWaiter now point to mailbox/conn, and the actor's hand-rolled
response registry is replaced by conn.ResponseRegistry. The registry's
early-response buffering closes the race where a fast response from the
remote mailbox could arrive (via the ingress loop) before AwaitRPC had
a chance to register its waiter -- previously that response would be
silently dropped. SendRPC now registers the waiter before calling
Edge.Send, and on any send failure the waiter is cleaned up immediately.
AwaitRPC uses a defer to remove its waiter on all exit paths.

SendClientEventRequest gains MsgID and IdempotencyKey fields that
survive TLV round-trips. When unset, Encode derives deterministic
defaults from the serialized proto payload via conn.StableEventMsgID
and conn.StableEventIdempotencyKey, ensuring that durable replay of the
same semantic event produces the same identifiers for remote dedupe.
The egress handler stamps these identifiers onto the outbound envelope.

The standalone ack_state_test.go is removed since those tests now live
in mailbox/conn.
In this commit, we add a Runtime struct that wires the
ServerConnectionActor, its DurableActor wrapper, and the UnaryFacade
into a single composition unit that higher-level daemon bootstrap can
use.

NewRuntime validates required config (Store, LocalMailboxID), fills a
default codec when one isn't supplied, and returns a fully assembled
but inert runtime. Start launches both durable egress processing and
the ingress pull loop; Stop tears them down in reverse order. Accessors
expose the Ref/TellRef (for round actor egress) and Unary (for typed
RPC stubs).

The DurableActorID helper extracts the "serverconn-" + mailboxID
convention into a single function shared by both the runtime and the
ingress checkpoint load/save paths.

Package docs are updated to describe the runtime composition stage and
note the shared mailbox/conn primitives.
In this commit, we remove the entire mailbox/client package. With the
serverconn package now serving as the unified connector boundary for all
mailbox traffic, the legacy client runtime is redundant. It maintained
its own pull loop, cursor persistence, response store, and ack logic --
all of which duplicated what serverconn and the durable actor runtime
already provide.

Keeping a parallel connector path would mean two competing durability
stores, two cursor management schemes, and two sets of retry semantics.
Removing it now prevents that divergence from accumulating.
In this commit, we add tests covering the new functionality introduced
in the prior commits.

The TLV round-trip tests verify that SendClientEventRequest produces
stable deterministic MsgID and IdempotencyKey for equal payloads, that
explicitly set identifiers survive encode/decode unchanged, and that
SendRPCRequest round-trips its full envelope. The Receive path is
tested for unknown message rejection and the SendRPCRequest dispatch
handler.

The runtime tests verify config validation rejects missing Store and
LocalMailboxID, and that NewRuntime fills a default codec and wires
all accessors (Ref, TellRef, Unary, Connector) correctly.

The connector test gains TestEgress_EventRetriesPreserveIdempotencyKey,
which verifies that two egress sends of the same semantic event produce
identical MsgId and IdempotencyKey on the wire. The unary facade test
gains TestUnaryFacade_ResponseBeforeAwait, which exercises the early-
response buffering path where the ingress loop delivers a response
before the caller starts AwaitRPC.
In this commit, we replace the panicking stubs in memCheckpointStore
with working in-memory implementations of the full actor.DeliveryStore
interface. The store now supports EnqueueMessage with ON CONFLICT DO
NOTHING semantics for receiver-side dedup, LeaseNextMessage with FIFO
ordering and lease expiry, AckMessage, NackMessage with retry-after
delays, ExtendLease, MoveToDeadLetter, IsProcessed/MarkProcessed, and
the outbox claim/complete/fail cycle.

This upgrade is needed by upcoming tests that exercise the durable
actor runtime integration path where the runtime calls through to the
full DeliveryStore contract rather than just the checkpoint subset.
In this commit, we add tests covering the remaining gaps identified
during review: ingress error paths, crash/restart replay semantics,
property-based ack invariant checks, high-concurrency unary response
delivery, codec round-trips, and runtime lifecycle.

The ingress error tests use a configurable MailboxServiceClient stub to
verify behavior under Pull failures, AckUpTo failures, partial dispatch
failures, and responses with missing or nil RPC metadata. The
restart/replay test exercises egress idempotency by verifying that a
failed Send followed by a retry produces identical MsgId and
IdempotencyKey on the wire.

The property-based test uses pgregory.net/rapid to randomize ack and
partial-dispatch progressions, asserting that AckCommittedTo never
exceeds DispatchCommittedTo under arbitrary interleaving.

The high-concurrency unary test fires 200 requests, injects responses
in shuffled order before AwaitRPC begins, and verifies each caller
receives its correct response through the early-response buffering
path. The runtime lifecycle test verifies Start/Stop complete promptly
on context cancellation without goroutine leaks.
@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Feb 21, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 3m 37s —— View job


PR Review (Pass 7): mailbox+serverconn: durable response store and unified connector boundary

Tasks

  • Gather context and understand PR structure
  • Read key source files (current state)
  • Analyze for bugs, correctness, and design issues
  • Post final review

This is the seventh review pass. All issues from the previous six passes have been addressed. The codebase is clean and ready for merge.


Previous review issues — status

Issue Status
Pull cursor off-by-one on partial dispatch Fixed
Misleading "No waiter, dropping" log Fixed
Stale-waiter hangs blocked AwaitRPC Fixed
handleSendRPCRequest returns wrong response type Fixed
rawServerMessage.ToProto() silently returns nil Fixed — logs at ErrorS with type_url
Unnecessary bodyBytes marshal on replay Fixed — guarded with if msgID == "" || idempotencyKey == ""
Data race on a.cancel Fixed — channel-based cancelCh pattern
WrappedProto.Record() missing precondition comment Fixedproto_record.go:11-14
AwaitRPC second RegisterWaiter missing comment Fixedunary_facade.go:77-82
NewRuntime missing Edge/RemoteMailboxID validation Fixed
Checkpoint load failure silent fallback FixedStartIngress returns error
SendRPC discards RegisterWaiter return without comment Fixed
ConnectorConfig.ActorSystem dead code Fixed — field removed
Checkpoint save failure after ack resets failCount Fixed
DefaultConnectorConfig doc comment repeats "mailbox IDs" Fixed
Checkpoint save failure on full-dispatch-success path Fixed — applies backoff and continue
cancelCh send precedes wg.Add Fixedactor.go:529 now does wg.Add(1) before channel send
AdvanceAck defensive branch needs comment Fixedack_state.go:67-74

Verification of the two Pass 6 open items

cancelCh / wg.Add ordering (actor.go:527-531): Confirmed fixed. The sequence is now:

ingressCtx, cancel := context.WithCancel(ctx)
a.wg.Add(1)          // ← counter incremented first
a.cancelCh <- cancel // ← then delivered to StopIngress
go a.ingressLoop(ingressCtx, state)

StopIngress draining the channel and calling wg.Wait() will always observe wg counter ≥ 1, ensuring it blocks until the goroutine actually exits even in concurrent Start/Stop scenarios.

AdvanceAck defensive branch comment (ack_state.go:67-74): Confirmed fixed. The comment now reads:

"Defensive: under normal operation PullCursor is always advanced to at least AckTarget before AdvanceAck, so this branch is unreachable. It exists as a crash-recovery safety net for the case where a partial checkpoint write leaves AckCommittedTo > PullCursor after restart."


No new issues found

After reading all key files (actor.go, ingress.go, unary_facade.go, types.go, runtime.go, response_registry.go, ack_state.go, proto_record.go, runtime_test.go), no new correctness issues were found. A few design properties worth confirming are all correct:

  • dispatchBatch lastCommitted not updated for skipped envelopes (nil Rpc, missing corrID, unknown kind): On the success path batchNextCursor covers all skipped envelopes. On the error path skipped envelopes are re-processed idempotently on retry. Correct by design.

  • StopIngress wg.Wait() outside stopOnce.Do: Multiple concurrent callers will all block in wg.Wait() until the goroutine exits, which is the correct behavior.

  • pruneStaleLocked called on every RegisterWaiter/DeliverResponse: O(n) scan per operation. Acceptable for typical RPC concurrency levels (tens to low hundreds of inflight requests).

  • AwaitRPC TypeUrl validation skip: Now explicitly documented at unary_facade.go:187-192. No action needed.


LGTM. The PR has had a thorough review cycle and all identified issues have been resolved. No further action required.

@Roasbeef
Roasbeef merged commit 32f7a1e into main Feb 21, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant