numinnex commented on code in PR #3753:
URL: https://github.com/apache/iggy/pull/3753#discussion_r3657986828
##########
core/consensus/src/client_table.rs:
##########
@@ -345,33 +401,47 @@ impl ClientTable {
return;
};
- let slot = self.slots[slot_idx].as_ref().expect("index/slot mismatch");
- let slot_header = slot.reply.header();
- let slot_commit = slot_header.commit;
- let slot_request = slot_header.request;
+ let entry = self.slots[slot_idx].as_mut().expect("index/slot
mismatch");
assert_eq!(
- slot.session, session,
- "commit_reply: session mismatch for client {client_id}: \
- entry={}, prepare={session}",
- slot.session
+ entry.epoch, epoch,
+ "commit_reply: epoch mismatch for client {client_id}: \
+ entry={}, prepare={epoch}",
+ entry.epoch
);
+ let latest_commit = entry.latest().header().commit;
assert!(
- new_commit >= slot_commit,
- "commit_reply: commit regression for client {client_id}:
{slot_commit} -> {new_commit}",
+ new_commit >= latest_commit,
+ "commit_reply: commit regression for client {client_id}:
{latest_commit} -> {new_commit}",
);
assert!(
- new_request >= slot_request,
- "commit_reply: request regression for client {client_id}:
{slot_request} -> {new_request}",
+ new_request >= entry.watermark,
Review Comment:
Remedy landed: neither apply nor replay asserts anymore. `commit_reply`
returns a typed `CommitReply { Cached, NoEntry, SkippedRegression { stored,
received } }` and the callers log it (`warn!` for the regression, trace for the
missing entry). The wire reply ships either way, so a skip degrades one entry's
dedup instead of taking down a shard pump or blocking boot.
The reachability chain does not hold though, which matters for whether this
is a regression or a latent hazard.
The shape needs two `Register(X)` for the same X inside one replay window,
and a second Register is only ever proposed when the table has no entry for X.
That is the same fast path you point out on `submit_register_in_process`: entry
present means the epoch is returned and nothing is proposed.
- Within one process, absence means `evict_oldest` dropped it, so X has to
come back. The Rust SDK abandons the key on re-login (`*self = Self::new()`, as
you note on dispatch.rs:2553) and the HTTP counter is monotonic per process, so
no id repeats.
- Across a restart, absence means the entry was not recovered, which means
`Register(X)@a` fell below the replay floor. Replay then never sees `@a`, so
there is no rebind branch to take and the watermark starts at 0.
`R < 5` via HTTP renumbering needs the cross-restart case, which is the one
that cannot also supply `@a`.
It is also not introduced here. On master the same WAL panics through a
different door: `Register(X)@c` carries a different `commit`, so
`commit_register` takes its session-mismatch warn-and-skip arm and leaves the
old entry in place with `reply.request == 5`, and the next `commit_reply` for
`R < 5` fails `assert!(new_request >= slot_request)`. Same input, same panic,
before this PR.
Fixed regardless: asserting on state that diverges replica-locally was the
wrong failure mode whether or not the current tree can produce the input, and
the epoch param went with it (your client_table.rs:375 comment).
##########
core/consensus/src/metadata_helpers.rs:
##########
@@ -116,29 +122,45 @@ where
// Session evicted under capacity pressure. SAFETY: catch-up gate makes
// this replica authoritative for session truth.
RequestStatus::NoSession =>
PreflightOutcome::Evict(EvictionReason::NoSession),
- RequestStatus::SessionMismatch { expected, received } => {
- // expected > received: stale session (rotated post-eviction) ->
terminal eviction.
- // expected < received: client bug; silent drop, log.
- // SAFETY: catch-up gate makes this replica authoritative.
- if expected > received {
- PreflightOutcome::Evict(EvictionReason::SessionTooLow)
- } else {
- // Catch-up gate rules out network race; newer-than-issued
- // session = client bug. Error log, no eviction (transient bug
- // must not kill session), no rate limit (per-event).
- tracing::error!(
- client_id,
- expected,
- received,
- "request_preflight: ignoring newer session (client bug)"
- );
- PreflightOutcome::Drop
- }
+ // Zombie holdover from before a re-register: terminal for that
+ // holder. SAFETY: catch-up gate makes this replica authoritative.
+ RequestStatus::Fenced { current, received } => {
+ tracing::debug!(
+ client_id,
+ current,
+ received,
+ "request_preflight: fencing stale-epoch request"
+ );
+ PreflightOutcome::Evict(EvictionReason::SessionTooLow)
+ }
+ // Catch-up gate rules out network race; an epoch newer than any this
+ // table minted = client bug. Error log, no eviction (transient bug
+ // must not kill the session), no rate limit (per-event).
+ RequestStatus::EpochAhead { current, received } => {
+ tracing::error!(
+ client_id,
+ current,
+ received,
+ "request_preflight: ignoring future epoch (client bug)"
+ );
+ PreflightOutcome::Drop
+ }
+ // Same request id, different request bytes: replaying the cached
+ // reply would answer the wrong request, and re-executing would
+ // double-apply. Loud drop; the client must fix its numbering.
+ RequestStatus::ChecksumMismatch { request } => {
+ tracing::error!(
+ client_id,
+ request,
+ "request_preflight: request id reused for a different
operation (client bug)"
+ );
+ PreflightOutcome::Drop
+ }
+ // Applied once, original reply aged out of the ring: refuse
+ // re-execution, nothing to replay. Silent drop.
+ RequestStatus::AlreadyApplied { .. } |
RequestStatus::AlreadyRegistered { .. } => {
Review Comment:
Payload is logged now: `warn!` with `client_id`, `request` and `watermark`,
since a run of these means the reply ring is undersized for the client's retry
latency.
The last line does not hold as scoped. A correct client has at most one
request in flight per session, which is the invariant the ring capacity doc
rests on, so the in-doubt id is by definition its own watermark. The
watermark's reply is structurally the newest ring entry: `push_latest` pops the
front, and only a strictly newer request triggers a pop. So the watermark's
reply cannot age out while it is still the watermark, and retrying an in-doubt
id resolves as `Duplicate`, never here. Reaching `AlreadyApplied` needs a
request strictly below the watermark whose reply has aged out, which means
either more than one op in flight or a return to an older id.
The path where a well-behaved client does land below an inherited watermark
is the post-restart renumbering on recovery.rs:220. The cross-user half of that
is closed now, `submit_register_in_process` requires the authenticated user to
own the entry before binding. The same-user half remains: a fresh session that
collides with its own prior entry inherits a watermark it has no way to read,
which is the DX gap on client_table.rs:577.
Kept as a drop rather than an empty Reply. The requests that reach it did
commit, so `InvalidCommand` would be a wrong answer rather than a fast one, and
the clients that can reach it are misnumbering. Happy to emit the reply instead
if you would rather have fail-fast there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
