139 lines
6.9 KiB
Markdown
139 lines
6.9 KiB
Markdown
---
|
|
id: pg-fix-forwarder-diagnosability
|
|
name: Surface the forwarder's swallowed connect/poll failures (review 004 item 7 arms)
|
|
status: completed
|
|
depends_on: [decide-log-facade-adoption, pg-fix-forwarder-hardening]
|
|
scope: narrow
|
|
risk: low
|
|
impact: component
|
|
level: implementation
|
|
tags: [review-004-fixes, postgres-engine, forwarder]
|
|
---
|
|
|
|
## Description
|
|
|
|
The two narrow diagnosability arms in the pg forwarder, per
|
|
`docs/reviews/004-pre-release-general-review.md` §"Findings recorded,
|
|
not fixed" item 7: the reconnect arm's connect failures are swallowed
|
|
with `.ok()` (`forwarder.rs:839` — the review's ref; re-locate) and
|
|
the loop's exit discards the poll error (`forwarder.rs:855-869`) — an
|
|
operator sees nothing before wake loss.
|
|
|
|
Fix: surface both failures. The *output form* follows the
|
|
log-facade decision of record (`decide-log-facade-adoption`): through
|
|
the adopted facade, or through the documented `eprintln!` discipline
|
|
if the decision declined adoption. The error content must stay
|
|
secret-free (no DSN/password material — the standing verified
|
|
property; re-grep after).
|
|
|
|
Sequenced after `pg-fix-forwarder-hardening` deliberately: same file,
|
|
and the hardening's restructure may move the reconnect arm and the
|
|
loop-exit path this task touches.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] Failed reconnect connects are surfaced (not `.ok()`-swallowed)
|
|
with enough context to diagnose (error class, attempt/backoff
|
|
state — no endpoint credentials)
|
|
- [x] The loop's exit path surfaces the poll error instead of
|
|
discarding it
|
|
- [x] The output form matches the log-facade decision of record; the
|
|
secret-free property re-verified by grep
|
|
- [x] Existing forwarder tests green unchanged; the hardening task's
|
|
new pins green
|
|
- [x] `cargo test -p alkstore-postgres` (harness server), clippy
|
|
`-D warnings`, fmt clean; workspace gates green server-less
|
|
|
|
## References
|
|
|
|
- docs/reviews/004-pre-release-general-review.md §"Findings recorded,
|
|
not fixed" item 7 (the finding, the swallowed-error sites)
|
|
- tasks/decide-log-facade-adoption.md (the output-form decision)
|
|
- tasks/pg-fix-forwarder-hardening.md (the same-file sequencing)
|
|
|
|
## Notes
|
|
|
|
> Decisions of record the implementation made that the description
|
|
> didn't pin:
|
|
|
|
- **`reconnect_with_retry`'s generic signature is unchanged** — the
|
|
attempt future still outputs `Option<GenerationPair>`, so the two
|
|
existing unit pins (`reconnect_retry_attempts_failed_connects_…`,
|
|
`reconnect_retry_aborts_before_any_attempt…`) stay byte-identical
|
|
(the acceptance bar's "existing forwarder tests green unchanged").
|
|
The surfacing rides the *production closure* (the closure at the
|
|
loop's reconnect arm) instead of the retry core: the raw connect
|
|
error is only available inside the closure, and the retry core's
|
|
generic fabricated closures (returning plain `None`) have no error
|
|
to print — logging inside `reconnect_with_retry` would have meant
|
|
either a signature change (test churn) or an error-free generic
|
|
event (noise without class).
|
|
- **The attempt index is the closure's own `FnMut` counter**, live
|
|
across the arm's attempts; it deterministically maps the backoff
|
|
state (attempt *n* runs after the *n*-1th doubling, 50 ms base, 2 s
|
|
cap), which the event text states — the "attempt/backoff state"
|
|
context the ADR-025 output form pins, without threading
|
|
`backoff_ms` into the closure.
|
|
- **The poll error is the reconnect-arm closure's `Option` → the
|
|
`poll_loop` return type** (`Option<tokio_postgres::Error>`): the
|
|
once-discarded `Some(Err(_))` arm inside `poll_loop` now returns
|
|
the error, so the swallow point moves from the loop body to the
|
|
task's *result*, surfaced at the serve loop's poll-death select arm
|
|
(the loop's exit path the review named). All three endings surface:
|
|
a fatal poll error (`error`, with the error), a stream end without
|
|
one — the server closed the notification connection (`error`, the
|
|
review's "operator sees nothing before wake loss" shape, since the
|
|
wake-loss recovery is identical but the death cause differs), and
|
|
the defensive join-error arm (abort/panic — no abort precedes this
|
|
arm inside the serve loop, a panic would; it logs rather than
|
|
panics, the library posture).
|
|
- **The failed-verify settle (`Done(_)` arm) still discards the
|
|
reaper's output** — deliberately, scope-narrow: that arm is already
|
|
acting on a verify failure (the reconnect follows either way) and
|
|
the verify error itself was never surfaced; only the review's two
|
|
named arms moved. The same applies to the settle's `Ok(None)`-era
|
|
pre-pause reaper ends.
|
|
- **No emission-asserting test was added** — holding ADR-025's
|
|
"tracing-subscriber stays a dev-dep only if a test ever needs to
|
|
assert on emitted events (none does yet)": adding a subscriber
|
|
assertion here would have introduced the first subscriber dep for a
|
|
mechanical surface change verified by the gates. The swallow removal
|
|
is structural (the `.ok()` shape is gone from the arm entirely).
|
|
- **Secret-free verification of record**: the re-grep found no
|
|
DSN/password/endpoint-credential material in any engine emission —
|
|
and the two new emission contents were verified against the
|
|
substrate: tokio-postgres 0.7.18's connect path errors are
|
|
`Error::connect(io::Error)` (io-class: refused/denied/timeout/DNS —
|
|
no config/user/password rides the `Display`), and the poll error is
|
|
a server protocol/connection error (no credential material). The
|
|
remaining `eprintln!` set is tests' harness-skip notices only
|
|
(test-path, never the ADR's library-path scope).
|
|
|
|
## Summary
|
|
|
|
> What landed, verified how:
|
|
|
|
- **`alkstore-postgres/src/forwarder.rs`**, both review-004 item 7
|
|
arms, output form per ADR-025 (warn = retried/degraded, error =
|
|
lost wake until reconnect):
|
|
- the reconnect arm's connect failures — the `.ok()` swallow
|
|
replaced by a surfaced `tracing::warn!` inside the loop's
|
|
attempt closure (attempt index + the backoff policy text + the
|
|
raw connect error, io-class); `reconnect_with_retry`'s generic
|
|
signature and its two unit pins unchanged;
|
|
- the loop's exit poll error — `poll_loop` returns the fatal
|
|
`poll_message` error instead of discarding it; the serve loop's
|
|
poll-death arm surfaces it as `tracing::error!` (with the error,
|
|
or the no-error stream-end and the join-error variants — each
|
|
death shape names its cause and the stall-until-reconnect
|
|
consequence).
|
|
- **Verified**: `cargo test -p alkstore-postgres` against the
|
|
harness server (135 lib + 25 contract-suite + 9 schema, green —
|
|
every pre-existing forwarder/reconnect/no-replay/shutdown/
|
|
stale-UNLISTEN/LISTEN-window-death pin and the hardening task's
|
|
four new pins green unchanged); workspace `cargo build`,
|
|
server-less `cargo test --workspace`, `cargo clippy --all-targets
|
|
-- -D warnings` (workspace), `cargo fmt --check` — all green; the
|
|
doc gate (`RUSTDOCFLAGS="-D warnings" cargo doc --no-deps`) green;
|
|
the fuzz corpus replay 6/6; the secret-free grep re-run (no
|
|
DSN/password/credential material in any emission). |