Isolate the active-HighState stack per execution context (#63056)#69782
Open
ggiesen wants to merge 2 commits into
Open
Isolate the active-HighState stack per execution context (#63056)#69782ggiesen wants to merge 2 commits into
ggiesen wants to merge 2 commits into
Conversation
…3056) Concurrent state/orchestration renders shared a single class-level HighState.stack list. When the reactor renders orchestrations in parallel worker threads (it runs them inline, without forking), one render's push_active was visible to another, so HighState.get_active could return the wrong HighState in the middle of a render. This surfaced as an IndexError popping an empty pydsl render stack, or KeyError: '__env__' from a spuriously detected conflicting ID. Store the stack in a contextvars.ContextVar instead, mirroring salt.loader's loader_ctxvar, so each execution context (and therefore each reactor worker thread) gets its own stack. The pydsl top-file matches, previously cached in a module-level SLS_MATCHES global with the same sharing problem, are now cached per HighState instance. SSHHighState.push_active was a redundant override reaching for the removed class attribute, so it now inherits the context-backed base method. The conflicting-ID error formatter also uses .get() so a genuine conflict reports cleanly instead of raising KeyError.
…sh_active The saltstack#63056 refactor put the active-HighState stack accessors (push_active/pop_active/get_active/clear_active) on HighState. But SSHHighState subclasses BaseHighState, not HighState, and the salt-ssh state wrapper calls st_.push_active() on every run -- so every salt-ssh state execution raised "'SSHHighState' object has no attribute 'push_active'". Move the accessors to BaseHighState (their shared ancestor) so both HighState and SSHHighState resolve them and share the one context-isolated stack, restoring the original shared-stack semantics without an SSH-specific override. Add a regression test that exercises SSHHighState directly -- the original test only covered HighState, which is why the break reached CI.
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.
What does this PR do?
Isolates the stack of active
HighStateobjects per execution context soconcurrent state/orchestration renders don't corrupt one another.
The stack lived on the class (
HighState.stack = []), shared by every thread.The reactor renders orchestrations in a
ThreadPool(reactor_worker_threads),running the runner inline in worker threads rather than forking. When two
reactor events fire close together, their orchestrations render concurrently in
different worker threads and each
push_active/pop_activemutates the sameshared list.
HighState.get_active()then returns the wrongHighStatein themiddle of a render, which shows up as:
IndexError: list index out of rangepopping an empty pydsl render stack(
renderers/pydsl.pyreadsget_render_stack()[-1]off the wrong HighState), andKeyError: '__env__'when a cross-contaminatedbuilding_highstateproduces aspurious conflicting-ID and the error formatter indexes a chunk that lacks
__env__.The fix stores the stack in a
contextvars.ContextVar, mirroringsalt.loader'sloader_ctxvar(which the failing traceback already runsthrough), so each execution context -- and therefore each reactor worker thread
-- gets its own stack. Three follow-on pieces make the isolation complete:
SLS_MATCHESglobalwith the same cross-run sharing problem; they're now cached per
HighStateinstance.
SSHHighState.push_activewas a redundant override that reached for theremoved class attribute; it now inherits the context-backed base method (so
salt-ssh pydsl states keep working).
.get()for__env__/__sls__, so agenuine conflict reports cleanly instead of raising
KeyError.Single-threaded behavior is unchanged: within one context the lazy per-context
list behaves exactly like the old shared list, and the canonical
push_active()...try/finally: pop_active()pattern keeps the stack balanced.What issues does this PR fix or reference?
Fixes #63056
Previous Behavior
Concurrent reactor orchestrations (or any concurrent renders that touch the
active-HighState stack) intermittently failed with
Rendering SLS ... failed, render error: '__env__'followed byIndexError: list index out of range.Events that arrived too close together dropped their orchestration.
New Behavior
Each render gets its own active-HighState stack and its own pydsl top-file
match cache, so concurrent renders no longer corrupt one another.
Merge requirements satisfied?
A deterministic two-thread regression test asserts each thread sees only the
HighStateit pushed. It fails against the pre-fix code(
{'A': 'B', 'B': 'B'}) and passes with the fix.Commits signed with GPG?
No