baselib/actor: add abstract Mailbox interface

[Roasbeef]

This PR ports the abstract mailbox interface from lnd PR 10142, replacing direct channel usage in actors with a pluggable Mailbox interface. The implementation uses Go 1.23's iter.Seq pattern for clean message iteration and includes atomic synchronization to prevent race conditions.

The refactoring simplifies the Actor's process loop from ~50 lines to ~30 lines while enabling future mailbox strategies like priority queues or durable storage. All 15 new mailbox tests pass with the race detector, and the change is backward compatible.

[gemini-code-assist]

Summary of Changes

Hello @Roasbeef, 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 refactors the actor system by introducing an abstract Mailbox interface. This change decouples the actor's message processing logic from the underlying message queue implementation, making the system more flexible and extensible. The initial ChannelMailbox implementation utilizes Go 1.23's iter.Seq pattern for clean message iteration and incorporates atomic operations for robust concurrency control. This foundational change simplifies the actor's internal processing and paves the way for advanced mailbox features in the future.

Highlights

• Abstract Mailbox Interface: Introduced a new Mailbox interface to abstract message queuing, replacing direct Go channel usage within actors.
• Flexible Mailbox Implementations: The refactoring enables pluggable mailbox strategies, such as priority queues or durable storage, without altering the core actor logic.
• Simplified Actor Process Loop: The Actor.process loop has been streamlined from approximately 50 lines to 30 lines by leveraging the new Mailbox interface and Go 1.23's iter.Seq pattern.
• Robust ChannelMailbox Implementation: A concrete ChannelMailbox implementation was added, providing thread-safe send/receive operations, context cancellation support, and atomic synchronization to prevent race conditions.
• Comprehensive Testing: 15 new tests were added for the ChannelMailbox implementation, all passing with the race detector, ensuring the robustness and correctness of the new design.

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

[claude]

Code Review - PR #18: baselib/actor: add abstract Mailbox interface

Summary

This PR successfully ports the mailbox abstraction pattern from lnd, replacing direct channel usage with a pluggable Mailbox interface. The implementation uses Go 1.23's iter.Seq pattern and includes comprehensive test coverage. The refactoring is clean and backward compatible.

✅ Strengths

1. Excellent abstraction design: The Mailbox interface is well-defined with clear contracts for Send, TrySend, Receive, Close, IsClosed, and Drain methods.

2. Comprehensive test coverage: 15 test cases covering concurrent sends, context cancellation, draining, promises, and edge cases. Tests appear thorough and use the race detector.

3. Code simplification: The process() function is significantly cleaner, reducing from ~50 lines to ~30 lines while improving readability.

4. Thread-safety: Proper use of sync.RWMutex, atomic.Bool, and sync.Once to prevent race conditions.

5. Modern Go patterns: Leverages Go 1.23's iter.Seq for clean iteration semantics.

🔍 Issues & Suggestions

1. Critical: Context Checking Race Condition (baselib/actor/channel_mailbox.go:54-71)

The Send method checks contexts before acquiring the lock, creating a TOCTOU (time-of-check-time-of-use) race:

// Check contexts before attempting send to ensure deterministic
// behavior when contexts are already cancelled.
if ctx.Err() != nil {
    return false
}
if m.actorCtx.Err() != nil {
    return false
}

m.mu.RLock()
defer m.mu.RUnlock()

if m.closed.Load() {
    return false
}

Issue: Between the context check and the select, the context could be cancelled, yet we still attempt the send. While the select will catch this, the comment claims "deterministic behavior" which isn't fully accurate.

Suggestion: Either remove the early checks and rely solely on the select statement (which already handles context cancellation), or update the comment to be more precise about the optimization purpose rather than claiming determinism.

2. Potential Panic in Concurrent Close/Send (baselib/actor/channel_mailbox.go:66-84)

While the RWMutex prevents most send-on-closed-channel panics, there's still a theoretical window:

m.mu.RLock()
defer m.mu.RUnlock()

if m.closed.Load() {
    return false
}

// If Close() acquires write lock here and closes channel...
select {
case m.ch <- env:  // This could panic if channel was closed between check and send

Analysis: The RWMutex should prevent this since the read lock blocks Close()'s write lock. However, the code would be more defensive if it used a select with a default case for TrySend, and recovered from potential panics or restructured the locking.

Recommendation: Add a comment explaining why this is safe (the read lock held during send blocks Close() from acquiring write lock), or consider moving the channel close to happen after all senders complete.

3. Actor Shutdown Ordering Issue (baselib/actor/actor.go:122-140)

The process() function closes the mailbox after the main receive loop exits:

for env := range a.mailbox.Receive(a.ctx) {
    // Process messages
}

// The actor's context has been cancelled. Close the mailbox to prevent
// new messages and drain any remaining messages.
a.mailbox.Close()

Problem: When a.ctx is cancelled, the Receive iterator stops, but messages could still be sent to the mailbox between when Receive returns and when Close() is called. These messages won't be in the channel when Drain() runs.

Impact: Messages sent during this window will be in the channel but won't be drained to the DLO.

Recommendation: Close the mailbox first when stopping the actor (in Stop() or earlier in process()), then drain. The current order may lose messages.

4. TrySend Missing Context Validation (baselib/actor/channel_mailbox.go:89-105)

Unlike Send, TrySend doesn't check if the actor context is cancelled:

func (m *ChannelMailbox[M, R]) TrySend(env envelope[M, R]) bool {
    m.mu.RLock()
    defer m.mu.RUnlock()

    if m.closed.Load() {
        return false
    }

    select {
    case m.ch <- env:
        return true
    default:
        return false
    }
}

Question: Should TrySend also check m.actorCtx.Err() before attempting to send? Currently, it could succeed even after the actor is stopped, which may not be the desired behavior.

5. Style: Tab Width and Line Length (baselib/actor/channel_mailbox.go:35-36)

Per CLAUDE.md, tabs should be 8 spaces and lines should wrap at 80 characters:

func NewChannelMailbox[M Message, R any](
	actorCtx context.Context, capacity int) *ChannelMailbox[M, R] {

Issue: This appears correct if tabs=8, but please verify the line length when rendered.

6. Test Coverage: Missing DLO Integration Test

The tests thoroughly cover the mailbox implementation but don't test the integration with the DLO (Dead Letter Office) in the actor's process() function when draining messages.

Suggestion: Add a test that:

• Creates an actor with a DLO configured
• Sends messages to the actor
• Stops the actor while messages are still queued
• Verifies the DLO receives the drained messages

7. Documentation: Interface Contract Clarity (baselib/actor/interface.go:117-154)

The Mailbox interface documentation is good, but could be clearer about:

• Whether Send/TrySend can be called after Close() (currently returns false)
• Whether Receive can be called multiple times concurrently (probably not safe)
• Whether Drain can be called multiple times (currently yes, safely)

Suggestion: Add a "Thread Safety" section to the interface documentation specifying the concurrency guarantees.

8. Minor: Error Handling Logic (baselib/actor/actor.go:202-209)

The error determination logic could use a comment:

if !ok {
    // Determine the appropriate error based on the state.
    if ref.actor.ctx.Err() != nil {
        promise.Complete(fn.Err[R](ErrActorTerminated))
    } else {
        promise.Complete(fn.Err[R](ctx.Err()))
    }
}

Suggestion: Add a comment explaining why we check actor context first (actor termination takes precedence over caller context cancellation).

🎯 Priority Recommendations

Must Fix:

1. Issue chainbackend+chainsource: add chainsource actor and backend impl #3 (shutdown ordering) - This could cause message loss
2. Add comments explaining the locking strategy for send-on-closed prevention (Issue multI: add initial repo scaffolding #2)

Should Fix:
3. Issue #1 (context checking) - Update comments for accuracy
4. Issue #4 (TrySend validation) - Clarify intended behavior
5. Issue #6 (DLO test) - Improve test coverage

Nice to Have:
6. Issue #7 (interface documentation) - Improve developer experience
7. Issue #8 (error handling comments) - Improve code clarity

📝 Additional Notes

• The use of iter.Seq is clean and idiomatic for Go 1.23+
• The separation of concerns between Actor and Mailbox is well done
• Test names and structure follow good practices
• The PR description accurately reflects the changes

Verdict

This is a solid refactoring with a well-designed abstraction. The main concern is the shutdown ordering issue (#3) which could lead to message loss. Once the critical issues are addressed, this will be a great improvement to the codebase.

Would you like me to suggest specific code changes for any of these issues?

[gemini-code-assist]

Code Review

This pull request introduces a well-designed Mailbox interface, successfully abstracting away the message queue implementation from the actor logic. The refactoring significantly simplifies the actor's process, Tell, and Ask methods by delegating concurrency and lifecycle management to the Mailbox. The new ChannelMailbox implementation is robust and thread-safe, and it's accompanied by a comprehensive set of tests that cover concurrency and edge cases, which is excellent.

I have one high-severity comment regarding a potential bug in the Ask method's error handling, where a failed send could incorrectly result in the promise being completed with a success value. Addressing this will make the implementation more resilient.

[Roasbeef]

@codex review this

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

[claude]

PR Review: Abstract Mailbox Interface

Summary

This is an excellent refactoring that introduces a clean abstraction for the actor mailbox, replacing direct channel usage with a pluggable interface. The implementation is well-designed with proper concurrency primitives and comprehensive test coverage. The changes align well with the repository's style guidelines.

---

✅ Strengths

Architecture & Design

• Clean abstraction: The Mailbox interface is well-designed and future-proof for alternative strategies (priority queues, durable storage)
• Modern Go patterns: Great use of Go 1.23's iter.Seq pattern for clean iteration
• Simplified actor logic: Reducing the process loop from ~50 to ~30 lines while improving clarity is impressive
• Backward compatibility: The changes maintain existing semantics while enabling future extensibility

Concurrency Safety

• Proper synchronization: The use of sync.RWMutex + atomic.Bool + sync.Once is correct and prevents race conditions
• Well-documented safety: Excellent comments explaining the locking strategy in Send() (channel_mailbox.go:66-74)
• Race detector tested: PR mentions all 15 tests pass with race detector

Code Quality

• Excellent documentation: Function comments follow the "starts with function name" convention
• Literate programming: Comments explain why and how, not just what (as per development_guidelines.md)
• Logical stanzas: Code is properly organized with blank lines between logical sections
• Test coverage: 612 lines of comprehensive tests covering edge cases, concurrent access, and lifecycle management

---

🔍 Issues & Recommendations

Critical Issues

None found. The implementation is solid.

Minor Issues & Suggestions

1. Potential Message Loss Window (Low Risk)

Location: actor.go:149-153

The comment at line 149 explains that messages cannot be lost between Receive() exiting and Close(), but there's a subtle edge case to consider:

// Note: Messages cannot be lost between Receive() exiting and Close() being
// called because Send() checks actorCtx.Err() first, failing fast after
// context cancellation. Any message that passes the actorCtx check before
// cancellation will either complete its send or see actorCtx.Done() in the
// select and return false.

Analysis: While the reasoning is sound, there's a theoretical race where:

1. Send() passes the context check at channel_mailbox.go:62
2. Context is cancelled immediately after
3. Send() acquires read lock and proceeds to select statement
4. Message is enqueued in the channel
5. Meanwhile, Receive() exits and Close() is called

However, this is actually safe because:

• The select in Send() (line 84-93) checks actorCtx.Done() as one of the cases
• Even if the message enters the channel, Drain() will catch it

Recommendation: The current implementation is correct. Consider adding a test case that explicitly validates this race condition scenario to document the behavior.

2. Error Handling in Ask() Could Be Clearer

Location: actor.go:222-230

The fallback error case seems overly defensive:

if err == nil {
    // This indicates an unexpected state: the send
    // failed, but neither the actor nor the caller
    // context appears to be done. Default to
    // ErrActorTerminated as the most likely cause
    // (e.g., mailbox was closed directly).
    err = ErrActorTerminated
}

Question: Can this actually happen in practice? The mailbox's Send() should only return false if:

1. Caller context is cancelled → ctx.Err() != nil
2. Actor context is cancelled → actorCtx.Err() != nil
3. Mailbox is closed → But close happens after actor context cancellation

Recommendation: This defensive code is fine, but consider adding a comment about whether this path is actually reachable or if it's purely defensive programming.

3. TrySend Actor Context Check Ordering

Location: channel_mailbox.go:99-105

The actor context check happens before acquiring the lock:

if m.actorCtx.Err() != nil {
    return false
}
m.mu.RLock()

This creates a TOCTOU (Time-of-Check-Time-of-Use) situation where the actor could be cancelled between the check and the lock acquisition. However, this is not a bug because:

• The check is an optimization for the common case
• The worst case is attempting a send to a channel that gets closed, but the RWMutex prevents panic
• The semantic behavior is still correct

Recommendation: Consider adding a comment explaining this is a fast-path optimization, similar to the comment in Send().

4. Style: Line Length in Tests

Location: mailbox_test.go:330-332

Some test lines exceed 80 characters (e.g., line 330). While the CLAUDE.md notes this is "best effort," consider wrapping for consistency:

// Current:
mailbox := NewChannelMailbox[*testMessage, string](
    actorCtx, totalMessages,
)

// Suggestion: Already good! This follows the wrapping convention.

Actually, on inspection, the test file generally follows good line wrapping. No action needed.

5. Test Coverage Gap: Send/Close Race

Location: mailbox_test.go

While there's excellent test coverage overall, consider adding a test that explicitly validates the race condition between concurrent Send() operations and Close(). Something like:

// TestChannelMailboxConcurrentSendAndClose validates that concurrent sends
// and close operations don't cause panics or data loss
func TestChannelMailboxConcurrentSendAndClose(t *testing.T) {
    // Launch many senders, then close mailbox mid-flight
    // Verify no panics and all accepted messages are received
}

The existing TestChannelMailboxConcurrentSends comes close but doesn't test concurrent close.

---

🔒 Security Considerations

No security concerns identified. The synchronization primitives are correctly used to prevent:

• Race conditions (verified with race detector)
• Panics from sending to closed channels (RWMutex protection)
• Deadlocks (no lock held while blocking on channel operations... wait, see below)

⚠️ Potential Issue: Lock Held During Blocking Send

Location: channel_mailbox.go:75-93

The Send() method holds the read lock while performing a potentially blocking channel send:

m.mu.RLock()
defer m.mu.RUnlock()
// ...
select {
case m.ch <- env:  // This can block indefinitely if channel is full!
    return true
case <-ctx.Done():
    return false
case <-m.actorCtx.Done():
    return false
}

Analysis:

• If the channel is full, this goroutine will block while holding the read lock
• Multiple senders can hold read locks concurrently, so this doesn't prevent other sends
• However, Close() acquires a write lock, which will block until ALL readers release
• This means Close() could be delayed waiting for a blocked send operation

Impact:

• Low to Medium: Close() may be delayed if senders are blocked on a full mailbox
• However, when actorCtx is cancelled (which happens before Close()), the actorCtx.Done() case will unblock the send

Recommendation:
This is actually acceptable behavior! The actor context cancellation will unblock sends before Close() is called. The current implementation is correct, but consider adding a comment in Close() noting that it may block briefly waiting for in-flight sends to complete (which will happen quickly due to context cancellation).

---

🎯 Performance Considerations

Excellent Optimizations

1. Lock-free closed check (atomic.Bool for IsClosed())
2. Early context checks before lock acquisition (channel_mailbox.go:55-63)
3. RWMutex for concurrent reads allowing multiple simultaneous sends

Potential Optimization (Very Minor)

The double context check in Send() (lines 59-63 before lock, line 78 after lock) is intentional but creates redundancy. The performance impact is negligible, and the clarity benefit outweighs any micro-optimization.

---

📝 Documentation & Comments

Excellent Examples

• channel_mailbox.go:66-74: Detailed safety explanation
• actor.go:149-153: Message loss prevention reasoning
• interface.go:126-133: Thread safety contract clearly documented

Minor Suggestion

Consider adding a package-level comment to channel_mailbox.go explaining:

• Why both atomic.Bool and sync.RWMutex are needed
• The overall concurrency model

Example:

// Package actor provides an actor model implementation with pluggable mailboxes.
//
// Concurrency Model:
// - ChannelMailbox uses sync.RWMutex to prevent send-on-closed-channel panics
// - atomic.Bool provides lock-free closed state checks
// - Multiple senders can proceed concurrently (RWMutex read locks)
// - Close() blocks all sends by acquiring write lock

---

✅ Testing Assessment

Coverage: Excellent (15 tests, 612 lines)

Well-tested scenarios:

• ✅ Basic send/receive operations
• ✅ Context cancellation (both caller and actor)
• ✅ Closed mailbox behavior
• ✅ Drain functionality
• ✅ Concurrent sends (10 goroutines × 100 messages)
• ✅ Zero capacity default behavior
• ✅ Promise handling in envelopes
• ✅ Actor-to-DLO message drainage

Missing test (minor):

• Concurrent send and close (mentioned above)
• Very high contention scenario (100+ concurrent senders)

Test Quality

• Proper use of t.Parallel() for independent tests
• Clear test names following Go conventions
• Good use of timeouts to prevent hanging tests
• Race detector compatibility confirmed

---

📋 Checklist Against Repository Guidelines

Guideline	Status	Notes
Function comments start with function name	✅ Pass	All functions properly documented
Comments explain WHY not WHAT	✅ Pass	Excellent literate programming style
80-character line limit (best effort)	✅ Pass	Generally followed, reasonable exceptions
Logical stanzas with spacing	✅ Pass	Code is well-organized
Structured logging (not applicable)	N/A	No logging in this code
Error levels (not applicable)	N/A	No error logging
Test coverage	✅ Pass	Comprehensive tests, race detector clean
No generated code edits	✅ Pass	All hand-written code

---

🎉 Final Verdict

Recommendation: APPROVE with minor suggestions

This is high-quality code that demonstrates:

• Strong understanding of Go concurrency primitives
• Careful attention to edge cases and race conditions
• Excellent documentation and literate programming
• Comprehensive test coverage
• Clean abstraction that enables future extensibility

The few suggestions above are minor improvements rather than blockers. The PR is ready to merge.

Action Items (Optional, Non-Blocking)

1. Consider adding a test for concurrent send/close race
2. Add a note in Close() about briefly blocking for in-flight sends
3. Consider package-level concurrency model documentation

Great work! 🚀