Skip to content

fix(unload): keep ownership across repeated loads of one plugin - #582

Merged
ss-o merged 4 commits into
nextfrom
bug-113-repeated-load
Oct 1, 2026
Merged

ss-o merged 4 commits into
nextfrom
bug-113-repeated-load

Conversation

@ss-o

@ss-o ss-o commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

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-plugin keeps the widget and bindkey records on a repeated load, and .zi-keep-previous-load-functions adds 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-report resets 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 on next today.

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 than next (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 correction
  • ci - CI/workflow change

Checklist

  • Ordinary work targets next; only same-repository hotfixes target main
  • A next to main promotion uses a merge commit and records both parent SHAs (not a promotion)
  • Commit messages follow Conventional Commits format
  • Any Co-authored-by trailer credits a real human, never a bot, AI agent, or automation (none)
  • I have read the contribution guidelines
  • Existing tests pass (zsh -n zi.zsh / Trunk checks)
  • Documentation updated if needed (internal behaviour; comments updated, no user-facing docs change)

Verification

Zsh 5.9.2, Linux, each in a scratch copy of the tree:

  • New 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. On next (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-twice from round 1 (failed on 4b682fe, fixed in ed8af15) and main-taken from round 2 (failed on ed8af15, fixed in 2832076). A mutant that skips every zi load now fails all 12 scenarios.
  • Full suite (tests/*.zsh, run as CI does from a committed snapshot): 40 of 40 pass, including hook-ownership.zsh, unload-ownership-contracts.zsh and ci-registration.zsh.
  • 10 hand mutants of the change, run on 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 the FUNCTIONS_OWNED__ reset from clear-report is not covered by a test.
  • zsh -n and zcompile on the changed Zsh files, trunk check on the five changed files, and actionlint on zsh-n.yml: clean.
  • .github/workflows/zsh-n.yml runs the new test as job Repeated Load Ownership and adds it to the path filter. contracts/public-contract-v1.json names none of the touched internal functions or keys.

Known limits:

  • Only bindkey records with a key take part in the takeover check (the default map, or -M MAP, with -M main counted as the default map). bindkey -A and -N records do not.
  • Only another plugin's records count as a takeover. If the user rebinds the plugin's key or redefines its widget between the two loads, the second load overwrites that, and unload restores the state from before the first load, where next would keep the user's value. A single load, the same user change and an unload already behave this way on next. This limit is recorded on fix(unload): preserve runtime ownership across repeated plugin loads #113.
  • zi report still shows only the newest load's functions for a plugin.

ss-o added 2 commits October 1, 2026 01:04
…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
@ss-o
ss-o requested a review from a team as a code owner October 1, 2026 00:31
@ss-o ss-o added area:ci Continuous integration or GitHub Actions work. area:zi Zi core behavior, APIs, or documentation. type:bug Something is broken or behaving incorrectly. labels Oct 1, 2026
@ss-o

ss-o commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Review request record for head 4b682fe:

  • 2026-10-01T00:31:47Z: requested copilot-pull-request-reviewer[bot] once through the REST requested_reviewers endpoint. The call succeeded, but the response listed no requested reviewers.
  • By 00:31:59Z the timeline's only review_requested event (00:31:03Z) names the CODEOWNERS team tsc, not Copilot, and there is no Copilot review, so the request did not register. Cause unconfirmed. The same symptom occurred on zsh-eza#131 and zsh-eza#134 today.

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 ss-o left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 than next. Reproduced: shared_fn=0 on this head, shared_fn=1 on 75d722c.
  • 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-compute reruns 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.json names none of .zi-register-plugin, .zi-repeat-*, .zi-keep-previous-load-functions or FUNCTIONS_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 next in isolated shells:
    • this head cleans up and next leaks:
      • three loads
      • zi light twice
      • 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-for resets FUNCTIONS_OWNED__.
    • The bindkey identity matches the record layout written in zi.zsh and read by unload. -A and -N records are excluded, as the body states.
  • 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.

Verification limits

  • Zsh 5.9.2 on Linux only.
  • zi report output, functions defined by annex hooks, and bindkey -A or -N across 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.

Comment thread lib/zsh/autoload.zsh Outdated
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
@ss-o

ss-o commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Review request record for head ed8af15:

  • 2026-10-01T00:51:24Z: requested copilot-pull-request-reviewer[bot] once through the REST requested_reviewers endpoint. The call succeeded, but the response listed no requested reviewers.
  • By 00:51:39Z the timeline still had no review_requested event naming Copilot (the only one is the CODEOWNERS team tsc at 00:31:03Z), so the request did not register.

The fallback review of record for 4b682fe is stale after this push. An independent read-only review of ed8af15 runs next under the same ADR-0026 fallback, and its review of record will be posted on this head. My review-thread reply also shows as a comment-only review entry on ed8af15; that entry is not a review of record.

@ss-o ss-o left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-twice passes, and the round-1 sequence leaves shared_fn in place, as on next.
  • Minor, worse than next, zi.zsh:1252 (inline): a plugin that rebinds p's key with bindkey -M main between p's loads is not detected as a takeover, so unload p removes 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, so unload p restores the state from before p's first load instead of the user's value.
  • Nit, tests/repeated-load-ownership.zsh twice, changed, prior, wrapped: these scenarios assert only absence after unload. A mutant that skips every zi load still 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 PATH runs a full load (STATES__=2). True light mode (light-mode) loaded twice leaks the same way on this head and on next. Recorded as workspace note 7d77a0d1.

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 next leaks: 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-as with a /.
    • Identical on both: interleaved loads in both unload orders; three plugins; interleaved id-as loads; 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.
  • Filesystem and update safety: not affected.
  • Verification. Hosted CI on this head shows 99 check runs succeeded. One Public Contract Impact run 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 on next.

Verification limits

  • Tested only with Zsh 5.9.2 on Linux, path-loaded and id-as plugins.
  • Not exercised: cloned plugins, turbo mode, annex hooks, zi report, and bindkey -A or -N across repeated loads.

Comment thread zi.zsh Outdated
Comment thread zi.zsh
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
@ss-o

ss-o commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Review request record for head 2832076:

  • 2026-10-01T01:33:12Z: requested copilot-pull-request-reviewer[bot] once through the REST requested_reviewers endpoint. The call succeeded, but the response listed no requested reviewers.
  • The timeline still has no review_requested event naming Copilot (the only one names the CODEOWNERS team tsc, at 00:31:03Z), so the request did not register.

The round-2 review of record on ed8af15 is stale after this push. The maintainer approved a third, narrow round: -M main treated as the default keymap, presence checks in four scenarios, and user changes between loads kept as a documented limit. One more independent read-only review of 2832076 runs next under the ADR-0026 fallback, and its review of record will be posted on this head. My two thread replies also appear as comment-only review entries on 2832076; neither is a review of record.

@ss-o ss-o left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-taken passes.
    • A mutant that drops the main mapping fails only main-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): when main is linked to viins or emacs, a plugin that binds p's key with -M viins or -M emacs between p's loads is not seen as a takeover, so unload p removes 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-mode that defines p's function between p's loads loses it when p is unloaded (shared_fn=0 on this head, 1 on next). 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, REPLY and reply are 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-registration catches an unregistered test.

Verification limits

  • Zsh 5.9.2 on Linux only.
  • Snippet time keys and ids containing a literal --- were reasoned about, not executed.

Comment thread zi.zsh
Comment thread zi.zsh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci Continuous integration or GitHub Actions work. area:zi Zi core behavior, APIs, or documentation. type:bug Something is broken or behaving incorrectly.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant