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

[bootjp]

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:

1. 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.
2. 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

1. 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.
2. 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.
3. Performance — One errors.Is on an error path that was already returning. No hot-path or allocation change.
4. Data consistency — Untouched. No MVCC, OCC, HLC, or route-catalog surface involved.
5. 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」ではなく、サーバーが返した実際のエラー内容を表示できるようになりました。
  • スナップショット送信で、受信側の拒否理由（容量不足、形式未対応など）が送信側へ正しく伝わるようになりました。

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-15T14:32:08.551164Z	b121633	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]

@codex review

[bootjp]

@claude review

[coderabbitai]

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 b1216

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.

• Fix all pre-merge checks with AI

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

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]

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".