Reject duplicate connection names on create - #1346
Merged
Merged
Conversation
connections.create silently upserted when a connection with the same (owner, integration, name) already existed, clobbering the stored credential and metadata. Fail with ConnectionAlreadyExistsError (409) instead, checked before any provider write and re-checked inside the insert transaction. OAuth minting keeps its intentional re-mint upsert.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-cloud | 05a9e71 | Aug 28 2026, 10:16 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-marketing | 05a9e71 | Commit Preview URL Branch Preview URL |
Aug 28 2026, 10:14 PM |
Contributor
Cloudflare previewTorn down — the PR is closed. |
@executor-js/cli
@executor-js/config
@executor-js/execution
@executor-js/sdk
@executor-js/codemode-core
@executor-js/runtime-quickjs
@executor-js/plugin-file-secrets
@executor-js/plugin-graphql
@executor-js/plugin-keychain
@executor-js/plugin-mcp
@executor-js/plugin-onepassword
@executor-js/plugin-openapi
executor
commit: |
health-checks swapped in a dead key by re-creating the connection; the fixture server now revokes the token instead. The cloud credentials scenario pinned replace-in-place; it now pins the conflict rejection.
The conflict error message is a getter, so it does not survive the wire; match the tag client-side and rebuild the message, mirroring the integration already-exists handling. Adds a browser scenario recording the rejected duplicate.
The callback mint upserts by (owner, integration, name), so a fresh oauth.start aimed at an existing name silently replaced that connection when the user returned from the provider. start now rejects the name up front and complete re-checks it (a connection can appear mid-flight), both leaving the existing connection untouched. Reconnect flows pass reconnect: true and keep re-minting the same connection.
The popup flow hook dropped the reconnect flag from its start payload, so every Reconnect that went through it hit the new oauth.start guard: the popup opened and closed with no message. Forward the flag, toast the reconnect handoff failure, and pre-check the connection list before opening a popup for fresh BYO/CIMD/DCR connects so a taken name surfaces as an error without the window blip.
Keep the connections.create conflict (409 ConnectionAlreadyExistsError) and drop the branch's OAuth-side guard: main now resolves a taken name for fresh OAuth connects with `newConnection`, which suffixes to the next free name, so the `reconnect` discriminator and the oauth.start/complete rejections are redundant and would break reconnect.
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
1 participant
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.
Summary
connections.createwith an existing (owner, integration, name) silently overwrote the stored credential — the provider item id derives from those three parts, so the secret was clobbered before the row write even ran. Creates now return a 409ConnectionAlreadyExistsError(checked before any provider write, re-checked in-transaction for the create/create race), surfaced throughPOST /connections, the core tool (connection_already_exists), and the MCP plugin.OAuth flows are deliberately unchanged: main's
newConnection: truealready resolves name collisions server-side on connect and reconnect re-mints on purpose, which supersedes this PR's originalreconnect: truedesign. The manual credential submit is the one UI path that still hits the 409 and now shows it as a toast.Normalized-name collisions (
my-api-keyvsmy api key) are caught; same name under different owners coexist. Additive wire change (new 409 response), noted in the changeset.Credential writes and compensation
Provider writes are ordered after the transactional row insert: only the create that wins the insert may touch the provider, so a losing create can never clobber the winner's (or a pre-existing connection's) secret. The writes then run inline, before tool production — GraphQL/MCP plugins do authenticated introspection via
getValues()at tool-production time, so the credentials must exist by then or the catalog is discovered empty and never re-discovered.If the write does not complete — failure, interruption, or defect — compensation removes the items already written and then the row, so no credential-less row survives to 409 every retry. Nothing in compensation is silent: a failed compensating delete makes the create fail with an error that names the stranded connection and keeps the original failure as its cause, and a provider with
setbut nodeletegets an explicit warning naming the item that may be stranded.Compensation identifies its own row by the storage surrogate
row_id, deletes it with that id in the WHERE clause, and then — becausedeleteManyreturns void — confirms the outcome with a read in the same transaction. If any row still holds the name, the guarded delete removed nothing and a concurrent remove/recreate interleaved: compensation skips all credential-item deletion and logs that the surviving connection owns the items. Only when no row remains does it undo the items it wrote.An earlier revision deferred the credential write past an enclosing transaction's commit via an after-commit hook. That was reverted: the deferral made nested creates produce their tool catalog before the credentials existed (silently committing an empty catalog that is never re-discovered), and exactly-once execution of the hook is not guaranteed under interruption. External credential stores cannot join a database transaction; pretending otherwise created worse failure modes than the pre-existing window below.
Known limitations
Provider credential stores expose no conditional delete, so perfect cleanup under a concurrent remove/recreate is impossible at this layer. The residuals below are accepted deliberately rather than papered over with more machinery.
Nested-transaction orphans (pre-existing).
transactionnests by pass-through, so a create running inside an enclosing pluginctx.transactionwrites credentials before the OUTER commit. If that transaction rolls back, the row vanishes with it but the credential items survive as orphans at deterministic ids, overwritten by the next same-shaped create.Skipped cleanup leaves orphans. Under concurrent remove/recreate, compensation may skip item deletion, leaving orphaned credential values at the deterministic item ids. Orphans are inert without a row and the next same-shaped create overwrites them; orphans are preferred over the alternative, clobbering a live successor's secrets. A successor that inserts only after the compensation transaction commits can also still interleave with the item deletes.
Partial successor over a stranded predecessor. A successor that overwrites one variable, fails before the next, and then also fails its own compensating row delete leaves a stranded connection able to resolve one stale predecessor value. Closing this requires provider-side conditional deletes, which do not exist. The stranded state is not silent: the create fails with a typed
StorageErrornaming the connection.Unconfirmed compensating delete on non-transactional adapters. On an adapter without interactive transactions (Cloudflare D1 runs
interactiveTransactions: false; every statement auto-commits, no rollback) the guarded delete may already have committed when the confirmation read fails. The row state is then unknown — deleted or stranded — so compensation skips all credential-item deletion and the create fails with a typedStorageErrorthat reports the unconfirmed state instead of claiming a stranded row; any items already written are left as inert orphans.