Skip to content

fix: prevent silent page loss in store_entity parallel upload#132

Open
LukasGold wants to merge 1 commit into
mainfrom
fix-parallelization-silent-drop
Open

fix: prevent silent page loss in store_entity parallel upload#132
LukasGold wants to merge 1 commit into
mainfrom
fix-parallelization-silent-drop

Conversation

@LukasGold

Copy link
Copy Markdown
Contributor

Summary

store_entity(..., parallel=True) could silently drop one or more pages during a
batch upload while still reporting the batch as fully successful. Three linked
defects combined to cause this; this PR fixes all three and adds offline unit tests
for each.

What went wrong

Each entity in a parallel batch is uploaded on its own worker thread, but every
thread shares one mwclient session (a single cookie jar and CSRF-token cache). Three
problems interacted:

  1. Unserialized session resets. On a transient edit failure, the recovery path
    refreshes the CSRF token and, if that fails, runs a full re-login: clear cookies,
    clear tokens, log in again. That is a multi-step, non-atomic wipe-and-rebuild of
    shared auth state. When two threads hit a failure at the same time, their reset
    sequences interleave and leave the session in a corrupt intermediate state (for
    example, cookies cleared by one thread while another is mid-login). A subsequent
    request from a third thread then goes out with missing or invalid auth and its
    edit fails.

  2. Mutation during iteration. _clear_cookies() looped over the live cookie jar
    while calling .clear(...) inside the loop, so it could skip cookies or raise
    while mutating the collection it was iterating.

  3. Silently swallowed failure. After exhausting its retries, WtPage.edit()
    returned None instead of raising. store_entity treated that non-exception
    result as success and dropped the entity from the batch result, so a page that
    never got written was reported as stored. This is what turned the races above
    into silent data loss.

The fixes

Fix A - WtPage.edit() raises on exhaustion (wtsite.py)

The retry loop now records the last exception and, once all attempts are used up,
raises RuntimeError(...) from last_exc instead of returning None. A page that
genuinely cannot be written now surfaces as an error instead of vanishing.

Fix B1 - serialize destructive session mutations (wtsite.py)

A per-WtSite reentrant lock (self._session_lock) now guards the three operations
that destructively mutate the shared session:

  • _relogin()
  • _clear_cookies()
  • the token-refresh / re-login recovery block inside edit()

Because all three acquire the same lock, two threads can never be inside a
clear/re-login sequence simultaneously, so the wipe-and-rebuild is effectively atomic
with respect to any other thread that also wants to reset the session. This removes
the writer-writer race and the double-re-login corruption. The lock is reentrant
because edit()'s recovery block holds it and then calls _relogin(), which acquires
it again on the same thread; a non-reentrant lock would self-deadlock there.
_clear_cookies() also now iterates a list(...) snapshot of the jar, fixing the
mutation-during-iteration bug. A lazy _get_session_lock() accessor keeps instances
constructed via WtSite.__new__ (used in tests) working; those are single-threaded at
construction, so on-demand creation is race-free.

Fix C - surface per-entity outcomes (util.py, core.py)

  • parallelize() gains a return_exceptions flag (mirroring asyncio.gather): a
    failing task's exception is captured in place instead of aborting the whole batch
    and discarding the other results, and results stay aligned with input order.
  • store_entity now collects per-entity outcomes for both the serial and parallel
    paths, records failures in the new StoreEntityResult.failed, and raises
    OSW.StoreEntityPartialError carrying the partial result (which pages were stored
    and which failed). Callers learn exactly what happened without a separate existence
    query.

Caveat / scope of the lock

The lock serializes the destructive mutations against each other; it does not
wrap the normal request path. edit()'s actual API call reads cookies/tokens without
taking the lock. This is deliberate: locking every request would serialize all
uploads and defeat parallelism. Because clears and re-logins only happen on the rare
failure/timeout recovery path, guarding just those removes the damaging races while
keeping the common upload path fully parallel. In other words, this narrows the
"a normal request reads a half-cleared jar" window rather than eliminating it
absolutely.

Tests

Offline unit tests (no network) covering each fix:

  • tests/test_wtsite_edit.py - edit re-raises after max retries, returns result on
    success, recovers after transient failures.
  • tests/test_wtsite_session_lock.py - _clear_cookies removes only the intended
    revision cookie and is safe under concurrency.
  • tests/test_store_entity_failure.py - partial-failure reporting via
    StoreEntityResult.failed / OSW.StoreEntityPartialError.

Three linked defects caused store_entity() to drop a page during batch
uploads while reporting success.

Fix A (wtsite.py, WtPage.edit): the retry loop returned None after
exhausting all attempts, discarding the last exception. It now raises a
RuntimeError chained from the underlying error so callers see the failure.

Fix B1 (wtsite.py): parallel uploads share one mwclient session. Guard the
destructive session mutations (_clear_cookies, _relogin, and edit()'s
token-refresh recovery) with a per-WtSite RLock, and iterate a snapshot of
the cookie jar in _clear_cookies. A lazy _get_session_lock() accessor keeps
WtSite instances built via __new__ (tests) working.

Fix C (util.py, core.py): add return_exceptions to parallelize() (captures
per-task exceptions instead of aborting the batch on the first). store_entity
now collects per-entity outcomes for both serial and parallel paths, adds
StoreEntityResult.failed, and raises OSW.StoreEntityPartialError carrying the
partial result (stored + failed) on any failure.

Adds offline unit tests for all three fixes.
@LukasGold
LukasGold requested a review from simontaurus July 18, 2026 08:14
@LukasGold LukasGold self-assigned this Jul 18, 2026
@LukasGold LukasGold added bug Something isn't working enhancement New feature or request labels Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant