Skip to content

fix(dsl): isolate partial recursion guards - #2489

Closed
lntutor wants to merge 1 commit into
567-labs:mainfrom
lntutor:fix/partial-context-isolation
Closed

fix(dsl): isolate partial recursion guards#2489
lntutor wants to merge 1 commit into
567-labs:mainfrom
lntutor:fix/partial-context-isolation

Conversation

@lntutor

@lntutor lntutor commented Jul 26, 2026

Copy link
Copy Markdown

What\n\nPrevents concurrent partial-model construction from sharing recursion-detection state.\n\nCloses #2461.\n\n## Changes\n\n- Replace the module-global recursive-model guard with context-local state.\n- Scope guard lifecycle to each top-level partial-model construction.\n- Add a deterministic two-thread regression test.\n- Document the fix in the changelog.\n\n## Testing\n\n- uv run pytest tests/coverage/test_dsl_partial_coverage.py\n- uv run ruff check instructor/v2/dsl/partial.py tests/coverage/test_dsl_partial_coverage.py\n- uv run ruff format --check instructor/v2/dsl/partial.py tests/coverage/test_dsl_partial_coverage.py\n- uv run ty check instructor/v2/dsl/partial.py

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is done correctly, including the part that's easy to get wrong.

You used default=None rather than a mutable default, and established the guard with .set(set()) plus reset(token). That matters more than it looks: ContextVar("...", default=set()) followed by .get().add(...) would have compiled, read naturally, and still leaked, because context copies are shallow so every scope that never called .set() mutates the same default object. Sidestepped cleanly here.

The placement is also right. Establishing the guard only when get() is None means the outermost call owns it and nested _process_generic_arg calls reuse the existing set, so the self-referential recursion guard still does its job. Creating a fresh set per call would have silently disabled cycle detection for TreeNode-style models, which is the failure mode I'd have looked for first.

Verified the isolation holds on both concurrency models, using the same guard shape as the patch:

threads : Home -> Address wrapped   |  Office -> Address wrapped
asyncio : Home -> Address wrapped   |  Office -> Address wrapped

And confirmed the test isn't vacuous by replaying the old global-set logic under the same interleaving:

thread 1 adds Address to the global set and blocks inside _make_partial_type
thread 2 arrives -> sees Address already present -> returns raw Address

which is exactly what makes "PartialAddress" in office_partial.model_json_schema()["$defs"] fail on main. Good choice of assertion, checking the $defs of the second model is what distinguishes "guard working" from "guard stomped", where a same-model test would pass either way.

Two optional notes, neither blocking:

The test covers ThreadPoolExecutor. The asyncio path is the one instructor's own streaming code actually hits (client.py does response_model = Partial[response_model] per call), and it isolates correctly, I checked. Worth a second test with asyncio.gather if you want the coverage to match the real call pattern, though the mechanism is identical so it's belt-and-braces.

assert processing_models is not None reads as a runtime guard but is a type-narrowing hint for the checker, and assert is stripped under python -O. That's harmless here since the preceding branch guarantees it's set, just noting it isn't load-bearing in case someone later reads it as one.

Nice turnaround on this one.

@jxnl

jxnl commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution.

@jxnl jxnl closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants