General review, wave 5 (docs/reviews/003): no correctness or security defects; findings are coverage-side — the pg scheduler's transient-error retry loop never executed under test (seam recommended for wave 6), forwarder race arms and SQLite rollback-failure arms parked, suite-overpinning and engine-row wiring cross-checks clean. Verified this session: workspace gates green server-less and the full pg column live vs the harness (121 lib + 25 suite + 9 schema), cargo-llvm-cov full-workspace inventory recorded (core 100%, suite 97.1%, sqlite 95.0%, pg 92.9%)
This commit is contained in:
glm-5.3-flash committed
1 parent
21074bbcdb
commit
ed883e06ea
1 file changed
+217
@@ -0,0 +1,217 @@
|
||||
# General review — wave 5 (contract suite + engine hardening)
|
||||
|
||||
- **Reviewer**: opencode (glm-5.3-flash)
|
||||
- **Scope**: general review of everything wave 5 landed (commit range
|
||||
`250511d..HEAD`: nine delivery tasks + the wave-5 review gate),
|
||||
distinct from the gate (`tasks/review-wave-5.md`, which audited the
|
||||
backlog discharge map, stamps, dispositions, and green-on-both-
|
||||
engines — and passed). Lenses: correctness, security posture, code
|
||||
smells, coverage gaps (`cargo-llvm-cov`), and the classic dev issues
|
||||
reviews 001/002 hunted. The wave-5 delta is overwhelmingly test-side
|
||||
(`properties.rs` +2379 lines, both engines' suite targets, three
|
||||
engine-side test additions); the only production-code change is the
|
||||
pg `wait_wake` lag logging (`stream.rs`, +13/-1).
|
||||
- **Input**: full read of `alkstore-contract-suite/src` (all 3344
|
||||
lines of `properties.rs`, factory, harness, version stamp), both
|
||||
engines' `tests/contract_suite.rs` executors, the three engine-side
|
||||
diffs (`sqlite seam.rs`/`store.rs`/`tx.rs`, `substrate/queue_ops.rs`,
|
||||
`substrate/watcher.rs`; pg `stream.rs`/`stream_tests.rs`), the
|
||||
wave-5 task Notes, and the lcov output. No live probes were needed —
|
||||
the harness server was up, so the verification ran the real thing.
|
||||
- **Gates at review time** (all re-run this session): `cargo build`,
|
||||
`cargo test --workspace` server-less (green, skips clean), the
|
||||
**full pg column live** (121 lib + 25 suite + 9 schema, vs the
|
||||
harness server), `cargo clippy --all-targets -- -D warnings`,
|
||||
`cargo fmt --check` — all green.
|
||||
|
||||
## Op note for future sessions (harness credentials rediscovery)
|
||||
|
||||
The pg tests' server-less skip posture hides which env the harness
|
||||
needs. For the record: `ALKSTORE_PG_HOST=localhost
|
||||
ALKSTORE_PG_PORT=15432 ALKSTORE_PG_USER=postgres
|
||||
ALKSTORE_PG_PASSWORD=poc ALKSTORE_PG_DB=blobs` (discovered via the POC
|
||||
crate's examples — the dockerized `pglo-poc` publishes :15432 through
|
||||
docker NAT, so the server's `host … 127.0.0.1 trust` pg_hba lines do
|
||||
*not* apply from the test host; the scram rule matches and the real
|
||||
password is required). A bare `cargo test -p alkstore-postgres` without
|
||||
these silently skips 155 tests — the workspace stays "green" with no
|
||||
signal that the pg column never ran. That is exactly the documented
|
||||
SKIP posture, but reviewers should set the env before believing a pg
|
||||
claim: this review's first baseline run reported zero pg tests executed
|
||||
(25 suite tests "passing" in 0.02 s).
|
||||
|
||||
## Verdict
|
||||
|
||||
**No correctness or security defects found; wave 5's deliverables are
|
||||
sound and the suite rows pin what the ADRs pin, no more and no less.**
|
||||
What the review did find is one genuine coverage gap on a wave-4
|
||||
hardening feature (the pg scheduler's transient-error retry loop has
|
||||
never executed under any test), a set of narrow untested
|
||||
race/failure arms around the forwarder and SQLite rollback paths, and
|
||||
a handful of no-action smells. Nothing here is engine-consumer-facing;
|
||||
the load-bearing findings are of the same "untested failure arm" class
|
||||
that produced review 002's live bugs, so they are recommended for the
|
||||
wave-6 window rather than accepted silently.
|
||||
|
||||
---
|
||||
|
||||
## Finding 1 — MEDIUM: the pg scheduler's transient-error retry loop
|
||||
is dead code under test (coverage gap, not a demonstrated bug)
|
||||
|
||||
**Site**: `alkstore-postgres/src/scheduler.rs:680-704`
|
||||
(`tick_with_retries`), 171-184 of the same file's uncovered inventory.
|
||||
|
||||
`pg-fix-scheduler-resilience` (the wave-4 fix batch, review 002
|
||||
Finding 5) added two halves: the tampered-row quarantine and the
|
||||
tick-level transient-error retry (3 retries at 250 ms → 500 ms → 1 s,
|
||||
then a `context_error` exhaustion arm). The quarantine half is test-
|
||||
pinned (`tampered_spec_row_is_quarantined_and_the_runner_survives`).
|
||||
The retry half is pinned **only at the constants level** —
|
||||
`the_tick_retry_policy_is_pinned` asserts `TICK_RETRIES == 3` and the
|
||||
`tick_retry_backoff` table server-less — while the loop that *uses*
|
||||
them (the retry, its backoff sleeps, the exhaustion error, and the
|
||||
`eprintln` diagnostic) has zero coverage: the lcov run shows
|
||||
scheduler.rs 171-184 and 237-260 unexecuted, and no fault seam exists
|
||||
anywhere in the pg scheduler for `tick_once`.
|
||||
|
||||
A grep of the task module doc shows the deferral is half-visible ("the
|
||||
transient-error injection's unit arm"), but the *injection* itself
|
||||
never landed. The review explicitly flags this not as a bug (the code
|
||||
reads correct — the retry loop is straight-line) but as the exact
|
||||
class that bit review 002: Finding 1 there was an untested reconnect
|
||||
failure arm that turned out to be a permanent-death bug; Finding 2 an
|
||||
untested tx-wake arm that turned out to be F-1's root cause.
|
||||
|
||||
**Recommendation**: a small wave-6 task — a `cfg(test)` transient-
|
||||
fault seam on the pg tick path (the `sqlite-commit-error-arm` pattern:
|
||||
one-shot armed flag feeding a fabricated error into the real retry
|
||||
loop), driving (a) one retried tick then success and (b) exhaustion to
|
||||
the typed error shape. Small-honest scope; it retires the last
|
||||
uncovered hardening arm from the wave-4 fix batch.
|
||||
|
||||
## Finding 2 — LOW: forwarder narrow untested arms (race-shaped; park
|
||||
or seam in wave 6, acceptably)
|
||||
|
||||
Coverage residue in `forwarder.rs` with genuine (if unlikely) failure
|
||||
shapes behind it:
|
||||
|
||||
- `forwarder.rs:693-698` — the "connection died before/during the
|
||||
LISTEN" fall-through to the reconnect path. The reconnect-success
|
||||
and kill-listener tests cover reconnect-while-idle and
|
||||
shutdown-while-idle; this arm is death *during* the LISTEN window.
|
||||
- `forwarder.rs:719-727` — mid-recv-loop stale-command abandonment
|
||||
(`command.generation() < generation` inside the run loop). This is
|
||||
the *race arm* of review 002 Finding 4's fix; the generation-top
|
||||
drain half is tested, this arm structurally needs a send-racing-
|
||||
reconnect seam.
|
||||
- `forwarder.rs:330-338` (`mid_listen_error` ack arm), `444` (forwarder
|
||||
gone), `744-745`/`762` (shutdown abort/await arms).
|
||||
|
||||
All are defensive or need deliberate race machinery to reach. The
|
||||
recommendation is the same shape as Finding 1 in spirit but weaker: a
|
||||
seam for (a) is cheap; the race arm (b) can stay parked with the
|
||||
code-read as its pin — record it either way so the next general review
|
||||
does not re-derive the same inventory.
|
||||
|
||||
## Finding 3 — LOW: SQLite rollback-failure-on-drop arms untested
|
||||
|
||||
`alkstore-sqlite/src/tx.rs:439-447` (`SqliteTxHandle::drop`) and
|
||||
`166-175` (`ConnLease::drop`): when `ROLLBACK` itself fails, the
|
||||
connection is dropped, the error is printed, and (the wave-3 fix's
|
||||
property) a fresh connection replenishes the writer slot. The new
|
||||
`failed_commit_replenishes_the_writer_slot` test covers the *commit*
|
||||
error arm via the commit-fault seam; the mirror rollback arms remain
|
||||
unexercised. Reachable on real failure (I/O errors mid-rollback), and
|
||||
the replenish-on-failure logic is identical in shape to the commit arm
|
||||
the seam now proves. A rollback-fault arm on the existing seam would
|
||||
cover both in one small test. Minor — the wave-3 fix's live behavior
|
||||
is already proven on the commit path.
|
||||
|
||||
## Finding 4 — INFO/no-action: pg stream bridge Lagged arm
|
||||
|
||||
`wait_wake`'s `Lagged(n)` arm (`stream.rs:644-650`) — the arm
|
||||
`pg-suite-infra-hardening` extended with the lag eprintln — is
|
||||
unreachable in tests without forcing >capacity broadcast lag (F-4's
|
||||
known untestability). The change is logging-only on an existing
|
||||
recovery behavior; the commit's "now true at both bridges" claim is
|
||||
code-read-true, not test-true, which is appropriately disclosed in the
|
||||
task record. No action.
|
||||
|
||||
## Finding 5 — INFO/no-action: smells (recorded, not churned)
|
||||
|
||||
- `tests/contract_suite.rs:50-52` (SQLite factory): `to_str()
|
||||
.unwrap_or_default()` would silently open `""` on a non-UTF-8 temp
|
||||
path — unreachable (`std::env::temp_dir()` base is UTF-8 on all
|
||||
supported targets), but it masks rather than fails. One-line fix
|
||||
whenever the file is next touched; not worth a green-gate-churning
|
||||
commit alone.
|
||||
- `tests/contract_suite.rs:91` (pg factory): `fresh_schema(&self)` is
|
||||
`async fn` with no await — needless.
|
||||
- `properties.rs:1683-1685`: `past_stamp_sleep` uses blocking
|
||||
`std::thread::sleep` inside async rows — safe under the multi-thread
|
||||
test flavors both engines' targets use (and the tolerance-sleep
|
||||
posture is documented), but it would stall a current-thread runtime;
|
||||
note it if the harness ever evolves flavors.
|
||||
- `harness_ready()` costs a 3 s timeout per pg suite test *only* when
|
||||
the server hangs (connection-refused fails fast) — correct posture.
|
||||
|
||||
## Coverage inventory (`cargo-llvm-cov`, full workspace, this session)
|
||||
|
||||
| Crate | Line coverage | Note |
|
||||
|---|---|---|
|
||||
| `alkstore` (core) | 100% | |
|
||||
| `alkstore-sqlite` | 95.0% | residue: defensive/closed/rollback-failure arms (Finding 3), watcher/substrate error arms |
|
||||
| `alkstore-postgres` | 92.9% | residue: Finding 1 (retry loop), Finding 2 (forwarder arms), stream-bridge defensive arms |
|
||||
| `alkstore-contract-suite` | 97.1% | residue: failure-path panic labels (expected — a failing row *is* the failure) |
|
||||
|
||||
The residuals are overwhelmingly the family's accepted posture:
|
||||
failure arms unreachable through the contract surface without a seam.
|
||||
Finding 1 is the one exception worth closing; Findings 2–3 are the
|
||||
judgment line between seam-cost and diminishing returns, called
|
||||
explicitly here so the call is recorded rather than implicit.
|
||||
|
||||
## Cross-checks that came back clean
|
||||
|
||||
- **The suite rows pin ADR text, not implementation detail** — checked
|
||||
for over-pinning (a row that a correct engine could fail): the
|
||||
tolerance postures hold (state outcomes, bounded waits, straddle /
|
||||
proximity tolerances on clock-adjacent values); the two engine
|
||||
asymmetries a row could over-pin (SQLite overtrigger delivery, pg
|
||||
cross-schema LISTEN fanout) are handled with by-construction
|
||||
observables (the wake's `channel` field, own-schema re-drain) rather
|
||||
than absence pins.
|
||||
- **Engine-specific rows are wired engine-specifically**: `*_on_sqlite`
|
||||
runs only in the SQLite column, `*_on_pg` only in the pg column —
|
||||
no cross-column mistakes in either executor.
|
||||
- **Derived-queue naming equivalence** (`__alkstore_outbox:mail`): both
|
||||
engines' `outbox_backing_queue_name` are format-identical and each
|
||||
has its own unit pin; the suite row pins the composed result.
|
||||
- **The N-5 panic probe is honest on both engines**: the SQLite panic
|
||||
unwinds through `with_tx` and the rollback runs in the unwind's
|
||||
drop; the pg detached rollback is waited out by the row's
|
||||
convergence loop; the silence window's placement (before any
|
||||
post-panic write commit) correctly avoids racing SQLite's
|
||||
documented commit-overtrigger.
|
||||
- **Security posture**: wave 5 introduces no new consumer surface, no
|
||||
new SQL (the test factories' DDL is parameter-quoted /
|
||||
minted-schema-scoped), no secrets, no dependency additions beyond
|
||||
dev/test tokio features.
|
||||
- **Task hygiene**: all nine wave-5 task files carry completed status
|
||||
with filled Notes and Summary; disposition records in the tasks
|
||||
match what the code shows.
|
||||
- **The gate's flake-watch echo**: the two unreproducible pg row
|
||||
failures (trim row, lock row) get independent concurrence on the
|
||||
gate's reasoning — the trim row's suspected failure shape (an event
|
||||
beyond the checkpoint in the post-trim absence window) is
|
||||
structurally blocked by the own-schema re-drain argument; the lock
|
||||
row's lapse assertions can only be delayed, never un-lapsed, by slow
|
||||
clocks. Supporting the gate's watch-not-block disposition; the
|
||||
wave-6 watch flag stands.
|
||||
|
||||
## Recommendation
|
||||
|
||||
No changes required to keep wave 5's verdict standing. For wave 6's
|
||||
window: Finding 1 is a decomposable small task (transient-fault seam +
|
||||
retry/exhaustion pin); Findings 2–3 can ride the same task or be
|
||||
accepted with the code-read as their pin — either way the disposition
|
||||
should be recorded. Findings 4–5 need no work.
|
||||
Reference in new issue
Block a user