Fix stale index after auto-pulling mlcommons@mlperf-automations - #285
Open
anandhu-eng wants to merge 9 commits into
Open
Fix stale index after auto-pulling mlcommons@mlperf-automations#285anandhu-eng wants to merge 9 commits into
anandhu-eng wants to merge 9 commits into
Conversation
call_script_module_function() wrote the refreshed repos/index to self.index after a successful auto-pull, but get_index() (used by search/find/rm) reads self._index — a different attribute, introduced by an unrelated lazy-indexing refactor (#205) that never updated this call site. A search immediately after an auto-pull in the same process would silently find nothing. Fixing the attribute name alone wasn't enough: ScriptAction.search()/ find()/rm() delegate to self.parent, not self, so the refresh must also propagate there. Verified: original repro via access()+search(), an isolated run()-direct variant, and two separate top-level access() calls (matching typical library usage) all now find the script post-pull. Existing test suite (13 tests) and plain CLI usage unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
🤖 AI PR Review SummaryThis change renames the attribute 'index' to '_index' and ensures that after an auto-pull operation, the refreshed repository and index state are also propagated to the parent action if it exists and is different from self. This prevents stale state issues when performing searches immediately after pulling. The design addresses state synchronization between related action objects but introduces a private attribute '_index' that should be consistently used throughout the class to avoid confusion or bugs. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Addresses PR review feedback: without this, the underscore is easy to misread as a typo rather than the intentional match to Action.get_index()'s lazy-build attribute. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
arjunsuresh
reviewed
Aug 4, 2026
|
|
||
| if result['return'] == 0: | ||
| self.repos = self.load_repos_and_meta() | ||
| # Must be self._index, not self.index: get_index() (used by |
Contributor
There was a problem hiding this comment.
Index update should be part of pull action right?
@arjunsuresh pointed out the index refresh belongs in the pull action, not duplicated in this one caller. Verified that was right: a direct `mlc pull repo` followed by an immediate search (not going through script_action.py's auto-pull path at all) had the exact same stale-index bug, unfixed by the previous commit. Moved the propagation into register_repo() (repo_action.py), which every pull already routes through, and simplified script_action.py back down to just refreshing self.repos (still needed there for the immediate find_target_folder() retry) — the index rebuild and self.parent propagation it had is now redundant with what register_repo() does at the source. Re-verified: the original auto-pull scenario, the general direct-pull scenario, and the full existing test suite (13 tests) all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
arjunsuresh
reviewed
Aug 4, 2026
| }) | ||
|
|
||
| if result['return'] == 0: | ||
| # Needed on self so the find_target_folder() retry just below |
Contributor
There was a problem hiding this comment.
These comments no longer needed?
anandhu-eng
marked this pull request as draft
August 5, 2026 06:44
Two independent problems that the previous stale-index fix exposed. 1. In-memory state was copied, not shared. get_action() builds a fresh delegate per dispatch and each subclass __init__ did self.__dict__.update(vars(parent)), so a delegate's repos/_index were a snapshot that diverged from the long-lived root the moment either side changed. register_repo() worked around it by copying repos/_index back onto self.parent by hand - which only fixed the one path someone remembered to patch. Action.repos is now a property and get_index() caches on the object returned by the new Action._state_owner(), which walks the parent chain to the root. Subclasses call _inherit_from_parent(), which copies startup config but deliberately not repos/_index/parent (Action._NOT_INHERITED). A delegate's write is now the root's write, so the manual copy-back is gone. Non-Action parents (test stubs) and parent=None keep working: the delegate simply owns its own state. CacheAction/ScriptAction still call self.parent.rm()/search() to reach the base implementation. That is not a state workaround and must not become super(): Action.rm() calls self.search(), so a CacheAction receiver would apply the expiry filter and skip exactly the expired caches `mlc rm cache` exists to delete. Commented at the rm site. 2. repos.json read-modify-write was unguarded. Registering and unregistering both read the whole list, change one entry and write it back, so two concurrent `mlc pull repo` runs could lose an entry - and an unlocked reader could catch the file mid-write. Reproduced on an installed mlcflow: of three concurrent registrars one died with JSONDecodeError and its entries vanished. Both writers now hold repos_json_lock() across the whole read-modify-write and write via utils.save_json_atomic() (temp file + os.replace), so readers see either the old or the new content. Action.__init__'s create-if-missing path takes the same lock with a double-check. The locked region stays narrow because two FileLocks on one file deadlock even within a process - register_repo() unregisters conflicts and pulls deps before entering it, and reloads repos after leaving it. Index's lock helper now delegates to utils.file_lock_with_incremental_ timeout so there is a single locking policy. Tests: test_action_state_sharing.py (5 of its 8 cases fail without part 1, including the original "registered repo invisible to the next delegate" bug) and test_repos_json_concurrency.py (3 concurrent processes, 36 registrations, no lost entries). Verified end to end on a fresh MLC_REPOS: auto-pull -> register -> find -> run in one process, run with deps, find/rm cache, re-pull, alias-conflict pull (unregister+register, no deadlock), rm repo. Both .github/scripts checks pass. Invariants documented in AGENTS.md and .claude/skill.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The wrapper's body was already a straight passthrough to utils.file_lock_with_incremental_timeout() after the previous commit moved the real implementation there; keeping it only added a layer of indirection between the 4 call sites and the actual lock logic, for no behavioral difference. Inlined the call at each site and dropped the now-unused contextmanager import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A few comments added in the state-ownership fix explained what the code used
to do before the fix ("without anything having to be copied back onto
self.parent by hand", "instead of each delegate mutating a private copy that
has to be pushed back by hand") rather than just why the current code is
this way. That framing has nothing to contrast against for a reader who
never saw the old code, and belongs in the PR description instead.
Tightened _state_owner()'s docstring, the _NOT_INHERITED comment, and
repos_json_lock()'s docstring and the register_repo() comment beside it to
state only the current design and its rationale.
Added a "Comment style" section to AGENTS.md so future contributions follow
the same rule: comment why code is this way, concisely; leave the "why it
used to be different" history to commit messages and PR descriptions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
anandhu-eng
marked this pull request as ready for review
August 5, 2026 08:02
🤖 AI PR Review SummaryThis PR refactors state ownership in Action subclasses to centralize mutable state (repos and index) on a single owner object per process, avoiding state divergence and stale data issues. It introduces _inherit_from_parent() for delegate initialization instead of copying all parent attributes, and updates CacheAction, CfgAction, and ExperimentAction to use this. It also adds detailed documentation on state ownership and locking requirements. The changes improve consistency and concurrency safety but require careful adherence to the new patterns to avoid subtle bugs. |
| @@ -217,9 +274,12 @@ def load_repos(self): | |||
| return None | |||
There was a problem hiding this comment.
Good use of a file lock when creating repos.json to avoid race conditions. Consider adding a brief comment explaining why the lock is needed here for future maintainers.
| @@ -102,6 +100,10 @@ def rm(self, i): | |||
| """ | |||
| i['target_name'] = "cache" | |||
| # logger.debug(f"Removing cache with input: {i}") | |||
There was a problem hiding this comment.
The comment explaining why rm() calls self.parent.rm() instead of super().rm() is very helpful. Consider expanding it slightly to clarify that this preserves the intended filtering behavior and prevents skipping expired caches.
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.
Summary
The original bug.
call_script_module_function()refreshedself.indexafter a successful auto-pull, butget_index()(used bysearch/find/rm) readsself._index— a different attribute since PR #205's lazy-indexing refactor renamed it there but missed this call site. A search immediately after an auto-pull in the same process silently found nothing.Fixing the attribute name alone wasn't sufficient:
ScriptAction.search()/find()/rm()delegate toself.parent, notself, and a directmlc pull repofollowed by a search (no auto-pull involved) had the identical bug. The propagation needed to live inregister_repo()(repo_action.py), the one place every pull already routes through — see the last commit's description for the full history of that back-and-forth.Two follow-on hardening changes, once the above exposed them:
Shared state instead of copy-back. Every
Actionsubclass (RepoAction,ScriptAction,CacheAction,ExperimentAction,CfgAction) is a throwaway delegate —get_action()builds a fresh one per dispatch — but was previously initialized viaself.__dict__.update(vars(parent)), a one-time copy ofrepos/_index. Any two delegates could silently diverge the moment either wrote to its copy, which is exactly what caused the bug above.Action.reposis now a property, andget_index()resolves through the newAction._state_owner(), which walks theparentchain to the single long-lived root (default_parent) reused across a process. A write from any delegate is now immediately visible to every other delegate — no more manual copy-back. Subclasses call the newself._inherit_from_parent(parent)instead of the old__dict__.update.repos.jsonread-modify-write wasn't safe under concurrentmlcprocesses.register_repo()/unregister_repo()both read the whole list, change one entry, and write it back with no lock — two concurrentmlc pull reporuns could lose one another's entry, or a reader could catch the file mid-write. Reproduced on an unpatched install: of 3 concurrent registrars, one died withJSONDecodeErrorand its entries vanished. Addedrepos_json_lock()(repo_action.py) held across the whole read-modify-write, andutils.save_json_atomic()(temp file +os.replace) so unlocked readers never see a partial file.Index's own file-lock helper was moved toutils.pyso there's one locking policy instead of two, and its now-redundant wrapper method inIndexwas removed (call sites now call theutilshelper directly).Also added a "Comment style" section to
AGENTS.md: comment why code is this way, concisely; leave "why it used to be different" to commit messages, not code comments that outlive the PR.Test plan
run(triggering auto-pull) followed bysearchin the same process returns an empty listmlc pull repofollowed by a search, no auto-pull involvedaccess()for run, thensearch())run()called directly (isolating the propagation logic fromaccess()'s extra dispatch layer)mlc.action.access()calls, matching typical library usagetests/test_action_state_sharing.py(new) — 8 cases pinning thatrepos/get_index()resolve through one shared owner; 5 of 8 fail against the pre-fix__dict__.updatecopy patterntests/test_repos_json_concurrency.py(new) — 3 concurrent processes registering 12 repos each; all 36 survive. Reproduced the pre-fix crash (JSONDecodeError, lost entries) with the same harness against an unpatched installMLC_REPOS: auto-pull → register → find → run in one process, run with deps, find/rm cache, re-pull, alias-conflict pull (unregister+register, confirmed no deadlock from the new lock), rm repo.github/scriptschecks (test_repo_pull_force.py,test_error_guidance.py) passautopep8 -a --diffclean on every file touched in this PRmlcr,mlc find script,mlc reindex) unaffected🤖 Generated with Claude Code