Guard NextActingInFixedOrder's turn state with its own lock - #286
Open
jaisinha77777 wants to merge 1 commit into
Open
Guard NextActingInFixedOrder's turn state with its own lock#286jaisinha77777 wants to merge 1 commit into
jaisinha77777 wants to merge 1 commit into
Conversation
NextActingInFixedOrder.pre_act() reads len(self._sequence) and then indexes into self._sequence, and mutates self._currently_active_player_idx, all without taking self._lock -- even though remove_actor_from_sequence(), add_actor_to_sequence(), get_state(), and set_state() on the same class already do. So did get_currently_active_player(), which indexes self._sequence using self._currently_active_player_idx with no lock either. remove_actor_from_sequence() mutates the same self._sequence list that pre_act() reads the length of and indexes into, so if they ever run on different threads at the same time, pre_act() can compute an index against the old length and then index into a list that's since been shortened. Whether that's reachable today: within a single EntityAgent.act() call, phases are sequenced and only one component's action_spec.output_type condition is normally true at a time, and I could not force an IndexError out of this pattern even under heavy synthetic thread contention. So, similar to the CallLimitLanguageModel fix, I'm not claiming an observed failure -- just that this class was the one inconsistent case among its own methods, and among sibling classes in this codebase, that didn't hold its own lock for state it otherwise protects. Adds next_acting_test.py, which had no coverage at all: sequence cycling and wraparound, ignoring non-NEXT_ACTING action specs, add/remove actor (including the ValueError case), get_state/set_state round-tripping, and a concurrency test exercising pre_act alongside remove_actor_from_sequence/add_actor_to_sequence from multiple threads.
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
While auditing this codebase's lock usage after the `Measurements.close()` deadlock fix (#282) and the `CallLimitLanguageModel` counter fix (#285), I found `NextActingInFixedOrder` (in `concordia/components/game_master/next_acting.py`) is inconsistent with its own locking discipline:
Concretely: `pre_act()` reads `len(self._sequence)` and then indexes into `self._sequence`, while `remove_actor_from_sequence()` (which does take the lock) mutates that same list. If these ever run on different threads at the same time, `pre_act()` can compute an index against the old length and then index into a list that's since been shortened.
Being precise about confidence here, same as in #285: within a single `EntityAgent.act()` call, phases are sequenced and (from what I could trace) only one component's `action_spec.output_type` condition is normally true at a time, so I could not identify a call path where this is reachable today, and I could not force an `IndexError` out of it even under heavy synthetic thread contention (many threads racing `pre_act`/`get_currently_active_player` against `remove_actor_from_sequence`/`add_actor_to_sequence`). So I'm not claiming this is an observed live bug. What I can say confidently: this class was the one inconsistent case, both among its own methods and relative to every sibling class using this same locking pattern elsewhere in the codebase (`Measurements`, `ObservationQueue` in this same file, etc.), that left this particular state unprotected.
Fix
Takes `self._lock` in `pre_act()` (only around the read-index-and-mutate block, not the whole method) and in `get_currently_active_player()`, matching the rest of the class.
Test plan
New `concordia/components/game_master/next_acting_test.py` (previously no coverage at all):
`python -m pytest concordia/components/game_master/ -q` → 177 passed (full directory, no regressions).
`python -m pyink --check` on the new test file → clean. (`next_acting.py` itself has substantial pre-existing formatting drift unrelated to this change in classes I didn't touch; I left that alone to keep this diff focused.)