Keep SortedSet membership and index consistent after failed add or update

[rupayon123]

SortedSet stores membership and its sorted index separately. A failed add() could leave a phantom member in _set. Review feedback identified a second path: a failed rebuild-style update() cleared _list after expanding _set, so ordinary bulk updates could leave membership and iteration permanently inconsistent.

This PR rolls back membership when add() fails. For the rebuild branch of update() (also used by |=), it now constructs and validates a candidate sorted list before mutating either live structure. Once validation succeeds, it installs the prepared internal list state in place, preserving the bound public bisect/index methods and the existing set object. Regression tests cover both failure paths, a retry after a temporary key error, and those bound methods after a successful bulk update.

Validation on the latest head: the new bulk-update cases failed against the previous PR head (7 members but an empty sorted list); PYTHONPATH=src uvx --from pytest pytest tests -q now passes 304 tests. Ruff checks on both changed files and git diff --check pass. The original add() change also passed its earlier broader checks. Remote CI and other runtimes remain to be verified. Values are still expected to keep stable hashes and ordering, as the class contract states.

Prepared with OpenAI Codex assistance; I reviewed the diff and regression behavior.

[feiiiiii5]

The add() fix is right, and I agree with the error handling choice here — propagating the original exception after rolling back is the opposite of swallowing it, and it leaves the object consistent. 369 passed on the branch, and I confirmed the protected path behaves as intended: a key failure during a small incremental update() leaves _set and _list equal.

I measured the part the description scopes out, because I think the exposure is larger than "bulk updates" suggests and one detail undercuts the retry test you added.

The excluded branch is the common one, not the edge case. update() picks the rebuild path when 4 * len(values) > len(_set):

existing  added   branch
    3       3     rebuild (no rollback)
    3       4     rebuild
   10       3     rebuild
   10      10     rebuild
  100      10     _add (protected by this PR)

Only a set much larger than the addition is protected. An ordinary update() of comparable size is not.

The state it leaves is worse than "not transactional". With a key that raises for the new values:

s = SortedSet([1, 2, 3], key=key)     # key fine at build time
s.update([10, 11, 12, 13])            # -> ValueError
len(s)        -> 7
list(s)       -> []
s._set        -> [1, 2, 3, 10, 11, 12, 13]
s._list       -> []

_list.clear() runs before the rebuild that raises, so the sorted index is left empty while the membership set is fully populated. The object reports seven members and iterates none.

add() cannot repair it, which is the part I would weigh. add opens with if value not in _set, and a phantom here is in _set, so re-adding is a silent no-op — measured: add(10) returns normally and changes neither _set nor _list. So the remedy your test_failed_add_key_can_be_retried establishes for add() does not extend to this path; once a bulk update has failed there is no public call that restores the index.

discard() makes it worse rather than better. On such a set, discard(10) raises ValueError: 10 not in list from _list.remove — but _set.remove(value) has already run, so the member is dropped from _set and the two stay inconsistent:

before: _set=[1,2,3,10,11,12,13]  _list=[]
discard(10) -> ValueError: 10 not in list
after:  _set=[1,2,3,11,12,13]      _list=[]      # one member lost, still inconsistent

Also worth noting __ior__ = update, so s |= [...] has exactly the same exposure.

None of this argues for widening the PR — the scope you stated is defensible. It is two things: whether an update that corrupts the index and cannot be repaired through the public API should be left as a known gap, and whether add()'s membership short-circuit is worth a note in the docstring, since it is what makes the state sticky. Happy to test a follow-up if you decide to cover the rebuild branch.

[rupayon123]

@feiiiiii5, thank you for testing the rebuild path. I reproduced your 3-plus-4 case: before the fix, update() and |= raised with seven members but an empty sorted index. I extended this PR on 1f0437f so the rebuild path validates a candidate sorted list before changing the live state; a temporary key failure now leaves the original three values intact and allows a later retry. The regression test also checks that previously bound bisect_left and isdisjoint methods still see the updated state. The focused test and all 304 repository tests pass locally, as do Ruff and diff checks. Exact-head Actions are awaiting maintainer approval.