Skip to content

fix: ack for an already cancelled ticket future crashes append session#27

Merged
quettabit merged 1 commit intomainfrom
qb/iss-21
Apr 10, 2026
Merged

fix: ack for an already cancelled ticket future crashes append session#27
quettabit merged 1 commit intomainfrom
qb/iss-21

Conversation

@quettabit
Copy link
Copy Markdown
Member

No description provided.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 10, 2026

Greptile Summary

This one-line fix adds a done() guard in _resolve_next before calling set_result on a future, mirroring the same pattern already used in _fail_all. Without it, if a caller's task is cancelled while awaiting a BatchSubmitTicket, the underlying future transitions to the cancelled state; when the ack arrives from the server, set_result raises InvalidStateError, which propagates through _run and crashes the entire append session.

Confidence Score: 5/5

Safe to merge — the fix is minimal, correct, and consistent with the existing pattern in _fail_all.

The single-line guard follows the identical pattern already present in _fail_all. No new logic is introduced; the only change is preventing set_result from being called on a future that was cancelled by an awaiting task. All remaining findings are at most P2.

No files require special attention.

Important Files Changed

Filename Overview
src/s2_sdk/_append_session.py Adds a done() guard in _resolve_next to skip set_result on futures already cancelled by a caller, preventing InvalidStateError from crashing the session. Fix is consistent with the existing guard in _fail_all.

Sequence Diagram

sequenceDiagram
    participant Caller as Caller Task
    participant Session as AppendSession
    participant Queue as _queue / _unacked
    participant Server as Server

    Caller->>Session: submit(inp)
    Session->>Queue: append _UnackedBatch(ack_fut, bytes)
    Session-->>Caller: BatchSubmitTicket(ack_fut)

    Caller->>Caller: await ticket (awaits ack_fut)

    Note over Caller: Task cancelled externally
    Caller->>+Caller: asyncio cancels ack_fut
    Note over Caller: ack_fut.done() → True (cancelled)

    Server-->>Session: AppendAck arrives
    Session->>Session: _resolve_next(ack)
    Session->>Queue: popleft() → _UnackedBatch
    Session->>Session: _permits.release(bytes)
    alt ack_fut already done (cancelled)
        Session->>Session: skip set_result ✓
    else ack_fut still pending
        Session->>Session: set_result(ack)
    end
Loading

Reviews (1): Last reviewed commit: "initial commit" | Re-trigger Greptile

@quettabit quettabit merged commit 496acfe into main Apr 10, 2026
5 checks passed
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.

[Detail Bug] AppendSession becomes unusable after ticket timeout due to InvalidStateError on late ack

1 participant