replace leader election mechanism with file lock + remove final threads and convert to async tasks - #11
Merged
Merged
Conversation
otherwise they are unavailable on shutdown scope
asafc
force-pushed
the
elections_to_lock
branch
from
March 15, 2021 18:25
f70e82d to
a40f122
Compare
asafc
changed the title
orweis
approved these changes
Mar 15, 2021
orweis
left a comment
orweis
left a comment
Contributor
There was a problem hiding this comment.
Good job!
Few improvement suggestions
roekatz
pushed a commit
that referenced
this pull request
Jun 9, 2023
…aster Disable OPAL Broadcaster
Zivxx
added a commit
that referenced
this pull request
Jul 29, 2026
…phan-sweep safety/perf, zombie clamp, doc drift Five HIGH / five MEDIUM / two LOW findings, all reproduced before fixing. Lifecycle (HIGH #1, #2): - The watcher's leader purge subscription now uses its OWN subscriber id. PubSubEndpoint files every server-side subscription under one shared id and EventNotifier.unsubscribe deletes that id's whole callback list for a topic, so unsubscribing by topic in stop() also dropped the every-worker handle_purge_message registered once at boot in server.py — leaving the process deaf to fleet purges for life, with nothing to re-add it. - stop() now cancels its tasks BEFORE draining in-flight purges, and bounds the drain (_PURGE_DRAIN_TIMEOUT=5s). The old order awaited lock_source held by a sync across a whole clone/fetch — unbounded when SCOPES_GIT_FETCH_TIMEOUT is 0 — whose release required the very cancellation the drain was blocking, while still holding the leadership lock. LeaderScopePurger gains signal_stop() and its "awaiting here cannot hang" docstring claim is gone (it was false). - stop() is idempotent, and the comment claiming start()/stop() run exactly once is corrected: __aexit__ and stop_server_background_tasks both call it. Orphan sweep (HIGH #3, MEDIUM #7): - A SUCCESSFUL empty store read is no longer taken as "everything is an orphan". ScopeRepository.all() is a Redis SCAN loop that returns zero keys and no error against a wrong or empty keyspace, so a REDIS_URL on the wrong DB index, a failover to an empty replica or a stray FLUSHDB would rmtree every tenant's clone and broadcast confirmed purges fleet-wide. Refused (error-logged) unless the new OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE opts in; the git-leak bed sets it, since its FLUSHALL gate wants exactly that reclaim. - The under-lock re-check now takes ONE fresh scopes.all() for the whole candidate batch instead of one per candidate (a full SCAN + a parse per record + two sha256 per live scope, each time): 2903ms -> ~10ms at 2000 all-orphan scopes, O(scopes + dirs) instead of O(orphans x scopes) Redis round trips. Validation and docs (MEDIUM #10, #9, LOW #11, #12): - SCOPES_GIT_MAX_ZOMBIES is clamped with max(0, ...). Unclamped, a negative value is truthy and `count >= cap` holds with nothing in flight, so the first git op was refused and no scope ever synced — while the one error line said "remotes appear stuck". - Deleted the service.py prose still arguing that phase 2 must NOT inherit phase 1's cap, which the same commit reversed. NOTE: phase-2 concurrency did drop from a hard 32 to SCOPES_GIT_MAX_WORKERS (default 10) — the knob is now a true ceiling, but that default change was not advertised. - Rewrote _periodic_orphan_sweep's docstring, which still described the sweep as living in _periodic_polling. - Restored OPAL_SCOPES_GIT_FETCH_TIMEOUT's configuration.mdx text verbatim from config.py (drifted again after adade57 fixed it once). - Documented the pop-before-publish invariant at both sites: publish() runs local subscribers inline, so handle_purge_message re-enters lock_source; the pop is what makes it mint a fresh lock instead of deadlocking. Addresses review comments: - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
No description provided.