numinnex commented on PR #3753:
URL: https://github.com/apache/iggy/pull/3753#issuecomment-5093410562
Thanks for this review -- the identity model was the right thing to block
on, and two of your findings turned out to be load-bearing in ways I argued
against before coming around. Everything from the numbered blocker list is
addressed; details are in the per-thread replies, and I have resolved the
threads that are fully closed. Summary of what changed and what I deliberately
left open.
## The identity model
**Fence is log-derived again.** `commit_register` sets the entry's epoch to
the register's commit op rather than a per-entry counter. Your remedy #3, which
I initially declined on the grounds that an op-derived value would not be
comparable across planes -- that conflated the fence *value* with the table's
*identity*. The key stays client-supplied; Register commits only in the
metadata group, so there is one minting authority and any group's slice stores
a plain `u64`. What the counter was quietly trading away is the thing you were
pointing at: a counter *resets* when an entry is dropped by capacity eviction
and re-created, so a zombie holding epoch 1 and a fresh incarnation holding
epoch 1 are indistinguishable. Ops do not reset.
**Binds commit.** The fast-path short-circuit in
`submit_register_in_process` is gone and `register_preflight` no longer
consults the table (`check_register` and `RequestStatus::AlreadyRegistered`
deleted), so every bind proposes and the epoch actually moves.
`Fenced`/`EpochAhead` are reachable on the live path for the first time -- the
inertness you flagged. The ownership gate stays ahead of the dispatch and still
refuses another user's entry; an owned entry falls through to a real Register
whose rebind branch preserves watermark and reply ring. Cost is one consensus
round trip per reconnect, which is the price of a bind being a logged event.
**Credential-free resume is deleted, not gated.** `try_resume_session` and
`MetadataSubmit::ResumeLookup` are gone; resume runs through the login path,
which already did it correctly. That took findings #1, #2, #5, #6 and #9's
first half with it.
**`client_seq` no longer starts fresh into a recovered table.** The
coordinator reseeds the minter above the highest recovered client-id sequence
at boot, before any listener accepts. This makes the restart re-mint collision
*unreachable* rather than merely refused, which demotes the ownership gate to a
backstop.
## Apply-path robustness
`commit_reply` returns a typed `CommitReply { Cached, NoEntry,
SkippedRegression }` instead of asserting; callers log it. The wire reply ships
either way, so a skip degrades one entry's dedup rather than taking down a
shard pump or refusing to boot. The unreachable `epoch` param went with it.
`install_client_table` is a logged refusal returning `bool`, not an `assert!`
-- your point that a boot-path panic is worse than a refusal.
`evict_oldest` no longer consults pipeline state, so the victim choice
depends only on committed state. It also no longer casts headers: `client_id`
and `latest_commit` are denormalized onto the entry, and it returns the freed
slot so `commit_register` skips the second array walk.
## Correctness
- PAT replay: a replayed `CreatePersonalAccessToken` is refused with
`PersonalAccessTokenAlreadyExists` rather than served a cached success with a
freshly minted secret spliced in. The complete fix is derivable secrets so a
replay can reproduce the original token; that is a wire change and wants its
own issue.
- HTTP request id is burned at stamp time, before any fallible work.
- `AlreadyApplied` now answers with a terminal `RequestAlreadyApplied` code
instead of silence, so the client learns immediately instead of waiting out its
read timeout. I pushed back on this initially; you were right that terminal
deserves a frame.
- The checksum reuse check runs at every cached-reply hit, not just at the
watermark.
- Slot reclaim: non-group-member disconnects defer reclaim by a grace window
and a sweeper submits the `Logout`. Verifying that turned up a leak in my own
fix -- the drained reclaim was dropped on a transient submit failure, leaving
exactly the slot-until-eviction case your comment describes. The sweeper now
re-arms before submitting and clears only on a committed `Logout`.
- `Operation::Reserved` is rejected at ingress, so the `Reserved, request =
0` path can no longer reach the dedup preflight ahead of the operation gate.
- Duplicate match arms collapsed; `ClientEntry` is private;
`session`/`request_checksum` documented on all three headers; stale contiguity
and `SessionMismatch` docs swept.
## Tests
Ingress validation is pinned (`RequestHeader::validate` through
`try_into_typed`: the field rules table plus zero-client rejection), since
several conclusions on both sides turned on whether it runs. The replay proof
is now direct -- `commit_request` returns the committed reply and the retry is
asserted byte-identical, with `op` equality being what rules out a re-apply; it
no longer rests on `CreateStream` rejecting a duplicate name. Both restart
specs run single-node and 3-node. New unit coverage for the reclaim retry, the
minter reseed, the terminal refusal, and the denormalized eviction ranking.
## Open, by intent
- **Watermark surfaced to SDKs.** The gateway case is closed (the register
submit returns `BoundSession { epoch, watermark }` and HTTP seeds its numbering
above the inherited watermark), so no wire change was needed. An SDK resuming
across its *own* process restart still needs it in the bind reply; that is
tier-2 identity and wants the protocol discussion.
- **Sessions below the snapshot floor** (recovery.rs:201) -- needs the table
in the checkpoint, which is the state-transfer work.
- **Status is not re-checked for bound sessions.** Deactivating a user does
not stop its live sessions. Master has the identical gap (same two
`remove_client` sites, same login-only check, no status reference in the gate),
so it is not from this PR; deletion *is* enforced per-op because `DeleteUser`
purges the permissioner and the rules fail closed. Filing separately, which is
why I left that thread open.
- Replayed `Logout` group staleness, and `Reserved`'s post-preflight gate
ordering beyond the ingress fix.
I also corrected two of my own earlier replies in this thread where the
epoch rework made them wrong.
--
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]
