test(unload): pin the ownership contracts that already hold - #504
Merged
Conversation
#113 proposes representing function, widget and bindkey ownership per load instance rather than by plug-in id. Measuring the current behaviour first narrows that considerably. single load then unload works two-plug-in widget owner chain works two-plug-in bindkey owner chain works unloading in a different order than loaded works repeated load of the same plug-in BROKEN for all three So the multi-plug-in chain machinery the issue calls into question is already correct. Only a repeated load of the same plug-in leaks, and it leaks the function, the widget and the key binding together. Pin the four working contracts so the eventual per-load-identity change cannot quietly break them; the issue asks for exactly that compatibility. The repeated-load case is deliberately absent, since a test for known-broken behaviour belongs with its fix. An earlier run of these probes in one shell reported out-of-order unload as broken. That was contamination: the preceding repeated-load case had already leaked the function, so the next load saw it as pre-existing. Each case now runs in its own shell, which is why the harness starts a fresh zsh per contract. Refs #113
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.
Measures the current unload ownership behaviour and pins the parts that already work. Test-only; no behaviour change.
What the measurement found
#113 proposes representing function, widget and bindkey ownership per load instance rather than by plug-in id. Probing each contract in an isolated shell narrows that considerably:
So the multi-plug-in chain machinery the issue calls into question is already correct: unloading the second of two plug-ins restores the first as live owner, and unloading out of order removes only the plug-in named. The defect is confined to a repeated load of the same plug-in, where all three resource kinds leak together.
That should narrow the design work the issue asks for. Per-load identity is needed to tell two loads of one plug-in apart; the chain representation across different plug-ins does not appear to need changing.
What this PR adds
tests/unload-ownership-contracts.zshpins the four working contracts, which is what #113's own acceptance criteria ask to preserve ("keep existing single-load behavior ... compatible", "preserve correct behavior when the same plugin is loaded more than once").The repeated-load case is deliberately absent. A test for known-broken behaviour belongs with its fix, or CI goes red for a defect nobody is fixing yet. When #113 is implemented, that case has a home here.
A correction worth recording
My first pass ran all the probes in one shell and reported out-of-order unload as broken. That was contamination: the preceding repeated-load case had already leaked the function, so the next load saw it as pre-existing and the unload legitimately left it alone. Re-run in isolation, out-of-order unload is fine. The harness now starts a fresh zsh per contract for that reason.
Full suite 23/23. Registered in
zsh-n.yml, both path filters and a job.Refs #113