fix(credentials): write an env value and its credential row together - #7160
Merged
Conversation
Every writer of a workspace or personal environment map read-modify-writes a single jsonb column, so they take an advisory lock on the map to serialize. `deleteCredentialRecord` took none, and did the read, the edit and the write-back outside a transaction: a secret written concurrently was read before that write and silently dropped by the write-back. The credential row was also written after its map transaction committed, in four places. The delete direction left a row describing a value that was gone; the create direction was worse than a stale row, because it cannot be repaired by retrying — the key is in the map by then, so the next attempt reads it as pre-existing, computes an empty `newKeys`, and never creates the row. Both helpers already accept `executor`, and `setWorkspaceSecret` has been passing the transaction since the parameter landed; these four were never migrated. The personal reconcile stays outside its transaction: it opens its own and takes the user-identity fence, so nesting it would have two transactions taking two locks in opposite orders. It reconciles against the stored keys, so a failure there is repaired by the next one rather than entrenched. Also folds the four copies of the lock into one helper, since this would have been the fifth.
`workflow_blocks.name` is NOT NULL, so a block missing `type` or `name` has to be dropped — but it was dropped silently. A block with no edges left no trace anywhere: not in the returned warnings, not in a log line. The client sanitizer warns on the identical condition; this is its server counterpart, and the warnings array it feeds is already returned by the internal PUT, the v2 write and the importer.
Nothing caps a chat transcript — no per-chat message limit on write, no pruning — and the v2 route keys continuity by `chatId`, so it read the whole thing on every resumed turn and dropped it. Opt out there. The load stays the default because the copilot send path does consume it.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Greptile SummaryThe PR makes environment-map and credential-row mutations atomic and serializes competing map writers. It also avoids loading unused chat transcripts and reports workflow blocks dropped during persistence.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/credentials/orchestration/index.ts | Credential-driven personal and workspace environment deletions now serialize map updates and remove credential rows atomically. |
| apps/sim/lib/credentials/env-locks.ts | Introduces shared transaction-scoped advisory-lock helpers with bounded lock waits. |
| apps/sim/lib/credentials/environment.ts | Chunks workspace credential and membership inserts to keep transactional writes within PostgreSQL parameter limits. |
| apps/sim/lib/environment/utils.ts | Personal and workspace environment updates now merge under the shared lock and create workspace credential rows within the map transaction. |
| apps/sim/app/api/workspaces/[id]/environment/route.ts | Workspace environment PUT and DELETE now update values and credential metadata within one locked transaction. |
| apps/sim/app/api/environment/route.ts | Personal environment replacement now participates in environment-map serialization. |
| apps/sim/lib/copilot/chat/lifecycle.ts | Adds an opt-out for transcript loading while preserving existing default behavior. |
| apps/sim/lib/workflows/persistence/prepare-state.ts | Reports each invalid block dropped before normalized workflow persistence. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Request[Environment mutation] --> Tx[Begin transaction]
Tx --> Lock[Acquire environment-map advisory lock]
Lock --> Read[Read current JSONB map]
Read --> MapWrite[Write updated map]
MapWrite --> CredentialWrite[Create or delete credential rows using transaction executor]
CredentialWrite --> Commit[Commit atomically]
Commit --> Cache[Invalidate effective environment cache]
Reviews (3): Last reviewed commit: "fix(credentials): chunk the credential-A..." | Re-trigger Greptile
Exporting the personal lock while two writers skipped it left the map unserialized: `upsertPersonalEnvVars` merged against a read taken outside any lock, and the settings PUT replaced the map wholesale. A wholesale replace landing between another writer's read and its write-back is discarded whole, so it takes the lock too. The delete path now removes the key's mirrors directly instead of reconciling against a key list. The reconcile prunes every mirror absent from that list, so a secret added between the read and the prune lost its mirror while its value survived. `setPersonalSecret` already takes the map lock and then the user-identity fence inside it, so the targeted delete introduces no new lock order.
Collaborator
Author
…ends on `createWorkspaceEnvCredentials` wrote keys x members membership rows in one statement, and neither side is bounded by the request contract. Past 65535 bind parameters that throws — previously a partial success, because the value had already committed, but this now runs inside the value's transaction, so it rolls the save back instead, deterministically, on every retry. A 50-member workspace saving 150 keys reaches it. Chunked the same way the two personal paths in this file already are. Also from the audit: - invalidate the decrypted-env cache after `deleteCredentialRecord` removes an env value, matching the dedicated delete paths; without it a deleted secret stayed resolvable for the cache TTL - correct the comment claiming the personal reconcile "matches the replace" — it prunes against this request's key list, so a secret added after the commit still loses its mirror. Naming the gap instead of asserting it away - name the one behavior change the in-transaction re-read introduces: a key whose submitted value already matched is not re-encrypted, so a concurrent write for that key now survives rather than being overwritten - drop the lock-timeout constant and TSDoc left behind when the lock moved into the shared helper, and stop shadowing `finalEncrypted`
Collaborator
Author
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
Three fixes found by tracing review findings on the release PR back to their source.
deleteCredentialRecordtook none and ran outside a transaction, so a secret written concurrently was read before that write and silently dropped by the write-back.newKeys, and never creates the row. Both helpers already acceptexecutor, andsetWorkspaceSecrethas passed the transaction since that parameter landed; these four were never migrated. Folded the four copies of the lock into one helper, since this would have been the fifth.workflow_blocks.nameis NOT NULL so a block missingtype/namehas to be dropped, but it was dropped with no warning and no log line — a block with no edges vanished without a trace. The client sanitizer warns on the identical condition; this adds its server counterpart, into a warnings array the internal PUT, the v2 write and the importer already return.chatId, so it loaded the whole thing on every resumed turn and discarded it. Opt out there; the load stays the default because the copilot send path does consume it.The personal reconcile deliberately stays outside its transaction — it opens its own and takes the user-identity fence, so nesting it would have two transactions taking two locks in opposite orders. It reconciles against the stored keys, so a failure there is repaired by the next one rather than entrenched.
Type of Change
Testing
bun run lint,bun run check:audits(36/36),tsc --noEmit, and the affected suites (6045 tests). New assertions mutation-tested: removing the workspace lock, the personal lock, orexecutor: txeach turns exactly one test red.Checklist