Skip to content

fix(workflows): refuse operation edits that would overwrite a concurrent change - #9902

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/workflow-ops-lock-base-read
Nov 11, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/workflow-ops-lock-base-read

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Applying workflow operations read the stored graph outside the workflow row lock, applied the batch in memory, then wrote the whole graph back. Two concurrent POST /api/v2/workflows/{id}/operations calls (or v2 and Copilot, or v2 and someone editing in the builder) could silently drop each other's edits.
  • The write now re-reads the stored graph under the row lock and refuses with a 409 conflict if another edit committed after the base read. The caller re-reads and retries; nothing is lost silently.
  • Applying the batch stays outside the lock on purpose: it does credential lookups, MCP server discovery, and lint queries, which must not run while holding the row lock (and would trip the transaction tripwire). Only one indexed graph read and a fingerprint comparison run under the lock.
  • The fingerprint is order-insensitive (sorted keys, edges sorted by id) because the loader doesn't order rows.
  • Unchanged: Copilot's caller-supplied base graph, dry run (no lock, no write), atomic, skipped-item reporting, lint, minted IDs, audit, and PUT /state.

Test plan

  • New unit tests: a concurrent change is refused; the same rows in a different order still commit. Both fail with the guard reverted.
  • apply-workflow-operations and replace-normalized-state tests pass locally
  • CI type-check, audits, full suite

@vercel

vercel Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Nov 11, 2026 9:57pm UTC

Request Review

@greptile-apps

greptile-apps Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no blocking issues remain.

Summary

Workflow operation writes now check the stored graph under the row lock and refuse stale edits.

  • Workflow operation batches stop when the saved graph has changed.

Reviews (2) · Last reviewed commit: "fix(workflows): fingerprint only stored ..." · Reviewed by Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files

Confidence score: 4/5

  • In apply-workflow-operations.ts, large or legacy workflows can hold the row lock longer while the migrated graph is loaded and normalized. Keep the lock around the staleness read if possible.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/lib/workflows/application/apply-workflow-operations.ts">

<violation number="1" location="apps/sim/lib/workflows/application/apply-workflow-operations.ts:262">
P2: The row lock now covers the full migrated graph load, including conditional credential lookups and graph-wide normalization, rather than just a short staleness read. Large or legacy workflows can therefore hold the workflow lock during avoidable work and make concurrent edits wait; use a lock-safe raw/canonical fingerprint read that does not run the migration pipeline under the lock.</violation>
</file>

Turn on auto-fix | Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 files

Confidence score: 2/5

  • In apply-workflow-operations.ts, delegated edits with a supplied baseGraph can overwrite concurrent v2 or builder changes because they bypass the conflict guard. Capture and validate the stored fingerprint before writing.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/lib/workflows/application/apply-workflow-operations.ts">

<violation number="1" location="apps/sim/lib/workflows/application/apply-workflow-operations.ts:250">
P1: Delegated edits with a supplied `baseGraph` bypass the new conflict guard. Because the write path sees no `storedFingerprint`, a Copilot edit can still overwrite a concurrent v2 or builder change; capture and validate a stored fingerprint for this path (or reject stale caller bases) before writing.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Turn on auto-fix | Re-trigger cubic

This branch was previously deployed

1 inactive deployment
Preview — 95660685 Deployed Nov 11, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant