oor: route transport outbox events through serverconn

[Roasbeef]

In this PR, we split the OOR client actor's outbox dispatch so that transport
events (submit, finalize, ack) are Tell'd to the ServerConnectionActor via
a TellOnlyRef instead of flowing through the monolithic OutboxHandler. This
mirrors the pattern already established in round/actor.go and solves the
tx-in-context problem (#137) for OOR transfers: once the outbox publisher CDC
layer lands, these transport messages get durable delivery for free.

The core change is in driveOutbox: before handing an outbox event to the
handler, we check isTransportEvent via an explicit type switch on the three
transport types (SendSubmitPackageRequest, SendFinalizePackageRequest,
SendIncomingAckRequest). We can't use a blanket serverconn.ServerMessage
interface assertion here because MarkInputsSpentRequest and
ScheduleRetryRequest also satisfy that interface via their ToProto methods,
yet must stay local. When a transport event matches, sendTransportEvent wraps
it in a SendClientEventRequest and Tell's it to the serverconn ref. The FSM
stays in its AwaitingX state until the server response arrives asynchronously
via DriveEventRequest, i.e., no recursive follow-ups.

When ServerConn is nil (the default today), isTransportEvent returns false
and all events fall through to OutboxHandler unchanged. All 55 existing tests
pass without modification.

On the test side, we add a mockServerConnRef that captures Tell'd messages
and a localOnlyOutboxHandler that fatals if a transport event leaks through.
TestOORClientActorTransportViaServerConn exercises the full transfer lifecycle
(start → submit → finalize → complete), verifying at each step that transport
events land in the mock while local events (signing, persistence) stay on the
handler.

Closes #138.

[gemini-code-assist]

Summary of Changes

Hello, 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 refactors the Out-of-Rollup (OOR) client actor's outbox dispatch mechanism to separate transport-related events from local events. It introduces a dedicated ServerConn reference to route transport events (like package submission, finalization, and acknowledgments) directly to the ServerConnectionActor for durable delivery, addressing a transaction-in-context problem. Local events continue to be processed by the existing OutboxHandler, ensuring backward compatibility and a clear separation of concerns.

Highlights

• Separated Outbox Dispatch: Transport events (submit, finalize, ack) are now routed through a ServerConnectionActor via a TellOnlyRef, distinct from the OutboxHandler.
• Durable Delivery for Transport Events: This change enables durable delivery for transport messages, aligning with patterns in round/actor.go and resolving the tx-in-context problem (oor: migrate OOR client actor from synchronous OutboxHandler to async OutboxPublisher #137).
• Conditional Event Routing: A new isTransportEvent function uses an explicit type switch to identify and route transport events, preventing local events from being misdirected.
• Backward Compatibility: If ServerConn is not configured, all outbox events default to the OutboxHandler, maintaining existing behavior.
• Enhanced Testing: New mockServerConnRef and localOnlyOutboxHandler types were added, along with a comprehensive test case (TestOORClientActorTransportViaServerConn), to validate the new routing logic.

🧠 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

• oor/actor.go
  • Imported the serverconn package.
  • Added a ServerConn field to ClientActorCfg to hold a TellOnlyRef for the ServerConnectionActor.
  • Implemented isTransportEvent to identify transport-related outbox events.
  • Implemented sendTransportEvent to wrap and send transport events to the ServerConnectionActor.
  • Modified driveOutbox to conditionally route transport events to ServerConn or the OutboxHandler.
• oor/actor_test.go
  • Imported the sync and serverconn packages.
  • Introduced mockServerConnRef to capture messages sent to the ServerConnectionActor for testing.
  • Created localOnlyOutboxHandler to ensure only local events are processed by the mock handler, failing on transport events.
  • Added TestOORClientActorTransportViaServerConn to verify the correct routing of transport and local events.

Activity

• No specific activity (comments, reviews, progress updates) was provided in the context.

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 refactors the OOR client actor to route transport-related outbox events through the ServerConnectionActor. This is a solid architectural improvement that aligns with existing patterns in the codebase and enables durable delivery for these messages. The changes are well-contained and backward-compatible. The new logic is thoroughly tested, ensuring that transport events are correctly diverted while local events are handled as before. The implementation is clean and well-documented. I have a couple of minor suggestions to improve the test code's robustness and style.

[gemini-code-assist]

This field assignment is split across two lines in a way that harms readability. For better code style and consistency, it would be clearer to have this assignment on a single line.

Suggested change

	FinalCheckpointPSBTs: msg.
	CoSignedCheckpointPSBTs,
	FinalCheckpointPSBTs: msg.CoSignedCheckpointPSBTs,

[gemini-code-assist]

The default case in this switch statement currently does nothing and returns. This could hide bugs if an unexpected OutboxEvent is processed by this handler, as the test would continue without any indication of the unhandled event. To make the test mock more robust, consider calling h.t.Fatalf to explicitly fail the test when an unhandled event type is encountered. This ensures that all events are either explicitly handled or explicitly marked as not-to-be-handled.

	default:
		h.t.Fatalf("unhandled local event %T", outbox)
		return nil, nil
	}

[Roasbeef]

@claude-review this

[Roasbeef]

@claude review this

[claude]

Claude finished @Roasbeef's task in 1m 36s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze implementation
• Post review

---

Overall this is a clean, well-motivated change. The architectural direction is right—splitting transport vs. local outbox dispatch mirrors the pattern in round/actor.go and the durable-envelope elimination is a welcome simplification. A few issues worth addressing:

---

Confirmed Gemini suggestions

oor/actor_test.go:506–507 — split field assignment

Agree with Gemini. The wrapped field assignment is unusual and msg.CoSignedCheckpointPSBTs fits on one line:

FinalCheckpointPSBTs: msg.CoSignedCheckpointPSBTs,

oor/actor_test.go:524–526 — silent default case in localOnlyOutboxHandler

Also agree. ScheduleRetryRequest and any future OutboxEvent would fall through silently, making test failures invisible. Should be:

default:
    h.t.Fatalf("unhandled local event %T", outbox)
    return nil, nil

Fix both →%20Line%20506-507%3A%20collapse%20the%20split%20field%20assignment%20%60FinalCheckpointPSBTs%3A%20msg.%5Cn%5C%09%5C%09%5C%09%5C%09CoSignedCheckpointPSBTs%2C%60%20to%20a%20single%20line.%202)%20Lines%20524-526%3A%20replace%20the%20silent%20%60default%3A%20return%20nil%2C%20nil%60%20case%20in%20%60localOnlyOutboxHandler.Handle%60%20with%20%60h.t.Fatalf(%22unhandled%20local%20event%20%25T%22%2C%20outbox)%60.)

---

Additional observations

actor_messages.go — nil check in DriveEventRequest.Encode is unreachable

// actor_messages.go (DriveEventRequest.Encode)
if m == nil {
    return fmt.Errorf("drive event request must be provided")
}

A pointer-receiver method called on a nil *DriveEventRequest would panic on the next line (m.SessionID), not reach this check. The guard is never reachable. Should be removed. Fix this →

actor_messages.go — OORDurableMsg adds no constraints beyond actor.TLVMessage

type OORDurableMsg interface {
    actor.TLVMessage
}

This is a nominal alias. That's fine, but the comment says it gives a "tighter, domain-specific type bound"—that's slightly misleading since it currently restricts nothing that actor.TLVMessage doesn't already restrict. If the intent is purely nominal (semantic clarity, not structural restriction), the comment should say so.

actor_test.go — mockServerConnRef.lastSendRequest redundant t parameter

func (m *mockServerConnRef) lastSendRequest(
    t *testing.T) *serverconn.SendClientEventRequest {

The mock already holds m.t from construction. Passing t again is inconsistent with other helpers in the file and the caller always passes t anyway. Consider dropping the parameter and using m.t directly.

actor_test.go — SendIncomingAckRequest path not tested end-to-end

TestOORClientActorTransportViaServerConn covers the submit→finalize→complete lifecycle but not the incoming-transfer ack path. TestIsTransportEventClassification confirms SendIncomingAckRequest is classified correctly, but there's no integration-level test that actually Tell's an ack through the mock. Not a blocker, but worth a follow-up issue or a note in the test.

actor.go — newOORActorCodec registration list is not enforced at compile time

Any new ActorMsg implementation that forgets to register itself in newOORActorCodec will silently fail at runtime (unknown message type). This is a pre-existing problem with codec-based dispatch, but since we just eliminated the envelope, the codec list is now the sole safeguard. A comment warning future contributors would help:

// newOORActorCodec creates a MessageCodec with all OOR actor message
// types registered. IMPORTANT: every type that implements ActorMsg
// must be registered here; omissions cause runtime dispatch failures
// with no compile-time warning.

[Roasbeef]

@claude review this

[claude]

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

---

Code Review

• Gather context
• Read changed files
• Analyze implementation
• Post review

---

The architectural direction here is sound—splitting transport vs. local outbox dispatch mirrors the established round/actor.go pattern, the isTransportEvent type-switch rationale is clearly documented via the compile-time assertions, and the backward-compat nil-ServerConn path is clean. A few issues worth addressing:

---

Bug: nil CallbackRef can cause panic on actor startup recovery

In darepod/server.go, signingHandler.CallbackRef is set after NewOORClientActor returns:

s.oorActor = oor.NewOORClientActor(oor.ClientActorCfg{
    ...
    // CallbackRef not set yet on signingHandler
})

// ← CallbackRef is nil between here and the next line
signingHandler.CallbackRef = oor.NewRetryCallbackRef(
    oorKey.Ref(s.actorSystem),
)

NewOORClientActor calls PrependRestartMessage then durable.Start(), which processes the restart message in a goroutine. If any checkpointed session is in a retry-waiting state, resumeRestoredSessions → driveOutbox → SigningOutboxHandler.handleScheduleRetry runs with h.TimeoutActor != nil but h.CallbackRef == nil. The timer is then scheduled with a nil callback. When it fires, req.Callback.Tell(...) panics.

Fix: move the CallbackRef assignment before NewOORClientActor, or add a nil guard in handleScheduleRetry:

if h.CallbackRef == nil {
    return nil, fmt.Errorf("callback ref not wired")
}

Fix this →

---

Lock held across Tell in timeout/actor.go

In handleSchedule, the timer goroutine holds a.mu while calling req.Callback.Tell:

// timeout/actor.go:73-99
a.mu.Lock()
defer a.mu.Unlock()
// ...
delete(a.timers, req.ID)
_ = req.Callback.Tell(context.Background(), &ExpiredMsg{...})

If the receiving actor's mailbox applies backpressure, Tell blocks and no concurrent ScheduleTimeoutRequest or CancelTimeoutRequest can proceed. The existing comment correctly captures why context.Background() is used, but does not address this. A simple fix is to capture the callback, release the lock, then Tell:

cb := entry.callback
delete(a.timers, req.ID)
a.mu.Unlock()
_ = cb.Tell(context.Background(), &ExpiredMsg{ID: req.ID})

(The defer a.mu.Unlock() would need to become an explicit Unlock call at the end of the non-timer path, or restructured.) Fix this →

---

Correcting prior review: DriveEventRequest.Encode nil check is reachable

The previous Claude review flagged the nil check at actor_messages.go:235 as unreachable. That was incorrect. In Go a pointer-receiver method on a typed-nil ((*DriveEventRequest)(nil).Encode(w)) calls the method with m == nil; the nil check fires before any field access. The code comment accurately describes this:

"The nil receiver check handles typed-nil pointers (e.g. (*DriveEventRequest)(nil)) that pass interface nil checks but would panic on field access."

No change needed here.

---

SendIncomingAckRequest server-push route not registered

registerOOREventRoutes wires SubmitPackage and FinalizePackage responses but has no entry for SendIncomingAckRequest. TestIsTransportEventClassification confirms ack is classified as a transport event, but there's no route to deliver the server's ack response back as a DriveEventRequest. If this is deferred to a follow-up, a // TODO comment here would help.

---

Minor: duplicate test outpoint across tests

TestOORClientActorTellFailurePropagation (line 897) and TestOORClientActorSubmitAcceptedNilArkPSBTEnrichment (line 729) both use Hash: [32]byte{0x04}. Since each uses a distinct ActorID and an independent delivery store, this is not a correctness issue, but using distinct hashes would make test failures easier to trace.

---

Confirmed Gemini suggestions — already applied ✓

Both Gemini suggestions are already addressed in the current code:

• FinalCheckpointPSBTs: msg.CoSignedCheckpointPSBTs is on one line (actor_test.go:509).
• The default case in localOnlyOutboxHandler.Handle already calls h.t.Fatalf("unhandled local event %T", outbox) (actor_test.go:527).

No changes needed.