Summary
This is a pure refactor — it fixes no bug and changes no observable behavior. It removes an asymmetry in how append result channels are delivered, so a whole class of "wrong channel type" mistakes (e.g. the retry bug fixed for #225 / #226) becomes structurally impossible.
There are two delivery models for an AppendOp's per-replica durability result today:
- Single-entry remote (
AppendEntry) folds the gRPC stream into the channel: it requires a *channel.RemoteResultChannel, calls InitResponseStream(...), and the channel's ReadResult pulls from the stream via stream.Recv() (common/channel/remote_result_channel.go:132,187; woodpecker/client/logstore_client_remote.go:112-118).
- Batch remote (
AppendEntries) keeps the channel as a plain mailbox: a demux goroutine reads the stream and fans each result out via ch.SendResult(...) into per-entry LocalResultChannels (woodpecker/client/logstore_client_remote.go:204-234).
- Embedded (
logStoreClientLocal) also treats the channel as a plain mailbox — it just hands it to store.AddEntry, and the store SendResults into it (woodpecker/client/logstore_client_local.go:40,49).
So two of the three paths already use the mailbox model; only single-entry remote embeds the stream in the channel. That single exception is the sole reason RemoteResultChannel (and its type assertion) exists, and the sole reason a channel installed by one path can be the "wrong type" for another.
Proposed change
Unify on the mailbox model: make single-entry remote AppendEntry own the stream read (spawn a reader that Recv()s and SendResults into a plain result channel), exactly as AppendEntries already does. Then:
Explicitly not in scope / not a bug fix
Acceptance
RemoteResultChannel removed (or reduced to a thin alias) and the type assertion in logStoreClientRemote.AppendEntry gone.
- All existing append/quorum/retry tests pass unchanged (behavior preserved).
- No new config, no API change.
Follow-up to #225. Not blocking any of the #225 fix PRs.
Summary
This is a pure refactor — it fixes no bug and changes no observable behavior. It removes an asymmetry in how append result channels are delivered, so a whole class of "wrong channel type" mistakes (e.g. the retry bug fixed for #225 / #226) becomes structurally impossible.
There are two delivery models for an
AppendOp's per-replica durability result today:AppendEntry) folds the gRPC stream into the channel: it requires a*channel.RemoteResultChannel, callsInitResponseStream(...), and the channel'sReadResultpulls from the stream viastream.Recv()(common/channel/remote_result_channel.go:132,187;woodpecker/client/logstore_client_remote.go:112-118).AppendEntries) keeps the channel as a plain mailbox: a demux goroutine reads the stream and fans each result out viach.SendResult(...)into per-entryLocalResultChannels (woodpecker/client/logstore_client_remote.go:204-234).logStoreClientLocal) also treats the channel as a plain mailbox — it just hands it tostore.AddEntry, and the storeSendResults into it (woodpecker/client/logstore_client_local.go:40,49).So two of the three paths already use the mailbox model; only single-entry remote embeds the stream in the channel. That single exception is the sole reason
RemoteResultChannel(and its type assertion) exists, and the sole reason a channel installed by one path can be the "wrong type" for another.Proposed change
Unify on the mailbox model: make single-entry remote
AppendEntryown the stream read (spawn a reader thatRecv()s andSendResults into a plain result channel), exactly asAppendEntriesalready does. Then:RemoteResultChanneland thesyncedResultCh.(*RemoteResultChannel)type assertion can be deleted.Explicitly not in scope / not a bug fix
receivedAckCallbackgoroutine, so this is likely a wash, but it should be confirmed, not assumed).Acceptance
RemoteResultChannelremoved (or reduced to a thin alias) and the type assertion inlogStoreClientRemote.AppendEntrygone.Follow-up to #225. Not blocking any of the #225 fix PRs.