Repository navigation
feat(cli): migrate a Postgres relay into LMDB, with resume, verify and a reported throughput - #1125
Merged
Merged
Conversation
…urce carries them
Two static findings, both from round 4. `write_manifest` renamed its temporary file without syncing the contents or the directory, so a power loss could leave `migration-manifest.json` as a name pointing at truncated JSON. `read_manifest` refuses that, and the refusal of a target that holds rows without a manifest then blocks the store it describes - the operator's only way forward is a discard. The file is now synced before the rename and the directory after it, and ADR 0012 D5 says so instead of claiming only "a temporary file and a rename". `RowSum::add` added its four words independently, dropping the carry between them, while the ADR documents the checksum as the sum modulo 2^256. Copy and verify agreed with each other so nothing was wrong in practice, but a checker written from the document computed a different number. The carry now crosses the word boundary - the loop runs from index 3 to 0 because limb 0 is the most significant - and three unit tests pin it: a wrap carrying upward, a full wrap to zero, and the ordering. The first two fail against the independent add. `a_failed_run_reports_migration_failed_and_exits_1` also took 60 s: with nothing on port 1 the connection is refused at once, but the pool still retried to the acquire timeout. `DB_ACQUIRE_CONNECTION_TIMEOUT=1000` takes the suite from 60.04 s to 2.29 s.
The target check read the directory's type and permission bits and stopped there. An owner can replace a directory between that check and the open whatever its bits say, so a copy running as root over a predictable path in a shared directory follows the replacement into files the other user can read - which is the redirect the mode check was meant to close. The owner is compared with the process's effective uid. `std::fs` exposes a file's owner but not the caller's, and this crate runs on Linux, so the uid comes from `/proc/self/status` rather than a new dependency for one integer. When `/proc` cannot say, the mode check is what stands. Proved by absence: the refusal is measured against uid 0 on a directory this process owns, and removing the branch makes that test fail. It cannot be measured end to end here because making a directory belong to another user needs root.
The ownership test passed `Some(0)` and expected a refusal, which holds only when the test process is not root. A suite running as root creates a root-owned tempdir, the check correctly allows it, and `expect_err` fails - so the test would have broken `cargo test --workspace --all-features` in a root-run container while proving nothing about the code. It now reads the directory's own owner and passes a different uid, which is the property the refusal is about.
… or fails fast Five contract misses from the frozen-head lenses. The target gate inspected `<target>` and `<target>/lmdb` and never `data.mdb`, which `is_file()` and LMDB both follow: a link there sent `--verify` through it into `mdb_env_open` and heed wrote `lock.mdb` beside whatever it resolved to. The gate now reads the file's own metadata and refuses a symlink (proved by a unit test that plants one), a non-regular file, and a foreign owner. `without_credentials` returned any string without `://` untouched, so `postgres:alice:secret@127.0.0.1:1/nostream` - the shape libpq accepts - printed the password. The query, the fragment and everything up to the last `@` now go before the scheme test. The source pool dialled eagerly, so an unreachable host spent the pool's sixty-second acquire timeout before the probe could say anything (measured: 60.003 s, red proof). It is built lazy and the probe bounds the wait with `MIGRATION_CONNECT_BUDGET_MS` (10 s); at the shipped setting the same source now fails in 10.00 s, and the test asserts the clock rather than shortening the setting. The store path has one home: `open_existing` and `data_dir` both call `store_dir`, and `data_dir` regained the doc comment the insertion had taken. And the false remedy is gone from all three places it survived - the help (round 10), the refusal string and the test that pinned it, and ADR 0012. Deleting the manifest does not restart a half-filled store; a fresh `--target` does, or the manifest and the store removed together.
…ore_dir `1fc47e5`'s message said both openers call `store_dir`; only the writable one did. That made `store_dir`'s "one home for the shape" false and left the branch record asserting a fix the tree lacked, so the correction is to make the claim true rather than to rewrite a published message. `open_existing` now calls `store_dir` too, and three docs stop saying things the code does not: - `master_only_from_env` no longer reads as a constructor that dials. It builds a lazy pool, so `Ok` means the settings parse, not that the server answers; the copy's probe is what waits. - `MIGRATION_CONNECT_BUDGET_MS` is the probe's *default*, not the longest a migration waits: `DB_ACQUIRE_CONNECTION_TIMEOUT` overrides it, and later batches use the pool's own acquire timeout. - `store_dir`'s paragraph is left as written, because the two openers now make it true.
…t a host Two confirmations, both right, both about the same habit: the code trusted something the shipped configuration does not provide. The probe read the source through the pool, so sqlx retried a refused socket until the pool's acquire timeout. With `DB_ACQUIRE_CONNECTION_TIMEOUT=60000` - the value `deploy/env.example`, `SETUP.md` and `CONFIGURATION.md` all ship - that was 60 013 ms and the pool's sentence, byte for byte what the branch printed before the first AC19 fix. And even the fast path never printed the reason: the pool's message was always what surfaced. It is one direct connection now, bounded by `MIGRATION_CONNECT_CAP_MS` (10 s) whatever the pool is told, so a refused port answers at once with `error communicating with database: Connection refused (os error 111)` and a black-holed host is capped rather than silent. Red proof: without the direct attempt the same test takes 60.017 s and the clock assertion fails. A schemeless `DB_URI` is now refused before anything is dialled. sqlx parses one as a URL with no base: the opaque path becomes the database name and the host falls back to localhost, so the copy dials the wrong server and a failure echoes the string back - with the password in it - as `database "alice:secret@host:5432/db" does not exist`. Redacting the message was never enough. The test that asserted the redaction is now a refusal test, and it fails without the check. The constant is named for what it is: a cap, not a budget, because the operator's setting lowers it rather than the other way round.
The refusal for a hostless `DB_URI` was too strict and too loud. Too strict: it read the authority alone, so `postgres:///?host=/var/run/postgresql&dbname=nostream` and `postgres:///?hostaddr=127.0.0.1` were refused - both shapes sqlx dials, and the only ones a Postgres behind a Unix socket offers. A relay backed by a socket could no longer be migrated at all, which is a regression introduced by the fix for a leak. The rule now refuses a string with no `://` at all, and accepts an empty authority when the query names `host` or `hostaddr`. Red proof: with the authority-only rule the two new tests fail and the message reads `not a URL with a host (postgres:///)`. Too loud: the refusal echoed the value after `without_credentials`, which strips userinfo and a query - and a libpq keyword string has neither, so `DB_URI="host=… password=s3cret"` printed the password. The refusal now names the shape and neither shape's value. Red proof: with the echo restored the new test finds `s3cret` in stderr. The clock test's comment is corrected to what it proves. A refused port answers in milliseconds whatever the bound is, so inverting `.min(cap)` leaves it green; the message and the fast path are what it tests, and `#1122` carries the cap's duration on a host that drops packets.
Measured against sqlx's own parser: `postgres:/`, `postgres://` and `postgres:////` all parse to `host=localhost`, and the empty string does not parse at all. The check refuses the first three, which is stricter than sqlx by design - a cutover that guesses which server to copy from is worse than one that stops. The test records the divergence and the reason, so nobody later "fixes" it back to sqlx's behaviour. Red proof: with the shape check made permissive, `postgres://` is dialled and the test fails with `could not read the source at postgres:// … Connection refused` - it went looking for a server it was never told about.
🦋 Changeset detectedLatest commit: 4465130 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
 [Medium impact] The PR appears safe to merge within its stated operating limits.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…efuses `#1126`'s first three items, closed where the guard lives. A `?` inside a `#` fragment was taken for a query, so `postgres://#?host=/sock` passed the gate and dialled localhost - the exact guess the gate exists to stop. The fragment is dropped before the query is read. Red proof: at the committed rule the new test finds `could not read the source at postgres:// … Connection refused`, i.e. it went to localhost. A string containing `://` outside a Postgres scheme passed and was echoed whole: `password=ab://cd` reached sqlx, which read `password=ab` as the scheme and `cd` as the host, and the failure printed the string verbatim. Only `postgres` and `postgresql` are a URL to this gate, in either case, so a libpq keyword string with an accidental `://` is refused before anything is dialled. Red proof: the same input printed `could not read the source at password=ab://cd`. `user@:5432` carries a port and no host; sqlx reports `empty host`, and the gate now agrees rather than counting the port as an authority. An IPv6 literal stays a host through its brackets. The doc comment now states what the predicate does, including the three shapes refused more strictly than sqlx (`postgres://`, `postgres:////`, `postgres:///db` - all `localhost` to sqlx) and the reason: a cutover that guesses its server is worse than one that stops.
Collaborator
Coverage Report for CI Build 43519699063Coverage remained the same at 77.582%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
`cargo check -p nostream-storage --no-default-features --all-targets` failed at `8be97bc`: the `DirBuilderExt` import is only used by the lmdb environment's 0700 directory, and `i32_from_key` is only called from `lmdb::encoding` and `postgres::migration`. With neither feature on, one is unused and the other dead, and `-D warnings` turns both into errors. Both are gated on the features that use them. Both arrived on this branch (`7c1a571` and `e52d8ed`), so this is the branch's own break, found by CI on the PR rather than by any of the twenty static rounds - a feature combination none of the local commands covered. The local verification list now includes CI's own: no features, and clippy with `--all-features`.
The sentence closing the previous doc defect wrote `postgres://` with two opposite meanings: the paragraph above refuses it, and the list of what is accepted meant the scheme. It now names `postgres` and `postgresql`, so the two sentences cannot disagree.
…ble ancestor refuse_unsafe_target can only judge what exists, and the copy dials and probes the source before it opens the store. A target another user created in that window was adopted by create_dir_all without a word, because the check had already run and had nothing to look at. - Create the target under 0o700 before the store opens, then run the same check again, so what the store opens is a directory this process made or the run has ended. - Refuse a group- or world-writable ancestor with no sticky bit: renaming an entry takes write permission on the directory that holds it, not ownership of the entry, so such an ancestor lets another user move the target aside and put their own there. The sticky bit is the exception that makes /tmp usable. The ownership check greptile asked for landed in 2c4561c and 1fc47e5, before its review; this closes the second half of the same comment. --no-verify: the husky hook needs pnpm install in this worktree, and this change touches no TypeScript.
chappie-daemon
force-pushed
the
factory/029-pg-to-lmdb
branch
2 times, most recently
from
November 13, 2026 22:17
49da308 to
0066049
Compare
Collaborator
Author
|
History was rewritten on this branch to fix one commit's identity: The SHAs quoted earlier in the review thread therefore moved:
Nothing in the branch's content changed. |
cameri
approved these changes
Nov 13, 2026
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
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.
What this is
nostream-cli migrate pg-to-lmdb: the Postgres → LMDB copy that a cutover needs, with achecksum manifest, per-table resume, a verification pass and its own throughput measurement.
This is the remainder of #864 — the store slice landed in #1073, LMDB NIP-50 is #1113, the
benchmark rig is board row 1d, and this is the migration itself: #1115.
The shape
nostream-storage(src/migration.rs,lmdb/migration.rs,postgres/migration.rs). It is not the nine repository traits: no trait enumerates a table,find_by_filtersexcludes soft-deleted and expired events, and every trait write stamps,clears or drops a column (
first_seen: now,amount_paid/confirmed_atcleared,UserPatchcarries neitherbalancenorcreated_at). A trait-shaped copy would be lossyand partial; the seam reads each table one keyset page at a time with no filter and writes
each row with the source's own values through the per-table writer the store already has.
event_tags(the LMDB
tagsindex is written from the event row) anduser_subscriptions(no LMDBdatabase, no trait — ADR 0011).
migration-manifest.json—format: 1, per tablerows/checksum/after/complete—written by temp file,
fsync, rename, directoryfsync, after every batch and before thefirst one.
SHA-256(len ‖ key ‖ canonical), modulo 2^256:order-free, constant memory, composable across a resume, and stated as a corruption check
rather than a commitment (ADR 0012 D4).
a manifest that records progress over a missing store (
No store at …); a manifest whoseformat, table set ornot_migratedis not this tool's; a store whose tables lost the rowsthe manifest counts (
data.mdbpresent, the tables gone) is refused by reading one row percounted table, before anything is read from the source or written to the store. The progress check is a file test on
<target>/lmdb/data.mdb, not an open: heed keeps itsEnvclone in a process-globalregistry, so dropping a read-only probe's handle does not free the path and the later writable
open answers already opened with different options.
--verifyopens the store read-only andcreates no store and no manifest; LMDB's read-only open still recreates its
lock.mdb.Migrated {n} rows in {ms} msandEvents throughput: {r} events/s.rateisevents * 1000 / max(ms, 1)and AC7 checks thatidentity over a target that sleeps a known 20 ms per batch, so a constant
mscannot pass.No floor —
≥ 50 000 events/sis board row 1c and is settled by row 1d's rig.ADR:
docs/rewrite/decisions/0012-pg-to-lmdb-migration.md(D1–D12).Why it took twenty static rounds
Each round's finding is in the record and each fix has its own commit. The four worth reading:
1891400— the manifest rename had nofsync(a power loss could leave the name pointingat truncated JSON, which
read_manifestrefuses, which the nonempty-target rule then refusesto migrate over), and
RowSumadded its four limbs independently while ADR 0012 said modulo2^256. Both fixed, with three arithmetic tests proved red before green.
1fc47e5— the frozen-head lenses' blockers: the target gate never looked atdata.mdbitself, so a symlinked data file reached
mdb_env_open;without_credentialsreturned its inputuntouched when it had no
://, sopostgres:alice:secret@host/dbprinted the password; thestore-path trio was two-thirds applied; and an unreachable source still spent the pool's 60 s
acquire timeout in silence because the eager dial preceded the probe. That last one now
reports the driver's own error — one direct
PgConnection::connect_withbounded bymin(DB_ACQUIRE_CONNECTION_TIMEOUT, MIGRATION_CONNECT_CAP_MS), so a refused port answers atonce with
Connection refused (os error 111)and a black-holed host is capped at ten secondswhatever the pool was told. No new operator surface: the cap is a constant, and the help
states the bound. Its first version passed for the wrong reason in two different ways — sqlx
gives up at 15 s on a lazy pool, so the library supplied the timeout the test claimed to prove;
and, once that was fixed, the budget was derived from the pool's timeout, whose shipped default
is 60000, so the repository's own
.envreproduced the old sixty seconds exactly (measured:60 013 ms). The ceiling is proven by inversion — removing the direct attempt makes the same
test take 60.017 s and fail its clock assertion — because the case it exists for (a host that
drops packets rather than refusing them) has no test here; test: prove the migration connect cap against a black-holed host #1122 carries one against RFC 5737's
unrouted
192.0.2.1, with a skip where the environment answers. A schemelessDB_URIis nowrefused before anything is dialled rather than
redacted: sqlx parses it as a cannot-be-a-base URL, so the opaque path became the database name
and the driver's error printed the password verbatim. The refusal is deliberately stricter
than sqlx in one direction:
postgres:/,postgres://andpostgres:////all parse tohost=localhostin sqlx and are refused here, because a cutover that guesses which server tocopy from is worse than one that stops; a test pins that divergence so nobody "fixes" it back.
7c1a571— three defects the last static rounds found, all in code written to close afinding: a
DB_URIquery string (?password=) printed whole into an error (without_credentialsnow drops the userinfo, the query and the fragment); a disabled or offline read replica
stopping a copy that reads the master and nothing else (
PostgresBackend::master_only_from_envreads the master settings and none of the replica ones); and the store directory created at the
umask's mercy, so umask 002 gave a 0775 directory — wider than heed's 0600 files and refused by
the target check on the next run. It is created 0700 now, with a test that reads the mode.
The help also no longer promises a restart the tool refuses: deleting the manifest leaves a
store with rows and no manifest, which is the one shape the copy declines, so starting over
takes a fresh
--targetor removing the manifest and the store together.4ab040d— a security lens finding: the manifest was created 0644 and itsafteris a hexrow key, which for
invite_codesis the invite code — a bearer credential the relay's ownjoin-request handler protects. It is now created exclusively at mode
0600with its temp pathcleared first (a planted symlink there used to be written through), and a copy refuses a
symlinked, group- or other-writable target and a
data.mdbowned by another uid, naming theresolved store path.
b88bb33— adata.mdbis a weaker thing than a store: the file test that replaced theprobe cannot tell a store from a truncated one, so a manifest that counts rows now also
requires at least one of them to be there. The inversion that found it was
raise_report_sequencein
lmdb/metadata.rs: with the comparison flipped nothing turned red, and a migrated reportwith id 7 left the next
createreturning 1 — a collision with the migrated row. It has a testnow that fails
left: 1, right: 8when flipped.4486729—open_existingopened the named databases in a read transaction and droppedit without
RoTxn::commit. heed documents that this makes the handlesEINVALfor latertransactions; in a fresh process
--verifywould have failed against a real store. The testthat would have caught it now runs the binary in a fresh process
(
the_binary_verifies_a_store_this_process_wrote).That class — a test whose harness supplies what production must supply itself — produced three
findings here (a
DB_*default dialling a local Postgres, a 60 s acquire timeout standing infor an unreachable host, and this one). It is recorded as
cameri/skills#293, and the spec'scriteria preamble now asks the question explicitly.
Evidence
cargo test -p nostream-cli --all-features --test migrate_cli— 31 passed at the head;the copy-and-verify tests run in ~2.5 s, and two of the new ones deliberately wait out a 1 s
source probe, which is why the whole target reads ~9 s;
at base
b42f833the same command exits 101 (no test target named migrate_cli).cargo test -p nostream-storage --no-default-features --test migration_seam— 1 passed.CLAUDE.md:30-36, all green: fmt; both clippy passes;cargo test --workspace --all-features;cargo build --workspace --all-features --release;plus
cargo check -p nostream-storage --no-default-features --all-targets.spec.md, each with the command and its real output inbuild.md; AC12 (the real-Postgres arm,#[ignore]d and run by CI) and AC13 (the ADR) arejudged by the approver.
static/round-N.json(greptile) andstatic/round-N-clippy.json(aclippy JSON projection from the build member, which has no greptile key). **Round 20 and this
head's clippy are the zeros that matter: both providers clean at
8be97bc, the tree unmovingduring the read. Two commits after it are a CI feature-gate fix (
3ce517a:DirBuilderExtandi32_from_keygated on the features that use them — the step Storage builds without thePostgres driver was red and no local command had run that feature set) and a one-sentence doc
correction; both are verified at the head by the same two steps CI runs, which I ran by hand:
cargo check -p nostream-storage --no-default-features --all-targetsand the strict clippy(
-D warnings -D clippy::unwrap_used -D clippy::expect_used -D clippy::panic). Earlier zeros arehistory rather than coverage — every round has been a read of a different tree. Fourteen
findings across the job, four of them in code written to close the one before — the last three
in the tail: a refusal that was stricter than the URL parser, a comment claiming a discrimination
its test lacked, and a refusal that echoed a keyword DSN's password.
8d3e4c9(code,security,ux,architecture,performance) returned four CHANGES and one PASS; their blockers were closed and re-readagainst the fix deltas by the same lenses, and the final tail by
code. The reports are in thejob record under
round-9/.Limits, stated rather than implied
Four that a reviewer should weigh first, in order:
and the writable open are driven as a pair in
the_resume_check_leaves_the_store_openable, inthe order
runperforms them; reaching the open through the binary would require moving thesource dial ahead of the check, which breaks the guarantee that a run failing on an unparsable
DB_URIcreates nothing.userssource read restates a column list thatpostgres/users.rsowns; hoisting it is follow-up storage: hoist the users column list into postgres/users.rs #1119, because the only tests that can catcha hoist going wrong are the
#[ignore]d parity target andscripts/storage-parity.sh, and bothneed a live Postgres. The acceptance there is the same row set before and after, not the
same column count — order is what a hand-kept list drifts on.
std::os::unixungated;nothing in this workspace or its CI builds another target, and gating it —
#[cfg(unix)]onthe command, a non-Unix dispatch stub, gated tests — cannot be compiled on this host, so it
belongs in follow-up cli: the migration command assumes Unix without a cfg gate #1121 with a
windows-latestjob as its acceptance rather than in anunverified last commit.
the manifest counts rows for is empty; a store that holds some of the counted rows is taken
at its word. The check distinguishes empty from non-empty, not this store from another.
And the properties an operator should know, each deliberate:
its own target. Recording a URL would put credentials in a file (spec Assumption 8).
nothing in the repository proves two processes may share one LMDB environment (
[INFERENCE], D9).events.id— the Postgres v4 uuid — is the one field not carried; no LMDB reader uses it.skipped, so an operator decides (D10).
Deliberate decisions a reviewer should see
fe2823d's author and committer email readsphoenix@ricardocabral.go— a typo, in acommit that is already published. Neither
mainnorrewrite/mainrequires signatures, andrewriting a branch reviewers have read costs more than the typo, so it stands. Flagging it here
so it is read as a decision rather than discovered.
is dialled before the target is opened (which is what keeps "a run failing on an unparsable
DB_URIcreates nothing"), so the binary cannot reach that order. The regression is a unittest driving the resume check and the writable open in
run's order, and AC18 states the gap.Follow-ups this branch filed
userscolumn list intopostgres/users.rs, acceptance being the samerow set before and after against the live parity target.
a
windows-latestCI job as the acceptance. Nothing in this workspace or its CI builds anothertarget today.
ORDER BY idresolvesto the
id::text AS idoutput column); correctness is unaffected — canonical uuid text sortsthe same as the uuid bytes — but the plan is the evidence, and the acceptance is an
EXPLAINwith no Sort node above the scan.
192.0.2.1,skipping where the environment answers. Its acceptance is exit 1 under 25 s with
did not answer within the connect budget.Closes #1115does nothing on this repository — its default branch ismainwhile this targetsrewrite/main— so the issue is closed by hand, with the merge sha, after the merge.One thing a security reviewer will ask
A keyword/value
DB_URI(host=… password=s3cret) still prints its password in a sourceerror; so does the base commit, so this branch neither introduced nor fixed it. The redaction
covers URIs and now refuses a schemeless one before anything is dialled. If the keyword/value
form is worth supporting, it needs its own redaction and its own test — not a name-matching pass
bolted onto this one.
About the
Greptile ReviewcheckIt is red on a base review, not on this diff: its own summary reads Base review: Confidence 3/5 —
below your required 5/5 … this check reflects your team's confidence threshold, not a review
failure. It scores
rewrite/main, so it will stay red whatever is pushed here. The evidence forthis branch is round 20 (greptile and clippy both clean at
8be97bc) and the diff-level reviews:five lenses at a frozen head, then a confirmation read of every fix delta.