Skip to content

fix: close the silent-corruption paths found reviewing for 0.4.0 - #12

Open
paqstd-dev wants to merge 6 commits into
mainfrom
fix/prerelease-0.4.0
Open

fix: close the silent-corruption paths found reviewing for 0.4.0#12
paqstd-dev wants to merge 6 commits into
mainfrom
fix/prerelease-0.4.0

Conversation

@paqstd-dev

Copy link
Copy Markdown
Owner

Four reviewers went over the library ahead of 0.4.0, one each for docs, correctness, API ergonomics and real-world use cases. This is what came back, verified and fixed. Every finding here was reproduced before it was acted on, and two of the reviewers' proposed fixes were rejected on inspection — see Findings not applied at the bottom.

One decision is still open

Correct out-of-order unwinding costs about twice as much to enter and leave a block.

operation before after
with provider(...), enter and exit 649 ns 1292 ns
the same with 8 providers already open 735 1395
with provider(..., sealed=True) 2147 2857
with provider(lazy(...)), entered unread 1600 2252
with provider(..., extend=True) 1788 2440
wrap(fn)() 543 644
use(), the hot read 60 61

Reads are untouched. The whole cost is two ContextVar operations per block, one on the way in and one on the way out.

Carrying the chain inside the registry instead was tried and measured at roughly 1.35x rather than 2x, but it puts a key that is neither a str nor a type into the mapping every consumer reads, which contradicts a recorded invariant and broke nine tests. That trade is available if the number matters more than the invariant.

Reverting 3c833fc alone drops back to detection without repair. The other five commits do not depend on it.

Silent corruption, now fixed

Two blocks closing in the wrong order left the first one's value behind for good. Two generators iterated together, an ExitStack closed out of order, anything that interleaves. Both with blocks close and use("tenant") still answers, with the wrong tenant, in whatever context ran it. On a pooled worker that context is reused by the next job.

def rows(slug):
    with provider("tenant", slug=slug):
        yield 1
        yield 2

a, b = rows("acme"), rows("globex")
list(zip(a, b))
list(a); list(b)
use("tenant")        # before: Namespace('tenant', slug='acme')

ContextVar.reset reinstates the snapshot taken at enter, which for a block that opened before one still open puts that block's value back. Neither the token nor the snapshots can say which blocks are still open, so _open_blocks now holds them and an exit that is not the newest rebuilds the mapping from the ones that remain. An enclosing block under the same key survives it, which the naive repairs did not — both were written and both were wrong on that case before the chain went in.

OrphanedProviderWarning could raise out of __exit__ and take the bookkeeping with it. warnings.warn raises whenever a filter says error, which is this repo's own pytest setting. The ledger entry leaked, the exception note was skipped, and a live exception was replaced by the warning. The unwind now finishes first and warns last, and a warning that would displace an exception on its way out is swallowed instead.

__class__ travelled through export() and adopt(). _RESERVED scanned vars(Namespace) and missed the data descriptors on object, so a payload attribute named __class__ arrived, showed up in vars(), and was never returned by getattr — the exact failure the check exists for. It now walks the MRO.

A flag carrying data ate a namespace attribute and turned itself on. provider("plan", extend="v1", tier="pro") silently dropped extend, and provider("bid", sealed=1200) additionally enabled a feature nobody asked for. Since sealed ships in this release, anyone with a field of that name would have got different behaviour on upgrade. Non-bool flags are now refused with the escape hatch in the message.

Injection

db: Db = from_ctx(Db) bound the marker object as the value. The default position is the shape FastAPI, typer and pydantic all use, both checkers accepted it, @inject was a complete no-op, and the first symptom was AttributeError: '_FromCtxMarker' object has no attribute 'dsn'. Now refused at decoration, naming the annotation form. The check moved above get_type_hints, which raises on unresolved forward references and was letting such a function past the guard entirely.

inspect.signature disagreed with the wrapper. functools.wraps aims __wrapped__ at the undecorated function, so an injected parameter read as required and any framework introspecting the handler tried to fill it. The wrapper now carries its own __signature__.

Messages

The orphan warning named an abandoned async generator as though it were the only cause. Three other shapes reach it, all verified — a sync generator collected elsewhere, a block entered inside Context.run and exited outside, and a block entered on one thread and exited on another. It now states the rule first and the causes after.

NoProviderError proposed provider(Storage(...)) for a Protocol, which cannot be written, and said nothing about exact keys when a subclass was active. It now names the exact-key rule and key= for the subclass case, and never offers a Protocol as a constructor.

A miss with debug mode off now names debug(), which is where the answer to a thread-boundary miss actually lives.

Lifetimes

A lazy cell held its copy_context() snapshot forever, pinning every sibling provider value in scope for as long as anything held the cell — and with sealed=True the escaping reference is exactly what retains it. The snapshot is dropped once the build settles, so a built value releases its scope by reference counting rather than waiting for the cycle collector.

Docs

Three pages promised an unwind the wrong-context exit cannot perform, including topics/providers.rst's "there is no cleanup to remember and no state to leak into the next request". topics/concurrency.rst said there was nothing asyncio-specific to know, which is no longer true.

Corrected besides: the claim that C-level type checks refuse a sealed value "where frozen=True would not" (every view refuses them, and dataclasses.asdict/replace/is_dataclass are the ones people actually hit), a how-to that raised NameError as pasted, and three places claiming the tutorial covers the whole library when it covers neither frozen, sealed, lazy, ref, extend, debug nor annotate_exceptions. Five two-sentence lines were split.

Findings not applied

Refusing FromCtx[T] without = injected. A reviewer proposed it for the same introspection problem. tests/test_inject_binding.py has seven tests pinning a required positional after a sentinel-defaulted one as deliberately supported, so refusing it would delete an argued decision. The __signature__ fix solves the runtime half without breaking it; the static half is inherent, since a checker reads the source.

A sentinel requiring __copy__/__deepcopy__/__reduce__. Refuted 0-3 under verification. Not a requirement.

Gate

999 tests, 100% branch coverage, mypy strict and pyright clean, Sphinx under -W, zizmor clean. pyright --verifytypes also reports 100% type completeness, up from 85.7% on main.

Still outstanding and not in this branch: an ASGI middleware recipe, the isolate() ordering rule for conftest fixtures, a UUID/datetime codec example, # type: ignore[type-abstract] on the documented use(Protocol) pattern, and a note that a paused generator holds its block open.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (78625f1) to head (edb9c79).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #12   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1417      1466   +49     
  Branches       187       200   +13     
=========================================
+ Hits          1417      1466   +49     

☔ 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.

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.

2 participants