fix(unload): keep ownership across repeated loads of one plugin - #582
Conversation
…113) Work in progress, not for review: the interleaved unload order (other plugin unloaded first) regresses against next; see the issue.
Keep the records of earlier loads only when no other plugin took one of the plugin's widgets or bindings over in between; otherwise keep the behaviour next already has, which leaves that case open in #113. Drop the relink, owner-tracking and record-dedupe helpers of the first attempt, and the unread REPEAT__ key. Compute the other plugins' function sets once per unload instead of once per owned function. Pin the interleaved case in both unload orders, a key rebound by another plugin, a plugin loaded before the first load, and a wrapped builtin widget in tests/repeated-load-ownership.zsh. Refs #113
|
Review request record for head
The maintainer has elected the ADR-0026 fallback for this repository class. An independent read-only review of this head runs next, and the fallback review of record will be posted on this head. |
ss-o
left a comment
There was a problem hiding this comment.
Fallback review of record (ADR-0026, class 3) for head 4b682fe3ce3a033270762cd14d04baebb5370aae. The Copilot request did not register (recorded above). An independent read-only reviewer checked this head against the scope the maintainer set for this PR: same-plugin repeated loads only, with the takeover case kept as on next. I reran each claim before posting.
Findings
- Major,
lib/zsh/autoload.zsh:1244(inline): unload deletes a function that a still-loaded plugin owns, when that plugin was itself loaded twice. With this sequence, this head is worse thannext. Reproduced:shared_fn=0on this head,shared_fn=1on75d722c. - Minor, PR body: "a function another still-loaded plugin also created is kept" holds only when the other plugin's newest load created it. Same cause as the major finding.
- Nit,
lib/zsh/autoload.zsh:1243:.zi-diff-functions-computereruns for every registered plugin on each unload. It is idempotent, so no action.
No blockers.
Checklist (zi AGENTS.md, Code review)
- Public contracts and consumers: no impact.
contracts/public-contract-v1.jsonnames none of.zi-register-plugin,.zi-repeat-*,.zi-keep-previous-load-functionsorFUNCTIONS_OWNED__. The only output change is extra "Deleting function" lines where the fix removes functions. - Loading and shell state: correct for the probed in-scope sequences, apart from the major finding. Probed on this head against
nextin isolated shells:- this head cleans up and
nextleaks:- three loads
zi lighttwice- a second load without a widget
- a takeover by a plugin that was unloaded before the repeated load
- an unrelated plugin in between
- the same key bound in another keymap in between
- the user redefining the function between loads
- identical on both: three-plugin interleaved loads.
.zi-clear-report-forresetsFUNCTIONS_OWNED__.- The bindkey identity matches the record layout written in
zi.zshand read by unload.-Aand-Nrecords are excluded, as the body states.
- this head cleans up and
- Filesystem and update safety: not affected.
- Verification.
- The five scenarios the change fixes fail on
next; the five pinned to today's behaviour pass there. - No scenario can pass vacuously.
- Assertions check the effect.
- The new CI job uses the same checkout pin as the other jobs and matches
tests/ci-registration.zsh.
- The five scenarios the change fixes fail on
Verification limits
- Zsh 5.9.2 on Linux only.
zi reportoutput, functions defined by annex hooks, andbindkey -Aor-Nacross repeated loads were reasoned about, not probed.- Hosted CI logs for this head were not read.
The fix and a shared-twice regression scenario follow in one push. That push makes this review stale, and the new head gets its own review.
The other-owner check read only the other plugin's newest load. When that plugin was itself loaded more than once, its functions live in its owned set, so unloading a third plugin deleted them. Count the owned set too, and pin the sequence as the shared-twice scenario. Refs #113
|
Review request record for head
The fallback review of record for |
ss-o
left a comment
There was a problem hiding this comment.
Fallback review of record (ADR-0026, class 3) for head ed8af154cc6fb717dadf1e31de3da608b3a6f75c, review round 2. The Copilot request on this head did not register (recorded above). An independent read-only reviewer checked this head against the maintainer's scope for this PR (same-plugin repeated loads; a takeover by another plugin stays as on next). I reran every finding's evidence before posting.
Findings
- Round-1 fix confirmed.
shared-twicepasses, and the round-1 sequence leavesshared_fnin place, as onnext. - Minor, worse than
next,zi.zsh:1252(inline): a plugin that rebinds p's key withbindkey -M mainbetween p's loads is not detected as a takeover, sounload premoves that plugin's live binding. - Minor, differs from
next,zi.zsh:1192(inline): a binding or widget the user changes between p's two loads is not treated as a takeover, sounload prestores the state from before p's first load instead of the user's value. - Nit,
tests/repeated-load-ownership.zshtwice,changed,prior,wrapped: these scenarios assert only absence after unload. A mutant that skips everyzi loadstill passes them; the other 7 scenarios catch it. A presence check after the first load would close this. - Nit, pre-existing on both:
zi light PATHruns a full load (STATES__=2). True light mode (light-mode) loaded twice leaks the same way on this head and onnext. Recorded as workspace note7d77a0d1.
No blocker. The first minor finding is a new defect class compared with round 1: round 1 was about function ownership, while this round is about how takeovers are detected. Per the review procedure, this PR is parked here. A third round waits for the maintainer's decision.
Checklist (zi AGENTS.md, Code review)
- Public contracts: no impact. None of the new names appear in
contracts/public-contract-v1.json. - Loading and shell state: correct for the same-plugin cases, with the two minor divergences above. Probed on this head against
next:- This head cleans up where
nextleaks: three loads; a second load without a widget; an unrelated plugin in between; a plugin binding the same key under-M emacs; a takeover by a plugin that was unloaded before the repeated load; a user-redefined function;id-aswith a/. - Identical on both: interleaved loads in both unload orders; three plugins; interleaved
id-asloads; light-mode plugins taking over. - The round-1 fix can only keep a function, never delete one.
FUNCTIONS_OWNED__is reset both on unload and on a fresh load.
- This head cleans up where
- Filesystem and update safety: not affected.
- Verification. Hosted CI on this head shows 99 check runs succeeded. One
Public Contract Impactrun was cancelled when a body edit started a newer run, and that rerun on the same SHA succeeded. Locally the full suite passes 40 of 40. The scenarios pinned to today's behaviour pass onnext.
Verification limits
- Tested only with Zsh 5.9.2 on Linux, path-loaded and
id-asplugins. - Not exercised: cloned plugins, turbo mode, annex hooks,
zi report, andbindkey -Aor-Nacross repeated loads.
A plugin that rebinds a key through `bindkey -M main` between two loads of another plugin changes the same binding as a plain `bindkey`, but the takeover check gave the two different identities, so the repeated load kept its records and unload removed the other plugin's live binding. Give `-M main` the default keymap's identity and pin it as main-taken. Check after the loads in twice, changed, prior and wrapped that the plugin really loaded, so those scenarios cannot pass when nothing ran. Refs #113
|
Review request record for head
The round-2 review of record on |
ss-o
left a comment
There was a problem hiding this comment.
Fallback review of record (ADR-0026, class 3) for head 2832076269c7871d923aa9fd6384f680979dcaff, review round 3. This is the last round the maintainer approved. The Copilot request on this head did not register (recorded above). An independent read-only reviewer checked this head against the maintainer's scope for this PR. I reran every finding's evidence before posting.
Findings
- Round-2 fixes confirmed:
main-takenpasses.- A mutant that drops the
mainmapping fails onlymain-taken. - A mutant that reverts the round-1 fix fails only
shared-twice. - A mutant that skips every load fails all 12 scenarios.
- Minor, worse than
next, same class as round 2,zi.zsh:1253(inline): whenmainis linked toviinsoremacs, a plugin that binds p's key with-M viinsor-M emacsbetween p's loads is not seen as a takeover, sounload premoves that plugin's live binding. - Nit, output only,
zi.zsh:1199(inline): a plugin that creates a keymap (bindkey -N), loaded twice, makes unload print two "no such keymap" errors and duplicated Deleting lines. The end state is correct. - Nit, same class as round 2, PR body "Known limits": the user-change limit applies to any change made without zi records, functions included. For example, a plugin loaded with
light-modethat defines p's function between p's loads loses it when p is unloaded (shared_fn=0on this head,1onnext). A single load gives the same result on both branches.
No blocker and no new defect class in behaviour. The output nit is new in kind but changes no state. Under the maintainer's decision there is no fourth round on my own; the next step is the maintainer's.
Checklist (zi AGENTS.md, Code review)
- Public contracts: no change. None of the touched symbols or keys are in the manifest.
- Loading and shell state: correct within scope, apart from the minor finding. The identity fields match the record writer.
map,REPLYandreplyare local. User functions defined between loads never become owned. - Filesystem and update safety: not affected.
- Verification: adequate and not vacuous.
- Hosted CI on this head: 98 check runs succeeded. Two runs (
Commit Lint,Public Contract Impact) were cancelled when the body edit started newer runs, and those reruns on the same SHA succeeded. - The local full suite passes 40 of 40.
ci-registrationcatches an unregistered test.
- Hosted CI on this head: 98 check runs succeeded. Two runs (
Verification limits
- Zsh 5.9.2 on Linux only.
- Snippet time keys and ids containing a literal
---were reasoned about, not executed.
Description
A plugin loaded more than once and then unloaded once left state behind. Each load reset the plugin's records, so unload knew only about the last load: functions from earlier loads survived, and a widget the plugin replaced was restored to the plugin's own first replacement instead of the original.
This change keeps the earlier records when the same plugin loads again:
zi.zsh:.zi-register-pluginkeeps the widget and bindkey records on a repeated load, and.zi-keep-previous-load-functionsadds the functions the previous load created to a per-plugin owned set (FUNCTIONS_OWNED__).lib/zsh/autoload.zsh: unload also deletes the owned functions, except those another registered plugin created too, on its newest load or an earlier one;clear-reportresets the new key..zi-repeat-taken-over: when another plugin loaded between the two loads recorded one of the same widgets or bindings, the records start again exactly as they do onnexttoday.That last case, another plugin taking the widget or binding over between two loads of the same plugin, is deliberately left as it is on
next. Fixing it needs unload to restore along a chain of owners (per-load records), not from each plugin's own snapshot; an attempt within the per-plugin model made one unload order worse thannext(see the issue comment). It stays open in #113.Related issues
Refs #113 (the same-plugin cases; the interleaved case and per-load-instance records remain open, so this does not close it).
Type of change
fix- bug fix (non-breaking)test- test addition or correctionci- CI/workflow changeChecklist
next; only same-repository hotfixes targetmainnexttomainpromotion uses a merge commit and records both parent SHAs (not a promotion)Co-authored-bytrailer credits a real human, never a bot, AI agent, or automation (none)zsh -n zi.zsh/ Trunk checks)Verification
Zsh 5.9.2, Linux, each in a scratch copy of the tree:
tests/repeated-load-ownership.zsh, 12 scenarios, each in a clean shell: twice, changed, interleaved, interleaved-other-order, older, binding-taken, main-taken, prior, wrapped, shared, shared-twice, reload. On this branch 12 of 12 pass. Onnext(75d722c) with the test copied in, 6 fail (twice, changed, older, wrapped, shared, shared-twice) and the 6 that pin today's behaviour pass. Two scenarios came from review:shared-twicefrom round 1 (failed on4b682fe, fixed ined8af15) andmain-takenfrom round 2 (failed oned8af15, fixed in2832076). A mutant that skips everyzi loadnow fails all 12 scenarios.tests/*.zsh, run as CI does from a committed snapshot): 40 of 40 pass, includinghook-ownership.zsh,unload-ownership-contracts.zshandci-registration.zsh.4b682fe. Each of these 8 fails the new test: records cleared on repeat, previous functions not kept, widget records cleared on repeat, unload ignoring the owned set, no other-owner check, takeover check ignored, bindings dropped from the takeover check, takeover check counting plugins loaded before the newest load. Two survive. Skipping the plugin itself in the takeover loop is equivalent, because no load newer than the plugin's newest load can be its own. Dropping theFUNCTIONS_OWNED__reset fromclear-reportis not covered by a test.zsh -nandzcompileon the changed Zsh files,trunk checkon the five changed files, andactionlintonzsh-n.yml: clean..github/workflows/zsh-n.ymlruns the new test as jobRepeated Load Ownershipand adds it to the path filter.contracts/public-contract-v1.jsonnames none of the touched internal functions or keys.Known limits:
bindkeyrecords with a key take part in the takeover check (the default map, or-M MAP, with-M maincounted as the default map).bindkey -Aand-Nrecords do not.nextwould keep the user's value. A single load, the same user change and an unload already behave this way onnext. This limit is recorded on fix(unload): preserve runtime ownership across repeated plugin loads #113.zi reportstill shows only the newest load's functions for a plugin.