baselib/actor+db: leaseless single-worker consume, transactional outbox - #727
Conversation
![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 pull request introduces a leaseless single-worker fast path for durable actors, optimizing performance by using read-only peeks instead of write-transaction leases when there is no competing consumer. It also folds target enqueues and outbox completions into a single transaction when supported. Feedback on these changes highlights two critical issues: first, transaction execution failures in the outbox publisher are silently ignored and should be logged; second, passing raw delivery attempts to the retry policy on the leaseless path causes an off-by-one discrepancy, which should be resolved by calculating effective attempts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err := txStore.ExecTx( | ||
| deliverCtx, false, deliver, | ||
| ); err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
If the transaction execution or commit fails (for example, due to a database conflict, lock, or connection loss), the error returned by txStore.ExecTx is silently ignored and not logged. While errors inside the deliver function are logged, commit-level failures occur after deliver returns successfully and will not be captured there.
We should log this error to ensure that transaction failures are visible and diagnosable.
if err := txStore.ExecTx(
deliverCtx, false, deliver,
); err != nil {
logger(p.ctx).WarnS(p.ctx,
"Failed to execute outbox delivery transaction", err,
"message_id", msg.ID,
"target", msg.TargetActorID,
)
return
}| @@ -1112,8 +1135,11 @@ func (a *DurableActor[M, R]) handleResultInTx( | |||
| retry, delay := a.tellRetryPolicy(err, delivery.Attempts) | |||
There was a problem hiding this comment.
On the leaseless (peeked) path, delivery.Attempts is not pre-incremented at claim time (unlike the leased path where it is incremented during LeaseNextMailboxMessage). As a result, delivery.Attempts is 0 on the first failure of a leaseless message, whereas it is 1 on the leased path.
Passing delivery.Attempts directly to tellRetryPolicy causes an off-by-one discrepancy in retry policy evaluation (e.g., incorrect backoff delays or allowing an extra retry attempt). We should calculate and pass the effectiveAttempts (adding 1 if the delivery is leaseless) to match the leased path's behavior.
effectiveAttempts := delivery.Attempts
if delivery.leaseless {
effectiveAttempts++
}
retry, delay := a.tellRetryPolicy(err, effectiveAttempts)![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.
Reviewed commit: 878de84382
ℹ️ 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".
| _, nackErr := nackMessage( | ||
| ctx, store, delivery.ID, delivery.LeaseToken, | ||
| delay, |
There was a problem hiding this comment.
In the single-worker TxBehavior path, delivery.Attempts is the pre-peek value, but this retry branch still passes it unchanged to TellRetryPolicy and then the new by-ID nack increments the stored attempts. With a common config such as MaxAttempts=5 and DefaultTellRetryPolicy, the fifth failed delivery is seen as attempt 4, so this nack raises the row to attempts == max_attempts; PeekNextMailboxMessage filters on m.attempts < m.max_attempts, so the message is never delivered again for the policy to return false or for dead-lettering to run. Use the effective attempt count for leaseless deliveries before deciding to retry, or dead-letter before nacking to the max.
Useful? React with 👍 / 👎.
6430a8a to
dedb777
Compare
878de84 to
87c3e04
Compare
ce06573 to
27a1bf4
Compare
5a88bd8 to
dcf9293
Compare
27a1bf4 to
1047332
Compare
9e6f144 to
6be5a1d
Compare
|
Finished the adversarial follow-up pass and pushed an autosquashed update (
Validation run after autosquash:
Second adversarial pass focused on stale lease metadata, in-flight attempt accounting, tx boundary observability, and the distributed ownership assumption behind |
6be5a1d to
8e93cfb
Compare
|
Follow-up correction: I had updated the package invariant docs, but missed the dedicated P-model additions:
Validation run after autosquash:
The full p-model script now compiles the P project, checks all green cases with 0 bugs, confirms the intentional negative counterexamples still find bugs, and runs the Go bridge conformance replay successfully. |
d48f908 to
bcad772
Compare
1e1ebf7 to
e0b941e
Compare
In this commit, we add Go benchmarks for the durable mailbox and FSM checkpoint write paths -- the "drip box" the OOR per-session refactor reshapes. The repo had no benchmarks before, so these establish the before-and-after baseline for the write path the refactor targets. BenchmarkDeliveryCheckpointWrite measures one checkpoint write as the state blob grows. This isolates the cost the old global OOR actor paid on every mutation: it serialized all in-flight sessions into one blob, so the per-mutation write grew with the session count and every mutation funneled through one actor. On SQLite with synchronous=full the write is fsync-bound at a few milliseconds, so the real lever is not the blob size but how many of these run in series -- which is exactly what per-session sharding parallelizes. BenchmarkDeliveryMailboxRoundTrip measures one message's full durable trip through a single mailbox (enqueue, lease, ack). A single global actor funnels every session's messages through one such serial trip; the per-session refactor spreads them across independent mailboxes, so this is the per-message floor each shard pays in parallel rather than in series. Run with: go test ./db/actordelivery/ -run=^$ -bench=BenchmarkDelivery -benchmem.
In this commit, we add BenchmarkDeliveryConcurrentActors, which runs the full enqueue/lease/ack mailbox trip across a growing number of independent mailboxes in parallel. It completes the drip-box suite: the checkpoint-write and round-trip benchmarks measure one mailbox's cost, and this one measures what happens when many run at once. This is the per-session sharding shape. The old global OOR actor funneled every session through one mailbox, so unrelated sessions queued behind each other; the per-session refactor gives each session its own mailbox. The SQLite writer is still a shared floor, so the benchmark shows write contention rather than linear scaling, which is the honest picture: the win is that no session blocks behind another's queue, and each per-session write is a small fixed-size row, not a whole-map blob.
Add PeekNextMailboxMessage (a read-only claim mirroring LeaseNextMailboxMessage's eligibility and ordering but taking no lease and not bumping attempts), AckMailboxMessageByID (unfenced delete), and NackMailboxMessageByID (unfenced release that increments attempts). These back the single-worker leaseless consume path. Regenerated via make sqlc.
On the Read/Commit path a NumWorkers==1 durable actor did two write transactions per consumed message: a LeaseNextMessage write to fence the ack against a competing worker, then the behavior Commit. A single worker has no competitor, so the lease write is pure overhead -- and because the DSN uses _txlock=immediate, every lease (even an empty poll) does BEGIN IMMEDIATE and grabs the global SQLite writer lock, serializing all concurrent pollers. Replace the lease with a read-only PeekNextMailboxMessage for single-worker Read/Commit actors: empty polls take a WAL read lock (concurrent), and the message is acked by id inside the existing Commit transaction. A crash before commit leaves the row untouched, so it is re-peeked on restart -- at-least-once preserved, identical to lease expiry. Attempts move to the by-id nack so dead-lettering is unchanged. The multi-worker egress pool (NumWorkers>1) and the classic path are byte-for-byte unchanged: they keep LeaseNextMessage and the fenced ack.
The incoming metadata path arms a give-up/backoff retry timer (ScheduleRetryRequest) alongside its query, added in fab1754. The synchronous driveIncomingOutbox test driver was never taught to handle that event type, so it tripped its default case and failed three OOR materialization systests. The driver resolves metadata immediately, so the timer never needs to fire; ignore it. Pre-existing on the branch, independent of the leaseless-consume change.
In this commit, we collapse the outbox publisher's per-message handoff from two write transactions down to one. Previously each delivery paid three commits: the batch claim, the target mailbox enqueue inside Tell, and the CompleteOutbox status flip. The enqueue and the completion always touch the same actor-delivery database, so there is no reason for them to commit separately. When the configured store supports transactions (the production wiring always does), deliverMessage now wraps the Tell and the CompleteOutbox in one ExecTx. The Tell joins the ambient transaction via the context, using the same mechanism session actors already rely on for atomic enqueue, and the completion runs on the transaction-scoped store. On any failure the whole transaction rolls back, leaving the claimed row to be redelivered after claim expiry. This also closes the window where a crash between enqueue and complete forced a redelivery that only receiver-side dedup could absorb. Stores without transaction support keep the existing two-step path. The actortest harness now builds the transaction-aware store to match production wiring, so every publisher end-to-end test exercises the folded path. A new test forces a CompleteOutbox failure after a successful Tell and asserts the rollback erases the target enqueue, then verifies exactly-once redelivery after the claim expires.
Add a P scenario and a Go bridge test that pin down the CDC outbox publisher's transactional fold (deliverMessage's ExecTx): the target mailbox enqueue and the outbox completion commit as one transaction. The P model adds an OutboxFoldSpec machine with AtomicFold (production) and SplitWrite (counterexample) profiles, plus two monitors: OutboxCompletionImpliesDelivery (no lost messages -- never complete without a durable enqueue) and OutboxTargetDeliveredAtMostOnce (exactly-once delivery via the idempotent enqueue). tcOutboxFold is green; tcOutboxSplitWriteCounterexample finds the lost-message bug. The Go bridge drives the real SQLite actordelivery store: a fold that fails after the enqueue rolls back with no orphan, the outbox row stays pending until the claim expires, redelivery lands exactly once, and a concurrent stale-token reclaim is fenced while the idempotent enqueue collapses the duplicate. check.sh runs both new cases.
Add bridge tests that model a process crash by reopening a fresh *sql.DB and store against the same on-disk SQLite file. A peeked but unacked message survives the restart with its attempt count unchanged (peek is read-only), and a leased but unacked message survives with its attempt bump durable and becomes re-leasable after ExpireLeases. Document the direct (non-trace) bridge tests in the bridge guide.
e0b941e to
691f484
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 2m 46s —— View job Review: leaseless single-worker consume + transactional outbox fold
Overall this is a careful, well-documented change. The two earlier bot findings (gemini/codex) are both resolved correctly, and the invariants are now mirrored across the package docs and the p-models. Notes below. Confirmed fixes from prior review
Design soundness
Observations (non-blocking)
Tests / modelingCoverage looks strong: the p-model adds I couldn't run |
2 participants
In this PR, we extend the durable actor runtime with two framework-layer
optimizations that came straight out of profiling the OOR payment path: a
leaseless consume path for single-worker actors, and a transactional fold for
outbox delivery.
The leaseless path attacks transaction #1 on every consume. The classic path
opens a write transaction just to lease the next mailbox message
(
LeaseNextMessageis an UPDATE under BEGIN IMMEDIATE), then a second one tocommit the turn's effects. For an actor with
numWorkers == 1there is nocompeting consumer, so the lease buys nothing: we add a read-only
PeekNextMailboxMessageplus by-id ack/nack variants and gate them strictly onthe single-worker case via a mailbox flag. Routing is keyed on the empty lease
token, so the multi-worker path is byte-for-byte unchanged. At its point in the
optimization chain this was worth ~1.6x: the peek is a pure read, so idle polls
go from one write transaction to zero and stop grabbing the single sqlite writer
under BEGIN IMMEDIATE, which is exactly what was serializing all the concurrent
session pollers. The head-to-head lives in lightninglabs/darepo#538's Experiment
6.
Because the peek takes no lease, it also doesn't pre-bump attempts; that bump
moves to nack-by-id, so a message that fails by returning an error still climbs
toward dead-lettering just like the leased path. The one case it doesn't cover
is a message that crashes the worker process on every attempt: it skips the
nack each time and so re-peeks forever instead of dead-lettering. That's
deliberate, not a gap to "fix" by bumping attempts at claim, which would put
every poll (including the empty ones that dominate under contention) back under
BEGIN IMMEDIATE and hand the 1.6x right back. The single-worker actors on this
path carry their own higher-level retry/backoff and treat the dead-letter table
as a manual sink, so retrying a process-poisoning message beats silently
dropping it. A wedged-DB nack now also backs off on the poll ticker instead of
tight-spinning re-peeks of the same row.
The outbox fold introduces
TxAwareDeliveryStoreand folds outbox delivery(enqueue into the destination mailbox + mark-complete) into one write
transaction. Before the fold those were two commits ordered for safety, which
left a crash window where an enqueued message could be redelivered because its
completion never landed; the fold closes that window structurally. Folding the
enqueue inside the publisher's tx means the row isn't visible to the target
until commit, so the in-process wake from
Sendraces ahead of visibility andfinds nothing. We restore the snappy same-process handoff with a post-commit
wake: once a folded enqueue commits, we rouse the local mailbox receive loops to
re-poll. It's a coarse broadcast (every local mailbox re-polls, a non-target
does one empty poll) with the poll ticker as the correctness fallback, so a
missed wake only ever costs a poll interval, never a message. The same
TxAwaremechanism is what the serverconn ingress fold builds on.
Rounding things out: drip-box and concurrent-mailbox throughput benchmarks for
the delivery store, and a systest driver fix so the incoming outbox driver
handles
ScheduleRetryRequest.This is part of the OOR optimization train; see each commit message for a
detailed description w.r.t the incremental changes.