fix: prevent silent page loss in store_entity parallel upload#132
Open
LukasGold wants to merge 1 commit into
Open
fix: prevent silent page loss in store_entity parallel upload#132LukasGold wants to merge 1 commit into
LukasGold wants to merge 1 commit into
Conversation
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.
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
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.
Summary
store_entity(..., parallel=True)could silently drop one or more pages during abatch 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:
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.
Mutation during iteration.
_clear_cookies()looped over the live cookie jarwhile calling
.clear(...)inside the loop, so it could skip cookies or raisewhile mutating the collection it was iterating.
Silently swallowed failure. After exhausting its retries,
WtPage.edit()returned
Noneinstead of raising.store_entitytreated that non-exceptionresult 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_excinstead of returningNone. A page thatgenuinely cannot be written now surfaces as an error instead of vanishing.
Fix B1 - serialize destructive session mutations (wtsite.py)
A per-
WtSitereentrant lock (self._session_lock) now guards the three operationsthat destructively mutate the shared session:
_relogin()_clear_cookies()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 acquiresit again on the same thread; a non-reentrant lock would self-deadlock there.
_clear_cookies()also now iterates alist(...)snapshot of the jar, fixing themutation-during-iteration bug. A lazy
_get_session_lock()accessor keeps instancesconstructed via
WtSite.__new__(used in tests) working; those are single-threaded atconstruction, so on-demand creation is race-free.
Fix C - surface per-entity outcomes (util.py, core.py)
parallelize()gains areturn_exceptionsflag (mirroringasyncio.gather): afailing 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_entitynow collects per-entity outcomes for both the serial and parallelpaths, records failures in the new
StoreEntityResult.failed, and raisesOSW.StoreEntityPartialErrorcarrying the partial result (which pages were storedand 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 withouttaking 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 onsuccess, recovers after transient failures.
tests/test_wtsite_session_lock.py-_clear_cookiesremoves only the intendedrevision cookie and is safe under concurrency.
tests/test_store_entity_failure.py- partial-failure reporting viaStoreEntityResult.failed/OSW.StoreEntityPartialError.