fix(plugins): refuse a concurrent install instead of reporting success - #153
Merged
Conversation
installPlugin returned silently when the same id was already installing, so every caller read the resolve as a completed install. Accepting a plugin-install deep-link sheet while the Settings tab was installing the same plugin closed the sheet reporting success having written nothing. The second request now throws PluginInstallInProgressError, which pluginInstallErrorMessage names for the user. It is refused rather than joined to the running install: the two callers may have reviewed different manifests for the same id, and the running install cannot stand in for the one this caller consented to.
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.
Follow-up from the #150/#152 review: the only case in the deep-link confirm flow where the UI actively lies.
The bug
marketplaceStore.installPluginreturned silently when the requested id was already ininstalling:Every caller reads that resolve as a completed install. Accept a plugin-install deep-link sheet while the Settings tab is installing the same plugin and the sheet resolves, dismisses, and tells the user it worked — having written nothing.
The fix
The second request throws
PluginInstallInProgressError(new, insrc/plugins/installErrors.ts), whichpluginInstallErrorMessagenames for the user via a newsettings.plugins.install.alreadyInstallingkey in all four locales. Both the Settings toast and the confirm sheet already route through that helper, so this adds no secondinstanceofladder.Refused rather than joined to the in-flight install, deliberately. The sheet and the Settings tab can hold different reviewed manifest texts for the same id (
reviewedManifestTextis what closes the fetch→consent→load TOCTOU). Joining would install the other caller's reviewed bundle while telling this caller its consent was honoured — trading a false success for a consent bypass.Callers after the change:
PluginsSection— unreachable in practice (the button is disabled while busy), same toast path if it ever is.plugins.tsdomain layer (MCPinstall_plugin) — propagates the message instead of the misleadingproduced no loaded plugin.restoreMissingPlugins— sequential and alreadycatching; unaffected.Tests
Three added to
marketplaceStore.installUpdate.test.ts, sharing onegatedFetch()helper that parks the first install insideplugin_fetch_url:All three fail on
dev, pass here.Verification
CI=true vitest run: 479/480 files, 3718/3719 tests. Sole failure istests/pluginBundleBuild.test.ts(39237ms — the known contention flake); re-run alone with the edited file it is 17/17 green.node ./node_modules/typescript/bin/tsc --noEmit -p tsconfig.json: exit 0, no output.