fix(core): drain effects registered during unload - #91
Open
Termina1 wants to merge 1 commit into
Open
Conversation
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.
While a fiber is unloading, a late async effect can still call
ctx.plugin()— and it succeeds. But_unloadhas already grabbed and cleared the disposable list, so the new child's disposer is never called. After the parent is fully disposed, the child is stillACTIVEin the registry and its listeners keep firing. Nothing will ever clean it up.This is easy to hit with HMR or config reconciliation: a plugin awaits something, its dependency goes away mid-await, the plugin wakes up during unload and registers a child.
Why it happens:
assertActiveonly checksuid, so it lets anUNLOADINGfiber register new effects;_unloadcallsthis._disposables.clear()once and awaits that snapshot — anything added during those awaits goes into the now-empty list, and nobody ever reads it again.The fix: keep draining. Instead of one pass over the snapshot,
_unloadloops until_disposablesis empty, so anything registered mid-unload gets torn down too. I chose to tear down late children rather than reject latectx.plugin()calls, so legitimate async effects that resolve mid-reload keep working — they just get disposed like everything else.The loop is capped at 16 rounds, so a disposer that keeps registering new effects forever can't hang unload: it gets a
ctx.logger.errorand the leftovers are dropped. There's a test for that case too.The new regression test fails on current
main(child.uidis2instead ofnull, the orphan's listener still fires after dispose) and passes with the fix. Full suite: 165/165.How I found this: I've been mechanizing the Cordis paper in Idris 2, mostly with AI agents doing the proof work under adversarial review: https://github.com/Termina1/dgamma. The paper's proof of Lemma 68 assumes every child registration is tied to a live step of the parent's activation, but the operational rules don't actually enforce that. So I had the agents check whether the real implementation has the same hole — and this race is it. The repo's README has the details and executable countermodels.
Side note: the same audit confirmed that name reuse is handled correctly here (callback identity + monotonic fiber uids), even though the paper's single raw-name renaming would conflate two components that share a freed name.