Skip to content

feat(sdk): act on s2s reconnect advice in append and read sessions - #352

Open
infiniteregrets wants to merge 6 commits into
mainfrom
m/sdk-reconnect-advice
Open

feat(sdk): act on s2s reconnect advice in append and read sessions#352
infiniteregrets wants to merge 6 commits into
mainfrom
m/sdk-reconnect-advice

Conversation

@infiniteregrets

Copy link
Copy Markdown
Member

A frontend that is shutting down sets the reconnect-advised flag on regular s2s frames while it is still healthy enough to serve. Sessions currently ignore it and ride the pod until it goes away.

This makes append and read sessions leave on their own terms:

  • Parse the flag (bit 4, regular frames only) in the frame parser.
  • Finish cleanly before moving. Appends end the request body so the server acknowledges everything it accepted and closes the response; reads deliver the batch the flag rode in on before reconnecting, so the resume position already covers it.
  • Drop idle pooled connections before reconnecting. Without this the new session reuses the same connection and lands back on the same draining server.
  • Treat advice as a planned handover, not a failure: it does not consume the retry budget.
  • Pace repeats. Three advised reconnects run immediately, then 100ms each; a gap longer than 10s starts a fresh streak. A draining server keeps acknowledging work, so progress cannot tell a storm from an ordinary handover, but how quickly advice returns can.

If the response ends cleanly with appends still unacknowledged, that is a truncated response rather than a handover, so it goes through the normal error path and the retry policy decides.

@infiniteregrets
infiniteregrets requested a review from a team as a code owner August 21, 2026 02:56
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds S2S reconnect-advice parsing and planned handover behavior for append and read sessions, including connection-pool rotation and pacing for repeated advice.

  • Parses the regular-frame reconnect-advised flag.
  • Delivers advised read batches before resuming from the updated position.
  • Half-closes advised append requests and reconnects after acknowledgments drain.
  • Adds shared reconnect-streak pacing and focused handover tests.

Confidence Score: 4/5

The append handover race should be fixed before merging because a clean advised shutdown can bypass reconnection and later consume the normal failure budget.

Both append-session channels become closed together, and the ACK-reader select can return through a branch that never invokes the newly required reconnect-advice handler.

Files Needing Attention: s2/append_session.go

Important Files Changed

Filename Overview
internal/framing/framing.go Adds parsing and test-frame construction for the regular-frame reconnect-advised flag.
s2/append.go Tracks reconnect advice and half-closes the append request while allowing outstanding acknowledgments to drain.
s2/append_session.go Implements planned append handover, but one closed-channel select path can skip the handover entirely.
s2/read.go Delivers advised batches before reconnecting from the advanced read position without spending retry attempts.
s2/client.go Forwards idle-connection closure through the scheme-aware streaming transport.
s2/reconnect.go Adds bounded pacing after three rapid advised reconnects.
s2/reconnect_test.go Covers read resumption, append handover, and advice pacing, but does not deterministically exercise selection of the closed acks channel.
s2/stream.go Adds streaming transport rotation by closing idle pooled connections.

Sequence Diagram

sequenceDiagram
    participant Server
    participant Transport as transportAppendSession
    participant Reader as AppendSession.readAcks
    participant Pump as AppendSession pump
    Server-->>Transport: ACK with reconnect-advised flag
    Transport-->>Reader: advised ACK
    Reader->>Transport: halfClose request body
    Server-->>Transport: clean response end
    Transport-->>Reader: close errorsCh and acksCh
    alt errorsCh selected
        Reader->>Pump: handleReconnectAdvice
        Pump->>Pump: rotate transport and reconnect
    else acksCh selected
        Reader-->>Reader: return without handover
        Note over Reader,Pump: closed transport remains current
    end
Loading

Comments Outside Diff (1)

  1. s2/append_session.go, line 445-448 (link)

    P1 Closed ACK path skips handover

    When an advised append response closes both channels and this select chooses the closed acksCh, the reader returns without calling handleReconnectAdvice, leaving the closed transport current so the next append enters the ordinary error path and consumes retry budget.

    Knowledge Base Used: Streams and Append

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: s2/append_session.go
    Line: 445-448
    
    Comment:
    **Closed ACK path skips handover**
    
    When an advised append response closes both channels and this select chooses the closed `acksCh`, the reader returns without calling `handleReconnectAdvice`, leaving the closed transport current so the next append enters the ordinary error path and consumes retry budget.
    
    
    
    **Knowledge Base Used:** [Streams and Append](https://app.greptile.com/s2-dev/-/custom-context/knowledge-base/s2-streamstore/s2-sdk-go/-/docs/streams-and-append.md)
    
    ---
    
    For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Prompt To Fix All With AI
### Issue 1
s2/append_session.go:445-448
**Closed ACK path skips handover**

When an advised append response closes both channels and this select chooses the closed `acksCh`, the reader returns without calling `handleReconnectAdvice`, leaving the closed transport current so the next append enters the ordinary error path and consumes retry budget.

```suggestion
		case ack, ok := <-session.acksCh:
			if !ok {
				if session.ReconnectAdvised() {
					r.handleReconnectAdvice(session)
				}
				return
			}
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat(sdk): act on s2s reconnect advice i..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant