Fix: drop a dead guard and state two facts release_buffer left implicit - #1864
Conversation
`_abandoned_run_handles`' scan guarded `handle._resources` against `None` three lines below a loop that dereferences the same field directly. `RunHandle.__init__` substitutes a fresh `_RunResources()` when its argument is `None` and nothing else ever assigns the field, so the guard could not fire — while the asymmetry told a reader one of the two paths could. Both loops now dereference it the same way. Two facts the surrounding docstrings asserted only one side of: - `release_buffer` states that a failed close never tells a descendant to drop a mapping the owner still considers live. The converse is what actually needs saying: `Buffer.close()` unlinks from its `finally`, so a raising `shm.close()` still removes the name, and the backing can be nameless while descendants keep mappings this call never told them to drop. Recorded together with what makes that window diagnosable — the named `FileNotFoundError` from `ImportRegistry.materialize`. - `_record_touched_identities` runs before `_admit_task_submission`, which can raise, so the touched set can name a task that never went out. The group entry points already document the mirrored case (a rejected member dispatches nothing, so nothing is recorded) while this one, the reason the ordering is chosen at all, was unstated. Recording after admission would invert the direction of the error into the unsafe one. `release_buffer`'s docstring is also split into paragraphs and its one mid-sentence line break repaired; no wording in the pre-existing text changed.
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Review leftovers from #1850, all in
worker.py. No behavior change.A dead guard, and the asymmetry it created.
_abandoned_run_handles' scan guardedhandle._resourcesagainstNonethree lines below a loop that dereferences the same field directly:RunHandle.__init__substitutes a fresh_RunResources()when its argument isNone, and nothing else ever assigns the field, so the guard could not fire. The cost was not the branch but the reading: two spellings of one dereference, three lines apart, tell a reader that one of the paths can beNone. Both now dereference the same way; pyright is clean on the file, which is the check that would have caught it if the type really were optional.Two facts the docstrings asserted only one side of.
release_bufferstates that a failed close never tells a descendant to drop a mapping the owner still considers live. True, and the converse is the one that needs saying:Buffer.close()unlinks from itsfinally, so anshm.close()that raises has still removed the name — the backing can be nameless while descendants keep mappings this call never told them to drop. That window is pre-existing and deliberate (the named backing outlives the process, so it is the leak worth removing even when the local unmap fails), but a docstring that names only the safe direction reads as if the unsafe one does not exist. Recorded together with what makes it diagnosable: the namedFileNotFoundError#1850 added toImportRegistry.materializefires in exactly this case._record_touched_identitiesruns before_admit_task_submission, which can itself raise on a sticky ordered-cleanup failure — so the touched set can name a task that never went out, costing a spuriousrelease_bufferrefusal thatclose()still recovers. The group entry points already carry a comment for the mirrored case (a rejected member dispatches nothing, so nothing is recorded); this case, which is why the ordering is chosen at all, was unstated. Recording after admission would invert the error into the unsafe direction: a dispatched task whose identity is unrecorded, and a release that unlinks under it.Formatting.
release_buffer's docstring is split into paragraphs and its one mid-sentence line break repaired ((a / separate run-id namespace with / no callback…). No wording in the pre-existing text changed —ruff formatdoes not reflow docstrings, so this one needed doing by hand.Not included:
ImportRegistry.close()'sexcept BaseException(onlyerrors[0]is re-raised, so a laterKeyboardInterruptis dropped). It is a verbatim copy of_release_all_buffers' established shape, so changing one without the other would be worse than leaving both; it belongs in its own change if it is worth making.Testing
pytest tests/ut/py— 1508 passed, 13 skipped (rebuilt against this commit)test_release_buffer.py::TestReleaseBuffer::test_rejects_while_an_abandoned_run_still_names_the_buffercovers the edited lineruff check/ruff format --checkclean;pyright python/simpler/worker.py— 0 errorspre-commitis not installed on this box, so the hook set ran only as its individual tools (ruff, pyright). A docstring-and-dead-branch change reaches no device path, so CI's onboard jobs are the coverage that matters here.