fix(RcMap): invalidate should close scope regardless of refCount - #1799
Closed
kitlangton wants to merge 2 commits into
Closed
fix(RcMap): invalidate should close scope regardless of refCount#1799kitlangton wants to merge 2 commits into
kitlangton wants to merge 2 commits into
Conversation
RcMap.invalidate bails when refCount > 0, leaving the entry scope open and finalizers unrun. This is inconsistent with ScopedCache.invalidate which always closes the scope. Remove the refCount > 0 early return so invalidate always closes the scope and runs finalizers, matching ScopedCache semantics. Add test: invalidate with active consumer closes scope and runs finalizer.
🦋 Changeset detectedLatest commit: 2eddcbf The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
kitlangton
marked this pull request as ready for review
March 19, 2026 14:57
kitlangton
added a commit
to anomalyco/opencode
that referenced
this pull request
Mar 19, 2026
…ffect tests Legacy subscribeAll delivers InstanceDisposed via GlobalBus because the fiber starts asynchronously and may not be running when disposal happens. This bridge can be removed once upstream PubSub.shutdown properly wakes suspended subscribers. Add forceInvalidate in Instances that closes the RcMap entry scope regardless of refCount. Standard RcMap.invalidate bails when refCount > 0 — an upstream issue (Effect-TS/effect-smol#1799). Add PubSub shutdown finalizer to Bus layer so layer teardown properly cleans up PubSubs. Add Effect-native tests proving forkScoped + scope closure works correctly: ensuring fires when the scope closes, streams receive published events. Remove stale GlobalBus disposal test (instance.ts responsibility).
4 tasks
Collaborator
|
This is the intended behaviour. RcMap should only closed the resource when the ref count reaches 0. You can use ScopedCache if you don't need ref counting. |
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.
Howdy all! I ran into some unexpected semantics when using
LayerMapin the opencode repo. As it says at the bottom, I'm not sure if this is intended or not, but here's a reproducer and potential fix if it isn't indeed unintended.Human Approved Robot Summary 🤖🤝👵
Problem
RcMap.invalidatedoes not close the scope or interrupt consumers whenrefCount > 0. It removes the entry from the map and silently returns:This is inconsistent with
ScopedCache.invalidatewhich always closes the scope:Every other Effect lifecycle abstraction (FiberMap, FiberSet, ScopedCache) interrupts and tears down on invalidation/close. RcMap is the only one that silently abandons active consumers without interrupting them.
Impact
This makes
LayerMap(which wrapsRcMap) unable to cleanly tear down services with long-running consumers. When a layer has a PubSub that streams subscribe to,invalidatecan't trigger the layer's finalizers to shut down the PubSub, so streams never end andensuringcallbacks never fire.Fix
Remove the
if (entry.refCount > 0) returnline.invalidatenow always closes the scope and runs finalizers, matchingScopedCachesemantics.Test
Added: "invalidate with active consumer closes scope" — acquires a resource, holds the scope open, invalidates, and asserts the finalizer runs. This test fails before the fix and passes after.
All 8 existing RcMap tests continue to pass (they only test invalidate with refCount=0).
Question for maintainers
Is the silent abandon behavior (bail on refCount > 0) intentional? If so, should there be a separate
forceInvalidateAPI? The current behavior means "invalidate" doesn't actually invalidate when consumers are active — it just removes the entry from the map and hopes consumers eventually release on their own.