serverconn: fold ingress dispatch and checkpoint into one transaction

[Roasbeef]

In this PR, we fold the serverconn ingress path's durable dispatches and
cursor checkpoint into a single write transaction. Before this, a pulled
batch of k envelopes paid k+1 commits: each dispatcher Tell was its own
EnqueueMessage commit, then the cursor checkpoint paid one more. The
commit ledger (#729) measured the connection actors' checkpoints at ~21
writes per payment, the single biggest bucket nobody's estimate had
caught.

The fold reuses the ambient-transaction pattern from the outbox fold
(#727): the dispatch Tells join the transaction through the context, so
the cursor can never run ahead of the enqueues. That is an upgrade, not
just a saving. The pre-fold design was only safe by commit ordering; the
fold makes cursor-covers-enqueues atomic. The ack-path checkpoint becomes
lazy and rides the next dispatch commit, with an idle flush for quiet
connections; a crash costs at most one redundant, idempotent AckUpTo.

One contract worth calling out for review: the fold is two-phase. In-memory
KIND_RESPONSE envelopes deliver before the transaction opens, and only
durable dispatches plus the checkpoint fold inside it. Responses are what
unary callers block on with RPC deadlines, so gating them behind the
writer lock turns write contention into payment-wide timeout collapse (we
measured exactly that with a first cut that folded everything).

The second commit adds a P model spec for the no-loss contract: a
persisted cursor must never cover an envelope whose enqueue did not
commit. The spec passes against the folded design under nondeterministic
batch sizes, rollbacks, and crashes, and ships with two counterexamples
(eager cursor after rollback, checkpoint-before-enqueue) that the checker
correctly flags, so the monitor demonstrably has teeth.

On the bench: counted writes per payment fell 77.9 to 59.8 (the fold's own
commit runs on the uninstrumented TxAware path, so the true cut is
~18-20%), p95 fell 12-20%, with 4/4 clean runs at 100% success. The
operator-side clientconn deliberately does not get this fold; its
dispatchers run synchronous handler work, and folding them held the
operator writer lock across whole handlers (darepo#552 tracks the purity
split needed first). It gets the lazy ack only, in the darepo-side PR.

This PR builds on #729 and closes out the OOR optimization train.

[gemini-code-assist]

Code Review

This pull request introduces formal P specifications and tests for the connection-actor ingress cursor, alongside a Go implementation of transactional batch dispatch (runFoldedDispatch) when using a transaction-aware delivery store. The Go tests have been updated to align with this transactional contract. The review feedback highlights a potential issue where mutating the outer newState variable inside the transaction closure could lead to incorrect state on retries, and suggests using a local copy. Additionally, it is recommended to log failures when flushing dirty checkpoints during idle periods and to add defensive nil checks in splitIngressEnvelopes to prevent potential nil-pointer dereferences.

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.

[gemini-code-assist]

In runFoldedDispatch, the outer variable newState is captured and mutated directly inside the ExecTx transaction closure. If the database transaction fails and retries (e.g., due to serialization conflicts or concurrent write retries), newState will be mutated repeatedly across retries, starting from an already-modified state rather than the original state.

To ensure correctness under transaction retries, you should initialize a local copy of the state inside the closure, perform all mutations on that local copy, and only assign it to the outer return variable at the end of the closure.

	var finalState AckState
	err := txStore.ExecTx(ctx, false, func(txCtx context.Context,
		store actor.DeliveryStore) error {

		txState := state
		if len(durables) > 0 {
			_, dispatchErr := a.dispatchBatch(
				txCtx, durables, nextCursor,
			)
			if dispatchErr != nil {
				return dispatchErr
			}
		}

		txState.AdvanceDispatch(nextCursor)
		txState.PullCursor = nextCursor

		if err := a.saveCheckpointTo(txCtx, store, txState); err != nil {
			return err
		}

		finalState = txState
		return nil
	})
	if err != nil {
		return state, err
	}

	return finalState, nil

[gemini-code-assist]

If saveCheckpoint fails during the idle flush, the error is silently ignored without any logging. While retrying on the next idle poll is fine, logging a warning on failure is highly recommended to ensure visibility into checkpoint store issues during idle periods.

Suggested change

	if ackDirty {
	if err := a.saveCheckpoint(
	ctx, state,
	); err == nil {
	ackDirty = false
	}
	}
	if ackDirty {
	if err := a.saveCheckpoint(
	ctx, state,
	); err != nil {
	a.log.WarnS(
	ctx,
	"Failed to flush dirty ack checkpoint",
	err,
	)
	} else {
	ackDirty = false
	}
	}

[gemini-code-assist]

For defensive programming, it is recommended to add a nil check for env in splitIngressEnvelopes before accessing env.Rpc to prevent potential nil-pointer dereference panics if the slice contains any nil elements.

	for _, env := range envelopes {
		if env == nil {
			continue
		}
		isResponse := env.Rpc != nil &&
			env.Rpc.Kind == mailboxpb.RpcMeta_KIND_RESPONSE

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4359f3e0f

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

[chatgpt-codex-connector]

Keep durable response fallbacks inside the fold

When a KIND_RESPONSE has no active waiter, dispatchBatch falls back to the durable dispatcher (deliverResponse returns buffered, then dispatcher(ctx, env) is called in dispatchBatch around lines 351-387). Because this new path sends every response through dispatchBatch(ctx, responses, 0) before ExecTx, durable response routes such as the OOR indexer response routes are enqueued in their own commit instead of the folded transaction. If a later durable envelope in the same pulled batch fails or the process crashes before the folded checkpoint commits, the cursor stays behind and this response is replayed, causing duplicate/out-of-order durable actor messages; only responses actually delivered to an in-memory waiter should bypass the transaction.

Useful? React with 👍 / 👎.

[Roasbeef]

@claude review this

[claude]

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

---

Review: serverconn ingress dispatch + checkpoint fold

I read the full diff against origin/main plus the surrounding ingress.go, actor.go, response_registry.go, ack_state.go, and the P model. This is a clean, well-reasoned change and the two-phase design (in-memory responses before the writer lock, durables + cursor inside one commit) is the right call — the rationale in the runFoldedDispatch and splitIngressEnvelopes doc comments is excellent and the P spec genuinely has teeth (the two counterexamples exercise both failure modes).

Status of the existing bot reviews (all against the older commit d4359f3e)

The current HEAD (c4d1b30 + 6576963) already addresses the substantive ones:

• Codex — "keep durable response fallbacks inside the fold": ✅ Addressed. splitIngressEnvelopes (ingress.go:601) only routes a KIND_RESPONSE to the pre-transaction bucket when corrID != "" && hasWaiter(corrID). A waiterless response now falls into durables and is enqueued inside ExecTx. The new TestSplitIngressEnvelopes* tests lock this in. See the one residual race below.
• Gemini — idle-flush failure should be logged: ✅ Already done. ingress.go:149-156 logs WarnS and backs off, leaving ackDirty set.
• Gemini — newState mutation on retry: Not actually a correctness bug here. AdvanceDispatch(nextCursor) is a monotonic max and PullCursor = nextCursor is an absolute assignment (ack_state.go:53-61) — both are idempotent on the same nextCursor, so re-running the closure yields an identical newState. The suggested local-copy is harmless clarity and worth adopting only as future-proofing if a relative mutation is ever added inside the closure.
• Gemini — nil-env check in splitIngressEnvelopes: Low priority; consistent with the existing dispatchBatch loop, and protobuf repeated fields don't yield nil elements.

One residual issue worth a look (P3)

TOCTOU between the split-time hasWaiter check and deliverResponse — ingress.go:538-546

splitIngressEnvelopes decides a response belongs in the fast pre-transaction bucket by calling hasWaiter(corrID) at split time. The actual delivery happens slightly later in dispatchBatch(ctx, responses, 0) → deliverResponse. If the waiter is cancelled (RPC deadline → RemoveWaiter) or TTL-pruned in that window, deliverResponse returns DeliveryBuffered rather than DeliveryWaiter, so dispatchBatch falls through to the durable dispatcher fallback (ingress.go:395) and runs dispatcher(ctx, env) with the non-transactional ctx.

That re-creates exactly the situation the fold is meant to forbid: a durable response enqueue committed in its own transaction, ahead of the folded cursor commit. The blast radius is small — the window is microseconds, and the durable mailbox dedups the eventual replay by msg_id — so this is loss-free and almost certainly dup-free in practice. But since the PR's whole thesis is "the cursor never covers a durable enqueue that didn't commit in the same tx," it's worth either documenting the carve-out or having the pre-tx response path treat a non-DeliveryWaiter result as "defer to the durable fold" instead of dispatching durably outside it. Fix this →

Notes (no action required)

• Partial-batch semantics changed, intentionally. Legacy dispatchBatch committed the prefix before a failing envelope and advanced the cursor to committedCursor+1; the fold rolls the whole batch back on any dispatch error and re-pulls it intact. That's strictly safer (atomic), but it means envelopes before a permanently-failing ("poison") envelope are re-dispatched every retry instead of being committed once. Both old and new paths block forward progress at the poison envelope and neither dead-letters it — so this isn't a regression, just worth being aware of. The repeated re-dispatch is safe because the rollback erases the partial enqueues and the durable enqueue is identity-deduped.
• TestIngress_PartialDispatch_NoDuplicateRedelivery correctly documents its own scope limit (in-memory ExecTx has no real savepoint, so it asserts cursor non-advancement, not physical enqueue erasure) and points at the P model for the atomicity proof. Good.
• TestIngress_IdleFlushPersistsAckWatermark is a nice addition — it's the convergence guard for the lazy-ack ackDirty path that the rest of the suite wouldn't otherwise cover.

Overall: looks good to me. The only thing I'd genuinely consider before merge is the TOCTOU carve-out above, and even that is P3 given the dedup backstop.

---

• Read the changed files
• Analyze the folded dispatch transaction logic
• Evaluate existing reviewer feedback (gemini, codex)
• Review the P model spec
• Post review
  · branch serverconn-ingress-fold

[Roasbeef]

@claude review this

[claude]

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

---

Review: serverconn ingress dispatch + checkpoint fold

I read the diff against origin/main (HEAD 1bc79c5 + 08db1f2) plus the surrounding ingress.go, actor.go, response_registry.go, unary_facade.go, and the P model. This is a strong change — the two-phase design (in-memory waiter delivery before the writer lock, durables + cursor inside one commit) is the right call, and the doc comments on runFoldedDispatch / splitIngressEnvelopes / deliverWaiterResponses carry the reasoning well. The P spec genuinely has teeth: both counterexamples exercise distinct failure modes (eager cursor after rollback, checkpoint-before-enqueue).

Prior bot reviews — all resolved at this HEAD

The earlier reviews were against the older commit d4359f3e. The current HEAD addresses every substantive one:

• Codex (P2) — keep durable response fallbacks inside the fold: ✅ splitIngressEnvelopes (ingress.go:598) only routes a KIND_RESPONSE to the pre-transaction bucket when corrID != "" && hasWaiter(corrID); a waiterless response folds into durables and is enqueued inside ExecTx. Locked in by TestSplitIngressEnvelopes*.
• The TOCTOU carve-out a prior review flagged as P3: ✅ Now fully fixed, not just documented. deliverWaiterResponses (ingress.go:633) delivers only to still-live waiters and returns every straggler (waiter vanished between the split peek and delivery) so the caller folds them back into the durable batch via mergeEnvelopesByEventSeq. TestDeliverWaiterResponsesDefersVanishedWaiters covers it. Good — this closes the "durable enqueue commits outside the cursor fold" hole.
• Gemini — idle-flush failure should be logged: ✅ ingress.go:149-156 WarnS + backoff, leaving ackDirty set.
• Gemini — newState mutation on retry: still present but not a correctness bug — see nit below.

One new issue worth a look (P3): waiter-delivered response re-dispatched durably on whole-batch rollback

The fold rolls the whole batch back on any dispatch error (intentional, and strictly safer than the legacy prefix-commit). But combined with the pre-transaction waiter delivery, this opens a duplicate-delivery shape the legacy path didn't have. Consider a batch [resp(corrA, live waiter, seq=1), event(seq=2)]:

1. deliverWaiterResponses completes corrA's promise in memory → the unary caller unblocks. AwaitRPC then runs its defer removeWaiter(corrA) (unary_facade.go:166), so the waiter is gone.
2. event(seq=2)'s durable dispatch fails → ExecTx rolls back → cursor does not advance.
3. After sleepBackoff, the loop re-pulls the same batch. Now hasWaiter(corrA) is false, so splitIngressEnvelopes folds resp(corrA) into durables, and dispatchBatch runs the durable dispatcher for it (ingress.go:395).

So a response already delivered to the unary caller is also dispatched to the durable EventRouter on retry. The window is not microseconds — it's the whole backoff interval, so the waiter being gone is the likely case, not the rare one.

Why it's bounded (hence P3, not higher):

• The waiter side is safe: Promise.Complete is sync.Once-guarded (future.go:46), so even a re-delivery to a still-registered waiter is a no-op.
• A real duplicate only materializes for responses whose service/method has a registered durable dispatcher (e.g. the OOR indexer query responses); a plain SendRPCRequest with no durable route just logs a "Failed to deliver response envelope" warn and advances. The durable route is the crash-replay path, so its consumer already has to be replay-idempotent.

Still, it's a genuinely new dup shape vs. the legacy prefix-commit, and the legacy path explicitly avoided it (it committed resp(seq=1) and only retried event(seq=2)). It's worth either a sentence in the runFoldedDispatch contract comment acknowledging it, or — if it matters for the OOR indexer routes — tracking which responses were delivered to a live waiter this batch and skipping their durable re-dispatch on the immediate re-pull. Fix this →

Nits (no action required)

• newState mutated inside the ExecTx closure (ingress.go:562-578, gemini's point): not a bug. AdvanceDispatch(nextCursor) is a monotonic max and PullCursor = nextCursor is an absolute assign, both idempotent on the same nextCursor, so a retried closure yields an identical newState. The in-memory store's ExecTx doesn't retry anyway. The suggested local-copy is purely future-proofing for if a relative mutation is ever added inside the closure — worth it only as defensive clarity.
• Partial-batch semantics changed intentionally. Legacy committed the prefix before the failing envelope; the fold rolls the whole batch back and re-pulls intact. Strictly safer (atomic), at the cost of re-dispatching the pre-poison prefix every retry. Both paths block forward progress at a poison envelope and neither dead-letters it, so no regression. The P3 above is the response-flavored instance of this same re-dispatch.
• mergeEnvelopesByEventSeq relies on both inputs already being event_seq-ascending (true — both derive from one ordered pass over the pulled batch). Correctly documented and tested for the empty-input cases.

Tests / P model

TestIngress_PartialDispatch_NoDuplicateRedelivery correctly scopes its own limit (in-memory ExecTx has no real savepoint, so it asserts cursor non-advancement, not physical enqueue erasure) and defers the atomicity proof to ingress_fold.p. TestIngress_IdleFlushPersistsAckWatermark is the right convergence guard for the lazy-ack ackDirty path. The P model's no-loss invariant and two counterexamples are clean.

Overall: looks good to me. The only thing I'd genuinely weigh before merge is the P3 dup shape above — and even that is bounded by sync.Once on the waiter side and replay-idempotency on the durable side.

• Read the changed files
• Analyze the folded dispatch transaction logic
• Review the P model spec + tests
• Evaluate prior reviewer feedback (gemini, codex)
• Post review
  · branch serverconn-ingress-fold

[Roasbeef]

@claude Dug into the P3 (waiter-delivered response re-dispatched durably on a whole-batch rollback). It's structurally plausible but not reachable in the current wiring — the two conditions it needs are disjoint:

• Responses with an in-memory waiter come only from UnaryFacade: RegisterWaiter has exactly two call sites, both on the low-latency SendRPC/AwaitRPC path.
• Responses that durably enqueue require a registered cfg.Dispatchers[{service,method}]; otherwise the waiterless branch is WarnS("Failed to deliver response envelope") + break — no enqueue (ingress.go:373-393).

The OOR indexer query responses named as the example are actually immune: they flow through SendUnaryRequest → the EventRouter durable route and never register a waiter, so they sit permanently in the "durable dispatcher, no waiter" set. The intersection (live waiter and durable dispatcher) is empty today, so the dup can't fire:

• low-latency RPC response → has a waiter, no dispatcher → warn + advance on the retry, never enqueued;
• durable query response → has a dispatcher, never had a waiter → delivered exactly once, durably.

It is a genuine latent edge if a future response type is ever wired into both mechanisms (which the SendUnaryRequest design does contemplate — "if no in-memory waiter exists, falls back to durable route"). Even then it's bounded: it's just the response-flavored instance of the already-accepted whole-batch-rollback re-dispatch, and the durable route is the crash-replay path, so its consumer is replay-idempotent and absorbs the dup.

So I'm not adding the cross-iteration "delivered-this-batch" tracking to guard a case that can't currently occur — that's fragile state for no live benefit. Behavior stays as-is. Good shape to flag though; the reasoning holds, just not for the example given.