fix(cli/windows): exclude parent process chain from concurrent-instance detection (salvage of #31712)#31806
Merged
Merged
Conversation
…on Windows On Windows, the setuptools-generated hermes.exe launcher is a separate native process that spawns python.exe (the interpreter running the update code). os.getpid() returns the Python PID, but the launcher (which holds the file lock) is the parent. Without walking the parent chain, every 'hermes update' reports its own launcher as a concurrent instance - a false positive. This patch builds an exclusion set containing the Python process and its entire ancestor chain, so the running invocation never reports itself.
…ction Follow-up to @Strontvod's fix. Tests: - Five new tests in test_update_concurrent_quarantine.py cover the parent- chain exclusion: the .exe launcher is excluded, an unrelated sibling hermes.exe is still reported, multi-level ancestry is fully excluded, PID cycles in the parent chain don't hang, and a partially-stubbed psutil (no Process attribute) degrades gracefully instead of crashing. - New _fake_psutil_with_parent_chain helper builds a fuller stand-in (Process / NoSuchProcess / AccessDenied + process_iter) than the process_iter-only SimpleNamespace the older tests use. Hardening: - Broaden the except in the parent-walk to bare Exception. The original fix listed (NoSuchProcess, AccessDenied, ValueError), but those names are evaluated lazily during exception matching — if psutil is a partial stub without the attribute, the exception handler itself raises AttributeError that escapes. The function is documented as 'never raises' (the surrounding update flow depends on it), so the broader catch keeps the contract regardless of how the dependency is shaped. AUTHOR_MAP: - Map schepers.zander1@gmail.com -> Strontvod so the salvaged commit resolves to @Strontvod in the release notes. All 18 detect_concurrent + quarantine tests pass.
Contributor
🔎 Lint report:
|
2 tasks
Collaborator
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
3 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
hermes updateon Windows no longer aborts with a false "another hermes.exe is running" when the only "other" instance is the setuptools.exelauncher that spawned the running Python interpreter. Salvage of #31712 (@Strontvod) with a test + hardening follow-up on top.Root cause
The setuptools-generated
hermes.exeis a separate native process that spawnspython.exe(which actually runs the update code).os.getpid()returns the Python PID; the launcher (which holds the file lock that blocks the .exe rename) is the parent. The old code excluded only the Python PID, so the launcher was always flagged as a concurrent instance.Changes
hermes_cli/main.py: walkpsutil.Process.parent()upward and add every ancestor to the exclusion set. Includes a loop-detection break so a broken parent chain can't hang the walk. (Contributor commit, authorship preserved.)hermes_cli/main.py(follow-up): broaden the outerexcepton the parent-walk to bareException. The original(NoSuchProcess, AccessDenied, ValueError)tuple is evaluated lazily during exception matching — ifpsutilis a partial stub without those attributes, the handler itself raisesAttributeErrorthat escapes. The helper documents itself as "never raises"; the broader catch keeps the contract regardless of how the dependency is shaped.tests/hermes_cli/test_update_concurrent_quarantine.py(follow-up): 5 new tests covering the parent-walk — launcher excluded, unrelated sibling still reported, 3-deep ancestry, PID-cycle handled without hang, partial psutil stub degrades gracefully. New_fake_psutil_with_parent_chainhelper models the fuller psutil surface (Process/NoSuchProcess/AccessDenied+process_iter).scripts/release.py: AUTHOR_MAP entry mappingschepers.zander1@gmail.com→Strontvod.Validation
hermes updateon Windowshermes.exeoutside treetest_update_concurrent_quarantine.py*4 existing tests would have failed CI against the bare contributor commit because their
fake_psutil = SimpleNamespace(process_iter=...)stub lacksProcess/NoSuchProcess/AccessDenied, which the new parent-walk code now references. The bare-Exceptionhardening makes them pass without touching the tests themselves.Notes
[](unchanged), so no effect on Linux/macOS.Infographic