211 lines
11 KiB
Markdown
211 lines
11 KiB
Markdown
---
|
||
id: review-wave-4
|
||
name: Wave 4 review gate — the postgres engine vs the contract
|
||
status: completed
|
||
depends_on: [suite-column-postgres]
|
||
scope: narrow
|
||
risk: low
|
||
impact: phase
|
||
level: review
|
||
tags: [wave-4, review]
|
||
---
|
||
|
||
## Description
|
||
|
||
The wave-4 review gate — the engine-wave review shape
|
||
(docs/plans/implementation.md §Review gates: "engine-vs-contract
|
||
conformance"), before wave 5 decomposes. The gate reads the
|
||
implementation against the pinned ADR text line by line (the
|
||
wave-1/2/3 gates' method).
|
||
|
||
Gate checks:
|
||
|
||
- **Engine-vs-contract conformance**: the suite column green against
|
||
dockerized postgres (server-less runs skip cleanly — a skip never
|
||
substitutes for acceptance); the engine-state clause families
|
||
contract-tested per the applicability row (ADR-012 §5's postgres
|
||
row — every family yes); virgin-store semantics; the `size`
|
||
metadata-only discipline (the size-column probe never loads the
|
||
value blob).
|
||
- **The postgres posture vs ADR-007**: the B5 deltas carried as
|
||
requirements, not hedges — the `synchronous_commit` parity knob
|
||
(configurable, per-connection wiring, default `on`); pooled
|
||
connections with per-connection prepared-statement discipline
|
||
(statements die with sessions); autovacuum + fillfactor tuning
|
||
stated in the module doc; the ~40× small-tier storage overhead
|
||
accepted and documented; the B5 tuning numbers recorded as
|
||
POC-derived posture defaults (ADR-007 §6's deferred-cost pattern).
|
||
- **The sweep-safety proof vs ADR-007 §5**: `list()` complete and
|
||
well-formed as a stable MVCC-snapshot cursor over a vacuuming
|
||
table; the batch-delete helper (`key = ANY($1)`) contract-tested;
|
||
the exact-count sweep-outcome rows green against the server.
|
||
- **The seam families vs ADR-010 §2 / ADR-012 §4/§5**: the full
|
||
clause families on postgres (the fleet host) — `ensure_gc_schema`
|
||
(both tables, the embedder's liveness table never created),
|
||
`window_stage`/`window_take` (the sqlite semantics' dialect twin),
|
||
the joint tx ownership rule honored in the `JointTx` shape (the
|
||
store holds; the engine contributes), `pin_commit`'s atomicity
|
||
(publish + protection in one step, zero residue on failure),
|
||
`pin_renew`'s never-resurrect semantics, `pin_check`'s
|
||
classification-only discipline, `arbitrate`'s finger-lock fold,
|
||
the advisory lock's ADR-013 §2 lease semantics (atomic
|
||
test-and-set, session-scoped holdership, same-connection
|
||
release). **Fleet activation absent by construction** — no
|
||
`fleet:` parameter, no store-core fleet wiring (wave 6); the
|
||
clause bodies exist and are contract-tested per applicability.
|
||
- **The non-fleet posture vs ADR-012 §3**: the constructors are
|
||
implicitly `fleet: false` — the in-process protocol verbatim; the
|
||
recorded posture names wave 6's slotting-in as a non-event.
|
||
- **Async/blocking discipline**: tokio-postgres natively async — no
|
||
`spawn_blocking` seam (the sqlite discipline does not carry); no
|
||
blocking I/O on the async path; the pool is the concurrency
|
||
posture; no `unwrap`/`expect` outside tests.
|
||
- **Named design points audited as resolved** (each recorded in its
|
||
task's Notes): the schema name + quoted-identifier boundary; the
|
||
pool default size; the prepared-statement discipline; the DSN +
|
||
opts constructor shape; the virgin-store tolerance split; the
|
||
list-cursor fetch shape; the timestamp encoding; the `JointTx`
|
||
concrete type; the `pin_renew` successor-time source; the lock
|
||
key + scope; the lease's connection holding.
|
||
- **Family conventions**: no comments in code (doc comments on public
|
||
API fine), thiserror, tokio-only async, no unwrap/expect outside
|
||
tests, naming, one-module-per-file re-export shape, the
|
||
`#![allow(dead_code)]` scope notes matching their remaining scope
|
||
(the batch-delete helper's wave-6 note intact).
|
||
- **Gates green**: `cargo test` (default), `cargo test --features
|
||
mem`, `cargo test --features postgres` (server-less: skipping),
|
||
`cargo test --all-features`, `cargo check --no-default-features
|
||
--features postgres` (the pg-pure build), `cargo clippy
|
||
--all-targets -- -D warnings` (default + `--features postgres` +
|
||
`--all-features`), `cargo fmt --check`.
|
||
|
||
Findings are fixed inline where cheap, or recorded as wave-5
|
||
amendments where they shape the next decomposition. The gate's
|
||
completion is what unblocks wave 5's decomposition.
|
||
|
||
## Acceptance Criteria
|
||
|
||
- [x] Every gate check above read and verified against the cited ADR
|
||
text (not from memory of it)
|
||
- [x] Named design points each decided, recorded, and consistent with
|
||
the specs
|
||
- [x] Findings fixed inline or recorded as wave-5 amendments with
|
||
owners
|
||
- [x] All gates re-run green after any fixes
|
||
- [x] Wave 5 decomposition unblocked (recorded in this task's Summary)
|
||
|
||
## References
|
||
|
||
- docs/plans/implementation.md (§Wave 4, §Review gates — engine wave
|
||
reviews)
|
||
- docs/architecture/decisions/007-two-kv-engines-sqlite-and-postgres.md
|
||
- docs/architecture/decisions/010-backend-io-seams-and-fleet-state-home.md §2
|
||
- docs/architecture/decisions/012-pre-decomposition-consistency-rulings.md §3/§4/§5/§6.6/§6.7
|
||
- docs/architecture/decisions/013-family-composition-reactive-boundary-and-feature-graph.md §2/§4
|
||
- docs/architecture/store-api.md
|
||
- tasks/review-wave-3.md (the gate method; the structural twin's gate)
|
||
|
||
## Notes
|
||
|
||
> Gate completed 2026-10-12, the wave-1/2/3 method: every gate check
|
||
> read against the pinned text (ADR-007 §4/§5/§6, ADR-010 §2,
|
||
> ADR-012 §3/§4/§5, ADR-013 §2/§4, store-api.md, the seam trait in
|
||
> src/gc.rs), then verified against the landed diff (1efc46f…0fb05fc)
|
||
> and re-run gates. Findings fixed inline:
|
||
>
|
||
> - **PG-1 — in-instance sweep-lease reaping after session loss
|
||
> (fixed, ADR-013 §2).** `sweep_lock_acquire` answered `Ok(false)`
|
||
> from the mutex even when the held lease's session had died —
|
||
> blocking re-acquisition on the same backend until an explicit
|
||
> release, against the holdership rule ("holdership ends at …
|
||
> engine-session loss"). Fixed: the acquisition answers against a
|
||
> live session only (dead checkouts are reaped in-instance); new
|
||
> test `sweep_lock_in_instance_session_loss_reaps_the_lease`
|
||
> (terminates the lease's own held backend via
|
||
> `pg_terminate_backend` and re-acquires without a release). The
|
||
> foreign-session reaping test (the wave-4 test) is unchanged.
|
||
> - **PG-2 — the `PgJointTx` mid-flight-drop hazard (doc fix +
|
||
> amendment A1 below).** deadpool's `Fast` recycle never resets
|
||
> transaction state, so a `PgJointTx` dropped between
|
||
> `begin_joint_tx` and `pin_commit` returns a pooled *dirty*
|
||
> session to the pool. The clause bodies' own failure paths all
|
||
> ROLLBACK before surfacing (wave-4 contract holds — the zero-
|
||
> residue atomicity test proves it); the hazard is the store core's
|
||
> future use, where Rust `?` propagation between the two calls is
|
||
> the natural error shape. Not fixable inline without async-drop or
|
||
> a seam reshape — recorded on [`PgJointTx`]'s doc and as amendment
|
||
> A1 for wave 6.
|
||
> - **PG-3 — engine-level pg tests leaked per-test schemas on the
|
||
> shared harness db (fixed).** 380 `abt_*` schemas had accumulated
|
||
> (the suite column's `alkblobs_suite_*` teardown was verifiably
|
||
> clean — 0 residue — the leak was src/postgres.rs's + src/store.rs
|
||
> construction tests' per-test schemas). Added crate-internal
|
||
> `test_support::SchemaJanitor` (a drop-guard running
|
||
> `DROP SCHEMA IF EXISTS … CASCADE` synchronously over a std
|
||
> thread + a one-shot current-thread runtime — no async-drop
|
||
> exists), the `Harness` deref-guard wrapper for the engine tests',
|
||
> and `fresh_opts` carrying a janitor in the construction tests. A
|
||
> one-time manual cleanup dropped the legacy residue (the 16
|
||
> hostile-name stragglers dropped via `quote_ident` — incidentally
|
||
> exercising the boundary). Post-fix: **0 schemas residue** after
|
||
> full `--features postgres` and `--all-features` runs.
|
||
>
|
||
> Amendments for the next waves (owners recorded here, no spec text
|
||
> changed this gate):
|
||
>
|
||
> - **A1 (wave 6, owner: the fleet-GC mechanism task)** — the joint-tx
|
||
> drop discipline: the store core's joint-tx driver owes `pin_commit`
|
||
> on **every** path from `begin_joint_tx` onward (PG-2). Design it
|
||
> catch-propagate-terminate, or grow an explicit abort verb via a new
|
||
> ADR (the seam shape is pinned; `pin_commit`'s terminal-consume
|
||
> reading stays).
|
||
> - **A2 (wave 5, owner: the pg-lo engine task)** — pg-lo's engine
|
||
> tests share the `pglo-poc` cluster: adopt `SchemaJanitor` (or the
|
||
> column's own teardown pattern) from the start — the wave-4 lesson
|
||
> is not to re-learn it.
|
||
>
|
||
> Minor observations, no action: `sweep_lock_acquire`'s mutex check is
|
||
> not atomic with its checkout (two racing acquires both check out;
|
||
> the advisory lock itself arbitrates and exactly one stores its
|
||
> checkout) — sound by server arbitration. `list` rides the collected
|
||
> query rather than `query_raw` — the recorded design point; the
|
||
> bounded-memory claim holds for the keys-only materialization.
|
||
>
|
||
> Verification (post-fixes, all live unless noted): `cargo test`
|
||
> 69/19/3 ok; `cargo test --features mem` 124/54/4 ok; `cargo test
|
||
> --features postgres` 103/36/3 ok against the pglo-poc server, 51
|
||
> clean skip notes server-less; `cargo test --all-features` 158/71/4
|
||
> ok against the server; `cargo test --no-default-features
|
||
> --features postgres` (pg-pure) 76/19/3 ok; clippy `-D warnings`
|
||
> green (default / --features postgres / --all-features / pg-pure);
|
||
> `cargo fmt --check` green; `cargo doc --no-deps` clean; shared-db
|
||
> schema residue 0.
|
||
|
||
## Summary
|
||
|
||
> Completed 2026-10-12 (wave 4). The engine-wave gate read the landed
|
||
> postgres engine against the contract line by line and found it
|
||
> conformant: the suite column green against the harness server (36
|
||
> conformance rows live; server-less runs skip cleanly — a skip never
|
||
> substituted), every clause family of ADR-012 §5's postgres row
|
||
> contract-tested (both-tables schema + never-the-embedders,
|
||
> stage-idempotence/take-atomicity/the `<=` boundary, joint-tx
|
||
> atomicity with zero residue, `pin_renew` never-resurrect,
|
||
> two-variant `pin_check`, the folded `FOR UPDATE` arbitrate, the
|
||
> advisory lease's ADR-013 §2 semantics), the B5 deltas carried as
|
||
> requirements (the per-connection `synchronous_commit` knob tested,
|
||
> per-connection `prepare_cached`, fillfactor 90 stamped and
|
||
> reloption-asserted, autovacuum/40× overhead stated in the module
|
||
> doc), the `list` MVCC-snapshot cursor + the batch-delete helper
|
||
> exact-count-gated vs server, the schema-less virgin-store split,
|
||
> the size metadata-only EXPLAIN gate, and the non-fleet/posture
|
||
> recordings verbatim where wave 6 needs them. All 11 named design
|
||
> points audited as decided and recorded in the wave-4 task Notes.
|
||
> Three findings fixed inline (in-instance lease reaping — new test;
|
||
> the `PgJointTx` mid-flight-drop hazard — doc + amendment A1; the
|
||
> engine-test schema janitor — 380 leaked schemas → 0), two
|
||
> amendments recorded for waves 5/6 with owners (A1 the joint-tx
|
||
> drop discipline; A2 pg-lo adopts the janitor). All gates re-run
|
||
> green after the fixes (the verification block is in Notes). Wave 5
|
||
> (pg-lo large engine) decomposition is unblocked; its review gate is
|
||
> the engine-wave shape again. |