fix: runtime bugs and lifecycle gaps from code review - #62
Merged
Conversation
High severity: - SetArgs no longer leaks an orphaned Query (with a live background fetch loop) on every cache hit; the losing candidate is disposed like PrefetchAsync already did. - Cancel() no longer permanently bricks the query — the CancellationTokenSource is recreated under lock instead of being cancelled once and reused forever. - Value-type TData is now handled correctly: QueryState<TData> tracks presence via explicit HasData/HasLastData flags instead of null-checks, and QueryOptions.InitialData is now an Optional<TData> so default(TData) (e.g. 0) is distinguishable from "not set". Transition.razor uses the explicit flags. Medium severity: - RefetchInterval now routes through Invalidate(), so it respects the subscriber-count and stale-time gates instead of fetching forever with nobody listening. - Cache entries are now evicted automatically when the last State subscriber leaves (and the pending eviction is cancelled if a new one joins), closing the gap where entries only left the cache via manual Detach(). - QueryCache.Remove now holds its lock for the whole method and disposes any overwritten pending-removal subscription, closing a race with GetOrCreate and a double-Detach leak. Also guards against CacheTime values too large for the scheduler's underlying timer to represent. - QueryRefreshMonitor.js now exports unregister() to remove its listeners, and both DevTools/RefreshMonitor DisposeAsync methods catch JSDisconnectedException from normal circuit teardown. Minor cleanup: consistent locking around _lastSuccessAt, disposed-guards on Refetch/Invalidate, suppressed a spurious Idle emission from a superseded fetch, TryAdd in AddDotNetQuery, a cached JsonSerializerOptions instance, a descriptive error on query-key type collisions, an unforgeable QueryKey.Default sentinel, and doc notes on Success/Failure replay semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FINDINGS.md and REVIEW.md were scratch output from the fabel review; their content is now addressed and belongs in the PR description, not the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
+ Coverage 94.56% 94.98% +0.41%
==========================================
Files 21 21
Lines 773 877 +104
Branches 133 150 +17
==========================================
+ Hits 731 833 +102
- Misses 19 21 +2
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ng presence Simplifies the value-type TData fix by constraining TData to class on IQuery<TArgs,TData>, QueryOptions<TArgs,TData>, QueryState<TData>, and the internal Query/QueryObserver/QueryCache/QueryClient plumbing, plus Suspense and Transition. Reference types are the overwhelmingly common case for query results (protobuf messages, HTTP API DTOs), and constraining TData lets the library rely on null as a reliable "no data yet" sentinel again instead of threading explicit presence flags through every state transition. Removes the Optional<TData> wrapper and QueryState's HasData/HasLastData tracking added for the previous fix, reverting to plain nullable checks. Mutations are unaffected — MutationState already derived HasData from Status rather than a null-check, so they never had this bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Raises patch coverage by exercising code paths introduced in the review-fix commits that weren't yet hit by any test: Query.Invalidate/Cancel after Dispose, QueryCache.GetOrCreate's type-collision guard, Remove() called twice before its eviction timer fires, QueryKey.Default's marker ToString, and QueryRefreshMonitor.DisposeAsync's full body plus its JSDisconnectedException catch. Also fixes the shared bUnit JS module setup to call .SetVoidResult() — SetupVoid alone left the mocked Task pending, invisible until a test actually awaited disposal end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Addresses all findings from the fabel code review (
FINDINGS.md/REVIEW.md).High severity
SetArgsno longer leaks an orphanedQuery(with a live background fetch loop) on every cache hit — the losing candidate is now disposed, mirroringPrefetchAsync.Cancel()no longer permanently bricks the query — theCancellationTokenSourceis recreated under lock instead of being cancelled once and reused forever.TDatais now handled correctly:QueryState<TData>tracks presence via explicitHasData/HasLastDataflags instead of null-checks, andQueryOptions.InitialDatais now anOptional<TData>sodefault(TData)(e.g.0) is distinguishable from "not set".Transition.razoruses the explicit flags instead of??.Medium severity
RefetchIntervalnow routes throughInvalidate(), so it respects the subscriber-count and stale-time gates instead of fetching forever with nobody listening.Statesubscriber leaves (and the pending eviction is cancelled if a new one joins), closing the gap where entries only left the cache via manualDetach().QueryCache.Removenow holds its lock for the whole method and disposes any overwritten pending-removal subscription, closing a race withGetOrCreateand a double-Detachleak. Also guards againstCacheTimevalues too large for the scheduler's underlying timer to represent.QueryRefreshMonitor.jsnow exportsunregister()to remove its listeners, and both DevTools/RefreshMonitorDisposeAsyncmethods catchJSDisconnectedExceptionfrom normal circuit teardown.Minor cleanup: consistent locking around
_lastSuccessAt, disposed-guards onRefetch/Invalidate, suppressed a spuriousIdleemission from a superseded fetch,TryAddinAddDotNetQuery, a cachedJsonSerializerOptionsinstance, a descriptive error on query-key type collisions, an unforgeableQueryKey.Defaultsentinel, and doc notes onSuccess/Failurereplay semantics.