Skip to content

BUG: recover_sub_pool no longer raises on concurrent create_actor - #204

Merged
qinxuye merged 3 commits into
xorbitsai:mainfrom
AmirF194:fix/48-recover-sub-pool-dict-race
Aug 30, 2026
Merged

BUG: recover_sub_pool no longer raises on concurrent create_actor#204
qinxuye merged 3 commits into
xorbitsai:mainfrom
AmirF194:fix/48-recover-sub-pool-dict-race

Conversation

@AmirF194

Copy link
Copy Markdown
Contributor

What do these changes do?

MainActorPool.recover_sub_pool (indigen/pool.py) iterates
self._allocated_actors[address].values() directly and awaits
self.call(...) inside the loop for every actor being recreated.
create_actor (pool.py) writes into that same dict across an await
too: it sets a placeholder key before calling out, then replaces or
pops it once the call returns. If a create_actor call lands on the
same sub pool address while recover_sub_pool is mid-loop, the dict's
size changes between two iteration steps and Python raises
RuntimeError: dictionary changed size during iteration, aborting
recovery for every actor still left in the loop.

The fix snapshots .values() into a list before iterating, so later
mutations of the live dict don't affect the iteration in progress.
monitor_sub_pools already guards its own iteration over
self.sub_processes the same way a few lines down, so this brings
recover_sub_pool in line with that existing pattern rather than
introducing a new one.

Related issue number

Fixes #48

Check code requirements

  • tests added / passed
  • Ensure all linting tests pass

Verification

  • New test test_recover_sub_pool_concurrent_create_actor drives the
    real recover_sub_pool/create_actor methods on a MainActorPool
    instance with call/start_sub_pool/wait_sub_pools_ready stubbed,
    and controls the interleaving with asyncio.Events so a
    create_actor call is guaranteed to land while recovery is paused
    mid-iteration. Confirmed it fails on unmodified main with the
    exact RuntimeError from the issue, and passes on this branch, both
    in the same container.
  • Full xoscar/backends suite: 82 passed, 1 pre-existing failure
    (test_sub_pool_quit_with_main_pool, a zombie-reaping assertion that
    fails identically on unmodified main in this container due to no
    init process; unrelated to this change) plus 6 skipped, both before
    and after the fix.
  • black, flake8, isort and mypy (this repo's own pre-commit
    hooks) all clean on the two changed files.
  • Not run: the real ci.yml matrix across all five Python versions
    and three OSes; verified on Python 3.11 in a Debian container only.

@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@XprobeBot XprobeBot added the bug Something isn't working label Aug 28, 2026
@XprobeBot XprobeBot added this to the epic milestone Aug 28, 2026
@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.85%. Comparing base (d6465c9) to head (c8bf238).
⚠️ Report is 91 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #204       +/-   ##
===========================================
- Coverage   88.97%   75.85%   -13.13%     
===========================================
  Files          48       57        +9     
  Lines        4038     5326     +1288     
  Branches      770      581      -189     
===========================================
+ Hits         3593     4040      +447     
- Misses        358     1145      +787     
- Partials       87      141       +54     
Flag Coverage Δ
unittests 75.42% <ø> (-13.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

recover_sub_pool iterated self._allocated_actors[address].values()
directly while awaiting self.call() inside the loop. create_actor()
mutates that same dict across an await too (a placeholder key set
before its own call, popped after), so a create_actor() landing
during recovery raised RuntimeError: dictionary changed size during
iteration. Snapshot the values into a list before iterating, mirroring
the same guard already used for self.sub_processes in
monitor_sub_pools.

Fixes xorbitsai#48
@AmirF194
AmirF194 force-pushed the fix/48-recover-sub-pool-dict-race branch from 7e8707c to e406f91 Compare August 29, 2026 04:02
Comment thread python/xoscar/backends/indigen/pool.py Outdated
create_actor() inserts a placeholder under key None before awaiting
the sub pool create and only pops it once that call returns. If the
sub pool dies mid create, the placeholder is left in the dict holding
the caller's original, unfinished message. recover_sub_pool()'s replay
loop was iterating that entry too, resending an unfinished,
from_main=False CreateActorMessage straight to the sub pool and
duplicating the pending create.

Skip entries keyed by None when replaying, and add a test that pins
this behavior.

@qinxuye qinxuye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify

mergify Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@qinxuye
qinxuye merged commit 020f39e into xorbitsai:main Aug 30, 2026
17 of 18 checks passed
@AmirF194

Copy link
Copy Markdown
Contributor Author

Thanks for catching the placeholder replay case and for the quick review, appreciate it.

@AmirF194
AmirF194 deleted the fix/48-recover-sub-pool-dict-race branch August 30, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] recover_sub_pool may raises RuntimeError: dictionary changed size during iteration

3 participants