pg: surface the forwarder's swallowed connect/poll failures (review 004 item 7's two narrow arms, pg-fix-forwarder-diagnosability) — output form per the ADR-025 decision of record: the reconnect arm's .ok() swallow at the loop's attempt closure becomes a tracing::warn! per failed connect (attempt index mapping the backoff state — 50 ms base doubling to the 2 s cap — plus the raw io-class connect error; reconnect_with_retry's generic signature and its two unit pins unchanged); poll_loop returns the fatal poll_message error instead of discarding it and the serve loop's poll-death arm surfaces the ending as a tracing::error! (fatal error, no-error stream close, and the join abort/panic variant — each naming the death and the stall-until-reconnect consequence); secret-free re-verified by grep — both new emission contents checked against the substrate (tokio-postgres 0.7.18 connect errors are Error::connect(io::Error), no DSN/user/password in Display; no subscriber-asserting test added, holding ADR-025's dev-dep stance); gates green: full crate suite against the harness server (135+25+9, all pre-existing forwarder/reconnect pins and the hardening task's pins green unchanged), workspace build/tests server-less, clippy -D warnings, fmt, doc gate, fuzz corpus replay 6/6; task -> completed
This commit is contained in:
1 parent
a9b6cf0c86
commit
321f579ac2
2 files changed
+162
-14
No files matched your search
@@ -753,7 +753,9 @@ impl Drop for FanoutRelease {
|
|||||||
/// ([`LISTEN_SETTLE_TIMEOUT`]) reaps-or-aborts the poll task → serve
|
/// ([`LISTEN_SETTLE_TIMEOUT`]) reaps-or-aborts the poll task → serve
|
||||||
/// commands (each also bounded, the shutdown watch selected with the
|
/// commands (each also bounded, the shutdown watch selected with the
|
||||||
/// await) → reconnect via [`reconnect_with_retry`] (exponential
|
/// await) → reconnect via [`reconnect_with_retry`] (exponential
|
||||||
/// backoff 50 ms → 2 s cap; failed connects never leave the arm). The
|
/// backoff 50 ms → 2 s cap; failed connects never leave the arm, and
|
||||||
|
/// each failure surfaces a `warn` event — attempt index, error class,
|
||||||
|
/// no credential material, ADR-025's secret-free law). The
|
||||||
/// loop-carried connection is *always present* at the generation's
|
/// loop-carried connection is *always present* at the generation's
|
||||||
/// top by construction: the first iteration holds the handed-in
|
/// top by construction: the first iteration holds the handed-in
|
||||||
/// connection, and only a successful connect re-binds it — there is
|
/// connection, and only a successful connect re-binds it — there is
|
||||||
@@ -1060,7 +1062,43 @@ async fn forwarder_loop(
|
|||||||
// The poll task ended = the connection died (a
|
// The poll task ended = the connection died (a
|
||||||
// fatal poll error or server close). The session
|
// fatal poll error or server close). The session
|
||||||
// is already gone server-side; reconnect below.
|
// is already gone server-side; reconnect below.
|
||||||
_ = &mut poll_task => {
|
// The poll error is surfaced, not discarded (the
|
||||||
|
// review 004 item 7 arm): an operator must see
|
||||||
|
// the death and its class before the wake loss —
|
||||||
|
// the error content is io/connection-class
|
||||||
|
// material, never DSN/password material (the
|
||||||
|
// secret-free law, ADR-025).
|
||||||
|
poll_outcome = &mut poll_task => {
|
||||||
|
match poll_outcome {
|
||||||
|
Ok(Some(err)) => {
|
||||||
|
tracing::error!(
|
||||||
|
"alkstore-postgres: forwarder poll \
|
||||||
|
ended with a fatal \
|
||||||
|
notification-polling error; \
|
||||||
|
notifications are stalled until \
|
||||||
|
the reconnect re-issue: {err}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
Ok(None) => {
|
||||||
|
tracing::error!(
|
||||||
|
"alkstore-postgres: forwarder poll \
|
||||||
|
ended without an error (the \
|
||||||
|
server closed the notification \
|
||||||
|
stream); notifications are \
|
||||||
|
stalled until the reconnect \
|
||||||
|
re-issue"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
Err(join_err) => {
|
||||||
|
tracing::error!(
|
||||||
|
"alkstore-postgres: forwarder poll \
|
||||||
|
task aborted or panicked; \
|
||||||
|
notifications are stalled until \
|
||||||
|
the reconnect re-issue: \
|
||||||
|
{join_err}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1079,13 +1117,37 @@ async fn forwarder_loop(
|
|||||||
connected.store(false, Ordering::Release);
|
connected.store(false, Ordering::Release);
|
||||||
let attempt = {
|
let attempt = {
|
||||||
let slot = reconnect_config.clone();
|
let slot = reconnect_config.clone();
|
||||||
|
let mut attempt_no: u64 = 0;
|
||||||
move || {
|
move || {
|
||||||
let guard = slot
|
let guard = slot
|
||||||
.lock()
|
.lock()
|
||||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||||
let config = guard.clone();
|
let config = guard.clone();
|
||||||
drop(guard);
|
drop(guard);
|
||||||
async move { config.connect(NoTls).await.ok() }
|
attempt_no += 1;
|
||||||
|
let attempt_no = attempt_no;
|
||||||
|
async move {
|
||||||
|
// The connect failure is surfaced, not `.ok()`
|
||||||
|
// swallowed (the review 004 item 7 arm): the
|
||||||
|
// attempt index maps the live backoff state (the
|
||||||
|
// next sleep doubles per attempt up to the cap).
|
||||||
|
// The error content is connect/io-class — no DSN
|
||||||
|
// or password material ever reaches the event (the
|
||||||
|
// secret-free law, ADR-025).
|
||||||
|
match config.connect(NoTls).await {
|
||||||
|
Ok(pair) => Some(pair),
|
||||||
|
Err(err) => {
|
||||||
|
tracing::warn!(
|
||||||
|
"alkstore-postgres: forwarder reconnect \
|
||||||
|
connect attempt {attempt_no} failed; the \
|
||||||
|
retry continues on the exponential \
|
||||||
|
backoff (50 ms base doubling, 2 s cap): \
|
||||||
|
{err}"
|
||||||
|
);
|
||||||
|
None
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
let Some((next_client, next_connection)) =
|
let Some((next_client, next_connection)) =
|
||||||
@@ -1115,8 +1177,14 @@ async fn forwarder_loop(
|
|||||||
/// The poll loop over one connection generation: `poll_message`
|
/// The poll loop over one connection generation: `poll_message`
|
||||||
/// forever, fanning notifications into the bounded broadcast. Returns
|
/// forever, fanning notifications into the bounded broadcast. Returns
|
||||||
/// when the connection dies (a fatal poll error or server close) —
|
/// when the connection dies (a fatal poll error or server close) —
|
||||||
/// the forwarder loop reconnects.
|
/// the forwarder loop reconnects. The fatal poll error, when one
|
||||||
async fn poll_loop(mut connection: ListenerConnection, fanout: broadcast::Sender<RawNotification>) {
|
/// exists, is returned so the loop's exit path surfaces it (the
|
||||||
|
/// review 004 item 7 diagnosability arm — an operator must see the
|
||||||
|
/// death instead of the discarded error before the wake loss).
|
||||||
|
async fn poll_loop(
|
||||||
|
mut connection: ListenerConnection,
|
||||||
|
fanout: broadcast::Sender<RawNotification>,
|
||||||
|
) -> Option<tokio_postgres::Error> {
|
||||||
loop {
|
loop {
|
||||||
match std::future::poll_fn(|cx| connection.poll_message(cx)).await {
|
match std::future::poll_fn(|cx| connection.poll_message(cx)).await {
|
||||||
Some(Ok(tokio_postgres::AsyncMessage::Notification(n))) => {
|
Some(Ok(tokio_postgres::AsyncMessage::Notification(n))) => {
|
||||||
@@ -1131,7 +1199,8 @@ async fn poll_loop(mut connection: ListenerConnection, fanout: broadcast::Sender
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
Some(Ok(_)) => {}
|
Some(Ok(_)) => {}
|
||||||
Some(Err(_)) | None => return,
|
Some(Err(err)) => return Some(err),
|
||||||
|
None => return None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1,7 +1,7 @@
|
|||||||
---
|
---
|
||||||
id: pg-fix-forwarder-diagnosability
|
id: pg-fix-forwarder-diagnosability
|
||||||
name: Surface the forwarder's swallowed connect/poll failures (review 004 item 7 arms)
|
name: Surface the forwarder's swallowed connect/poll failures (review 004 item 7 arms)
|
||||||
status: pending
|
status: completed
|
||||||
depends_on: [decide-log-facade-adoption, pg-fix-forwarder-hardening]
|
depends_on: [decide-log-facade-adoption, pg-fix-forwarder-hardening]
|
||||||
scope: narrow
|
scope: narrow
|
||||||
risk: low
|
risk: low
|
||||||
@@ -32,16 +32,16 @@ loop-exit path this task touches.
|
|||||||
|
|
||||||
## Acceptance Criteria
|
## Acceptance Criteria
|
||||||
|
|
||||||
- [ ] Failed reconnect connects are surfaced (not `.ok()`-swallowed)
|
- [x] Failed reconnect connects are surfaced (not `.ok()`-swallowed)
|
||||||
with enough context to diagnose (error class, attempt/backoff
|
with enough context to diagnose (error class, attempt/backoff
|
||||||
state — no endpoint credentials)
|
state — no endpoint credentials)
|
||||||
- [ ] The loop's exit path surfaces the poll error instead of
|
- [x] The loop's exit path surfaces the poll error instead of
|
||||||
discarding it
|
discarding it
|
||||||
- [ ] The output form matches the log-facade decision of record; the
|
- [x] The output form matches the log-facade decision of record; the
|
||||||
secret-free property re-verified by grep
|
secret-free property re-verified by grep
|
||||||
- [ ] Existing forwarder tests green unchanged; the hardening task's
|
- [x] Existing forwarder tests green unchanged; the hardening task's
|
||||||
new pins green
|
new pins green
|
||||||
- [ ] `cargo test -p alkstore-postgres` (harness server), clippy
|
- [x] `cargo test -p alkstore-postgres` (harness server), clippy
|
||||||
`-D warnings`, fmt clean; workspace gates green server-less
|
`-D warnings`, fmt clean; workspace gates green server-less
|
||||||
|
|
||||||
## References
|
## References
|
||||||
@@ -53,8 +53,87 @@ loop-exit path this task touches.
|
|||||||
|
|
||||||
## Notes
|
## Notes
|
||||||
|
|
||||||
> To be filled by implementation agent
|
> 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
|
## Summary
|
||||||
|
|
||||||
> To be filled on completion
|
> 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).
|
||||||
Reference in new issue
Block a user