langgenius / langgenius/dify

[Refactor/Chore] Make plugin uninstall credential cleanup failure-safe

Open
#39,893 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
TypeScript
Stars
156k
Forks
24.6k
Avg merge
20h 50m
Merged PRs (30d)
586

Description

- [x] I have read the [Contributing Guide](https://github.com/langgenius/dify/blob/main/CONTRIBUTING.md) and [Language Policy](https://github.com/langgenius/dify/issues/1542).
- [x] This is only for refactors or chores; if you would like to ask a question, please head to [Discussions](https://github.com/langgenius/dify/discussions/categories/general).
- [x] I have searched for existing issues [search for existing issues](https://github.com/langgenius/dify/issues), including closed ones.
- [x] I confirm that I am using English to submit this report, otherwise it will be closed.
- [x] 【中文用户 & Non English User】请使用英语提交,否则会被关闭 :)
- [x] Please do not modify this template :) and fill in all the required fields.

### Description

After #39764, `PluginService.uninstall` performs the plugin-daemon uninstall **before** deleting `TenantPreferredModelProvider` / `ProviderCredential` records and invalidating provider caches (see `api/core/plugin/plugin_service.py`, `uninstall`). If the credential cleanup raises (DB failure, transient lock, etc.), the endpoint reports an error but the plugin is already uninstalled, leaving orphaned credentials and stale caches. The previous implementation deleted credentials first and only uninstalled after a successful cleanup, so a failure left the installation and its credentials consistent.

### Motivation

The current ordering makes an ordinary uninstall non-atomic across two systems (plugin daemon and database). For a multi-tenant SaaS/self-hosted deployment, a transient DB error during uninstall can silently orphan model-provider credentials; the next install with the same plugin may reuse credentials that were supposed to be removed, or the operator sees an error although the plugin is gone.

### Additional Context

`preserve_credentials=True` intentionally skips cleanup for local replacement flows, so the fix only needs to cover the ordinary path. Suggested directions (open to maintainer preference):

1. Keep the current order but make cleanup resilient: on cleanup failure, log a tenant-scoped warning and queue a bounded retry (Celery task) so credentials are eventually removed; the API still reports the uninstall result accurately.
2. Restore cleanup-before-uninstall for the non-preserve path and accept the smaller failure window (uninstall fails → credentials already removed).
3. Treat credential cleanup failure as an uninstall failure and record the plugin as "uninstalled with pending cleanup" for reconciliation.

No PR is attached yet; this is a behavior/consistency question that deserves maintainer input before implementing.

Contributor guide

Open the contributing guide

Research direction

Read api/core/plugin/plugin_service.py and the uninstall flow, focusing on the ordering of plugin-daemon uninstall, TenantPreferredModelProvider and ProviderCredential cleanup, and provider-cache invalidation. Confirm how preserve_credentials=True behaves and ask maintainers to choose the failure policy before implementation. Done means the selected policy keeps plugin state, credentials, and caches consistent when cleanup fails, with focused coverage for the ordinary path.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend, database
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.