MailboxRPC 7/7: end-to-end behavior tests

[bhandras]

Summary

Adds end-to-end behavioral tests for the MailboxRPC client stack.

Covers:

• Concurrent in-flight responses surviving a client restart.
• Pull cursor persistence and monotonicity guard behavior.

Motivation

These tests make the expected durability semantics explicit and help prevent
regressions as we introduce additional mailboxrpc payload types and client-side
storage backends.

Stack order

• Base: MailboxRPC 1/7: durability foundation #48
• Depends on: MailboxRPC 6/7: sqlc-backed durable response storage #90 (durable store), which stacks on MailboxRPC 5/7: edge-backed mailboxrpc client #89/MailboxRPC 4/7: mailboxrpc ServeMux router #88/MailboxRPC 2/7: contract + codegen + make rpc #86.

References

• Durability reference: MailboxRPC 1/7: durability foundation #48

[gemini-code-assist]

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist¹! 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 robustness of the mailboxrpcclient by adding critical end-to-end behavioral tests. These tests validate the client's ability to maintain state and process messages reliably, specifically focusing on the persistence of in-flight RPC responses across client restarts and ensuring the monotonic progression of message processing cursors. This work is essential for guaranteeing the integrity and fault tolerance of the RPC-over-mailbox client stack when integrated with a durable store.

Highlights

• New Behavioral Tests: Added a new file, mailboxrpcclient/behavior_test.go, to introduce end-to-end behavioral tests for the RPC-over-mailbox client stack.
• Concurrent Response Persistence: Implemented TestClient_ConcurrentResponsesSurviveRestart to verify that multiple in-flight RPC responses are correctly persisted and can be retrieved after a client restart when using a durable store.
• Cursor Monotonicity Guard: Introduced TestClient_CursorPersistenceIsMonotonic to ensure that the client's message processing cursor updates are strictly monotonic, preventing accidental regressions in message processing order.

🧠 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.

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]

Code Review

This pull request adds two important end-to-end behavioral tests for the RPC-over-mailbox client. The first test, TestClient_ConcurrentResponsesSurviveRestart, correctly verifies that concurrent in-flight responses are persisted and can be retrieved after a client restart. The second test, TestClient_CursorPersistenceIsMonotonic, ensures that the cursor used for pulling messages is always updated monotonically. The tests are well-written and cover their intended functionality thoroughly. I have one suggestion to improve the robustness of a test.

[bhandras]

@codex review pls

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.

[litbot-9000]

@bhandras, remember to re-request review from reviewers when ready

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context and read changed files
• Review mailbox/rpc/ additions (event_mux, grpc_status, interface)
• Review serverconn/ additions (e2e_test, event_router, actor changes)
• Review test fixtures (hellotestpb, hello.proto)
• Post comprehensive review feedback

---

Overall this is a clean PR — the layering between EventMux (pure function dispatch) and EventRouter (actor-system dispatch) is well-motivated and clearly documented. The test matrix covers the meaningful behavioural corners. A few issues worth addressing before merge:

---

Bug: DispatcherMap type declaration swallows AsDispatcherMap's GoDoc

serverconn/event_router.go:186–196:

// AsDispatcherMap returns a shallow copy of the registered routes as an
// EnvelopeDispatcher map suitable for use as ConnectorConfig.Dispatchers.
//
// The returned map is safe to read concurrently. Callers should call
// AsDispatcherMap after all routes have been registered, before constructing
// the ConnectorConfig.
// DispatcherMap is the type returned by AsDispatcherMap, mapping envelope
// routing keys to their dispatch closures.
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

func (r *EventRouter) AsDispatcherMap() DispatcherMap {

The type DispatcherMap declaration appears before the func AsDispatcherMap declaration, so GoDoc attaches the entire comment block above to DispatcherMap, not to the function. The method AsDispatcherMap ends up with no doc comment at all — a CLAUDE.md violation ("every function must have a comment").

The type and function should have separate GoDoc blocks, with DispatcherMap declared at its own comment-separated location:

// DispatcherMap maps envelope routing keys to their dispatch closures. It is
// the type returned by AsDispatcherMap and consumed by
// ConnectorConfig.Dispatchers.
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

// AsDispatcherMap returns a shallow copy of the registered routes as a
// DispatcherMap suitable for use as ConnectorConfig.Dispatchers.
//
// The returned map is safe to read concurrently. Callers should call
// AsDispatcherMap after all routes have been registered, before constructing
// the ConnectorConfig.
func (r *EventRouter) AsDispatcherMap() DispatcherMap {

Fix this →

---

Nit: Confusing variable names in AddRoute

serverconn/event_router.go:110,152:

key := cfg.Key          // actor.ServiceKey[M, R]
...
key2 := mailboxrpc.ServiceMethod{...}   // route lookup key
r.mu.Lock()
r.routes[key2] = dispatcher

key and key2 for semantically different things in the same scope is a code smell. Suggest actorKey / serviceMethod (or just sm) to make the distinction obvious at a glance.

Fix this →

---

Nit: helloFinalizedMsg is defined but never exercised

serverconn/e2e_test.go:48–103:

helloFinalizedMsg (along with its FromProto, MessageType, and the compile-time assertion at line 102) is declared but no test actually registers or dispatches it. Since TestE2E_UnaryAndPush and TestE2E_ServerPushEvent only push HelloStartedEvent, HelloFinalizedEvent is dead code.

Either add a test scenario that uses HelloFinalizedEvent (which would strengthen coverage) or remove the type and its assertion to keep the test file lean.

---

Nit: Silently discarded error in testServer.handleRequest

serverconn/e2e_test.go:228:

body, _ = anypb.New(respMsg)

anypb.New can return an error if respMsg has no registered type URL. Discarding it silently means a broken serialization would produce a nil body, send a response with an empty Any, and cause the client to see a proto unmarshal failure instead of a clear test failure at the source. Even in a test helper t.Fatalf or require.NoError would make debugging much faster. The method doesn't receive t currently, so it would need to be added, or the error should be propagated to the caller.

---

Minor: status.FromError fallback is redundant

mailbox/rpc/grpc_status.go:36–39:

st, ok := status.FromError(err)
if !ok {
    st = status.New(codes.Unknown, err.Error())
}

Per the gRPC-Go docs, when ok == false, status.FromError already returns a Status with Code=Unknown and the original error message. The if !ok branch creates an identical object. The comment "fall back to Unknown for plain errors" is accurate in intent but the code is redundant. This is harmless, but worth cleaning up for clarity.

---

Positive observations

• Forward-compatible DiscardUnknown: true in both EventMux.ServeEvent and EventRouter's dispatcher closure — good hygiene that keeps clients survivable across server-side schema evolution.
• The package-level AddRoute generic function workaround (vs. a method) is clearly documented; this is the right pattern.
• EncodeErrorHeaders / DecodeErrorHeaders round-trip tests cover nil, empty, plain-error, and each relevant gRPC status code — solid test matrix.
• The compile-time var _ EventHandler = (*EventMux)(nil) sentinel keeps the interface assertion close to the implementation.
• Gemini's suggestion (avoid require.NoError inside require.Eventually) was correctly incorporated.

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context and read changed files
• Review mailbox/rpc/ additions
• Review serverconn/ additions (event_router, e2e_test, actor changes)
• Review test files and fixtures
• Post comprehensive review feedback

---

The layering is well thought out — AddRoute/NewEventRoute as a wiring-time generic helper over a type-erased EnvelopeDispatcher is a clean pattern. A few issues worth addressing before merge:

---

Bug: DispatcherMap GoDoc swallows AsDispatcherMap's comment

serverconn/event_router.go:205–215:

// AsDispatcherMap returns a shallow copy of the registered routes as an
// EnvelopeDispatcher map suitable for use as ConnectorConfig.Dispatchers.
//
// The returned map is safe to read concurrently. Callers should call
// AsDispatcherMap after all routes have been registered, before constructing
// the ConnectorConfig.
// DispatcherMap is the type returned by AsDispatcherMap, mapping envelope
// routing keys to their dispatch closures.
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

func (r *EventRouter) AsDispatcherMap() DispatcherMap {

GoDoc attaches the entire contiguous comment block to the DispatcherMap type declaration immediately below it. The AsDispatcherMap method ends up with no doc comment at all — a CLAUDE.md violation. The two declarations need separate, disjoint comment blocks:

// DispatcherMap maps envelope routing keys to their dispatch closures. It is
// the type returned by AsDispatcherMap and consumed by
// ConnectorConfig.Dispatchers.
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

// AsDispatcherMap returns a shallow copy of the registered routes as a
// DispatcherMap suitable for use as ConnectorConfig.Dispatchers.
//
// The returned map is safe to read concurrently. Callers should call
// AsDispatcherMap after all routes have been registered, before
// constructing the ConnectorConfig.
func (r *EventRouter) AsDispatcherMap() DispatcherMap {

Fix this →

---

Minor: NewEventRoute doesn't eagerly validate cfg.NewMsg

serverconn/event_router.go:188–202:

AddRoute correctly panics at registration time for nil NewEvent and nil Adapt. But NewEventRoute captures cfg.NewMsg without checking it:

func NewEventRoute[M InboundActorMessage, R any](r *EventRouter,
    cfg InboundEventRouteConfig[M, R]) {

    newMsg := cfg.NewMsg   // nil check missing here

    AddRoute(r, EventRouteConfig[M, R]{
        ...
        Adapt: func(p proto.Message) (M, error) {
            m := newMsg()   // panics at dispatch time if nil

A nil cfg.NewMsg silently passes AddRoute's validation (since Adapt isn't nil), then panics at the first dispatched envelope rather than at registration time. Adding a panic guard at the top of NewEventRoute would keep the fail-fast behaviour consistent with AddRoute.

Fix this →

---

Minor: Silently discarded error in handleRequest

serverconn/e2e_test.go:212:

body, _ = anypb.New(respMsg)

anypb.New fails if the message type URL isn't registered in the global proto registry. Discarding the error produces a nil body in the response envelope, which the client then fails to unmarshal — the failure appears on the wrong side, making test debugging harder. Since handleRequest doesn't currently take t *testing.T, either thread it through (from the caller) or propagate the error back to run and call t.Fatal there.

---

Nit: key / key2 variable names in AddRoute

serverconn/event_router.go:110,152:

key  := cfg.Key   // actor.ServiceKey[M, R]
...
key2 := mailboxrpc.ServiceMethod{...}   // routing lookup key

Two semantically different values, one with a sequential suffix. actorKey / serviceMethod (or just sm) would remove the mental mapping at every read.

---

Nit: helloFinalizedMsg is dead code in the test file

serverconn/e2e_test.go:44–94:

helloFinalizedMsg is declared with full MessageType/FromProto methods and a compile-time assertion, but no test actually pushes a HelloFinalizedEvent or registers a route for it. Either add a TestE2EFinalizedEvent scenario (which would strengthen coverage of multi-event dispatch) or drop the type to keep the file lean.

---

Nit: status.FromError fallback is redundant

mailbox/rpc/grpc_status.go:36–39:

st, ok := status.FromError(err)
if !ok {
    st = status.New(codes.Unknown, err.Error())
}

Per the gRPC-Go docs, status.FromError already returns (codes.Unknown, err.Error(), false) for non-gRPC errors, so the if !ok branch constructs an identical object. The comment "fall back to Unknown for plain errors" is accurate in intent but the defensive code is a no-op.

---

Positive observations

• DiscardUnknown: true in the dispatch closure (event_router.go:128) is good forward-compatibility hygiene — clients survive server-side schema additions without redeployment.
• The rationale for AddRoute being a package-level function rather than a method is clearly documented. This is the right way to work around Go's lack of generic methods.
• Eager panics in AddRoute for nil NewEvent/Adapt are the right pattern for wiring-time configuration errors — better a clear panic at startup than a confusing nil dereference during operation.
• newTestConnectorConfig nicely DRYs up 6–7 lines of repeated setup across connector_test.go, runtime_test.go, and restart_replay_test.go.
• The EncodeErrorHeaders/DecodeErrorHeaders test matrix covers nil, empty, absent-key, empty-value, invalid base64, invalid proto, and each significant gRPC status code — thorough.
• t.Parallel() on every test and subtest — no accidental serial test runs.

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context and read changed files
• Review mailbox/conn/ new package
• Review mailbox/rpc/grpc_status.go
• Review serverconn/ changes (actor, event_router, ingress, unary_facade)
• Review test files
• Post comprehensive review feedback

---

This is a well-structured PR — the layering between mailbox/conn (shared primitives), mailbox/rpc (gRPC status encoding), serverconn/ingress (pull-dispatch-ack loop), EventRouter (type-erased dispatch), and UnaryFacade (unary RPC) is clean and clearly documented. The test matrix covers meaningful behavioral corners including property-based tests for the ack state machine. One significant functional bug and several style issues need addressing before merge.

---

Bug: AwaitRPC ignores server-error headers — RPC errors are silently swallowed

serverconn/unary_facade.go:180–198

AwaitRPC never calls DecodeErrorHeaders(env.Headers) before attempting to unmarshal the body. The test server in e2e_test.go:206–213 correctly encodes errors into HeaderGRPCStatusB64 and sets body = &anypb.Any{} when a handler returns an error. But AwaitRPC only checks env.Body == nil (it's not nil — it's &anypb.Any{}), then unmarshals env.Body.Value which is empty bytes. proto.Unmarshal([]byte{}, resp) succeeds silently, leaving all fields zero. The caller gets a zero-value response and nil error, with no indication that the RPC failed.

The fix is to check headers before unmarshaling:

// Check for a server-side gRPC status error before attempting to
// unmarshal the body. Server handlers encode failures into
// HeaderGRPCStatusB64 rather than returning a populated body.
if rpcErr := mailboxrpc.DecodeErrorHeaders(env.Headers); rpcErr != nil {
    return rpcErr
}

This should be inserted immediately after the env.Body == nil check (unary_facade.go:183).

Note: the round-trip infrastructure is fully wired — EncodeErrorHeaders/DecodeErrorHeaders exist and are tested, handleRequest in the test server uses them, and ServeMux propagates handler errors. The only missing piece is this call in AwaitRPC. There is also no test exercising the error path end-to-end through AwaitRPC (e.g., a SayHello variant that returns an error), so the gap has gone unnoticed.

Fix this →

---

Bug: DispatcherMap GoDoc swallows AsDispatcherMap's comment

serverconn/event_router.go:206–216

The existing comment block mixes the doc for both the type and the method together, and GoDoc attaches the entire contiguous block to DispatcherMap, leaving AsDispatcherMap undocumented — a CLAUDE.md violation:

// AsDispatcherMap returns ... (method doc)
// DispatcherMap is ... (type doc)
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

func (r *EventRouter) AsDispatcherMap() DispatcherMap {

The fix is to give each declaration its own separate comment:

// DispatcherMap maps envelope routing keys to their dispatch closures.
// It is the type returned by AsDispatcherMap and consumed by
// ConnectorConfig.Dispatchers.
type DispatcherMap = map[mailboxrpc.ServiceMethod]EnvelopeDispatcher

// AsDispatcherMap returns a shallow copy of the registered routes as a
// DispatcherMap suitable for use as ConnectorConfig.Dispatchers.
//
// The returned map is safe to read concurrently. Callers should call
// AsDispatcherMap after all routes have been registered, before
// constructing the ConnectorConfig.
func (r *EventRouter) AsDispatcherMap() DispatcherMap {

Fix this →

---

Minor: NewEventRoute doesn't eagerly validate cfg.NewMsg

serverconn/event_router.go:188–204

AddRoute correctly panics at registration time for nil NewEvent and nil Adapt. But NewEventRoute captures cfg.NewMsg without a nil check. A nil cfg.NewMsg passes AddRoute's guards (because Adapt is non-nil — it's the closure), then panics at dispatch time when newMsg() is called. Adding a guard at the start of NewEventRoute keeps the fail-fast behavior consistent:

func NewEventRoute[M InboundActorMessage, R any](r *EventRouter,
    cfg InboundEventRouteConfig[M, R]) {

    if cfg.NewMsg == nil {
        panic("serverconn: nil NewMsg in InboundEventRouteConfig")
    }
    ...

Fix this →

---

Minor: status.FromError fallback is redundant

mailbox/rpc/grpc_status.go:36–39

Per the gRPC-Go docs, status.FromError already returns (codes.Unknown, err.Error(), false) for non-gRPC errors. The if !ok branch constructs an identical object. The comment "fall back to Unknown for plain errors" is accurate in intent, but the code is a no-op and can be simplified to:

st, _ := status.FromError(err)

---

Minor: Silently discarded error in handleRequest

serverconn/e2e_test.go:212

body, _ = anypb.New(respMsg)

anypb.New can fail if respMsg has no type URL registered in the global proto registry. Discarding the error produces a nil body in the response, causing the client to receive a confusing unmarshal failure. Since handleRequest doesn't currently receive t *testing.T, either thread it through or propagate the error back to run and call t.Fatal there.

---

Nit: key/key2 variable names in AddRoute

serverconn/event_router.go:108,150

key  := cfg.Key        // actor.ServiceKey[M, R]
...
key2 := mailboxrpc.ServiceMethod{...}  // routing lookup key

Two semantically different values, one with a sequential suffix. actorKey / serviceMethod (or sm) would remove the mental mapping at every read.

---

Nit: helloFinalizedMsg is dead code

serverconn/e2e_test.go:44–96

helloFinalizedMsg is declared with full MessageType/FromProto methods and a compile-time assertion, but no test pushes a HelloFinalizedEvent or registers a route for it. Either add a test scenario (which would strengthen multi-event dispatch coverage) or remove the type to keep the file lean.

---

Nit: time.Sleep as a synchronization barrier

serverconn/unary_facade_test.go:282

// Brief pause to let all waiters register before injecting responses.
time.Sleep(50 * time.Millisecond)

This is a race condition on slow CI: the sleep may expire before all goroutines have called RegisterWaiter. A proper barrier (e.g., a sync.WaitGroup that all awaiter goroutines signal before the response injection loop starts) would make this deterministic. The same pattern appears in connector_test.go:274 and runtime_test.go:88–89.

---

Positive observations

• DiscardUnknown: true in both EventRouter's dispatcher closure and AwaitRPC is good forward-compatibility hygiene.
• The AddRoute/NewEventRoute package-level generic function pattern is clearly documented — the right workaround for Go's lack of generic methods.
• Eager panics in AddRoute for nil NewEvent/Adapt follow fail-fast conventions.
• newTestConnectorConfig DRYs up repeated wiring across multiple test files cleanly.
• The TestUnaryFacade_HighConcurrencyOutOfOrder test with 200 concurrent requests and a seeded shuffle of response delivery order is a strong stress test for the ResponseRegistry routing.
• Property-based tests for both AckState invariants and the partial-failure cursor conversion (TestIngress_AckNeverExceedsCommitted_Property, TestIngress_PartialFailureCursor_Property) provide wide coverage over the monotonicity invariants.
• EncodeErrorHeaders/DecodeErrorHeaders round-trip test matrix covers nil, empty, absent-key, invalid base64, invalid proto, and multiple gRPC codes.
• t.Parallel() on every test and subtest — no accidental serial test runs.
• pullLocked correctly skips envelopes below the ack watermark (env.EventSeq < acked) in addition to those below the cursor, preventing acked envelopes from being pulled again after a checkpoint restore.

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context and read changed files
• Review mailbox/rpc/ additions
• Review serverconn/ changes (event_router, unary_facade, e2e_test, actor)
• Check which issues from previous reviews were addressed
• Post comprehensive review feedback

---

All previously flagged issues have been addressed. Here's the current state:

Previously flagged — now resolved

All items from the prior reviews are fixed:

• Bug: AwaitRPC swallowing server errors — resolved in commit afa0465. DecodeErrorHeaders is called before body unmarshal, and TestE2EUnaryRPCError was added to exercise the full EncodeErrorHeaders → HeaderGRPCStatusB64 → DecodeErrorHeaders path end-to-end. The errHelloServer correctly exercises both NotFound and InvalidArgument codes through separate methods.

• Bug: DispatcherMap GoDoc swallowing AsDispatcherMap's comment — resolved. event_router.go:210–233 now has separate, disjoint comment blocks for the type declaration and the method.

• Minor: NewEventRoute nil check for cfg.NewMsg — resolved. event_router.go:191–193 panics at registration time.

• Minor: status.FromError fallback redundancy — resolved. Simplified to st, _ := status.FromError(err).

• Minor: silently discarded anypb.New error in handleRequest — resolved better than suggested. Rather than threading t *testing.T through, the error is now encoded as a gRPC Internal status in EncodeErrorHeaders and sent to the client, so the failure surfaces on the correct side with a clear message.

• Nit: key/key2 variable names — resolved. Now actorKey/serviceMethod.

• Nit: helloFinalizedMsg dead code — resolved. The type and compile-time assertion have been removed from e2e_test.go.

---

Minor: AwaitRPC header check order is inverted relative to its own comment

serverconn/unary_facade.go:182–194

if env.Body == nil {
    return fmt.Errorf("response envelope has nil body")
}

// Check for a server-side gRPC status error before attempting to
// unmarshal the body. Server handlers encode failures into the
// HeaderGRPCStatusB64 header rather than returning a populated body.
if rpcErr := mailboxrpc.DecodeErrorHeaders(env.Headers); rpcErr != nil {
    return rpcErr
}

The comment says "before attempting to unmarshal the body" but the DecodeErrorHeaders call happens after the nil-body check. The current handleRequest always sets body = &anypb.Any{} for errors so this works in practice, but it's still an inconsistency: a spec-compliant server that sends error headers with a nil body would hit the wrong error path and surface "response envelope has nil body" instead of the actual gRPC status.

Swapping the order would make the code match the comment and be robust against any future server that omits the body on errors:

// Check for a server-side gRPC status error before inspecting the
// body. This covers servers that set error headers with or without
// a populated body field.
if rpcErr := mailboxrpc.DecodeErrorHeaders(env.Headers); rpcErr != nil {
    return rpcErr
}

if env.Body == nil {
    return fmt.Errorf("response envelope has nil body")
}

Fix this →

---

Nit: time.Sleep as synchronization barrier (pre-existing)

serverconn/connector_test.go:274, serverconn/unary_facade_test.go:282,419, serverconn/runtime_test.go:88

These are all pre-existing and not introduced by this PR, but worth tracking. A 50ms sleep before injecting responses is a race condition on slow CI — if goroutines haven't all called RegisterWaiter yet, the test can fail spuriously. A sync.WaitGroup signaled by each awaiter goroutine before the response injection loop starts would make the synchronization deterministic. Not a blocker for this PR but worth addressing in a follow-up.

---

Positive observations

• The round-trip of EncodeErrorHeaders → DecodeErrorHeaders through the full test stack (TestE2EUnaryRPCError) closes the gap flagged in all three prior reviews. Exercising two distinct status codes (NotFound, InvalidArgument) through two separate handler methods is thorough.
• handleRequest now gracefully handles anypb.New failure by converting it to a gRPC Internal error rather than producing a silent zero-value response on the client. This is cleaner than the original suggestion of threading t *testing.T through.
• InboundActorMessage constraint in event_router.go:19–22 composing actor.Message with InboundServerMessage is clean; it puts both requirements in one place with a clear comment explaining the dual purpose.
• pullLocked correctly short-circuits on both EventSeq < acked and EventSeq < cursor, preventing acked envelopes from being re-delivered after a checkpoint restore.
• The four-phase TestE2EUnaryAndPush scenario (pre-RPC push → unary RPC → post-RPC push) gives good concurrent ingress coverage with the EventRouter and UnaryFacade both active.
• t.Parallel() on all new tests.
• No time.Sleep introduced by this PR.

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context from prior reviews
• Read current state of changed files
• Check if prior issues were resolved
• Identify any new issues
• Post comprehensive review feedback

---

All issues from the three prior reviews have been addressed. Here's the current state:

Resolved since last review

The one remaining item flagged in the third review has been fixed:

• AwaitRPC header check order (serverconn/unary_facade.go:182–193): DecodeErrorHeaders is now called before the nil-body check, matching the comment "before inspecting the body." The previous ordering was inverted relative to the comment and would have mis-classified errors from spec-compliant servers that omit the body on error responses.

---

All prior issues — confirmed resolved

Issue	Status
Bug: DispatcherMap GoDoc swallowing AsDispatcherMap's comment	✅ Fixed — separate comment blocks at event_router.go:210–233
Bug: AwaitRPC swallowing server-side errors	✅ Fixed — DecodeErrorHeaders called before unmarshal
Minor: AwaitRPC check order inverted vs. its own comment	✅ Fixed in current state
Minor: NewEventRoute nil check for cfg.NewMsg	✅ Fixed — panics at registration time (event_router.go:191–193)
Minor: status.FromError fallback redundancy	✅ Fixed — simplified to st, _ := status.FromError(err)
Minor: anypb.New error silently discarded in handleRequest	✅ Fixed — converted to gRPC Internal error and encoded in headers
Nit: key/key2 variable names	✅ Fixed — now actorKey/serviceMethod
Nit: helloFinalizedMsg dead code	✅ Fixed — removed
Gemini: require.NoError inside require.Eventually	✅ Fixed — errors return false to retry

---

Observation: env.Body accessed without nil guard in TestE2EClientFireAndForget

serverconn/e2e_test.go:421:

var joinReq hellotestpb.JoinGreetingRequest
err := proto.Unmarshal(env.Body.Value, &joinReq)

env.Body is dereferenced directly. In practice this is safe — the DurableActor path always sets the body via anypb.New(m.Message.ToProto()) in SendClientEventRequest.Encode, and inMemoryMailbox.send preserves it. A nil guard would make the test assertion self-documenting (and easier to diagnose on failure), but this is not a blocking issue.

---

Positive observations

• The full EncodeErrorHeaders → DecodeErrorHeaders round-trip is now exercised end-to-end through two distinct gRPC status codes (NotFound, InvalidArgument) in TestE2EUnaryRPCError. This closes the most significant gap from the earlier reviews.
• handleRequest's anypb.New failure path now converts to a gRPC Internal status rather than producing a silent zero-value response — cleaner than threading t *testing.T through.
• pullLocked correctly applies two independent skip conditions (EventSeq < acked and EventSeq < cursor), preventing re-delivery of acked envelopes after checkpoint restore.
• The four-phase TestE2EUnaryAndPush scenario (pre-RPC push → unary RPC → post-RPC push) gives solid concurrent ingress coverage with both EventRouter and UnaryFacade active.
• t.Parallel() on every test and subtest — no accidental serial runs.
• newTestConnectorConfig DRYs up wiring across all test files.
• InboundActorMessage constraint composing actor.Message with InboundServerMessage in one place is clean.
• No new time.Sleep synchronization barriers introduced.

LGTM — this is ready to merge.