[PureGo] Generalize stream durability model - #682
Conversation
Add protocol-neutral unit-count acknowledgments, partial replay slicing, and submission receipts while preserving atomic proto and JSON behavior. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
355d04d to
dec7d80
Compare
Building AckState allocated a fresh slice of submitted ranges on every acknowledgment that arrived while a send was in flight, which profiling put at 27% of allocations under single-record ingestion. Models are already forbidden from retaining Ranges, so one reused buffer can back every ack. Single-record ingestion against a live table drops from 1417 to roughly 1011 bytes per record, and per-record cost no longer grows with the number of outstanding ranges. Batched ingestion was already unaffected. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
| waiting := true | ||
| for waiting { | ||
| select { | ||
| case acknowledged := <-ackProgress: |
There was a problem hiding this comment.
A terminal receive status can be lost once ackProgress satisfies this wait. We
then close the transport and wait for receiverDone, but never consume the
receiver result published during that close, so runOnce returns the earlier
send error. A permanent server rejection can consequently be retried, and an
authentication rejection skips credential invalidation.
Could we preserve a non-cancellation receiver error after reaping the receiver,
using the same arbitration rule as the other sender-first paths?
<-receiverDone
if !receiverReported {
select {
case receiverErr := <-receiverExitCh:
if receiverErr != nil && !errors.Is(receiverErr, context.Canceled) {
cause = receiverErr
}
default:
}
}It would be good to cover a partial receipt followed by its prefix ACK, with
Recv returning codes.Unauthenticated when teardown closes the wire, and
assert that Flush returns codes.Unauthenticated rather than the send error.
There was a problem hiding this comment.
Fixed as suggested
| // larger than one timeout's worth of work must not be torn down. A | ||
| // duplicate or stale partial makes no progress and leaves the | ||
| // deadline untouched, so a stalled server cannot postpone recovery. | ||
| b.flight[0].pendingAt = time.Now() |
There was a problem hiding this comment.
Refreshing only the head item's pendingAt leaves every trailing item on its
original deadline. If items A and B enter flight together and partial ACKs keep A
alive past one timeout, completing A makes B the head with an already-expired
deadline. The timer read at
purego/internal/stream/core.go:1352
then tears down a healthy stream immediately, even though durable progress just
arrived.
Since partial progress is intentionally allowed to extend the head item here,
could we carry that extension forward when the next item becomes head, or base
the timeout on the last genuine durability progress instead?
A focused regression case can enqueue two row-count items, partially ACK A before
the timeout, fully ACK A after B's original deadline, and assert that B is not
immediately replayed on a second connection.
There was a problem hiding this comment.
Yes, I'll allow the partial progress to extend to the next item as well. Also another case I've noticed - if a partial ack extends progress of the current head, then the full ack of a previous item should refresh the new head's deadline as well, which we do not currently do. Now all progress refreshes the deadline.
Adding the regression cases too.
| receipt.SubmittedUnits = it.units | ||
| } | ||
| } | ||
| if receipt.SubmittedUnits > it.units { |
There was a problem hiding this comment.
An over-reported receipt or a short successful receipt is a deterministic adapter
contract violation, but these paths create ordinary fmt.Errorf values, which
the supervisor treats as retryable. The duplicate checks at
purego/internal/stream/core.go:1550
do the same. Recovery therefore reconnects and replays the same payload after an
impossible local result.
Could we classify both validation sites as non-retryable, for example:
err = wrapValidation(fmt.Errorf("stream: invalid submission receipt: ..."))
return rejectSubmission(wrapValidation(fmt.Errorf(
"stream: invalid submission receipt: ...",
)))The existing overReportingRowWire is a deterministic regression seam. With
recovery enabled and a second wire available, the test can assert that the error
is terminal and no second connection receives a replay.
There was a problem hiding this comment.
fixed now, made these non-retryable with wrapValidation, also added the suggested test
| ranges := submitted | ||
| limit := submittedUnits | ||
| if includeSending && sending != nil { | ||
| ackScratch = append(append(ackScratch[:0], submitted...), *sending) |
There was a problem hiding this comment.
The scratch buffer removes the per-ACK allocation, but this append still copies
all unacknowledged ranges whenever an ACK arrives while Send is active. Under
continuous ingestion sending is usually non-nil, so ACK processing remains
O(n) in the outstanding window and can consume substantial memory bandwidth at
the default in-flight limit.
Could we represent the active range separately in AckState, or otherwise let
the resolver consume the submitted slice and the one active range without
materializing a combined slice?
There was a problem hiding this comment.
Yes, thank you! I went with representing the active range separately in AckState (now holds Active+HasActive boolean). It exposes NumRanges&RangeAt functions to be iterated over. The ResolveAcknowledgedUnits now loops over the submitted and active range in submission order without combining them into a merged slice.
Building AckState combined the submitted ranges and the in-flight one into a single slice, so every acknowledgment that arrived mid-send copied the whole outstanding window. Under continuous ingestion a send is almost always active, leaving acknowledgment processing linear in the in-flight window and burning memory bandwidth at the default limit. AckState now carries the active range separately and exposes NumRanges and RangeAt, so the resolver walks the submitted slice and the active range in submission order without materializing a combined slice. Building the state is now a fixed-size copy regardless of window size. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
An over-reported receipt, or a short receipt from a Send that reported success, is a deterministic transport defect rather than a network failure. Both were ordinary errors, so recovery reconnected and replayed the same payload until the retry budget ran out. Classify all four validation sites as non-retryable so the stream fails on first sight. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
The lack-of-ack budget was refreshed only when an ack left a partial in the head item, so a boundary-aligned full ack promoted the next item with the deadline it received on entering flight. Once partial acks had carried the head past that deadline, completing it tore down a healthy stream on the very ack that proved the server alive. Refresh the head on any durable progress instead. Ordered acks mean a promoted item could not have been made durable sooner, and an ack at or below the current watermark still returns before reaching the buffer, so a stalled server earns no extension. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
A partial submission whose prefix ACK ended the durability wait reaped the receiver without reading its result, so the server's status was dropped and the earlier send error was returned instead. Recovery then replayed against a server that rejects every attempt, and an auth rejection never invalidated its credential. Apply the sender-first arbitration after the reap, restricted to definitive rejections: the abort happens first here, so a retryable receiver error is an artifact of it rather than the cause. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
| @@ -882,20 +1014,57 @@ waitLoop: | |||
| default: | |||
| } | |||
| case errors.As(cause, &ps): | |||
There was a problem hiding this comment.
On main the pause case was an immediate hard abort (stream.Close()). Switching it to gracefulTeardown, plus resolvePendingOnExit(!isPause) now waiting up to DrainTimeout for the in-flight Send instead of aborting it, is a real behavior change on the shipping proto/JSON path — server CloseStreamSignal hits this.
Effects for proto/JSON:
- the in-flight Send is allowed to complete (and can become durable if an ack was already buffered) rather than being aborted and replayed;
- the server sees
END_STREAMinstead of an abrupt cancel; - teardown can take up to ~2x
DrainTimeoutwhen a Send is slow/stuck — the receiver'sresolvePendingOnExitwait gatesreceiverDone, andgracefulTeardownthen waits onreceiverDoneagain with its own budget.
It's bounded (no hang) and probably a fine change, but it's not behavior-neutral vs main, so the "no user-visible difference" rationale for skipping the changelog doesn't quite hold. Can we confirm it's intended and covered by a pause test that exercises a slow/stuck Send (not just fakeRPC.CloseSend closing Recv)?
There was a problem hiding this comment.
Added two pause tests with a controlled Send: TestCoreStreamPauseAwaitsSlowSendAndKeepsAck
(Send completes after the pause deadline, the buffered ack becomes durable, nothing replays — fails against main's abort) and TestCoreStreamPauseStuckSendStaysBounded (Send never returns, with and without a buffered ack: bounded abort, record replays).
| payload := it.payload | ||
| if it.ackedUnits > 0 { | ||
| var err error | ||
| payload, err = cs.enc.slice(payload, it.ackedUnits) |
There was a problem hiding this comment.
This slice-before-decode makes the GetUnackedBatches doc comment above (lines 691-696) inaccurate for the generic core: it still says the grouping is "the unit the server acks atomically" and reproduces "the original durability boundaries," but for a record-count protocol the returned group is now the unacked suffix, not the original ingest group. Still true for proto/JSON (ackedUnits is always 0), but since this PR introduces the divergence, worth updating the comment here — and the public Stream.GetUnackedBatches wording ("grouped by ingest call") too.
There was a problem hiding this comment.
Good catch, fixed both. The internal comment now says an entry is safe to replay as one batch — exactly the ingest call for proto/JSON, only the unacked suffix when a protocol acks part of an item — and the public doc reads "grouped for replay: one group per ingest call, minus any prefix already made durable."
A server-requested pause now tears the connection down gracefully rather than aborting it, so a Send that is still outstanding finishes and an acknowledgment already received for it makes the record durable instead of being discarded and replayed. That is intended, but it is not behavior neutral on the shipping proto/JSON path: record it in NEXT_CHANGELOG.md and guard it with pause tests for a slow Send that completes and a stuck Send that only the drain budget can end. GetUnackedBatches no longer claims every group reproduces the original ingest call. A protocol that can acknowledge part of an item returns only the unacknowledged suffix, so both the internal and public doc comments describe the group as a replay unit. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
Summary
Generalizes the internal stream core so one implementation can serve both the existing atomic protocols (Protocol Buffers, JSON) and a future record-count protocol, with no public API change.
EncoderHooks,AckModelHooks,OpenFunc, and an exportedWireStreamlet a protocol instantiate the core over its own payload type.Atomic proto/JSON behavior is unchanged: those encoders report one unit per item, never produce partial acknowledgments, and continue to resolve wire offsets through the existing path.
Behavior fixes found while reviewing this change
Sendis still outstanding, where thatSendthen fails, is no longer converted into a non-retryable protocol error. The core cannot know how much of that send reached the server, which is not a server violation — so the unusable ack is discarded and the retryable send failure drives recovery. Without this, a transient failure killed the stream permanently instead of reconnecting, and blamed the server for a violation it never committed.Close()/Terminate()caller.Sendthat reported success, is a deterministic transport defect: reconnecting replays the same payload into the same defect, so the stream burned its whole retry budget before failing. Accepting a short receipt from a successful send would also record unsubmitted units as submitted, so it has to fail loudly instead.Performance
Resolving an acknowledgment while a
Sendis in flight no longer allocates or copies the outstanding window.AckStatecarries the active range beside the submitted slice (Active/HasActive, read throughNumRanges/RangeAt) instead of materializing a combined slice, so the resolver walks both in submission order and ack processing is no longer O(n) in the in-flight window.An A/B run against a real Delta table showed no throughput change for proto/JSON ingestion from the generalized ack model. That run predates the copy removal above, so it is a lower bound on the current state.
Test plan
cd purego && go test -race ./...— 543 tests pass, with and without-racecd purego && go test -count=3 ./internal/stream— no flakiness in the timing-sensitive testscd purego && go vet ./...andgofmt -lcleandeadcode ./...reports nothingdurability_model_test.goexercises the new generic seam through a protocol-neutral row-based fake: unit-resolution table cases, partial acks, replay of only the unacknowledged remainder, receipt-preserved prefix acks, and hook validation.Every behavior fix above has a regression guard that was confirmed to fail without its fix, by running the new test against the unmodified core in a scratch worktree:
TestHookProtocolImpossibleReceiptIsTerminal— both receipt shapes fail withrecovery exhausted after 4 attempt(s), showing the payload was replayed into the same defect.TestHookProtocolPromotedItemGetsFreshAckBudget— fails withopened 2 connections, want 1, showing the promoted item was torn down and replayed.TestHookProtocolPrefixAckPreservesTerminalRecvStatus— fails withrecovery exhausted after 4 attempt(s)instead of surfacing the server'sUnauthenticatedstatus, and asserts the credential is invalidated exactly once.core_test.gopasses against the pre-change core but failed against this branch; the teardown guard hangs to the test timeout without its fix.Notes
NEXT_CHANGELOG.mdentry: the change is confined topurego/internal/. Partial acknowledgments, submission receipts, and the record-count ack path are all unreachable for the shipping proto/JSON protocols, and the recovery fix restores behaviormainalready has, so there is no user-visible difference relative tomain.main. It was previously stacked onpurego-descriptor-cache-lifecycle, which was closed without merging.