Repository navigation
fix(append): keep final S2Error reachable through max-attempts envelope - #415
Closed
detail-app[bot] wants to merge 1 commit into
Closed
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
|
PR author is not in the allowed authors list. |
infiniteregrets
added a commit
that referenced
this pull request
Oct 16, 2026
Evaluated the open Detail bug reports against latest `main`. Eight are real and fixed here; one (#345) is not a bug. ## Fixed | Issue | Fix | |---|---| | #416 | `ReadSession.Next` drains records the reader already buffered before surfacing a fatal error, so they are not skipped by a resume from `NextReadPosition`. Once `errorCh` yields, `Next` reads only `recordsCh` until it closes. | | #365 | The read retry loop resets the failure streak whenever an attempt delivered records, including reconnect-advised and server-draining handovers. | | #414 | The max-attempts error wraps the final attempt error with `%w`, so `HasNoSideEffects` and `withPriorUncertainty` can reach the `*S2Error`. The message text is unchanged. | | #403 | `APPEND_CONDITION_FAILED` (412) is classified as side-effect free (non-retryable), so after an uncertain attempt it is wrapped in `AppendIndefiniteFailureError`. | | #405 | `checkTimeouts` reads the head and claims `currentSession` together under `inflightMu`, so an ACK that pops the head can't be followed by a stale timeout tearing down the session. `handleSessionError` is split into claim and `handleClaimedSessionError`. Lock order is `inflightMu → sessionMu`; nothing takes them in reverse. | | #362 | `FrameReader` returns `io.ErrUnexpectedEOF` when the body ends mid-frame. Read sessions treat that as retryable instead of a clean end. | | #363 | When the last session on a poisoned pool entry closes its body, the entry's idle connections are closed. Streaming transports also get a 90s `IdleConnTimeout` as a backstop. | | #404 | When `ClientOptions.HTTPClient` uses an `*http.Transport`, streaming requests use its `TLSClientConfig` and `Proxy`. HTTP(S) proxies are tunnelled with CONNECT (with Basic auth); SOCKS5 goes through `x/net/proxy`. Clients without an `HTTPClient` behave exactly as before. | ## Also fixed: reconnect during failure handling While testing, `TestAppendSession_NoSideEffectsNoRetryWithNonIdempotentSentPipeline` turned out to be flaky on `main` (6/200 under `-race`), and the #405 refactor shifted timing so it failed more often (46/200). The cause is real: after a session failure is claimed, the pump could open a new session and resend inflight appends before the failure handler decided to fail them. Under `AppendRetryPolicyNoSideEffects`, that could resend a non-idempotent append. Claimed failures are now counted in `failuresPending`, and `getSession` won't reconnect while one is pending or the pump is cancelled (checked under `sessionMu`). The test now passes 0/900. ## Not a bug: #345 When batch A's checkpoint write fails and batch B's later succeeds, the checkpoint covering A and B is correct, because A's records were processed successfully. The suggested fix (set `Acked` only after `cache.Set` succeeds) would make things worse: combined with the `sync.Once` ack guard, A would never be retried and would block the checkpoint permanently. ## Relation to the Detail PRs This covers #406, #407, #408, #415 and #417. #408 is not a safe approach: it drops the ACK of a timed-out head, which reports a successful append as a timeout. ## Testing Following `AGENTS.md`, tests cover only behavior that spans components or does I/O; the one-line rules (#403, #365) are explained in comments instead. Each test below fails on `main`: - `TestReadSessionDeliversBufferedRecordsBeforeFatalError` (#416) - `TestAppendSession_MaxAttemptsKeepsFinalErrorReachable` (#414) - `TestFrameReaderReportsTruncatedFrameAsUnexpectedEOF` (#362) - `TestSpreadTransportClosesPoisonedEntryWhenLastSessionEnds` (#363) - `TestStreamingClientUsesUserTLSConfig`, `TestStreamingClientUsesUserProxy` (#404) The #405 race window can't be hit deterministically, so it has no test. `go test -race ./...` passes. Integration tests against S2 were not run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
Superseded by #423, which fixes this issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Closes #414
Bug
AppendSession's max-attempts-exhausted terminal error wrapped the final attempt's*S2Errorwith%vinstead of%wat two sites (handleSessionStartErrorandhandleSessionErrorins2/append_session.go). This dropped the*S2Errorfrom theerrors.Unwrapchain, so:HasNoSideEffectsreturnedfalseeven when every attempt was provably side-effect free (e.g.rate_limited), breaching its godoc contract ("no append took effect across every attempt of the operation") — a conservative misclassification that would suppress a safe auto-retry.withPriorUncertaintysilently no-op'd, so the terminal error was not wrapped in*AppendIndefiniteFailureErrorand the final*S2Errorwas unreachable for diagnostics.The bug was latent until commit 0e72870 ("preserve uncertainty across append retries") started consuming the envelope via
HasNoSideEffects/errors.As.Fix
Wrap the final attempt error with
%w(Go 1.23 supports multi-%w) at both max-attempts call sites:The rendered error string is unchanged; only the unwrap chain now includes the final
*S2Error, soerrors.As(&s2Err)andHasNoSideEffectsbehave per contract.Testing
TestAppendSession_MaxAttemptsExhaustedUncertaintyandTestAppendSession_StartMaxAttemptsExhaustedUncertaintycover both max-attempts sites, assertingerrors.As(&s2Err)reachability + pointer identity,errors.Is(ErrMaxAttemptsExhausted), theHasNoSideEffectscontract for both all-definite and indefinite-then-definite traces, the*AppendIndefiniteFailureErrorwrap, and that the rendered envelope string is unchanged. These fail before the fix and pass after; they fill the regression-test gap that let the bug ship (the existing uncertainty test bypassed thefmt.Errorfpath).go vet,gofmt,golangci-lint(v2.1.6), andgo build ./...all pass; the new and existing append-session/uncertainty tests pass under-race.s2-package lite integration suite (the CItest_cmdskip pattern) and theinternal/faulttestrace harness against a real S2 Lite server (s2 CLI v0.43.1,s2 lite); allTestAppendSession_*,TestReadSession_*, and fault-tests pass under-race.task test:integration) — it requiresS2_ACCESS_TOKEN/S2_BASIN_ENDPOINTsecrets not available here, so those tests skip with "S2_ACCESS_TOKEN not set". The lite-server runs above exercise the same append-session code paths end-to-end.Automatic Fixes PRs can be configured here.