Repository navigation
fix: maintain typed auth clients as remote source headers change - #8181
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 675f7ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
A remote MCP source's credential headers only got their typed upstream auth client, header copies and server links from the organization-wide backfill, so a source configured between runs had none. Keep them in line inside the transaction of the write that changes them: the source's header create, update and delete, the source's own deletion, and the creation, backend switch and deletion of an MCP server in front of it. The sync reuses the backfill's derivation for a single source, so it records exactly what a backfill run would. It leaves administrator-created clients, administrator detachments, credential choices and grants alone, and leaves a source without an Authorization header to the backfill, whose decision needs the organization's upstream OAuth and EMA clients. Every path now locks the source before any of its MCP servers, so the sync's writes to a source's servers never wait on a server lock in the opposite order to the source's own writers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A source without an Authorization header keeps the client the backfill recorded for it, but kept every backfilled link to that client, including one from a server that had since switched to another backend. That server would have gone on sending the old source's credential headers. Keep only the links of servers still backed by the source, and retire the client with them once no server is left on it, as the backfill decides. Also lock the remote source before inserting the server row in the shared creation command, so no path reaches the sync holding a server lock first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s server The shared MCP server creation command checked the source's liveness without holding its row lock, so a source a concurrent deletion tombstoned while the check waited could still get a server committed onto it. Take the live, project-scoped source lock there instead, which also gives the lock the auth client sync needs. The two callers that took that lock themselves now leave it to the command, so the invariant has one home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ange The serving path loads an auth client's header copies and sends them with the upstream request, so a copy the configuration no longer yields is a deleted header still going out, or a rotated one going out with its old value. Only the organization-wide backfill maintained the copies on upstream OAuth and EMA clients, so a source written between runs kept the stale ones. Each write that changes them now re-decides them in its own transaction, through the backfill's own planner so a sync and a run record the same copies. A client is decided from every source its servers sit on, not just the one being written, because the rule refusing a client shared across sources reads them all; a client left with no live server on any source carries none. Tunneled sources get the same treatment as remote ones. No management API writes a tunneled source's headers, so its own lifecycle is what has to retire the records: its deletion, and servers arriving on or leaving it. A server that leaves every credential source is reached only through itself, so server updates and deletions re-decide their own clients' copies too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removed as unused on main while this branch was open. The ema copy test needs it: a client whose last live server is gone must stop carrying its source's credentials, and tombstoning the server is how the test gets there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
danielkov
force-pushed
the
fix/remote-header-typed-maintenance
branch
from
October 28, 2026 00:15
ba043fb to
321b569
Compare
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…its server The shared creation command settled a remote source's liveness under its row lock but took the tunneled lock without one, and that lock accepts a tombstoned source so the retirement paths can use it. A deletion committing while the create waited therefore let a server be committed onto a deleted tunneled source. Both kinds now verify liveness and project ownership under the lock, through one helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… them Associating an upstream OAuth client with a server on a second source makes the client shared, and the ownership rule then says it carries neither source's credential headers. The copies the first source left stayed on it, and the serving path loads them by auth client id and organization alone and revalidates no source ownership, so the first source's secret went to the second source's upstream until the next backfill run. Detaching the server whose source lent the headers leaves the mirror state. All three writers of an association now re-decide the copies in their own transaction: the identity commit, the catalog identity-provider attachment and the dashboard detach. The copies pass locks only the header copy rows it writes, so each caller runs it after its own writes and the copy rows are the last locks the transaction takes. Two orders needed correcting for that to hold: a source sync now marks servers before writing copies, and MCP server deletion runs its syncs after the issuer cascade, which row-locks the remote session clients an identity commit writes before reaching the same copies.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The test configured the arriving server without the client ever reaching the header-bearing source, so the copies were retired because the client's only source carried no headers, not because it became shared. Associate the sole server first and assert it keeps its association, so the commit really does make the client span two sources.
danielkov
added
the
review:bypass
danielkov
enabled auto-merge
October 28, 2026 11:04
![gram-bot[bot]](https://avatars.githubusercontent.com/in/1956617?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Auto-approved: review:bypass label applied by @danielkov. Required status checks still gate this merge.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
A credential source's typed auth clients, server links and copied credential headers now stay in line as the source and its servers are written, inside the same transaction as the write. Before this, only the organization-wide
authclientbackfillrun maintained them.Covered writes:
remoteMcp.createServerHeader,updateServerHeader,deleteServerHeader,remoteMcp.deleteServer,tunneledMcp.deleteServer, and the creation, backend switch and deletion of an MCP server.Motivation
Two gaps, both leaving records that no longer match the configuration:
Tunneled sources had no typed reconciliation at all: deleting one left its derived clients, links and copies live.
Impact
Correctness and freshness of the records the upstream serving path reads. No change to how a request is authorized or to which client serves it. Administrator-created clients, administrator detachments, credential choices and auth grants are untouched, and every path is scoped to the writing organization and project. A write whose evidence changes under it rolls back rather than committing records that do not match.
Technical details
The sync reuses the backfill's planner rather than restating it
The per-client copy decisions were extracted from
copyOntoOAuthClientsandcopyOntoEMAClientsintocopyOntoOAuthClientandcopyOntoEMAClient, which the run and the sync both call.ListOAuthClientSourcesgained an optional client-id filter so the sync reads one client's sources instead of the organization's. A test asserts that a backfill run straight after a sync reports no pending clients, links, copies, retirements or gaps.A client is decided from all of its servers' sources
The rule that refuses a client shared across sources with credential headers reads every source its servers sit on, so the sync reads them all for the clients it re-decides — scoped to those clients, never the organization. A client left with no live server on any source carries no copies, so its own are retired.
A server that leaves every credential source
Switching a server to a toolset backend, or deleting a server that had none, leaves no source to re-decide its clients from.
SyncServerDerivedCopiesre-decides them from the server itself for those cases. It takes no lock: every copy write repeats the ownership rule it was decided from, so a concurrent change loses the write rather than overwriting it.Tunneled sources
SyncTunneledSourcemirrorsSyncRemoteSource. No management API writes a tunneled source's headers and its deletion leaves them on the tombstoned row, so the source's liveness is what says they are gone. No tunneled header API was added.Lock order is source-then-server everywhere
The sync writes to a source's
mcp_serversrows and to their links, while a source's own writers lock the source first. The shared MCP server creation command,updateMcpServeranddeleteMcpServertake the source lock up front and reject a concurrent backend switch as a retryable conflict.The creation command also settles each source's liveness and project ownership under that lock, for both kinds. It was previously checked before any lock, so a deletion committing while the create waited could leave a server in front of a tombstoned source.
LockRemoteSourceandLockTunneledSourcestill accept a tombstoned source, because the update and delete paths have to lock one to retire its clients.Association writes
Associating an upstream OAuth client with a server on a second source makes the client shared, and the ownership rule then says it carries neither source's credential headers — while the first source's copies are still on it.
ListAuthClientHeadersloads those copies by auth client id and organization alone and revalidates no source ownership, andwithoutDeniedSourcesonly strips forwarded headers from refused request headers, so the first source's secret went to the second source's upstream until the next backfill run. Detaching the server whose source lent the headers leaves the mirror state.All three writers of an association now re-decide the copies in their own transaction: the identity commit, the catalog identity-provider attachment, and the dashboard detach. The mechanism predates this PR, but this PR records copies at header-write time rather than only at backfill time, so the stale state was reachable in more windows than before.
Lock behaviour of the copies pass
It locks only the header copy rows it writes — no source, server, link or upstream client row — and every caller runs it after its own writes, so the copy rows are the last locks the transaction takes. Two orders needed correcting for that to hold: a source sync marks servers before writing copies, and MCP server deletion runs its syncs after the issuer cascade, which row-locks the
remote_session_clientsan identity commit writes before reaching the same copies.Consistency under a concurrent source or header write comes from the writes rather than a lock: an added or updated copy repeats the ownership rule it was decided from, so an invalidating change makes the write affect no row; a retired copy is unconditional, so the decision to stop sending a credential always lands. A copy wrongly retired by a racing writer is restored by the next source write or backfill run.