feat(outbox)!: range-partition persistent_outbox_events by sequence (Stage 1) - #106
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7aa0bc0. Configure here.
| _ = current_job.shutdown_requested() => { | ||
| return Ok(JobCompletion::RescheduleNow); | ||
| } | ||
| _ = tokio::time::sleep(self.interval) => {} |
There was a problem hiding this comment.
this is breaking with the existing convention of having hourly/daily jobs being relying on event from the ephemeral outbox (ie: hourly and daily cron events that job relies on)
There was a problem hiding this comment.
as a result: does not consider/use manual time
There was a problem hiding this comment.
this matter too if we eventually want to expose the hourly job to similarly to how we expose daily job
There was a problem hiding this comment.
of course this means couple with lana / upstream system. but not too different in how this PR "couples" it too #105, ie: it's exposing the put/get interface, but to be active the upstream service needs to plug the relevant data store (ie: local storage or GCS)
There was a problem hiding this comment.
Yeah that is a deep flaw in the approach of #105!
This job is a maintenance / control-plane task. You never want to couple a controlling task on the thing it's monitoring.
Lana and its events are a domain layer - obix is a service to the domain layer and thus cannot depend on it.
Consider: there is a deserialization issue that stalls the consumer but the producer is still shipping events - then all maintenance jobs stall even though there is back-pressure... that would not be good.
There was a problem hiding this comment.
this is breaking with the existing convention of having hourly/daily jobs being relying on event from the ephemeral outbox (ie: hourly and daily cron events that job relies on)
This is only a convention in the domain layer (aka Lana) - obix is NOT part of the application domain layer. Its a service that must be independent.
There was a problem hiding this comment.
as a result: does not consider/use manual time
This can be achieved independently if we need it - but I think that this control job should probably stay on realtime regardless (just like some job - crate constructs).
There was a problem hiding this comment.
this matter too if we eventually want to expose the hourly job to similarly to how we expose daily job
Again these are Lana layer domain concerns - obix must work independently of that.
… bump Rust `stable` now resolves to clippy 1.93, which tightened `await_holding_lock` and `type_complexity`. These flag pre-existing recording-hook tests in post_persist_hook.rs (a `std::sync::Mutex` guard held across an `await` in assertion sections) that are unrelated to the partitioning work. File-scoped `#![allow(...)]` keeps `nix flake check` green without changing behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stage 1 of the persistent-outbox partitioning plan: convert `persistent_outbox_events` to a declarative RANGE partition on `sequence`, add a timer-scheduled partition-maintainer job that pre-creates partitions ahead of the sequence head, and keep an always-empty DEFAULT partition as an insert-can-never-fail backstop. Nothing is dropped — the gapless-sequence + replay-from-zero contract is fully intact. No retention / offload / read-side watermark (those are Stage 2/3, tracked separately by the archive work). Schema (BREAKING migration): - `persistent_outbox_events` is now `PARTITION BY RANGE (sequence)`. - PRIMARY KEY moves from the random UUID `id` to `sequence` (a partitioned parent's PK must include the partition key; the per-partition unique index on `sequence` becomes the `ON CONFLICT (sequence)` arbiter gap-fill relies on, and the ~8x-WAL random-UUID index is removed). - `id` is demoted to a plain, unindexed column (RETURNING-only) but retained for the public `OutboxEventId`. - Ships an initial wide `p0` ([0, 10_000_000)) + a DEFAULT partition, with per-partition autovacuum/freeze/fillfactor storage params and JSONB `COMPRESSION lz4`. Maintainer (src/out/partition): - `Outbox::register_partition_maintainer` runs one premake pass synchronously (before serving traffic) then spawns a `job` runner that reschedules on an interval; each tick is idempotent (`CREATE ... IF NOT EXISTS`). DDL never runs in the commit path. - `ensure_partitions` (the tick) and `recover_default_partition` (a one-txn DEFAULT-strand repair that never regresses MAX(sequence)) are exposed for operators; recovery is NOT auto-run (runbook + alert first). - `partition_width` / `partition_premake` / `partition_maintainer_interval` added to `MailboxConfig` (conservative defaults: 10M / 2 / 1h). All `query!` macros are partition-transparent (zero query-text edits); adds a generated `persistent_outbox_events_table()` accessor. `.sqlx` cache regenerated (no diff — nullability/types unchanged). Composes with the 0.5.0 Result-streaming load path (undecodable events) unchanged. Tests: 6 live-PG tests covering gap-fill across a boundary, a batch straddling two partitions, replay across a boundary, synchronous maintainer premake, and DEFAULT-fill -> recovery contiguity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ternal timer Addresses review: move all maintainer job code (config, job data, initializer, runner) out of partition/mod.rs into partition/job.rs, leaving mod.rs with just the DDL primitives (ensure_partitions, recover_default_partition). The runner now re-runs on an internal `tokio::time::sleep` loop rather than returning `JobCompletion::RescheduleIn` each tick: the job stays resident and only falls out of the loop on shutdown (reschedule to resume after restart) or on an ensure_partitions error (propagates so the scheduler retries per retry_settings — the alert). Rescheduling therefore signals a problem, not the steady-state cadence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review: replace the top-level `ensure_partitions` / `recover_default_partition` free functions with an idiomatic service struct `Partitions<Tables>` (mirrors `Inbox`/`Outbox`) that caches the pool and sizing config (`width` / `premake`) and exposes `.ensure()` / `.recover_default()`. The maintainer job and the registration premake now share one cheap-to-clone handle instead of threading `(pool, width, premake)` through every call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file was committed in the initial commit before `.direnv/` was added to `.gitignore`; because it was already tracked, the ignore rule never took effect. It is a per-checkout direnv artifact (hardcodes the absolute source path), so it should never have been tracked — untracking lets `.gitignore` apply. Also removes an accidental worktree-path modification a prior `git add -A` had swept into this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a breaking-change note (persistent_outbox_events is now range-partitioned by sequence; recreate the table) and a "Partition maintenance" section showing register_partition_maintainer and — per the review question — explicitly stating that registration is optional and the outbox never breaks without it: rows beyond the initial partition fall into the always-present DEFAULT partition (still read/gap-filled/replayed), trading only the per-partition vacuum/locality wins, never correctness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ner) Locks in the "no maintainer, transaction tips over the boundary" guarantee: with only p0 + DEFAULT, a gap straddling the boundary is filled with the placeholder routed into DEFAULT, and delivery stays contiguous. Complements default_fill_then_recover (which covers a straddling publish batch) by exercising the fill_gaps ON CONFLICT path across the p0/DEFAULT boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fixed Using measured staging outbox rows (~760 B/row, payloads average ~400 B, <0.5% TOAST): - Default partition width 10M -> 2M (~1.5 GB/partition) so the hot partition stays cache-resident and per-partition vacuum is quick; p0 shrinks to match. - Premake 2 -> 5, so the initial ensure() at registration lays a 5-partition runway (the migration still ships only p0 + DEFAULT). - Width is now the fixed DEFAULT_PARTITION_WIDTH constant, coupled to p0's range, rather than a MailboxConfig field: exposing it invited a mismatch that would make the maintainer create partitions overlapping p0. premake/interval stay configurable (no migration coupling). Partitions::new drops the width arg and uses the constant internally. - Drop JSONB COMPRESSION lz4: it only fires on >2KB TOASTed values (near none here) so it buys ~nothing, while adding a hard --with-lz4 build dependency the migration would fail on. - Trim the over-specific migration header comment to a short note. - Trim the partition tests to the three obix-specific behaviours (maintainer premake, DEFAULT recovery, replay-from-BEGIN); drop the boundary-routing tests that were really exercising Postgres tuple-routing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…events `seen_at` was write-only — set to DEFAULT NOW() on insert and never read by any query, struct, or test. Dropping it removes 16 bytes/row (a small further trim to partition size) with no behavioural change; the recovery path's `SELECT *` adapts to the column set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sleeps `inbox_reprocess_in_with_artificial_clock` asserted state a fixed 200ms after publishing, which intermittently failed on loaded CI runners (the job runner sets Processing -> Pending, so a fixed delay could catch it mid-run at the status assertion, tests/inbox.rs:269). Replace the fixed-sleep positive assertions with polling: use the existing `wait_for_inbox_status` for the event status and a small `wait_for_executions` helper for the execution count. The negative "did not run yet" check keeps its short sleep (the artificial clock gates it). Unrelated to partitioning; this pre-existing flake was the sole cause of the red "all tests" runs on earlier commits of this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ensure(): CREATE TABLE IF NOT EXISTS ... PARTITION OF is not
concurrency-safe (DuplicateTable / catalog unique-violation when two
sessions create the same partition). Since registration runs ensure
synchronously on every instance, multi-instance startup could fail
registration on some nodes and leave no maintainer running. Serialize
creators cluster-wide on a transaction-scoped advisory lock keyed on
the table name.
- recover_default(): a repair that crashed between commit and the
artifact DROP left {table}_default_old behind, and the next repair
then failed the RENAME on a relation-name conflict. Drop any drained
leftover up front and move the artifact DROP inside the repair
transaction so no new leftovers can occur.
Adds regression tests for both paths.
c6caf2b to
85a4af5
Compare
- repair the broken intra-doc link to recover_default and drop redundant explicit link targets (cargo doc is warning-free again, modulo a pre-existing ctx.rs lint from main) - README / register_partition_maintainer claimed partition width is a MailboxConfig option; it is deliberately the fixed DEFAULT_PARTITION_WIDTH constant coupled to the migration
|
Re-reviewed the full diff after the bugbot fixes. A few smaller things found and handled: Fixed in 7612ff9 (docs only):
Checked, no action needed:
One thing to decide — in-place migration edit vs. upgrade path: |
The in-transaction artifact DROP already makes leftovers impossible, so
the up-front DROP TABLE IF EXISTS only guarded against debris from the
unreleased pre-fix version of this function (or manual meddling). Failing
loudly on an unexpected {table}_default_old is preferable to silently
deleting a table in a manual repair path — and it removes the 'guaranteed
drained' reasoning a reader had to verify.
2 participants
Stage 1 — range-partition
persistent_outbox_eventsbysequenceImplements the handoff spec
obix-dev/handoff-partition-persistent-outbox.md(raw: https://raw.githubusercontent.com/GaloyMoney/drua-library/main/spaces/obix-dev/handoff-partition-persistent-outbox.md).Converts
persistent_outbox_eventsto a declarative RANGE partition onsequence, adds a timer-scheduled partition-maintainer job that pre-creates partitions ahead of the sequence head, and keeps an always-emptyDEFAULTpartition as an insert-can-never-fail backstop. Stage 1 = partitioning alone — nothing dropped, the gapless-sequence + replay-from-zero contract fully intact.Non-goals (Stage 2/3): no partition drop, no retention, no offload/archiving, no read-side watermark. Total on-disk size is unchanged; the wins are per-operation cost, vacuum trajectory, and index locality.
Semver + migration story
0.5.1-dev); commit isfeat(outbox)!:.Outbox::register_partition_maintainer(...)at startup. A consumer that applies the migration but never registers the maintainer keeps working (rows fall intoDEFAULT, still read normally) but forfeits the per-partition wins.What changed
migrations/…_obix_setup.sql— partitioned parent;PRIMARY KEY (sequence)replacing the random-UUID PK +sequence UNIQUE;iddemoted to a plain unindexed column (RETURNING-only, kept forOutboxEventId); initial widep0[0, 10_000_000)+DEFAULT; per-partition storage params; JSONBCOMPRESSION lz4.obix-macros/src/tables.rs+src/tables.rs— everyquery!verified partition-transparent (zero query-text edits: INSERT routes to child,ON CONFLICT (sequence)arbiter is the new PK, range predicates prune, sequence-object read unchanged); adds a generatedpersistent_outbox_events_table()accessor.src/out/partition/(new) +src/out/mod.rs—PartitionMaintainerConfig, ajob::JobInitializer/JobRunner(mirrors the existing outbox jobs),ensure_partitions(idempotent tick),recover_default_partition(one-txn DEFAULT-strand repair), andregister_partition_maintainer(synchronous premake → spawn recurring job; DDL never in the write path).src/config.rs—partition_width/partition_premake/partition_maintainer_interval(defaults 10M / 2 / 1h — conservative, real lana/cala rates unknown).tests/partition.rs(new) — 6 live-PG tests.Deviation from the handoff (necessary)
The handoff's
WITH (vacuum_freeze_min_age = 0, …)uses a GUC name that Postgres rejects as a table storage parameter (ERROR: unrecognized parameter "vacuum_freeze_min_age"). The correct per-table reloption isautovacuum_freeze_min_age— used in both the migration and the maintainer. Same intent (freeze on first insert-driven vacuum). Verified against PG 17.lz4
COMPRESSION lz4is supported on the nix Postgres (17.8) and applied to both JSONB columns (attcompression = 'l'confirmed). If a downstream PG build lacks lz4, drop the twoCOMPRESSION lz4clauses (handoff-sanctioned; only affects >~2KB TOASTed rows).Interaction with in-flight PRs
main, so the load path already returnsVec<Result<PersistentOutboxEvent<P>, UndecodableEventError>>. Partitioning composes with it unchanged (no rebase needed).DROP PARTITIONvs per-row DELETE) — a substrate for feat!: archive old outbox events to object storage #105, not a competitor.Verification
nix flake check(fmt / clippy--all-features -D warnings/ audit / deny) — green.cargo nextest run --workspace: 64 passed, 0 skipped (58 existing + 6 new)..sqlxregenerated against the migrated DB — no diff (query text unchanged;idstays NOT NULL,sequencestays int8-not-null, so cached metadata is identical).DEFAULT-fill → recovery (DEFAULT drains, rows land in explicit partitions,MAX(sequence)never regresses); replay-from-BEGINcontract unchanged.Downstream ripple (flag only — not done here)
cala and lana-bank vendor this schema; when they pick this up they'll re-run
sqlx prepareand register the maintainer. Not touched in this PR.Note on the second commit
chore(tests): silence pre-existing clippy lints from stable toolchain bumpfixes pre-existingawait_holding_lock/type_complexityfailures inpost_persist_hook.rs(surfaced by clippy 1.93 via thestablechannel, unrelated to partitioning) sonix flake check/ CI can go green. Confirmed to fail identically without this branch's changes.Open decisions for review
recover_default_partitionis shipped + tested but not auto-run by the maintainer (handoff recommendation: runbook + alert first; a non-emptyDEFAULTsurfaces as a failing maintainer job). Automate later if it recurs.partition_width/premake/ interval are conservative guesses — tune from real event-rate numbers.idretained (unindexed, RETURNING-only) for the publicOutboxEventId.🤖 Generated with Claude Code
Update (review round 1)
tokio::time::sleeploop inside the job), notJobCompletion::RescheduleIn. The job stays resident and only reschedules when it falls out of the loop — on shutdown (resume after restart) or anensure_partitionserror (propagates → scheduler retries perretry_settings= the alert). Rescheduling now signals a problem, not the steady-state cadence.src/out/partition/job.rs;partition/mod.rskeeps only the DDL primitives.CI note
The first run's
all testsfailure wasinbox_reprocess_in_with_artificial_clock(tests/inbox.rs) — a pre-existing, timing-sensitive inbox test (200 ms sleep + artificial clock + job race), unrelated to partitioning (different table + config, no partition code path). It passes locally (full--workspace= 64/64) and CI uses fail-fast so it cancelled the other 58. Re-triggered by the follow-up push.Note
High Risk
Breaking PK/schema on the core outbox table affects every consumer and requires maintainer registration for optimal layout; DDL and
recover_defaultcan block writes during repair, though correctness is preserved via DEFAULT routing.Overview
Breaking schema change:
persistent_outbox_eventsbecomes RANGE-partitioned onsequencewithPRIMARY KEY (sequence), initialp0covering[0, 2_000_000), and aDEFAULTpartition so inserts never fail to route. Upgrades require recreating the table from the shipped migration (no in-place data migration).Adds
Outbox::register_partition_maintainer(sync premake viaPartitions::ensure, then a timer-driven job) plus operatorPartitions::recover_defaultto drain strandedDEFAULTrows in one transaction without regressingMAX(sequence).MailboxConfiggainspartition_premake(default 5) andpartition_maintainer_interval; width is fixedDEFAULT_PARTITION_WIDTH(2M), tied to the migration.Existing outbox SQL paths stay partition-transparent; macros expose
persistent_outbox_events_table()for DDL. New live-PG tests cover premake,DEFAULTrecovery, concurrentensure, and replay-from-BEGIN.tests/inbox.rsswitches to polling instead of fixed sleeps to reduce CI flakes.Reviewed by Cursor Bugbot for commit 276571e. Bugbot is set up for automated code reviews on this repo. Configure here.