feat: workflow engine mailbox - #20091
Open
vxkc wants to merge 65 commits into
Open
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 281 files, which is 181 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (281)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
skip-releasenotes
vxkc
marked this pull request as draft
August 20, 2026 13:48
vxkc
marked this pull request as ready for review
August 20, 2026 18:18
A coarse BackgroundService closes open mailboxes past their deadline by running exactly the routine DELETE runs, with disposed_reason = 'deadline'. The FOR UPDATE SKIP LOCKED claim is the mailbox row lock the closure requires, one transaction per mailbox, and a close that throws is contained to its own mailbox rather than wedging the batch behind it. A tick drains — SweepBatchSize bounds the statement, not the tick — so the deadline-to-close gap really is at most one cadence rather than ceil(overdue / batch) cadences. There is no second half: nothing is enqueued, because the receiver that concludes the exchange already exists and is released rather than created. Retention purges closed mailboxes past the cutoff with their deliveries and waiters, children first — an order the RESTRICT foreign keys enforce. The sweep runs on its own coarser cadence, EngineSettings.MailboxSweepInterval, so the term MaxMailboxTimeout's derivation charges for it is repointed there in both places step 1 named. The setting carries no property initializer, so Defaults is the only source of its value and the tripwire that reads Defaults guards the number the engine actually runs on. Step 5's dashboard half is split out as step 5b.
…d observability gaps)
…SS fix; step 7 scoped)
vxkc
force-pushed
the
feat/workflow-engine-mailbox
branch
from
August 20, 2026 18:51
72f48c1 to
6239611
Compare
The stack (wnoznnum..trtzopwn) landed roughly 8 200 lines of comment across 197 files, much of it multi-paragraph prose restating the code beside it. This trims them to short statements and deletes the ones that said nothing the name did not: essays collapsed to one or two lines, XML-doc <remarks> folded into their summary or dropped, Arrange/Act/Assert markers and "Gets or sets the X" property docs removed, and per-test preambles cut where the test name already says it. Comment lines added by the stack drop from ~8 200 to ~4 900.
Delete the mailbox proposal — a design record whose implementation plan is complete and whose shipped behavior is specified in the technical guide — and cut the engine AGENTS.md mailbox bullets to a pointer plus the agent-operational invariants (lock discipline, Held's semantics, the birth/idempotency rules, the frozen rendezvous read, the closure routine, the token-lifetime coupling, retention order). What the bullets alone carried moves into the technical guide first, so nothing is lost: the mailbox metrics table with tag values and alert semantics (rendezvous violations, the open.overdue gauge), the mailbox settings reference, and the dashboard mailbox view. technical-guide.md is now prettier-formatted — the md format hook is a git-side lefthook, so jj-made commits had let it drift.
A readability pass over what the mailbox stack added: the same structure, the same headings (every anchor is linked from AGENTS.md and within the guide), every fact, invariant, and warning retained — each now stated once, with its justification compressed to the clause that earns its place. Connective rhetoric and restatements are gone; tables, examples, and the subtle passages (the frozen-meaning rule, the single-snapshot read, the critical-versus-retryable choice) keep their full reasoning. Audited for information loss: every code span, metric name, setting, and status code from the old text survives, checked mechanically.
vxkc
force-pushed
the
feat/workflow-engine-mailbox
branch
from
August 20, 2026 20:01
6239611 to
a3a1da8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private bool PrintMembers(StringBuilder builder) | ||
| { | ||
| builder.Append("InstanceDataMutator = ").Append(InstanceDataMutator); | ||
| builder.Append(", CancellationToken = ").Append(CancellationToken.ToString()); |
| _logger.LogWarning( | ||
| "Workflow engine reported no mailbox to close. URL: {Url}. The mailbox was purged, or it was never " | ||
| + "minted in this namespace.", | ||
| url |
| _logger.LogError( | ||
| "Workflow engine mailbox close failed with status {StatusCode}. URL: {Url}. Response body: {Body}", | ||
| response.StatusCode, | ||
| url, |
|
|
||
| | Outcome | Response | | ||
| | --------------------------------------- | ---------------------------------------------- | | ||
| | Appended at a new position | `202 Accepted` with the assigned `idx` | |
Contributor
There was a problem hiding this comment.
We're returning 201 from POST /workflows, where 202 was reserved for truly async tasks like POST /cancel. Food for thought.
| does not exist. The dashboard's _Retry now_ / _Check now_ buttons drive the same operation through | ||
| `POST /dashboard/nudge`. | ||
|
|
||
| ## Mailboxes |
Contributor
There was a problem hiding this comment.
General observations after grilling a local agent on some edge cases:
Two dependsOn × mailbox corners that are currently neither blocked nor documented:
1. A receiver with dependsOn: if the dependency fails, the receiver dies without ever running its handler — no successor gets enqueued, the exchange silently stalls until the deadline sweep, and the message
at its position is never processed (and isn't reported as unconsumed at close, since its position was already claimed).
2. Depending on a receiver: every receiver in a multi-message exchange completes successfully — including the ones that just said "wait for the next message." So a dependent fires after the first reply, not
when the exchange actually concludes. Works by accident for single-message exchanges, breaks quietly for multi-message ones (e.g. Fiks Arkiv).
Suggest either validating both combinations away or spelling out the semantics in the technical guide.
Warnings are errors when CI=true, so the stray second <summary> on LockedMailboxState failed WorkflowEngine.Data with CS1570. Local builds only warned, which is why it survived.
Adds BatchCloseMailboxes, the flush the close buffer will call: one connection and one transaction for a batch of close requests, locking every distinct (mailbox, namespace) pair it decides on in one sorted statement as the transaction's first act. The sorted lock is hoisted as LockMailboxesForMutationSql so the delivery flush can share it, and its plan is pinned at a full batch's width -- one primary-key probe per mailbox named, with LockRows above the sort so the lock order binds. Requests are answered positionally with the verdict a separate call would have received: a pair the lock did not match is NotFound, and a mailbox named twice in one batch is closed once, its repeat replaying the disposal the first occurrence wrote. Per-request CloseMailbox is now that same transaction with a batch of one, keeping its own slot, retry and telemetry envelope; the deadline sweep is untouched and still runs the closure core under its own SKIP LOCKED claim. Introduces IBufferedRequest<TResult> and BufferedMailboxCloseRequest for the buffer layer to build on.
Adds BatchMintMailboxes, the flush the mint buffer will call: one connection, no lock and no transaction, because the unique index on (namespace, idempotency_key) is what serializes minters. The bespoke single-row MintMailboxSql CTE is deleted and replaced by MintMailboxesSql: an ordinality-numbered input CTE, a fresh CTE dropping the keys the snapshot already sees, a materialized open_counts, and one INSERT ... SELECT in unique-index order with ON CONFLICT DO NOTHING. A second statement classifies the keys the insert did not return, the ClassifyExistingIdempotencyKeys pattern. Requests are answered positionally with the verdict a separate call would have received. Only fresh candidates rank against the collection cap (row_number() - 1 as peers_ahead), so a flush counts its own mints against the cap instead of admitting all of them off one reading, while replays are answered even at the cap and consume none of it. A key named twice in one batch mints once and its repeat replays the row, costing the collection one slot rather than two. Minted versus Existing stays decided by returned id == candidate id, which is also how an attempt whose commit the client never saw recognises its own mailbox on retry. The mint plan test is rewritten against the new statement and now probes a hundred-wide batch as well as a singleton: written as a GROUP BY over a join, open_counts planned as one index probe at width one and a hash join against every open mailbox at width a hundred. Per-request MintMailbox is the same two statements with a batch of one, keeping its slot, retry and telemetry envelope. Also renames the close plan test after the const it exercises, left over from the previous revision's rename.
Extracts WorkflowWriteBuffer's channel/drain/flush mechanics into a generic BatchBuffer<TItem, TResult> over IBufferedRequest<TResult>, and adds the first subclass: MailboxDeliveryBuffer, which flushes through BatchDeliverToMailboxes and fans the positional results back out. Deliveries are never refused admission — the bounded channel waits — and the fan-out records nothing. Delivery batches are bounded by a cumulative payload budget as well as by batch size, so a hundred requests at the payload ceiling cannot build one enormous command. The same bound applies to the shutdown drain. Adds BatchBufferSettings/MailboxBufferSettings with defaults and the <= 0 back-fill for all three mailbox buffers, even though the mint and close buffers themselves come next: the settings are one coherent block. Nothing is registered in DI or reachable from an endpoint yet.
Engine's MintMailbox, CloseMailbox and DeliverToMailbox now enqueue into the three mailbox buffers instead of calling the repository per request, and the buffers are registered as singleton hosted services. Everything else about the three methods stays put: the pre-database validation that produces the Invalid verdicts (which no repository path can return), the candidate id and the caller's own instant, and the verdict-shaped metrics — MailboxesCreated on a mint, MailboxDeliveriesReceived for every delivery outcome including the refusals decided before the database. Commit-gated metrics stay in the repository. The buffers register above the HeartbeatService ordering comment, beside the write and update buffers: hosted services stop in reverse registration order, so they are stopped after the processor and answer their queued callers from the 30-second drain rather than being cut off. What they have to outlive is request handling, since they are fed from the HTTP path. BatchBuffer takes an operation tag and emits engine.mailbox_buffer.flushed immediately after FlushCore returns — the one point where a batch is known to have been answered without faulting. MetricsCollector feeds the new engine.mailbox_buffer.depth gauge from the three queue depths each tick. The 219 integration tests and the repository suites pass unmodified: HTTP semantics are unchanged, including that a full delivery queue waits rather than refusing.
Strict bar: a comment stays only if it carries knowledge the surrounding code cannot give — behaviour of something outside this codebase, a consequence in another component, or the reason something absent is absent. On interface members, judged against the signature alone. Comments, docstrings and blank lines only; no behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Verification