Skip to content

fix(validation): isolate cached validators across threads - #1442

Merged
bokelley merged 2 commits into
mainfrom
bokelley/adcp-client-python-issue-1434
Oct 7, 2026
Merged

bokelley merged 2 commits into
mainfrom
bokelley/adcp-client-python-issue-1434

Conversation

@bokelley

@bokelley bokelley commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Threaded callers could receive the same validator from either schema loader and corrupt its mutable reference-resolution scope stack, causing intermittent _RefResolutionError exceptions on valid payloads.

Cache schema inputs and reference stores, then create a fresh Draft7Validator and RefResolver for each lookup. Schema reads and reference discovery remain cached. Normalized bundles with fragment-only references use a root store; modular schemas use a prepared closure that preserves all reachable document aliases and ID precedence. Ambiguous scopes and missing targets retain the full registry and existing offline failure behavior. The resolver class is imported lazily once under a lock so concurrent lookups do not race on process warning filters.

Public signatures and validation semantics remain unchanged. Callers should obtain a validator per request or thread, as documented in both loader docstrings.

Validation:

  • Eight threads each perform 150 validations and repeated lookups for named, bundled task, and modular task schemas under warnings-as-errors.
  • Cached-lookups regression checks independent resolver state, no schema reads, and no warning-filter changes.
  • Source lint, mypy, strict adopter mypy, pyright, and type-ignore contracts pass.
  • Loader/version/concurrency regressions: 42 passed; 11 final fragment/closure regressions and two boolean-schema regressions passed.
  • Optimized schema/model parity and validation checks with coverage: 143 passed in 7m16s.
  • Full unit, serial conformance/integration, packaging, and legacy suites passed locally before the performance follow-up.
  • Warm construction benchmarks on three expensive modular tasks improved approximately 4–45x. CI timeouts and test selection are unchanged.
  • Independent Python/concurrency, protocol/schema, and code reviews completed; all findings resolved.

Fixes #1434.

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 7, 2026

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — thread-safety fix for #1434 with no blocking or medium findings.

What I checked:

  • schema_loader.py now caches _ValidatorSpec inputs rather than a shared Draft7Validator, so each get_validator/get_named_validator lookup mints a fresh validator + RefResolver. This correctly eliminates the concurrent-corruption of the mutable scope stack that caused #1434.
  • RefResolver's class is imported once under lock; jsonschema's deprecation warning is on attribute access, not instantiation, so per-call make() stays clean under -W error.
  • Compact-store branch is gated on _has_external_refs and only applied to flattened bundled schemas; internal callers use the returned validator immediately, so the singleton→fresh-instance change breaks nothing.
  • Public signatures unchanged; fix(validation): is the correct semver signal. No public-surface break, no generated-code hand-edits, no CI-gate tampering, no credential-in-metadata concern.
  • New tests added for per-version loading and thread safety covering the fixed branch.

No findings. high_risk false, gated_paths false, no author-team gate, no prior decision. Rows 1–8 do not fire; falls through to row 9.

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — clean subsequent pass.

This PR adds _reachable_registry_store (a reachable-subset optimization for the offline validator store) plus a fragment-$id guard in get_validator, with three accompanying test additions (equivalence, per-version, thread-safety). The function is conservative: any relative ref, nested $id, fragment root ID, non-http scheme, or missing target falls back to the full registry, and urlsplit().geturl() normalization plus insertion-order preservation mirror jsonschema's URIDict so full-vs-compact resolution is equivalent (asserted by the new equivalence tests).

No Critical/High/Medium findings in this run. The prior decision was also approve and the delta maintains that clean state. No high-risk paths, no gated paths, no author-team gate. review_decision is REVIEW_REQUIRED but gated_paths is false, so row 2 does not fire. Falls through to row 9 → approve.

@bokelley
bokelley merged commit b83a0c5 into main Oct 7, 2026
48 checks passed
@bokelley
bokelley deleted the bokelley/adcp-client-python-issue-1434 branch October 7, 2026 20:11
@bokelley bokelley mentioned this pull request Oct 7, 2026
12 of 13 tasks
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.

get_named_validator returns one validator shared across threads; concurrent validation fails with _RefResolutionError

1 participant