Skip to content

multi: add attempt-preserving postpone semantics to the durable actor runtime - #1122

Open
Roasbeef wants to merge 15 commits into
mainfrom
actor-postpone-semantics
Open

multi: add attempt-preserving postpone semantics to the durable actor runtime#1122
Roasbeef wants to merge 15 commits into
mainfrom
actor-postpone-semantics

Conversation

@Roasbeef

@Roasbeef Roasbeef commented Aug 8, 2026

Copy link
Copy Markdown
Member

In this PR, we give durable actor behaviors a way to say "not now" without
walking an innocent message toward the dead-letter table.

The problem: a nack spends an attempt budget it did not mean to

Today a behavior that cannot handle a message has exactly one way to say so:
return an error. The consume path treats that as a failure, nacks the message,
and increments attempts. Do it enough times and max_attempts sends the
message to dead_letters.

That is the right shape when a message failed. It is the wrong shape when the
message is fine and the consumer simply is not ready: a concurrency cap is
full, a peer is still draining, an operator has not caught up. Those conditions
clear on their own, and the message that happened to arrive during the window
did nothing to deserve a shortened life. Worse, nacking spends the budget
fastest exactly when the system is busiest, since that is when the condition
holds longest.

There is a subtler trap here too. The obvious workaround is a custom
TellRetryPolicy that always answers "retry", so the message never
dead-letters. That does not work, because both the claim and the peek queries
filter on attempts < max_attempts. Suppressing the dead-letter write does not
stop the counter from climbing, so once it hits the cap the row falls out of
the eligible set entirely. It never dead-letters, exactly as promised, and it
also never redelivers again. The OOR registry had one of these, and this PR
found it going dark after ten redeliveries.

The contract: postpone

A behavior returns actor.Postpone(delay), optionally wrapped with context
(detection matches anywhere in the wrap chain), and the consume path releases
the message for redelivery after the delay with its retry budget untouched:

if len(r.incoming) >= maxIncoming {
        return fmt.Errorf("%w: %w", errIncomingAdmissionCapped,
                actor.Postpone(incomingCapBackoff))
}

The detection runs before the Tell retry policy on both execution paths (the tx
fold and the non-tx tail), a postponed message is never marked processed (so
the redelivery is not dedup-skipped), and it logs at debug rather than warn
since a postpone is control flow, not a failure.

DeliveryStore gains the fenced and unfenced pair mirroring the ack/nack
shape. PostponeMessage validates the lease token and decrements attempts to
compensate the increment the leased claim already applied, with a CASE WHEN attempts > 0 clamp so a corrupt row can never wrap negative.
PostponeMessageByID is the leaseless counterpart and leaves attempts alone,
because the single-worker peek never bumped it. Either way the budget after a
postpone is byte-identical to what it was before the delivery.

Only Tell deliveries honor a postpone. An Ask has a caller parked on the
promise, so postponing it would strand that caller for the length of the delay
with nothing to observe and no way to learn why. An Ask behavior that returns a
PostponeError gets ordinary error treatment: the promise completes with it,
and the caller decides whether to re-issue.

The tradeoff we are choosing on purpose

A postponed message never climbs toward max_attempts, so nothing dead-letters
it automatically. We have removed the only mechanism that would eventually give
up on it, and a behavior that postpones against a condition that never clears
will postpone forever.

We think that is the right default anyway, because the framework genuinely
cannot make this call. A capacity cap that clears in seconds and an operator
response that may never come deserve different horizons, and a generic attempt
counter cannot tell them apart. So the rule is explicit and documented in three
places: a behavior that postpones must bound its own horizon. Track the
wait, and return a real error once it is no longer justified so the normal nack
path can dead-letter it. Waiting forever is a legitimate choice, but it has to
be a choice.

One related interaction worth knowing: a postpone does not exempt a message
from per-correlation-key FIFO. The row is still in the mailbox, so a postponed
head still blocks its key lane for the length of the delay, and every further
postpone extends the block. Blocking stays bounded to the key, not the mailbox,
but a long backoff on a busy key is a throughput decision, not just a retry
decision.

The adopter: OOR over-cap admission

This is the OOR over-cap case from #705, and the reason the feature exists.

The oor registry bounds how many incoming receive sessions one daemon keeps
resident via ReceiveLimits.MaxConcurrentIncomingSessions, enforced at the
ensureChild choke point that every resident-making path funnels through. That
cap is a real defense: without it, an operator streaming unanswered hints over
an owned receive script could pin unbounded children, mailboxes, and rows.

But the hint that arrives while the daemon is full is ordinary traffic, and the
cap clears the moment an earlier session terminates and is reaped. Failing that
hint's turn nacked its durable message and spent one of its attempts on a
condition it did not cause, so a daemon that stayed full long enough would
eventually dead-letter a perfectly valid incoming transfer.

The registry now postpones on the Tell-driven routed paths only:
handleResolveIncoming (the hint the event router pushes) and
handleDriveEvent's lazy restore of an already-admitted session. Both wrap the
postpone alongside the existing errIncomingAdmissionCapped sentinel with a
double %w, so boot restore's skip check and the RPC surface keep matching the
sentinel while the consume path sees the postpone. StartTransferRequest is
deliberately untouched: it arrives as an Ask from the RPC layer, and it is
outgoing, so the incoming cap never applies to it. Boot restore keeps its own
treatment as well, skipping over-cap rows rather than aborting the boot.

The deferred self-transfer hint in the same registry is the second adopter, and
it is the never-dead-letter TellRetryPolicy described above. Postponing on the
same 30 second flat backoff gives the semantics that policy was reaching for,
and the registry goes back to the default Tell retry policy for everything else.

Testing

TestOORRegistryOverCapHintKeepsAttempts drives an over-cap hint through the
registry's real durable mailbox and asserts the row is still queued with
attempts == 0 after several redeliveries, that nothing dead-lettered, and
that it admits once the resident session goes terminal and frees a slot. It
fails with attempts == 3 when the postpone is removed.
TestOORRegistryOverCapAdmissionPostpones and TestOORRegistryDefaultCapBackoff
cover the error shape and the backoff fallback at the behavior level, and the
self-transfer defer test now asserts its postpone delay.

Follow-up

A separate change will adopt postpone for ErrMailboxSaturated on in-turn
sends, once #1121 (backpressure watermarks) merges. That is the other place
where a behavior currently nacks for a condition that clears on its own.

In this commit, we add the two release queries behind the new postpone
semantics. PostponeMailboxMessage is the fenced variant: the leased
claim pre-incremented attempts, so the decrement here restores the
retry budget to exactly what it was before the delivery, with a CASE
clamp so a corrupt row can never wrap negative.
PostponeMailboxMessageByID is the leaseless counterpart and leaves
attempts untouched, because the peek never incremented them.
In this commit, we regenerate the actor delivery query layer via make
sqlc to pick up the two postpone queries. No handwritten changes.
In this commit, we give behaviors a way to say "not now" without
walking a message toward the dead-letter table. A nack burns one of the
message's finite delivery attempts, so a consumer that is merely
waiting on an external condition (a capacity slot, a peer draining, an
operator catching up) dead-letters an innocent message one redelivery
at a time. Returning actor.Postpone(delay) instead releases the message
for redelivery after the delay with its retry budget fully intact.

The consume path detects the typed PostponeError before the Tell retry
policy on both execution paths (the tx fold and the non-tx tail), never
marks a postponed message processed (so the redelivery is not
dedup-skipped), and logs at debug level since a postpone is control
flow, not a failure. Only Tell deliveries honor it: an Ask has a caller
parked on the promise, so a postponed Ask would strand that caller; an
Ask behavior returning the error gets ordinary error treatment instead.

DeliveryStore gains the fenced/unfenced pair (PostponeMessage /
PostponeMessageByID) mirroring the ack/nack shape: the fenced variant
decrements attempts to compensate the lease-time bump, the by-ID
variant leaves them untouched because the leaseless peek never bumped
them. The flip side is deliberate and documented: a postponed message
never dead-letters by attempts, so behaviors must bound their own
postpone horizon.
In this commit, we stop burning a routed hint's finite delivery attempts
for a condition it did nothing to cause. When an incoming admission
arrives past MaxConcurrentIncomingSessions, the registry used to fail
the turn, which nacks the inbound durable message and walks it one
redelivery closer to the dead-letter table. Being over the cap is a
not-now condition: it clears the moment a resident session terminates
and is reaped, so the hint that happened to arrive while the daemon was
full should wait, not die.

The over-cap rejection on the routed-message path now returns
actor.Postpone(incomingCapBackoff) wrapped alongside the existing
errIncomingAdmissionCapped sentinel, so the consume path releases the
delivery on a five second backoff with its attempt budget fully intact,
while boot restore's skip check and the RPC surface keep matching the
sentinel. We apply this only where the admission is driven by a Tell:
handleResolveIncoming (the hint the event router pushes) and
handleDriveEvent's lazy restore of an already-admitted session. An
Ask-driven admission has a caller parked on the promise and gets
ordinary error treatment for a postpone anyway, and the incoming cap
never applies to StartTransferRequest, which is outgoing.

Boot restore keeps its own treatment: it already skips an over-cap row
rather than aborting the boot, and it consults ensureChild directly,
which still returns the bare sentinel.
In this commit, we retire the custom TellRetryPolicy that kept a
deferred self-transfer hint alive, because postpone now expresses what
that policy was reaching for and expresses it correctly. The policy
answered "retry after 30 seconds, always" for errSelfTransferDeferred so
the hint would never dead-letter while its outgoing session ran, and
delegated every other error to the default policy.

The problem is that a nack-with-retry increments attempts on every
release, and the claim and peek queries both filter on attempts <
max_attempts. So after ten deferrals (five minutes on the flat backoff)
the row fell out of the eligible set entirely: it never dead-lettered,
which is what the policy promised, but it also stopped redelivering,
which silently retired the crash-safety net the durable copy exists to
be. Returning actor.Postpone(selfHintRedeliveryBackoff) from the defer
branch gives the same 30 second flat wait with attempts untouched, so
the hint stays claim-eligible for as long as the outgoing session takes.

With the defer branch postponing, the policy override has nothing left
to special-case, so the registry goes back to the default Tell retry
policy and real errors keep their exponential backoff and dead-letter
boundary.
In this commit, we write down the postpone contract so the next behavior
that wants to say "not now" reaches for it instead of inventing another
never-dead-letter retry policy. The new Postpone Semantics section in
the durable actor architecture doc sets out why a nack is the wrong tool
for a transient condition, tabulates the one dimension where the two
paths differ (the attempt budget), and explains the fenced and leaseless
store mechanics: the leased variant decrements to compensate the
claim-time bump, the by-id variant leaves attempts alone because the
peek never bumped them.

Two consequences get their own treatment because they are easy to miss.
Postpone is Tell-only, since an Ask has a caller parked on the promise
that a delay would strand. And a postponed message never climbs toward
max_attempts, which means nothing dead-letters it automatically: the
framework has removed the only mechanism that would eventually give up,
so a behavior that postpones has to bound its own horizon or accept that
the message waits forever. We also spell out that a postponed head still
blocks its correlation-key lane, since the row is very much still in the
mailbox, and close with the OOR over-cap admission and the deferred
self-transfer hint as the first two adopters.

The per-package guides pick up the same material at the altitude each
one works at: baselib/actor gains the Postpone key type and four
invariants, db/actordelivery gains the two queries and the two store
methods plus why their attempt handling differs, and oor gains the
over-cap postpone invariant and the story of the retry policy it
replaced.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17b03e7d86

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

Comment thread baselib/actor/durable_actor.go Outdated
Comment on lines +1178 to +1180
_, ppErr := postponeMessage(
ctx, store, delivery.ID, delivery.LeaseToken,
delay,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject zero-row transactional postpones

When a classic actor's lease expires and another consumer reclaims the message during a long Receive, the fenced postpone updates zero rows, but this discarded row count leaves ppErr == nil. processInTransaction therefore commits the transaction—including any behavior writes made before it returned the postpone—as though the stale consumer still owned the delivery. Check the count and return ErrLeaseExpired when it is zero so the transaction rolls back instead of bypassing the lease fence.

AGENTS.md reference: baselib/actor/AGENTS.md:L29-L29

Useful? React with 👍 / 👎.

@litbot-9000

Copy link
Copy Markdown
Collaborator

📚 Doc drift advisory

Scoped audit of the packages this PR touches — baselib/actor, db/actordelivery, db/actordelivery/sqlc, oor, serverconn, unroll — found one stale doc: db/actordelivery/sqlc/CLAUDE.md still describes the generated Querier surface without the new PostponeMailboxMessage / PostponeMailboxMessageByID queries (the other five are already current in this PR — serverconn and unroll only gained test-only store stubs, which are not doc-visible).

diff --git a/db/actordelivery/sqlc/AGENTS.md b/db/actordelivery/sqlc/AGENTS.md
index 35036803..f22694cb 100644
--- a/db/actordelivery/sqlc/AGENTS.md
+++ b/db/actordelivery/sqlc/AGENTS.md
@@ -10,8 +10,14 @@ by `sqlc` from `db/actordelivery/queries/mailbox.sql`; regenerate with
 ## Key Types
 
 - `Queries` / `Querier` — generated query struct and interface (enqueue,
-  lease, peek, ack/nack, extend, expire, outbox claim/complete/fail,
-  dedup, FSM checkpoints, dead letters).
+  lease, peek, ack/nack, postpone, extend, expire, outbox
+  claim/complete/fail, dedup, FSM checkpoints, dead letters).
+- `PostponeMailboxMessage` / `PostponeMailboxMessageByID` — the
+  attempt-preserving release pair backing `actor.Postpone(delay)`. The
+  fenced variant validates `lease_token` and decrements attempts (clamped
+  at zero) to compensate the increment the leased claim applied; the by-id
+  variant is unfenced and leaves attempts untouched, because the leaseless
+  peek never incremented them.
 - `MailboxMessage`, `OutboxMessage`, `AskResult`, `FsmCheckpoint`,
   `DeadLetter`, `ProcessedMessage` — row models for the actor-delivery
   tables.
diff --git a/db/actordelivery/sqlc/CLAUDE.md b/db/actordelivery/sqlc/CLAUDE.md
index 35036803..f22694cb 100644
--- a/db/actordelivery/sqlc/CLAUDE.md
+++ b/db/actordelivery/sqlc/CLAUDE.md
@@ -10,8 +10,14 @@ by `sqlc` from `db/actordelivery/queries/mailbox.sql`; regenerate with
 ## Key Types
 
 - `Queries` / `Querier` — generated query struct and interface (enqueue,
-  lease, peek, ack/nack, extend, expire, outbox claim/complete/fail,
-  dedup, FSM checkpoints, dead letters).
+  lease, peek, ack/nack, postpone, extend, expire, outbox
+  claim/complete/fail, dedup, FSM checkpoints, dead letters).
+- `PostponeMailboxMessage` / `PostponeMailboxMessageByID` — the
+  attempt-preserving release pair backing `actor.Postpone(delay)`. The
+  fenced variant validates `lease_token` and decrements attempts (clamped
+  at zero) to compensate the increment the leased claim applied; the by-id
+  variant is unfenced and leaves attempts untouched, because the leaseless
+  peek never incremented them.
 - `MailboxMessage`, `OutboxMessage`, `AskResult`, `FsmCheckpoint`,
   `DeadLetter`, `ProcessedMessage` — row models for the actor-delivery
   tables.

How to apply: save the diff above and git apply it, or run the
doc-gardening skill locally (/doc-gardening db/actordelivery/sqlc) and let
it regenerate the same edit. Remember CLAUDE.md and AGENTS.md must stay
byte-identical.


Advisory only — this check never fails the build. Run: https://github.com/lightninglabs/wavelength/actions/runs/31230542413

In this commit, we give a postponing behavior the reference it needs to
bound its own horizon. Postpone deliberately removes the attempt-based
give-up mechanism, so the contract asks every adopter to decide for
itself when waiting has stopped making sense. That obligation is hard to
discharge without a trustworthy notion of how long the message has
already been waiting.

The mailbox row already knows. Its created_at is set once at enqueue and
no release path rewrites it: a nack moves available_at and bumps
attempts, a postpone moves available_at and restores attempts, and
neither touches the creation time. Delivery gains an EnqueuedAt field
copied from the leased row, and the consume path stamps it onto the
processing context via DeliveryEnqueuedAt. The stamp happens in
processDelivery, above the fork into the three execution paths, so the
tx fold, the non-tx tail, and the Read/Commit handle all agree.

Deriving the age from the row rather than from behavior-side state is
the point, not an implementation detail. A behavior that tracked waits
in a map would key that map on something the sender chose, and for an
attacker-controlled message stream the bookkeeping meant to protect the
actor becomes the resource the attacker grows. The row's own age costs
nothing to consult and cannot be inflated by fabricating messages.

The accessor returns a bool alongside the timestamp so absence stays
distinguishable from a zero time. A store that reports no creation time,
or a behavior invoked directly in a test, means "no horizon
information", which is not the same as "infinitely old", and a behavior
that conflated the two would give up on every message immediately.
In this commit, we close a coverage gap and a small silence in the
transaction path's postpone handling. handleResultInTx is a separate
implementation from the non-tx tail, and until now only the latter had a
test, so a regression in the tx branch would have passed the suite
untouched. The new test drives a classic behavior on a tx-aware store
through repeated postpones under a retry policy that fails the test if
it is ever consulted for a postpone, and asserts the message survives
past the point a nack would have dead-lettered it.

We also stop discarding the row count from the tx-path postpone. A zero
row count means the release did not happen, which on the leased path
means the lease expired or was claimed by another consumer, so the
attempt this delivery took stays uncompensated and the message
redelivers on the lease-expiry path rather than the requested delay. The
nack path treats a zero-row release the same way, so we keep the
behavior identical and only add a debug line: a postpone that quietly
did not happen is otherwise invisible, because the postpone path logs at
debug and never warns.
In this commit, we make the over-cap adopter live up to the rule the
postpone contract sets for every adopter: bound your own horizon. As
written, a capped hint postponed forever, which is exactly the failure
mode the contract warns about, and it mattered here more than most
because the hint stream is operator-controlled.

Every redelivery of a capped hint re-runs validateIncomingAdmission, a
wallet-ownership query against the database, and resolveSelfTransfer, a
registry-row read, before the cap check rejects it again. Both run
before the cap is consulted, so the work happens on every cycle no
matter how long the daemon has been full. An operator streaming
fabricated session ids could therefore build a churn queue against the
single-worker registry that no amount of waiting drains, with each entry
renewing itself every five seconds and none of them ever reaching a
horizon.

postponeOverCap now postpones only while the delivery is younger than
overCapPostponeHorizon, ten minutes, measured against the durable row's
enqueue time. Past the horizon it returns the plain capped sentinel, so
the ordinary nack path takes over and the hint dead-letters into a table
where it is visible and requeue-able rather than churning invisibly. Ten
minutes outlasts any realistic transient burst (the cap defaults to 1024
resident sessions and a slot frees on every terminal reap) while
converting a hostile backlog on a human timescale.

The age comes from actor.DeliveryEnqueuedAt rather than from a
per-session map, because the session ids in that stream are
operator-chosen and the map would be the very unbounded resource this is
defending. A delivery that reports no enqueue time postpones as before,
since absence means we have no horizon information rather than that the
message is infinitely old.

We also correct the backoff constant's comment. Nothing signals the
mailbox wake channel when a postponed row becomes eligible, so it is
rediscovered by the idle poll backoff and the effective redelivery gap
is roughly five to thirty five seconds, not the five the old wording
implied.
In this commit, we close an asymmetry in how the registry treats the
concurrency cap. handleDriveEvent postpones when lookupOrRestore cannot
make a session resident, but handleResumeSession returned the bare
sentinel from the identical call, so the two Tell-driven paths gave the
same condition opposite treatment.

A resume is not a lesser delivery. It arrives as a Tell from the timeout
retry callback and carries a real timer expiry that the session needs in
order to re-drive its outbox, so nacking it for a transient cap spends
its attempts and eventually dead-letters work that was never wrong.
Applying the same wrap makes every Tell-driven path that consults
lookupOrRestore agree, and the horizon added alongside it keeps the wait
bounded here too.
In this commit, we document a boundary case in the claim query's
correlation-key anti-join that only postpone can turn into a real
ordering bug, and we deliberately leave the SQL alone.

The anti-join passes over a predecessor that has exhausted its retry
budget, so a dead row cannot wedge its lane forever. A predecessor
leased on its final attempt already satisfies attempts == max_attempts
after the claim pre-increment, which means it is invisible to the
anti-join while it is still being processed and a competing worker may
claim its same-key successor. With only ack and nack available that is
harmless, because the predecessor can then only complete or dead-letter.
A postpone breaks it: the release decrements attempts back below the
cap, so the predecessor becomes eligible again and reprocesses after the
successor already ran, inverting per-key FIFO.

Reaching that state needs all three of keyed correlation lanes, more
than one worker, and a postponing behavior, and no adopter combines them
today. Complicating the hot claim query for a configuration nothing runs
would be the wrong trade, so we record the exclusion where the next
person will meet it, next to the predicate itself, along with the
intended repair: a lease-liveness disjunct so a currently-leased
predecessor blocks its successors regardless of its attempts. The sqlc
stubs are regenerated only to carry the comment through.
In this commit, we make the in-memory checkpoint store's PostponeMessage
validate the lease token, which its own NackMessage already does and the
real SQL query has always done. Without the check the mock accepted a
postpone from a stale consumer against a row another consumer now owns,
decremented that row's attempts, and reported success where the store
would have returned zero rows.

Nothing depends on the gap today, but a mock that is more permissive
than the thing it stands in for is a test that cannot fail for the right
reason, and the fence is precisely what the postpone path relies on to
keep the attempt budget honest.
In this commit, we fix a claim in the postpone documentation that was
wrong about which failure the old self-hint retry policy actually
produced, and we scope two guarantees that were stated more broadly than
they hold.

The dead-letter claim described the transaction path's behavior and
attributed it to the registry, which runs the Read/Commit path. On the
non-transaction path, which the Read/Commit tail also uses,
Delivery.Nack checks ShouldDeadLetter before releasing, so an
always-retry policy does not keep a message alive: it dead-letters at
exhaustion regardless. The go-dark shape belongs to handleResultInTx,
whose retry branch calls the store nack with no such check and pushes
the row past max_attempts into claim-ineligibility without ever writing
a dead letter. That asymmetry is a pre-existing tx-path bug rather than
anything postpone introduced, and the docs now say so and scope each
description to its path. The conclusion is unchanged and if anything
stronger: a policy override cannot express "wait indefinitely" on either
path.

The correlation-key section claimed per-key FIFO holds for postponing
consumers without qualification. It holds when the actor is
single-worker or the lane's messages never reach their final attempt,
and we now state that exclusion, explain the anti-join boundary case
behind it, and record the lease-liveness disjunct as the prerequisite
for lifting it.

We also document the horizon the framework now supports, with the
DeliveryEnqueuedAt accessor and the reason it is row-derived rather than
behavior-derived, and update the OOR adopter to describe its own ten
minute bound and the resume path that joined the postponing set.
@Roasbeef

Roasbeef commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

An adversarial audit pass ran against the first 6 commits; the 7 commits on top address what it found. Dispositions:

Fixed:

  1. Over-cap postpone had no horizon (major). The first adopter violated the PR's own bound-your-horizon rule: every capped hint retried forever at 5s, and each redelivery ran the admission validation queries before the cap check, so a hostile operator streaming fabricated-session hints could build a permanent churn queue on the single-worker registry. The horizon is now derived from the durable row's own enqueue time, exposed to behaviors via the new actor.DeliveryEnqueuedAt(ctx) helper (stateless, so fabricated distinct session ids cannot grow any map): postpone while the delivery is younger than 10 minutes, then fall back to the plain error so the nack path dead-letters the hint where the actor: wire up dead-letter handling with requeue, monitor, and operator surface #1119 tooling can see and requeue it.

  2. Keyed-lane FIFO blind spot at the max-attempts boundary (latent major). A predecessor leased at its final attempt is invisible to the claim anti-join, so a same-key successor can be claimed concurrently, and a postpone brings the predecessor back to reprocess after it. No adopter combines keyed lanes + a worker pool + postpone today, so the fix is scoping, not SQL: the docs now state the exclusion and the anti-join carries a caveat comment naming the lease-liveness disjunct as the future fix.

  3. handleResumeSession asymmetry: now wraps the capped sentinel with the same postpone treatment as the other Tell-driven handlers.

  4. Docs described the wrong failure mode for the retired self-hint policy. The go-dark-forever shape (row stranded past max_attempts, never dead-lettered) is the CLASSIC TX path's behavior; the registry's leaseless non-tx path dead-letters at exhaustion via Delivery.Nack's ShouldDeadLetter arm. Docs corrected and scoped. The pre-existing tx-path strand bug is now filed as actor: tx-path retry policy can strand a message past max_attempts with no dead letter #1124.

  5. Comment fixes (postpone delay is a floor given the idle poll backoff), a debug log on the tx-path lost-lease postpone, a lease-token fence on the serverconn test mock, and a new test covering the classic tx-path postpone branch (verified load-bearing: stubbing the branch fails it).

@Roasbeef

Roasbeef commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

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


PR Review: attempt-preserving postpone semantics

I read through the core runtime (postpone.go, durable_actor.go, delivery.go, delivery_store.go), the SQL layer (mailbox.sql), and the OOR adopter (registry.go). This is a carefully-built feature with unusually thorough docs and invariants. My review focuses on correctness; the design itself is sound.

  • Read core postpone implementation
  • Read DB layer
  • Read OOR adopter + horizon logic
  • Assess the Codex P1 finding
  • Post review

On the Codex P1 finding (zero-row transactional postpone) — I agree it's real, but calibrate the fix

The finding is correct that there is a lease-fence asymmetry on the classic tx path (handleResultInTx, durable_actor.go:1186-1212):

rows, ppErr := postponeMessage(ctx, store, delivery.ID, delivery.LeaseToken, delay)
if ppErr == nil && rows == 0 {
        // debug log only
}
return ppErr   // nil → processInTransaction commits the tx

Compare the three terminal branches in the same function:

  • Success (txDelivery.Ackdelivery.go:217): returns ErrLeaseExpired on zero rows → tx rolls back. Fence enforced.
  • Nack (nackMessage, line ~1232): ignores the row count → returns nil → tx commits. Fence not enforced.
  • Postpone (this PR): ignores the row count → returns nil → tx commits. Fence not enforced.

So postpone inherited the nack path's shape, not the ack path's. When a competing owner (another process/replica) reclaims the lease during a long Receive, this stale consumer's transaction — including any behavior writes made before it returned the postpone — commits anyway. That is a genuine hole.

Two things worth adding to the disposition, though:

1. Severity is narrow, and the PR already scopes most of it away. The classic tx path is single-worker-only (ErrConcurrentClassicBehavior), so a zero-row postpone requires cross-process/replica lease contention — a competing owner can't exist within one daemon. It also only causes damage if the behavior writes state and then postpones; the actual adopter (OOR ensureChild) postpones at the admission choke point before any write, so an empty tx commits harmlessly. This is a defense-in-depth / future-adopter hazard, not a live bug for anything in this PR. Given that, I'd rank it P2, not P1 — but it is still worth closing since the whole point of the feature is that hypothetical future adopters lean on it.

2. The suggested one-line fix (return ErrLeaseExpired) has a wrinkle — don't ship it verbatim. Returning an error routes into processInTransaction's failure handler (durable_actor.go:754-774), which calls delivery.Nack(ctx, err, 10s). On the max-attempts boundary, Nack takes the ShouldDeadLetter branch, and both MoveToDeadLetter(ctx, id, reason) and DeleteMessage(ctx, id) are unfenced (by-ID) — so a stale consumer that lost its lease could dead-letter and delete a row the legitimate owner is actively processing. Off the boundary it's harmless (the fenced nackMessage no-ops on token mismatch), but the boundary case turns a lost-lease postpone into data loss for the rightful consumer.

The correct shape is: roll the tx back without triggering the destructive nack. That needs processInTransaction to distinguish "lease lost — another owner has it, do nothing" from "genuine failure — nack for retry", e.g. a dedicated sentinel returned from handleResultInTx on zero rows that processInTransaction treats as roll-back-and-return. Same treatment would fix the pre-existing nack-path asymmetry too.

Fix this →


Things that look correct and well-handled

  • Fenced/unfenced attempt accounting is right. PostponeMailboxMessage decrements (compensating the lease-time bump) with the CASE WHEN attempts > 0 clamp; PostponeMailboxMessageByID leaves attempts alone (the peek never bumped). Net budget is byte-identical either way, and the clamp can't wrap a corrupt row negative. This is the subtle part and it's implemented correctly (mailbox.sql:186-215).
  • Detection before retry policy, on both paths (handleResultInTx:1176 and handleResult:1336), with postponeDelay matching anywhere in the wrap chain via errors.As. The double-%w in the OOR adopter keeps errIncomingAdmissionCapped matchable for boot-restore/RPC while the consume path sees the postpone — nicely done.
  • shouldMarkProcessed = false for postpone in finishNonTx:854-866 — a postponed message must not be dedup-skipped on redelivery. Correct, and easy to miss.
  • Tell-only enforcement is sound: Ask/DurableAsk never reach the postpone branch, so a parked caller is never stranded.
  • The horizon is the strongest part. Deriving the bound from DeliveryEnqueuedAt (the durable created_at, which no release rewrites) rather than a behavior-side map keyed on an operator-chosen session id is exactly right for an attacker-controlled stream — a per-message map would be the unbounded resource the cap defends. postponeOverCap (registry.go:498-519) treating "no timestamp" as "no horizon info" (postpone as before) rather than "infinitely old" is the correct failure-open choice for tests/stores that don't report one.

Minor / non-blocking

  • The keyed-lane + pool + postpone FIFO-inversion is documented as a scoped-out latent issue (anti-join m2.attempts < m2.max_attempts misses a predecessor leased on its final attempt). I agree scoping is the right call since no adopter combines all three today, and the CAVEAT comment naming the lease-liveness disjunct as the fix is the correct breadcrumb. Worth a tracking issue if there isn't one.
  • Doc drift: the advisory bot already flagged that db/actordelivery/sqlc/CLAUDE.md / AGENTS.md don't list the two new PostponeMailboxMessage* queries. Advisory-only, but trivial to apply while here.

Summary

Solid PR. The only substantive item is the Codex finding, and I'd land it slightly differently than proposed: it's a real fence asymmetry worth closing, but it's P2 (needs cross-process contention and a write-then-postpone behavior, neither of which any adopter here does), and the fix must roll back without invoking the unfenced dead-letter nack — returning ErrLeaseExpired as-is trades one narrow hazard for a worse one at the max-attempts boundary.
actor-postpone-semantics

In this commit, we stop the transaction path from committing a
behavior's writes under a row it no longer owns. The postpone and nack
branches of handleResultInTx both discarded the row count from their
fenced release, so a release that matched nothing (the lease expired
mid-turn and another consumer claimed the message) looked exactly like
success. The closure returned nil, the transaction committed, and one
consumer's state changes landed for a message a different consumer was
already reprocessing.

Both branches now interpret the count through releasedRowsInTx, which
distinguishes the two ways a zero can arise. An unfenced by-id release
matching nothing is benign, it just means the row is already gone. A
fenced release matching nothing means the lease-token comparison failed,
so the message has changed hands, and that returns errLostLeaseMidTurn
to roll the transaction back.

processInTransaction then treats that sentinel as roll-back-and-return:
no nack, no dead-letter, no processed mark. Skipping the nack is the
load-bearing half rather than a tidiness choice. Delivery.Nack
dead-letters once the attempt budget is spent, and that arm is unfenced,
running MoveToDeadLetter and DeleteMessage by id with no lease check, so
routing a stale consumer through it would let it delete a message the
legitimate owner is in the middle of processing. Rolling back and
walking away leaves the row and its outcome entirely to that owner.

The nack half of this predates the postpone work: that branch has
ignored its row count since it was written, and it shares the fence
asymmetry described in issue #1124. We fix both together because they
are the same mistake in adjacent branches and a partial fix would invite
the next reader to assume the other one was deliberate.

The regression test steals the lease from inside the behavior and
asserts the turn's writes never commit, the row is neither dead-lettered
nor deleted, it keeps the new owner's token, and it is not marked
processed. The mock's ExecTx grows real rollback semantics for a
behavior-visible write log, since without that a test cannot tell a
committed turn from a rolled-back one. The non-transaction path is
untouched: Delivery.Postpone already surfaces ErrLeaseExpired there.
In this commit, we add the two postpone queries to the generated query
layer's own package guide, which still described the mailbox surface as
enqueue, lease, peek, ack/nack, extend, and expire. Someone reading that
guide to find out what the layer exposes would have concluded postpone
did not exist there.

The entry spells out why the fenced and by-id variants treat attempts
differently, since that asymmetry is the whole point of the pair and is
easy to read as an inconsistency: the fenced query decrements to
compensate the increment the leased claim applied, and the by-id query
leaves attempts alone because the leaseless peek never applied one.
@Roasbeef

Roasbeef commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Review items addressed in 8d90e52 and 56aa61b:

P2 (lost lease mid-turn): fixed to your design constraint. A new errLostLeaseMidTurn sentinel flows from both the postpone and nack zero-row branches of handleResultInTx; processInTransaction treats it as roll-back-and-return with no nack, no dead-letter, and no MarkProcessed, so the legitimate owner's turn governs the row and the stale consumer's writes vanish with the rollback. The zero-row check is fenced-only (LeaseToken != ""): an unfenced by-ID release matching nothing is a benign already-gone duplicate, not a lost lease. The regression test steals the lease mid-turn with MaxAttempts 1 and was verified against both halves: removing the zero-row check commits the stale writes, and removing the sentinel guard reproduces exactly the hazard you predicted (the unfenced dead-letter arm deletes the row its legitimate owner holds). The nack half of the fix predates this PR and is noted as part of #1124's family.

Doc drift: fixed. The sqlc guide pair now lists both postpone queries with the fenced/by-ID attempts distinction.

Tracking issue: filed as #1125 (keyed-lane FIFO inversion at the max-attempts boundary, with the lease-liveness disjunct as the suggested fix).

CI re-running on the new head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants