fix: close the silent-corruption paths found reviewing for 0.4.0 - #12
Open
paqstd-dev wants to merge 6 commits into
Open
fix: close the silent-corruption paths found reviewing for 0.4.0#12paqstd-dev wants to merge 6 commits into
paqstd-dev wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
with provider(...), enter and exitwith provider(..., sealed=True)with provider(lazy(...)), entered unreadwith provider(..., extend=True)wrap(fn)()use(), the hot readReads are untouched. The whole cost is two
ContextVaroperations 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
strnor atypeinto 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
3c833fcalone 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
ExitStackclosed out of order, anything that interleaves. Bothwithblocks close anduse("tenant")still answers, with the wrong tenant, in whatever context ran it. On a pooled worker that context is reused by the next job.ContextVar.resetreinstates 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_blocksnow 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.OrphanedProviderWarningcould raise out of__exit__and take the bookkeeping with it.warnings.warnraises whenever a filter sayserror, 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 throughexport()andadopt()._RESERVEDscannedvars(Namespace)and missed the data descriptors onobject, so a payload attribute named__class__arrived, showed up invars(), and was never returned bygetattr— 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 droppedextend, andprovider("bid", sealed=1200)additionally enabled a feature nobody asked for. Sincesealedships 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,@injectwas a complete no-op, and the first symptom wasAttributeError: '_FromCtxMarker' object has no attribute 'dsn'. Now refused at decoration, naming the annotation form. The check moved aboveget_type_hints, which raises on unresolved forward references and was letting such a function past the guard entirely.inspect.signaturedisagreed with the wrapper.functools.wrapsaims__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.runand exited outside, and a block entered on one thread and exited on another. It now states the rule first and the causes after.NoProviderErrorproposedprovider(Storage(...))for aProtocol, which cannot be written, and said nothing about exact keys when a subclass was active. It now names the exact-key rule andkey=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
lazycell held itscopy_context()snapshot forever, pinning every sibling provider value in scope for as long as anything held the cell — and withsealed=Truethe 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.rstsaid 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=Truewould not" (every view refuses them, anddataclasses.asdict/replace/is_dataclassare the ones people actually hit), a how-to that raisedNameErroras pasted, and three places claiming the tutorial covers the whole library when it covers neitherfrozen,sealed,lazy,ref,extend,debugnorannotate_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.pyhas 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 --verifytypesalso reports 100% type completeness, up from 85.7% onmain.Still outstanding and not in this branch: an ASGI middleware recipe, the
isolate()ordering rule for conftest fixtures, aUUID/datetimecodec example,# type: ignore[type-abstract]on the documenteduse(Protocol)pattern, and a note that a paused generator holds its block open.