fix(cluster): serve metadata reads at or above the client's own writes - #4024
fix(cluster): serve metadata reads at or above the client's own writes#4024numinnex wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4024 +/- ##
=============================================
- Coverage 85.25% 68.41% -16.84%
Complexity 1402 1402
=============================================
Files 1236 1236
Lines 182594 156098 -26496
Branches 148898 122405 -26493
=============================================
- Hits 155664 106799 -48865
- Misses 22886 45335 +22449
+ Partials 4044 3964 -80
🚀 New features to boost your workflow:
|
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
two things I couldn't anchor to the diff:
responses.rs:241is the sharedconsumer_group_membershipsread behindGET_ME,GET_CLIENTSandGET_CLIENT, and the HTTPget_clients/get_clienthandlers reach it withoutread_localeither. Same gap as the binary arms, on both planes.dispatch/submit.rs:217andresponses.rs:1569are the sameCommand::Replyguard plustry_from_bytes::<ReplyHeader>decode, and both fire on the same message indispatch/mod.rs. Onereply_header()helper inresponses.rscovers both.
One thing I looked at and decided against: merging hold_for_frontier with await_recovery_barrier. They're bounded differently (poll count vs wall-clock deadline), and the merge would delete barrier_state's tests and turn the max_polls: 8 injection into a flaky wall-clock test. Worth taking only the one-line crumb - drop the local POLL const and reuse READ_FRONTIER_POLL.
github-actions
Bot
added
S-waiting-on-author
|
/ready |
82db2b4 to
3d95831
Compare
github-actions
Bot
added
S-waiting-on-review
github-actions
Bot
added
S-waiting-on-author
|
/ready |
github-actions
Bot
added
S-waiting-on-review
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
pr description paragraph 3 still says the forwarding-active case is not closed - "that is additive but touches every control-plane write response, so it is left out here and documented at the gate". 456c38a closes it in this same pr via the applied-op header. worth updating before merge, since the body lands in the squash commit message.
the two comments below are the same staleness in the code docs.
| // This node's own view only: on a follower with HTTP forwarding ON the | ||
| // write is relayed to the primary by the middleware and never reaches | ||
| // this task, so the follower's floor stays where the register left it | ||
| // and its read-your-writes guarantee is the register epoch's. Closing |
There was a problem hiding this comment.
the last three lines are stale as of this branch. forward.rs now copies iggy-applied-op off the relayed response and record_relayed_floor records it as this caller's floor, so the primary's commit op does come back with the relayed response. a future reader lands here and concludes the forwarding hole is still open.
one caveat that does survive and is worth keeping in the rewrite: classify_reply relays a 503 carrying TransientNotCommitted instead of retrying it, and record_relayed_floor only records on 2xx - so a write that did commit behind that code still leaves no floor. that's the honest version of what this comment is reaching for.
| /// | ||
| /// Scope is this node's own view. A user whose write this node relayed over | ||
| /// HTTP, or who wrote through a different node entirely, left no floor here; | ||
| /// closing that needs the serving primary's commit op to reach the reading |
There was a problem hiding this comment.
first clause is stale - a write this node relayed over HTTP does leave a floor now, record_relayed_floor records the serving primary's applied op off the response header. second clause still holds: a user who wrote through a different node entirely leaves nothing here.
something like:
/// Scope is this node's own view. A write this node RELAYED leaves a floor
/// here: the relay records the serving primary's applied op off the response
/// (see `http::forward::record_relayed_floor`). A user who wrote through a
/// different node entirely still leaves none.
2 participants
A client that commits a metadata write and then re-homes its session onto a backup can be served the pre-write state. auth.rs already documents the gap: the session epoch is the register's commit op, and on a backup that forwarded the proposal the local applied commit still lags it. A backup applies committed ops only as its commit walk advances, and nothing tied a read to the op the client's own write committed.
Metadata reads now gate on the connection's committed watermark. One applied-frontier counter per process advances after every metadata apply and is shared with every shard, which for the first time gives a shard without consensus the applied position it had no way to observe. The watermark comes from the commit field replies already carry, seeded at bind from the session epoch. The fast path is a single atomic load with no awaits, so the shared-nothing read path is unchanged; a lagging node parks briefly, then fails the read retryable rather than answering stale.
Over HTTP this closes the forwarded-register case, where a healthy backup forwards the register so the bound epoch can exceed the local frontier. It does not close the case where forwarding is active: the follower relays the write, its handler never runs, so the node that later serves the read holds no session and no watermark. Closing that needs the serving primary's commit op to travel back to the reading node, for instance a response header beside the view the forward middleware already relays. That is additive but touches every control-plane write response, so it is left out here and documented at the gate.
The BDD delete-then-get steps now assert "not the stream we deleted" instead of "nothing at this id". The server hands a deleted stream's numeric id straight to the next create, so once scenarios share a server the old assertion cannot hold, and removing the polling loop without this would have left the spec flaky for an unrelated reason.