Unbind model rules automatically when deleting a bound credential - #50
Conversation
DELETE /admin/credentials/{name}?unbind=1 removes the credential's
account key from every model rule binding in one control-store revision
before deleting the file; without the flag the 409 guard is unchanged.
The WebUI delete drawer now lists the bound rules and asks for a single
confirmation instead of sending operators to unbind manually.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @maiphucgiang, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 3 days and 18 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's Guide本 PR 将凭证删除扩展为可确认的一次性自动解绑流程：控制库在单次事务中清理所有模型规则中的目标指纹，后端仅对显式 unbind 确认执行解绑并删除，默认删除仍阻止已绑定凭证；WebUI 同步展示影响范围并提供明确的解绑删除操作。 Sequence diagram for confirmed credential unbinding and deletionsequenceDiagram
actor Admin
participant WebUI
participant AdminAPI
participant Management
participant ControlStore
participant CredentialPool
Admin->>WebUI: Click 解除绑定并删除凭证
WebUI->>AdminAPI: DELETE /admin/credentials/{name}?unbind=1
AdminAPI->>Management: admin_delete_guard(name, unbind=true)
Management->>ControlStore: unbind_credential(account_key)
ControlStore-->>Management: Updated model rules and revision
Management-->>AdminAPI: Allow deletion
AdminAPI->>CredentialPool: remove_file(name)
CredentialPool-->>AdminAPI: File removed
AdminAPI-->>WebUI: 200 removed
Flow diagram for credential deletion confirmationflowchart TD
A[Open credential deletion drawer] --> B{Credential has model bindings?}
B -- No --> C[DELETE credential]
B -- Yes --> D[Show affected rules and automatic unbind warning]
D --> E{Confirm automatic unbind?}
E -- No --> F[Keep credential and bindings]
E -- Yes --> G[DELETE credential?unbind=1]
G --> H[unbind_credential removes account key from all rules]
H --> I[remove_file deletes credential]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba57ae333a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pool = CONFIG.get("cred_pool") | ||
| if CONFIG.get("management") is not None: | ||
| CONFIG["management"].admin_delete_guard(os.path.basename(name)) | ||
| CONFIG["management"].admin_delete_guard(os.path.basename(name), unbind=unbind) |
There was a problem hiding this comment.
When unbind=true and pool.remove_file() returns false—for example, because os.unlink() encounters an OSError—this call has already committed the removal of the credential from every model rule. The endpoint then reports a failed deletion while the credential remains and its routing policy has silently broadened to automatic account selection. Make the unbind and file deletion a recoverable unit, such as by restoring the bindings when removal fails.
Useful? React with 👍 / 👎.
maiphucgiang
changed the title
admin_delete_guard now only confirms the unbind and returns the identity; the delete route strips the bindings after remove_file succeeds, so a failed removal never silently widens routing to automatic account selection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 899d254679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Bindings stay untouched so a failed removal never widens routing to automatic. | ||
| raise HTTPException(status_code=404, detail={"error": {"message": f"凭据不在池中: {name}", "type": "invalid_request_error"}}) | ||
| if unbind_identity is not None and CONFIG.get("management") is not None: | ||
| CONFIG["management"].admin_unbind_credential(unbind_identity) |
There was a problem hiding this comment.
If control.unbind_credential() raises—for example because SQLite becomes read-only, full, or locked—pool.remove_file() has already unlinked the credential and pruned it from inventory. The request then fails while its credential_ids references remain; retrying cannot repair them because admin_delete_guard() no longer finds the deleted credential, leaving affected rules with an orphaned binding and no eligible account. The new post-delete ordering is fresh evidence beyond the earlier removal-failure issue: it fixes that failure direction but moves the partial update to control-store failures, so the file deletion needs to be staged/rolled back or otherwise made recoverable with the unbind.
Useful? React with 👍 / 👎.
unbind_credential now captures each rule's previous binding list and restore_bindings puts them back in one revision bump, so a delete whose file removal fails leaves routing exactly as it was; unbind failures still happen before any file is removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 729567c47c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if rule is not None and rule.get("credential_ids") != ids: | ||
| rule["credential_ids"] = list(ids) |
There was a problem hiding this comment.
When credential removal fails after the unbind, another administrator can update an affected rule before this rollback runs; because _update(None) performs no revision check and this assignment restores the entire old ID list, that successful intervening edit is silently overwritten. Restore only the deleted identity when the current list still matches the post-unbind state, or make deletion, unbinding, and rollback participate in the same mutation transaction/lock.
Useful? React with 👍 / 👎.
restore_bindings re-adds only the deleted identity and only where a rule still matches its post-unbind state, so an administrator edit or rule deletion between the unbind and the rollback is preserved instead of being overwritten.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47084ccaec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rollback = CONFIG["management"].admin_unbind_credential(identity) | ||
| if pool is None or not pool.remove_file(os.path.basename(name)): |
There was a problem hiding this comment.
When another administrator updates a model after admin_unbind_credential() returns but before remove_file() runs, checked_rule() still sees the credential in inventory and can bind it again; the deletion then succeeds and leaves that rule referencing an identity that no longer exists, so the route has no eligible credential. The confirmed delete needs to share the model mutation lock or otherwise prevent/recheck binding changes through successful file removal.
Useful? React with 👍 / 👎.
The confirmed delete now runs under the admin mutation lock shared with model-rule updates, and a second identity sweep after a successful removal catches rebinds from writers outside that lock, so no rule can keep referencing a deleted credential.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a65a77e469
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| identity = management.admin_delete_guard(os.path.basename(name), unbind=unbind) | ||
| if identity is not None: | ||
| rollback = management.admin_unbind_credential(identity) | ||
| if pool is None or not pool.remove_file(os.path.basename(name)): |
There was a problem hiding this comment.
When credential removal raises before unlinking, this expression bypasses the rollback branch and leaves the file present but its model bindings removed. This is reachable because CredentialPool.remove_file() catches only OSError, while credential_file_lock() can raise CredentialFileError for a non-regular lock file (app/credential_io.py:65). Fresh evidence in this revision is that rollback is now confined to the false-result branch; normalize pre-unlink failures to False or catch them here and restore the bindings.
Useful? React with 👍 / 👎.
1 participant
Changes
ControlStore.unbind_credential: remove an account key from every model rule'scredential_idsin a single revision bump, returning each rule's previous list for rollback; identities nothing references leave the revision untouched.admin_delete_guard(name, unbind=False)keeps the original 409 guard and otherwise returns the identity; the route unbinds first (capturing the rollback record), deletes the file, and on a failed removalrestore_bindingsputs the bindings back.restore_bindingsre-adds only the deleted identity and only where a rule still matches its post-unbind state, so concurrent rebinds or rule deletions in between are preserved instead of overwritten.admin_mutation_lock, and sweep the identity once more after a successful removal so a rebind racing from outside the lock cannot leave a rule referencing a deleted credential. The admin middleware honours theunbind=1query flag and lets the delete route own the whole operation.DELETE /admin/credentials/{name}?unbind=1unbinds then deletes the file and pool entry; without the flag the 409 guard and its model list are unchanged.Verification
tests/test_credential_unbind.pyplus control-store and middleware regressions: only the target identity is removed with one revision bump; the guard itself never unbinds; a mocked failed removal returns 404 with bindings restored; a rollback after a concurrent rebind or rule deletion preserves the edit; a rebind racing the removal from outside the shared lock is swept; no-binding and unknown names stay on the fast path; the unflagged delete still returns 409 with the model list; the middleware skips its guard only for confirmed unbinds.hy3showsbindings: ["hy3"], the unflagged delete returns 409,?unbind=1returns 200 with the file removed andhy3back tocredential_ids: []while still enabled; the pool returns to 8 credentials and zero-multiplier chats return 200 against the real upstream.