MailboxRPC 1/7: durability foundation - #48
Conversation
Summary of ChangesHello @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 introduces a robust, fault-tolerant actor system designed to prevent message loss and ensure consistent state across crashes. It achieves this by integrating durable messaging patterns such as persistent mailboxes, transactional outboxes for inter-actor communication, and FSM state checkpointing. The new system provides developers with a reliable foundation for building resilient distributed applications, abstracting away complex recovery logic and guaranteeing message processing semantics. Highlights
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive pull request that introduces a well-designed durable actor framework. The separation of concerns into layers like the DeliveryStore, DurableMailbox, and OutboxPublisher is excellent. The use of a transactional outbox (CDC pattern) and lease-based delivery provides strong guarantees for fault tolerance and exactly-once processing semantics. The addition of DurableAsk for crash-safe request-response is a significant feature. The code is accompanied by exceptionally thorough tests, including property-based tests, and high-quality documentation that clearly explains the architecture and developer usage. I have found one critical issue regarding a TLV type collision that must be addressed, and one medium-severity point for clarification. Overall, this is a very strong contribution.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| @@ -0,0 +1,29 @@ | |||
| -- Rollback durable mailbox migration. | |||
There was a problem hiding this comment.
couldn't these migrations live in the actor package itself? golang-migrate supports reading from multiple filesystems and versions via the migrations table config.
There was a problem hiding this comment.
Sure, just was following the pattern of putting all the db stuff in a single location. I kinda like them to live here as the actor package just declares interfaces that we create/satisfy elsewhere, then pass them in.
|
|
||
| -- callback_actor_id is set for DurableAsk messages to route the response. | ||
| -- The response will be delivered to this actor's mailbox via outbox. | ||
| -- NULL for regular Ask/Tell messages. |
There was a problem hiding this comment.
NULL for regular Ask/Tell messages. wouldn't these not be persisted though?
There was a problem hiding this comment.
Tell messages are still persisted in the outbox to be delivered. This is for cases where the actor that 's processing needs to write another message into the mailbox for a response.
|
|
||
| -- promise_id is set for Ask messages to track the response. | ||
| -- NULL for Tell (fire-and-forget) messages. | ||
| promise_id TEXT, |
There was a problem hiding this comment.
I wonder if we really need to persist ask messages or if they could be dropped in general. As a durable tell can just retrigger a new tell in the receiving actor and the tell would carry the callers id?
There was a problem hiding this comment.
That's a good question, we may not actually need them at all depending on exactly what the actor needs. I added it as I realized there was a gap where on restart, the destination would process the ask, but then the response would never make its way back to the sender.
I can remove it if we don't think we need it at all.
The case where it's helpful is if after writing a message to an outbox, the actor reliably expects a response to come back across. You can emulate this with a double tell basically, but you'd need to include the "sender address" in the tell, then for the sender to know that this is a response to a request it sent, and not just any other event.
| @@ -0,0 +1,51 @@ | |||
| package actor | |||
|
|
|||
| // TxEnvironment is an interface that environments can implement to support | |||
There was a problem hiding this comment.
This really is a cool feature!
There was a problem hiding this comment.
Yeah this is a big part of the "magic". It's what lets the actors be written basically as normal, ensuring that all their db queries/modifcations run in the same context as the outbox interaction.
bhandras
changed the title
bhandras
changed the title
ba42f3c to
3a5dbec
Compare
|
@codex review pls |
|
To use Codex here, create a Codex account and connect to github. |
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
Added a few comments i consider higher importance. It's a huge PR so still going through it in detail which means i might add more over time as i process things but hope this is already enough as a start.
This commit introduces a comprehensive end-to-end test suite that validates the durable actor system using a simple counter actor as the test subject. The tests exercise the full stack from message delivery through persistence and back. The counter actor maintains an integer value and responds to Inc, Dec, Get, and Reset messages. Despite its simplicity, this actor provides sufficient complexity to test all durability features: state that must survive restarts, messages that must not be processed twice, and request-response patterns that span crashes. The test suite covers several categories of scenarios: Basic operation tests validate that the counter increments, decrements, and reports values correctly under normal conditions. These establish baseline functionality before testing failure modes. Durability tests simulate crashes by stopping and restarting actors, verifying that persisted messages are redelivered and that FSM state restores from checkpoints. This validates the restart flow including RestartMessage processing and deduplication. Exactly-once tests send duplicate messages and verify the counter only reflects one increment. This validates the deduplication layer using the processed_messages table. DurableAsk tests verify that request-response works across restarts. A caller sends a DurableAsk, the target crashes before responding, and after restart the response still arrives at the caller's mailbox. Property-based tests using rapid generate random message sequences and verify invariants hold regardless of ordering. This provides confidence that edge cases in timing and sequencing are handled correctly. The messages implement TLVMessage with compact encoding for efficient storage. The test harness provides helper methods for setting up actors with in-memory SQLite databases, simplifying test isolation.
This commit adds comprehensive documentation for the durable actor system across three files targeting different audiences and use cases. The actor_delivery_store.md in the db directory documents the database schema with an entity-relationship diagram showing how the six tables relate to each other. It explains each table's purpose, column semantics, and the indexes that support efficient queries. This serves as a reference for database administrators and developers debugging persistence issues. The durable_actor_architecture.md provides a deep dive into the system's design decisions and patterns. It covers the CDC pattern with transactional outbox, explaining why atomicity between FSM state and outgoing messages matters. The lease-based delivery section explains the tradeoffs in lease duration and heartbeat intervals. The crash recovery section walks through the restart flow with sequence diagrams. The DurableAsk pattern gets special attention, explaining how CallbackActorID and CorrelationID work together to route responses. The durable_actor_quickstart.md targets developers who need to make their actors durable. It opens with what the runtime handles automatically versus what developers must implement. The TLVMessage implementation section provides copy-paste examples for encoding and decoding. The codec registration section emphasizes that AskResponse must be registered for DurableAsk callers. The RestartMessage section explains the priority-based recovery flow. Throughout, the document uses mermaid diagrams to visualize message flow and component relationships.
This is a single fixup commit to make make lint pass after rebasing\nPR48's durability branch onto current main.\n\nChanges include minor formatting, line wrapping, and error handling\nadjustments surfaced by the linter.
Durability Branch Deep Review (Codex 5.3)
Method and ValidationReviewed code paths and schema from first principles against crash-stop, retry, and concurrent-delivery failure models. Focused on:
Validation commands run:
Executive SummaryThis branch is a substantial and thoughtful durability foundation. The lease model, persistence interfaces, tests, and docs are all strong. However, there are several correctness gaps where documented guarantees (especially "exactly-once processing" and "crash-safe DurableAsk") are not currently met under realistic failure modes. Most important: current outbox delivery and Findings (Ordered by Severity)1) Critical:
|
…tion In this commit we address two closely related safety issues in the Delivery type identified by the Codex 5.3 deep review: The heartbeat goroutine calls Extend and reads LeaseUntil concurrently with the main processing goroutine calling Ack or Nack. A sync.Mutex now guards all accesses to the mutable acked and LeaseUntil fields, preventing data races detected by -race. Promise completion (d.Promise.Complete) was previously called inside Ack before the durable ack succeeded. If AckMessage failed or the transaction rolled back, callers would observe a "successful" result for an operation that was never durably committed. The promise is now completed only after AckMessage succeeds. A new deferPromise flag allows the transaction path in DurableActor to suppress in-Ack completion entirely, handling it after ExecTx returns.
Two correctness issues from the Codex review are fixed here: DurableAsk messages that fail their outbox write were previously logged and then acked, permanently dropping the response while the sender believes the request was processed. Both processWithoutTransaction and handleResult now nack for retry when the outbox write fails, giving the actor a chance to succeed on redelivery. The transaction path (processInTransaction) now sets deferPromise on the delivery before entering ExecTx, suppressing the in-Ack promise completion. The behavior result is captured outside the closure and the promise is completed only after ExecTx returns successfully. If the transaction rolls back, the delivery is nacked and no promise completion leaks to the caller. Additionally, DurableAsk messages in the non-tx path now defer MarkProcessed until after the outbox write succeeds in handleResult, preventing a scenario where the message is marked "done" but the response outbox row was never written.
…edup Three mailbox safety improvements from the Codex 5.3 review: Poison messages (decode or type-cast failures) were previously nacked indefinitely, stranding them in the mailbox. A new handlePoisonMessage method checks attempts against max_attempts and dead-letters exhausted messages instead of retrying forever. Messages not yet at max attempts are nacked with a 60s backoff for transient issues. The promise registry leaked entries when EnqueueMessage failed. After a failed enqueue, Send now removes the registry entry so repeated failures don't accumulate unbounded stale promises. Outbox delivery retries could create duplicate inbox messages because the OutboxPublisher's Tell generated a fresh UUID on each attempt. The publisher now injects the outbox row ID into the context via WithOutboxID, and DurableMailbox.Send uses it as the inbox message ID when present. The EnqueueMailboxMessage SQL query gains ON CONFLICT (id) DO NOTHING so duplicate inserts are silent no-ops, matching the pattern already used by InsertAskResult.
Generated by make sqlc after adding ON CONFLICT (id) DO NOTHING to EnqueueMailboxMessage.
Fourteen new tests covering the five fixes from the Codex 5.3 deep review, all passing with -race: DurableAsk outbox safety (Fix #1): - TestDurableAskNacksOnOutboxWriteFailure Promise completion ordering (Fix #3): - TestPromiseCompletionDeferredUntilAfterAck - TestPromiseNotCompletedOnAckFailure - TestPromiseCompletionDeferredInTxPath - TestPromiseNotCompletedOnTxFailure Delivery mutex (Fix #4): - TestDeliveryConcurrentExtendAndAck - TestDeliveryConcurrentExtendAndNack Poison message handling (Fix #5): - TestDurableMailboxPoisonMessageDeadLetter - TestDurableMailboxPoisonMessageNackBeforeMax Promise registry cleanup (Fix #8): - TestDurableMailboxPromiseRegistryCleanupOnEnqueueFailure Outbox ID deduplication (Fix #2): - TestDurableMailboxSendUsesOutboxIDFromContext - TestDurableMailboxSendDuplicateOutboxIDIsIdempotent - TestDurableMailboxSendWithoutOutboxIDGeneratesFreshID - TestOutboxPublisherPropagatesOutboxID The mock delivery store is updated with ON CONFLICT DO NOTHING semantics for EnqueueMessage (matching the real SQL) and per-operation error injection fields for outbox and enqueue failures.
Durability Review Fixes PushedFive commits addressing findings from the Codex 5.3 deep review:
|
Two issues found by Codex round-2 review: The txDelivery created inside handleResultInTx did not copy the deferPromise flag from the original delivery. This meant txDelivery.Ack completed the in-memory promise inside the ExecTx callback, before the transaction committed. The flag is now propagated so promise completion is correctly deferred to processInTransaction's post-commit block. DurableAsk messages in the tx path wrote the response to the outbox but then fell through to the Tell retry policy. If the behavior returned an error, the message would be nacked and reprocessed, producing duplicate outbox responses for the same correlation ID. DurableAsk is now handled as a self-contained block in handleResultInTx that writes the response, marks processed, and acks in one path with no fallthrough to the Tell retry logic. Two regression tests added: - TestTxPathDeferPromisePropagatedToTxDelivery - TestTxDurableAskDoesNotRetryAfterOutboxWrite
The errcheck linter flagged three unchecked Tell return values introduced during the durability branch rebase. Each call site now logs a warning on failure rather than silently discarding the error.
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
LGTM pending last set of codex comments.
|
Re: Agreed this is worth fixing — Tracking this as a follow-up since it touches the |
Move AckMessage (lease validation) before SaveAskResult in Delivery.Ack to prevent stale lease holders from persisting results. Previously a stale holder could race with a new lease holder: the stale Ack would succeed because SaveAskResult used ON CONFLICT DO NOTHING, silently poisoning the result. Now the lease check gates the write.
Address two PR review comments: 1. Change all timestamp columns from INTEGER to BIGINT in the durable mailbox migration so sqlc generates int64 instead of int32. Update ActorDeliveryStore and TxActorDeliveryStore to remove unnecessary int32 casts (Unix() already returns int64). 2. Add claim_token and claimed_until columns to outbox_messages for concurrent publisher safety. ClaimOutboxBatch now sets a per-batch UUIDv7 claim token with configurable lease duration (default 30s). CompleteOutbox and FailOutbox validate the claim token to prevent stale publishers from mutating messages they no longer own. Update DeliveryStore interface with OutboxClaimParams struct, OutboxPublisher with ClaimDuration config, and all tests.
[2/?] batch: add various VTXO tree helpers
4 participants
In this PR, we implement a fault-tolerant durable actor layer. This is intended to slot right into the existing actors that we already use today. It handles a lot of book keeping under the hood to make sure that things are done safe.
This is quite a large PR (even granted it's mostly tests, the auto gen sql code, and the docs), so I'll split it up, but I wanted to put it all up at once so it was easy to navigate, etc.
TLV Codec Layer
In order to use this new framework, all messages that need to be written persistently will implement a new TLV-based interface and register that with the codec. The codex is used to encode/decode messages to/from the persistent outbox that all actors now maintain.
This may also cause a shift in the way we make the struct hierarchy of messages. Right now each package has their own message type/interface. We may end up actually merging this into a single package, then use type aliases in each actor package so it's easy to find which messages a given actor actually cares out.
See this getting started guide re the updates needed to existing actors to start using this new framework, and items that devs need to keep in mind.
DurableActorTransactional Message ProcessingA
DurableActor's message processing will take place in a single database transaction, so we'll want to think carefully about any network access to blocking that takes place. Expansive changes in existing actors (that need to be durable) shouldn't be required.The underlying actor framework does a lot of the heavy lifting here. It handles reading from the persistent mailbox, ACK'ing messages after processing has finished, suppressing duplicate messages, handling retransmission, etc, etc.
The main call site that'll change slightly is when an actor is processing the outbox messages from the FSM. As mentioned above, as everything is run in a single db transaction, if the actor crashes at any point, post restart it'll consume the last un ACK'd message, resuming as normal.
There's also a special
Restartmessage sent by the runtime when an actor is restarted. The actor should process this, then do any sort of init activities such as requesting for confirmations, etc.Actors can also optionally checkpoint some state. We may want to use this to checkpoint actual attributes in the actor, leaving the FSM state to dedicated DB methods (as the check pointed state is just TLV atm).
Along the way,
Tellnow returns an error, as it's possible we fail to write to the db for w/e reason.OutboxPublisherThe
OutboxPublisheris a key component of the entire architecture. There's a single global instance of this atm, but it's designed s.t we can have many instances of them. It handles taking a message from the outbox of an actor (in reality there's actually a global outbox) and writing it to the mailbox/inbox of the target actor. It'll continue to delver on start up until a message is fully ACK'd by the destination actor. This is also used to implement durable asks as mentioned below.DurableAsksThe final gap is to make sure that asks are durable. Today, the outbox abstraction is basically a
Tell. It's set and forget, and doesn't allow for responses w/o a custom flow.Durable acks fill this gap. When you need to reliably hear back about the response to a message (eg: asking an actor actor for a signature of something, or transaction publish, etc) you should reach for this. It's like a normal ask, but you also provide the response ID of the actor. A durable ask doesn't return a future like a normal ask. Instead, the response comes back across as a normal message for the actor.
This reuses the
OutboxPublisherlayer to handle delivery until the response is ack'd, etc, etc.actortestpackageA new
internal/actortestpackage has been added to test all the relevant scenarios and invariants with a real database. It can also double as some example code re using all the functionality mentioned above.Fixes https://github.com/lightninglabs/darepo/issues/4