adapter,raftengine: read the real status off a reset client stream by bootjp · Pull Request #1252 · bootjp/elastickv

bootjp · 2026-09-15T14:29:18Z

The finding

test (ubuntu-latest) failed on #1249 with TestGRPCS3BlobClusterAuthenticatedCapabilityPushAndFetch expecting 0x10 (codes.Unauthenticated) and getting 0x2 (codes.Unknown). The failure is unrelated to #1249 — it is a pre-existing bug on main, and it is a production bug, not a test flake.

gRPC reports a stream the server has already terminated to the sender as a bare io.EOF; the status it terminated with is only readable from the receive side (CloseAndRecv/RecvMsg). Four client-stream senders in the tree returned that io.EOF straight to the caller instead of falling through, so the peer's actual reason was discarded and replaced by an opaque EOF.

Site	Consequence
`adapter/s3_blob_cluster.go` `PushChunkBlob`	Behavioural. `s3_blob_replicator.go:136` decides whether a push is worth retrying from this error. An `Unauthenticated` from a peer missing its token was indistinguishable from a transport hiccup.
`internal/raftengine/etcd/grpc_transport.go` `sendSnapshot`	Diagnosability. A follower that rejected the snapshot for a nameable reason appears in the sender's log as `EOF`.
…`sendSnapshotSpool`	Same.
…`streamFSMSnapshot`	Same.

Whether a given call hits the bug is a race — small payloads usually get the Send in before the reset lands, which is why this surfaced as an intermittent CI failure rather than a constant one.

The fix

Treat io.EOF from the send loop as gRPC's "the status is on the receive side" signal and fall through to CloseAndRecv, which already runs immediately after in all four cases. Any other send error still returns as before. The contract is documented once on sendSnapshotChunk, the leaf both snapshot helpers go through.

Behavior change / risk

Callers that previously saw io.EOF from these four paths now see the peer's real status. Nothing that previously succeeded can now fail: the fix only widens which errors are reported accurately, and the CloseAndRecv it falls through to was already the next statement on the success path.

Test evidence

Two new tests, both reproducing before the fix:

adapter/s3_blob_push_status_test.go — a server that refuses the push before reading a frame; reproduced the exact CI signature (expected 0x10, actual 0x2) 3/3 before the fix.
internal/raftengine/etcd/grpc_transport_snapshot_status_test.go — three subtests, one per snapshot sender.

Revert-check, one guard at a time — each reverted guard fails exactly the test that covers it, and the file restores byte-exact (diff -q):

Reverted site	Test that failed
`streamFSMSnapshot`	`TestSnapshotSendersSurfaceTheReceiversStatus/streamed_FSM_snapshot`
`sendSnapshot`	`…/in-memory_payload`
`sendSnapshotSpool`	`…/spooled_payload`
`PushChunkBlob`	`TestPushChunkBlobSurfacesTheServersStatusWhenItRejectsEarly`

Two things had to be tuned before the tests measured anything, both recorded in comments so the next reader does not undo them:

The payload must stay under gRPC's 4 MiB send cap. My first version used 4 MiB and asserted ResourceExhausted — which the client produces locally for an oversized message. It passed against the buggy code. Now 1 MiB, asserting FailedPrecondition, a status the client cannot manufacture.
The snapshot test shrinks snapshotChunkSize to 32 KiB. At the 16 MiB default the payload is a single write, which can win the race against the reset; that left the in-memory path passing when reverted.

go test ./adapter/ ./internal/raftengine/... -race -count=1 -timeout 40m → ok adapter 662.431s, ok etcd 26.387s, ok transportsoak 1.700s
golangci-lint --config=.golangci.yaml run ./adapter/... ./internal/raftengine/... → 0 issues

Self-review

Data loss — No write, apply, or persistence path changed. A snapshot send that fails still fails; only the error object differs. If anything this reduces the risk of a data-loss-adjacent misdiagnosis, since a rejected snapshot now names its reason.
Concurrency / distributed failures — This is the leader→follower snapshot path and the peer blob-push path. The change adds no shared state and no ordering. It makes leader-change and partition symptoms more legible: a follower rejecting a snapshot mid-transfer used to be reported as EOF.
Performance — One errors.Is on an error path that was already returning. No hot-path or allocation change.
Data consistency — Untouched. No MVCC, OCC, HLC, or route-catalog surface involved.
Test coverage — Four new branches, four revert-checks, one per branch. No Jepsen suite applies — no replication behavior changed, only error reporting. (The snapshot transport is replication-adjacent, which is why all three senders were fixed together rather than only the one with a failing test.)

Note for #1249

#1249's red test (ubuntu-latest) is this bug, not its own change. It should go green once this merges and #1249 picks up main.

https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE

Summary by CodeRabbit

バグ修正
- チャンク送信中に発生するストリーム終了時、単なる「EOF」ではなく、サーバーが返した実際のエラー内容を表示できるようになりました。
- スナップショット送信で、受信側の拒否理由（容量不足、形式未対応など）が送信側へ正しく伝わるようになりました。

gRPC reports a stream the server has already terminated to the sender as a bare io.EOF; the status it terminated with is only readable from the receive side. Four client-stream senders returned that EOF instead of falling through to CloseAndRecv, so the peer's actual reason never reached the caller. PushChunkBlob is the one with a visible consequence: the replicator decides whether a push is worth retrying from this error, and an Unauthenticated from a peer missing its token looked identical to a transport hiccup. It is also a real CI failure -- TestGRPCS3BlobCluster- AuthenticatedCapabilityPushAndFetch asserted Unauthenticated and got Unknown whenever the reset won the race, which is exactly this. The three snapshot senders lose diagnosability rather than behaviour: a follower that rejected a snapshot for a nameable reason appeared in the sender's log as "EOF". The tests push more than the flow-control window so the sender is still writing when the reset lands, and reverting each of the four guards fails exactly the one test that covers it. They stay under gRPC's 4 MiB send cap: an oversized message makes the client reject it locally with a status of its own, which satisfied the assertion without the stream ever being reset -- the first version of this test passed against the bug for that reason. The snapshot test also shrinks the chunk size, because the 16 MiB default turns the payload into a single write that can win the race, which left the in-memory path uncovered. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE

chatgpt-codex-connector · 2026-09-15T14:29:30Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-22T17:26:34.945525Z	`e589954`	Manual request
🔒 Security Review	✅ Completed 2026-10-22T17:26:58.666611Z	`e589954`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

bootjp · 2026-09-15T14:29:31Z

@codex review

bootjp · 2026-09-15T14:29:33Z

@claude review

coderabbitai · 2026-09-15T14:29:35Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 8a3426d6-89b3-4acc-908b-f505b0875301

📥 Commits

Reviewing files that changed from the base of the PR and between c7660b2 and e589954.

📒 Files selected for processing (4)

adapter/s3_blob_cluster.go
adapter/s3_blob_push_status_test.go
internal/raftengine/etcd/grpc_transport.go
internal/raftengine/etcd/grpc_transport_snapshot_status_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 74f998de-6989-4778-b20c-fdab8389bb49

📥 Commits

Reviewing files that changed from the base of the PR and between 4ca7e90 and b121633.

📒 Files selected for processing (4)

adapter/s3_blob_cluster.go
adapter/s3_blob_push_status_test.go
internal/raftengine/etcd/grpc_transport.go
internal/raftengine/etcd/grpc_transport_snapshot_status_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

S3チャンク送信とRaftスナップショット送信で、送信中のio.EOF後にCloseAndRecvを実行します。これにより、受信側の実際のgRPC終了ステータスを返します。各送信経路のステータス伝播テストを追加しました。

Changes

gRPC終了ステータスの伝播

Layer / File(s)	Summary
S3チャンク送信のステータス取得 `adapter/s3_blob_cluster.go`, `adapter/s3_blob_push_status_test.go`	`PushChunkBlob`は送信中に`io.EOF`を受けても`CloseAndRecv`へ進みます。その他の送信エラーは即時に返します。サーバーが`Unauthenticated`で拒否した場合のステータスを検証します。
スナップショット送信のステータス取得 `internal/raftengine/etcd/grpc_transport.go`, `internal/raftengine/etcd/grpc_transport_snapshot_status_test.go`	`streamFSMSnapshot`、`sendSnapshot`、`sendSnapshotSpool`は、`io.EOF`後に`CloseAndRecv`へ進みます。受信側の`FailedPrecondition`を3つの送信経路で検証します。コメントでこの処理の理由を説明します。

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c7660

The updated senders preserve server termination statuses after stream resets, with coverage for all four paths and no remaining actionable risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	タイトルは、adapterとraftengineのクライアントストリームがリセットされた場合に、実際のgRPCステータスを取得する主要変更を明確に示しています。
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

claude · 2026-09-15T14:29:47Z

Claude finished @bootjp's task in 2m 14s —— View job

Review complete

Gather context (PR body, changed files list)
Read changed files (adapter/s3_blob_cluster.go, adapter/s3_blob_push_status_test.go, internal/raftengine/etcd/grpc_transport.go, internal/raftengine/etcd/grpc_transport_snapshot_status_test.go)
Review correctness of the EOF-fallthrough fix
Review new tests
Post review feedback

Note: the sandbox blocked go build/go vet (requires interactive approval I couldn't grant here), so this review is based on static reading, not a local run. The PR body's reported go test ./adapter/... ./internal/raftengine/... and golangci-lint results were not independently re-verified.

Correctness of the fix

The core change is sound. grpc-go's documented contract for client-streaming SendMsg is: if the error originates locally, it's returned as-is; if the server has already terminated the stream, SendMsg returns io.EOF and the real status is only obtainable via RecvMsg/CloseAndRecv. All four sites now correctly fall through to CloseAndRecv on io.EOF instead of returning the opaque EOF:

adapter/s3_blob_cluster.go:262-265 (PushChunkBlob)
internal/raftengine/etcd/grpc_transport.go:521-522 (streamFSMSnapshot)
internal/raftengine/etcd/grpc_transport.go:1023-1024 (sendSnapshot)
internal/raftengine/etcd/grpc_transport.go:1052-1053 (sendSnapshotSpool)

A few things I checked specifically because they're easy to get wrong with this pattern, and they all hold up:

errors.Is survives the errors.WithStack wrap. sendSnapshotChunk (grpc_transport.go:1181-1186) and sendS3ChunkBlobPushFrames (s3_blob_cluster.go:292-294) both wrap the raw stream.Send error with errors.WithStack(err) before returning it. cockroachdb/errors.WithStack implements Unwrap(), so errors.Is(err, io.EOF) at the call site still matches through the wrap — this is the same pattern already used elsewhere in the file (e.g. grpc_transport.go:1198), so it's not a new risk.
No conflation with the local io.EOF handling inside sendSnapshotReaderChunks. That function already uses errors.Is(err, io.EOF) internally (line 1198) to detect "local reader exhausted, payload fit in one read" — a completely different EOF (from the io.Reader, not the network Send). That case is fully resolved inside the function before it ever returns, so it can't be mistaken for the network-reset EOF this PR handles.
Mid-stream failures still propagate correctly. If an early chunk (not just the last one) hits the reset, sendSnapshotChunks/sendSnapshotReaderChunks returns immediately with the wrapped io.EOF, and the top-level check still matches regardless of which chunk failed — so partial-stream resets are handled the same as reset-before-first-chunk.
status.FromError/status.Code still find the status through the wrap. CloseAndRecv's error also gets errors.WithStack'd (e.g. s3_blob_cluster.go:268) before being returned to the caller; grpc-go's status.FromError uses errors.As to unwrap, and this pattern is already used elsewhere in the same function (s3_blob_cluster.go:271), so status codes still surface correctly to callers/tests.
Any other send error is unaffected. The && !errors.Is(err, io.EOF) guard only widens the fallthrough case; a genuine local error (context canceled, marshal failure, etc.) still returns immediately as before.

The doc comment on sendSnapshotChunk (grpc_transport.go:1176-1180) is a good call — it's the one leaf both snapshot helper paths go through, so documenting the contract there instead of at all three call sites avoids repeating the same explanation three times.

Tests

Both new tests are well constructed for what they're proving:

adapter/s3_blob_push_status_test.go: server rejects with Unauthenticated before reading any frame; the 4 MiB payload chunked into 256 KiB frames (s3BlobFetchFrameBytes, adapter/s3_blob_fetch.go:22) gives ~16 frames, which is enough to make the sender still be writing when the reset lands (avoiding the small-payload race the PR description calls out).
internal/raftengine/etcd/grpc_transport_snapshot_status_test.go: one test, three subtests, covering all three snapshot senders against a FailedPrecondition-rejecting server, with snapshotChunkSize shrunk to 32 KiB specifically to defeat the "whole payload in one write" race noted in the code comment (grpc_transport_snapshot_status_test.go:66-69). Using FailedPrecondition instead of ResourceExhausted for a 1 MiB payload avoids the case where the client locally rejects an oversized message before ever talking to the server — a good catch, since that would have made the test pass against the buggy code too (a false positive).
The revert-check table in the PR description (each guard removal fails exactly the test that covers it) is exactly the kind of coverage the "Test coverage" self-review checklist item asks for.

Minor observations (non-blocking)

The four call sites repeat err != nil && !errors.Is(err, io.EOF) verbatim. Given there are only four occurrences and each is a one-line diff on an existing if, I don't think a shared helper is warranted here — it would add an abstraction for a pattern that's more readable inline, consistent with the project's stated preference against premature abstraction.
Both test files register real listeners/servers rather than mocking the gRPC layer, which is the right call for pinning a wire-level contract like this (a mock could trivially miss the actual SendMsg/RecvMsg behavior being tested).

Overall this is a precise, well-scoped fix with solid regression coverage matching the PR's own self-review. I don't see correctness issues in the diff.

chatgpt-codex-connector · 2026-09-15T14:32:07Z

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: b1216333f3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot

🟢 Approval recommended

The EOF handling follows the gRPC streaming contract and is covered by targeted regression tests.

0 open findings

What changed in this PR

Fixes client-stream error handling so callers receive the server’s actual gRPC status instead of an opaque EOF.

Changes:

Falls through to CloseAndRecv when snapshot or S3 sends return EOF.
Adds regression coverage for all four affected send paths.

File	Description
`adapter/s3_blob_cluster.go`	Recovers server status after early stream termination.
`adapter/s3_blob_push_status_test.go`	Tests early S3 push rejection.
`internal/raftengine/etcd/grpc_transport.go`	Fixes status propagation across snapshot senders.
`internal/raftengine/etcd/grpc_transport_snapshot_status_test.go`	Tests all snapshot sender paths.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

bootjp · 2026-10-22T17:23:06Z

Please review current HEAD e589954 after its latest push. Merge will wait for a passing review of this exact HEAD and successful CI. @codex review

chatgpt-codex-connector · 2026-10-22T17:26:33Z

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: e589954b67

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-10-22T17:26:57Z

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: e589954b67

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

bootjp mentioned this pull request Sep 15, 2026

adapter: fix the XREAD wrong-type test's shared timing budget #1249

Merged

Merge main into fix/client-stream-status

c7660b2

Copilot AI balanced review requested due to automatic review settings October 11, 2026 10:44

Copilot started reviewing on behalf of bootjp October 11, 2026 10:52

View session

Copilot AI reviewed Oct 11, 2026

View reviewed changes

Merge commit '16b56f2d7a8c184e5ffba8faeb9a7df477707ff9' into HEAD

e589954

bootjp merged commit 852c690 into main Oct 22, 2026
9 checks passed

bootjp deleted the fix/client-stream-status branch October 22, 2026 17:28

Navigation Menu

Repository navigation

adapter,raftengine: read the real status off a reset client stream - #1252

adapter,raftengine: read the real status off a reset client stream#1252
bootjp merged 3 commits into
mainfrom
fix/client-stream-status

bootjp commented Sep 15, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026 •

edited

Loading

Uh oh!

bootjp commented Sep 15, 2026

Uh oh!

bootjp commented Sep 15, 2026

Uh oh!

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

claude Bot commented Sep 15, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

bootjp commented Oct 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 22, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bootjp commented Sep 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The finding

The fix

Behavior change / risk

Test evidence

Self-review

Note for #1249

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

bootjp commented Sep 15, 2026

Uh oh!

bootjp commented Sep 15, 2026

Uh oh!

coderabbitai Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

claude Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Correctness of the fix

Tests

Minor observations (non-blocking)

Uh oh!

chatgpt-codex-connector Bot commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

bootjp commented Oct 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 22, 2026

🛡️ Codex Security Review · Automatically triggered

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bootjp commented Sep 15, 2026 •

edited by coderabbitai Bot

Loading

chatgpt-codex-connector Bot commented Sep 15, 2026 •

edited

Loading

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

claude Bot commented Sep 15, 2026 •

edited

Loading