Skip to content

fix: contain a raising match hook instead of failing every feature - #897

Open
TKaltofen wants to merge 5 commits into
mainfrom
fix/harden-subclass-enumeration
Open

fix: contain a raising match hook instead of failing every feature#897
TKaltofen wants to merge 5 commits into
mainfrom
fix/harden-subclass-enumeration

Conversation

@TKaltofen

@TKaltofen TKaltofen commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Closes #845.

Both halves of the issue still reproduced on main.

Part 2: one broken matcher poisoned every feature

A FeatureGroup whose match_feature_group_criteria raised took the whole filter loop down, so the exception became the resolution error of whatever feature was requested. Reproduced on main: with a throwaway group whose matcher raises RuntimeError("boom"), resolve_feature("some_unrelated_name") returned error='boom'.

The shared filter seam now contains such a raise as a non-match for that candidate alone, and records it as a match hook near-miss so the reason still reaches the user:

No feature groups found for feature name: 'x'.
Feature group(s) eliminated while matching 'x':
  - BrokenFG (match hook): raised RuntimeError: boom

Containing every raise would have been wrong on its own: it also swallowed framework-owned raises that are deliberately loud, turning a conflicting-reader misconfiguration into a silently wrong resolution. Those are marked with escalate_match_abort and cross the seam untouched, keeping their original type, message and traceback:

  • the build_effective_options escalation,
  • both already set with different values data-access conflicts,
  • the mixed-batch and conflicting pinned-file errors,
  • the forwarded-name-mismatch guard.

Raises that are a per-candidate judgment stay contained, and each such site now says so in place, so the rule does not have to be rediscovered. The build-phase fail-closed contract is untouched: a raising compute_framework_definition() still aborts the environment build.

A contained raise also rolls back the writes its matcher made to the shared Feature.options, per candidate, so a half-mutated option set cannot reach the group that wins instead.

Part 1: the test-isolation footgun

The autouse cleanup that reclaims a test's throwaway FeatureGroup subclasses had been copy-pasted into eleven modules, and every new module defining one had to rediscover it. One autouse fixture in tests/conftest.py, backed by tests/registry_isolation.py, now gives every module isolation by default. It collects only when a new subclass actually appeared, and pays a second collection only when the first left something behind, so suite wall time is unchanged.

A structural test asserts the fixture is defined exactly once. That test earned its keep immediately: main landed a twelfth copy while this branch was open, and it caught it.

The two direct-matcher sweeps in test_resolve_feature.py now skip a candidate whose matcher raises, closing the mechanism behind the flake in #868.

Verification

Merged with main after it landed the resolution-module split (#891), the reader option surface (#893) and the gc.freeze() conftest contract (#894). The matcher_error stage and its label moved to whichever module now owns them; the marked raise sites were re-applied against the reshaped base_input_data.py.

tox green on the merged branch: 7567 passed, 170 skipped, ruff format, ruff check, licenses, mypy --strict over 910 files, bandit. CI green on all 17 jobs across 3.10 to 3.14.

A FeatureGroup whose match_feature_group_criteria raised took the whole
filter loop down, so one broken or leaked subclass became the resolution
error of every unrelated feature. The seam now contains such a raise as a
non-match for that candidate and records it as a "match hook" near-miss,
so the reason still reaches the user without poisoning other features.

A framework-owned raise inside the hook must stay loud. Those are marked
with escalate_match_abort and cross the seam untouched, keeping the
build_effective_options escalation and the conflicting-reader
BaseInputData error exactly as they were.

Refs #845
The autouse cleanup that reclaims a test's throwaway FeatureGroup
subclasses had been copy-pasted into eleven modules, and every new module
defining one had to rediscover it. One autouse fixture in tests/conftest.py,
backed by tests/registry_isolation.py, now gives every module isolation by
default. It collects only when the module actually added a subclass, and
pays a second collection only when the first left something behind, so the
suite cost stays flat.

The two direct-matcher sweeps in test_resolve_feature.py now skip a
candidate whose matcher raises, so a single broken subclass cannot turn an
unrelated test into an error.

Refs #845
@TKaltofen
TKaltofen requested a review from TomKaltofen as a code owner July 30, 2026 12:29
…seam

Review of the containment change found the escalation sweep incomplete and
the marker itself unsound. The forwarded-name-mismatch guard and both
"already set with different values" data-access conflicts were being
contained, so a rival candidate could win while the contradiction was
silently dropped. Those, plus the pinned-file conflicts, are now marked;
the per-candidate judgments that must stay contained say so in place.

The marker is read from the exception's __dict__ instead of getattr: a
raising __getattr__ blew up inside the seam's own except block, letting a
new exception escape uncontained, and a permissively truthy one faked
framework provenance. Marking is best-effort and preserves the exception
type.

A contained raise now rolls back the writes its matcher made to the shared
Feature.options, per candidate, so a half-mutated option set cannot reach
the group that wins instead.

The shared test fixture gates its collection on any new subclass rather
than only the running module's, so a class built by a helper module is
reclaimed too; the report stays scoped to the module under test.

Refs #845
@TKaltofen
TKaltofen force-pushed the fix/harden-subclass-enumeration branch from 0b2ac4c to d725ffe Compare July 30, 2026 16:28
The containment policy was re-argued at every marked site and the seam
docstring had grown to five paragraphs. State the policy once, at the seam,
and leave each site a one-line tag. Keeps what a reader cannot recover from
the code and drops the supporting argument around it.

Prose only, no behavior change.
…, reader options and gc.freeze

Merge origin/main into fix/harden-subclass-enumeration.

- The "matcher_error" EliminationStage moves to resolution_types and its "match hook"
  label to resolution_failure_renderer (#891 split); the seam itself stays in
  identify_feature_group and the re-export surface is unchanged.
- contained_raise_log_level stays the one public name in components/utils; nothing
  imports the retired private one and no alias is left behind.
- The escalate_match_abort marks and the contained-raise tags are re-applied on the
  reshaped BaseInputData (#893 reader option surface).
- tests/test_gc_freeze_contract.py drops its own copy of
  _no_feature_group_registry_pollution and inherits the shared conftest fixture, which
  still tears down after restore_default_plugin_registry.
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.

Harden get_all_subclasses(FeatureGroup) enumeration against leaked and raising subclasses

1 participant