From e67b3b582262beddecf2b14df6044bdccd7589ff Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Sat, 24 Oct 2026 20:41:48 +0200 Subject: [PATCH] Reject workflow saves from a stale draft Update and publish now require expectedVersion, the workflow version the client's draft is based on. A mismatch returns 409 workflow_version_conflict before anything is written, so a second tab can no longer replace a save it never saw. The existing lock and compare-and-swap checks stay in place; the guarded update already matches on the loaded version, which makes the check atomic. The designer sends the version it synced from the server and advances it after each save or publish. On a conflict it keeps the draft, shows a message and stops autosave until the workflow is reloaded. CLI, MCP, the load-test client and the admin scripts send the version they read. Fixes #609 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- .../Install-ContinuousTest1Min.ps1 | 4 +- scripts/nodepilot_http.py | 1 + scripts/stress-test/launch-40x.ps1 | 1 + scripts/tech-demo/seed.ps1 | 4 + scripts/test-suite/Install-TestSuite.ps1 | 2 + scripts/test_nodepilot_admin_scripts.py | 12 ++- .../Controllers/WorkflowEditingController.cs | 8 +- .../Controllers/WorkflowsController.cs | 2 + src/NodePilot.Api/Dtos/WorkflowDtos.cs | 16 ++- src/NodePilot.Cli/Api/Dtos/WorkflowDtos.cs | 4 +- .../Workflow/WorkflowLifecycleCommands.cs | 2 +- src/NodePilot.Mcp/Api/Dtos/Dtos.cs | 4 +- src/NodePilot.Mcp/Tools/WorkflowEditTools.cs | 4 +- .../content/de/api/endpoints.md | 6 +- .../content/de/api/workflow-control.md | 6 +- .../content/en/api/endpoints.md | 6 +- .../content/en/api/workflow-control.md | 6 +- .../hooks/useWorkflowPersistence.test.tsx | 83 ++++++++++++---- .../src/hooks/useWorkflowPersistence.ts | 45 +++++++-- .../src/i18n/locales/de/editor.json | 1 + .../src/i18n/locales/en/editor.json | 1 + .../src/pages/WorkflowEditorPage.tsx | 2 +- .../Controllers/WorkflowPortabilityTests.cs | 2 +- .../Controllers/WorkflowsControllerTests.cs | 12 +-- .../Controllers/WorkflowsEditLockTests.cs | 98 +++++++++++++++++-- .../Api/NodePilotApiClientTests.cs | 4 +- .../LoadTests/LoadHarnessContractTests.cs | 3 +- .../NodePilot.LoadTests/NodePilotApiClient.cs | 10 +- tests/NodePilot.LoadTests/Seeder.cs | 4 +- .../Tools/WorkflowEditToolsTests.cs | 1 + 31 files changed, 277 insertions(+), 79 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0ed75277..57873944 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -151,7 +151,7 @@ Routen + Rollen-Gating stehen an den Controllern in `src/NodePilot.Api/Controlle ## Edit-Lifecycle (SCOrch-style Edit-Lock) -Workflows haben einen per-User-Edit-Lock (`CheckedOutByUserId` + `CheckedOutAt`). Mutierende Endpoints liefern `423 Locked` wenn Caller nicht Lock-Owner. `Disable` ist **nicht** lock-gegated (Incident-Kill-Switch). +Workflows haben einen per-User-Edit-Lock (`CheckedOutByUserId` + `CheckedOutAt`). Mutierende Endpoints liefern `423 Locked` wenn Caller nicht Lock-Owner. `PUT` und `publish` verlangen `expectedVersion`; ein veralteter Entwurf → `409 workflow_version_conflict`. `Disable` ist **nicht** lock-gegated (Incident-Kill-Switch). | Endpoint | Verhalten | |---|---| diff --git a/scripts/continuous-test-1min/Install-ContinuousTest1Min.ps1 b/scripts/continuous-test-1min/Install-ContinuousTest1Min.ps1 index df88e3b9..7ca8520d 100644 --- a/scripts/continuous-test-1min/Install-ContinuousTest1Min.ps1 +++ b/scripts/continuous-test-1min/Install-ContinuousTest1Min.ps1 @@ -73,6 +73,7 @@ foreach ($workflow in @($bundle.workflows)) { Write-Host "Erstellt: $($workflow.name)" $existing += $current if ($workflow.isEnabled -eq $true) { + $body.expectedVersion = $current.version $current = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/publish" -Body $body -Headers $headers Write-Host "Veroeffentlicht und aktiviert: $($workflow.name)" } @@ -80,7 +81,8 @@ foreach ($workflow in @($bundle.workflows)) { else { # Existing workflows require an edit lock. Publish saves the definition, enables it and # releases the lock in one step, so a re-run cannot leave a workflow half updated. - Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/lock" -Headers $headers | Out-Null + $current = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/lock" -Headers $headers + $body.expectedVersion = $current.version $current = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/publish" -Body $body -Headers $headers Write-Host "Aktualisiert und aktiviert: $($workflow.name)" } diff --git a/scripts/nodepilot_http.py b/scripts/nodepilot_http.py index d587a6e6..8fcf2321 100644 --- a/scripts/nodepilot_http.py +++ b/scripts/nodepilot_http.py @@ -72,6 +72,7 @@ def publish_workflow(http, identity, name, definition): status, workflow = http("POST", "/api/workflows", token=token, body=payload) if status != 201: raise RuntimeError(f"Cannot create workflow (HTTP {status})") + payload["expectedVersion"] = workflow["version"] status, published = http("POST", f"/api/workflows/{workflow['id']}/publish", token=token, body=payload) if status != 200: raise RuntimeError(f"Cannot publish workflow (HTTP {status}); no executions started") diff --git a/scripts/stress-test/launch-40x.ps1 b/scripts/stress-test/launch-40x.ps1 index 70760b2a..77b5a06a 100644 --- a/scripts/stress-test/launch-40x.ps1 +++ b/scripts/stress-test/launch-40x.ps1 @@ -44,6 +44,7 @@ if ($wf) { $body = $payload | ConvertTo-Json -Depth 50 $wf = Invoke-RestMethod -Method POST -Uri "$BaseUrl/api/workflows" -Headers $headers -ContentType 'application/json; charset=utf-8' -Body ([Text.Encoding]::UTF8.GetBytes($body)) } +$payload.expectedVersion = $wf.version $body = $payload | ConvertTo-Json -Depth 50 $wf = Invoke-RestMethod -Method POST -Uri "$BaseUrl/api/workflows/$($wf.id)/publish" -Headers $headers -ContentType 'application/json; charset=utf-8' -Body ([Text.Encoding]::UTF8.GetBytes($body)) Log "Workflow id=$($wf.id) name=$($wf.name)" diff --git a/scripts/tech-demo/seed.ps1 b/scripts/tech-demo/seed.ps1 index 2e49c2b7..eebe9926 100644 --- a/scripts/tech-demo/seed.ps1 +++ b/scripts/tech-demo/seed.ps1 @@ -214,6 +214,7 @@ if ($existingChild) { } $childResp = Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows" -Headers $authHeaders -Body $childReq $childId = $childResp.id + $childReq.expectedVersion = $childResp.version Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows/$childId/publish" -Headers $authHeaders -Body $childReq | Out-Null Write-Host "Created child workflow: $childId" -ForegroundColor Green } @@ -231,6 +232,7 @@ if ($existingMain) { } $mainResp = Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows" -Headers $authHeaders -Body $mainReq $mainId = $mainResp.id + $mainReq.expectedVersion = $mainResp.version Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows/$mainId/publish" -Headers $authHeaders -Body $mainReq | Out-Null Write-Host "Created main workflow: $mainId" -ForegroundColor Green } @@ -250,6 +252,7 @@ if ($includeXl) { } $xlResp = Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows" -Headers $authHeaders -Body $xlReq $xlId = $xlResp.id + $xlReq.expectedVersion = $xlResp.version Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows/$xlId/publish" -Headers $authHeaders -Body $xlReq | Out-Null Write-Host "Created XL workflow: $xlId" -ForegroundColor Green } @@ -270,6 +273,7 @@ if ($includePlanar) { } $planarResp = Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows" -Headers $authHeaders -Body $planarReq $planarId = $planarResp.id + $planarReq.expectedVersion = $planarResp.version Invoke-JsonApi -Method POST -Url "$BaseUrl/api/workflows/$planarId/publish" -Headers $authHeaders -Body $planarReq | Out-Null Write-Host "Created Planar workflow: $planarId" -ForegroundColor Green } diff --git a/scripts/test-suite/Install-TestSuite.ps1 b/scripts/test-suite/Install-TestSuite.ps1 index 1f068798..6e49cd7e 100644 --- a/scripts/test-suite/Install-TestSuite.ps1 +++ b/scripts/test-suite/Install-TestSuite.ps1 @@ -361,6 +361,7 @@ foreach ($entry in @($manifest.workflows)) { $current = Invoke-NodePilotJson -Method POST -Path '/api/workflows' -Body $body $existing += $current if ($wantEnabled) { + $body.expectedVersion = $current.version $null = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/publish" -Body $body Write-Host "created + published : $($wf.name)" } @@ -387,6 +388,7 @@ foreach ($entry in @($manifest.workflows)) { if (-not $lockedByMe) { $null = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/lock" } + $body.expectedVersion = $current.version if ($wantEnabled) { $null = Invoke-NodePilotJson -Method POST -Path "/api/workflows/$($current.id)/publish" -Body $body Write-Host "updated + published : $($wf.name)" diff --git a/scripts/test_nodepilot_admin_scripts.py b/scripts/test_nodepilot_admin_scripts.py index 8d2b3b5a..44df8ddc 100644 --- a/scripts/test_nodepilot_admin_scripts.py +++ b/scripts/test_nodepilot_admin_scripts.py @@ -68,12 +68,12 @@ def respond(self): elif self.path == "/api/workflows/wf" and self.command == "DELETE": code = 204 elif self.path == "/api/workflows/wf": - response = {"id": "wf", "name": "Stress-Test", "isEnabled": True, + response = {"id": "wf", "name": "Stress-Test", "isEnabled": True, "version": 3, "folderId": "existing-folder", "checkedOutByUserId": fixture.owner} elif self.path == "/api/workflows" and self.command == "POST": - code, response = 201, {"id": "wf", "name": body["name"], "checkedOutByUserId": "caller"} + code, response = 201, {"id": "wf", "name": body["name"], "version": 1, "checkedOutByUserId": "caller"} elif self.path == "/api/workflows/wf/lock": - code, response = (409 if fixture.owner == "another-user" else 200), {"id": "wf"} + code, response = (409 if fixture.owner == "another-user" else 200), {"id": "wf", "version": 3} elif self.path == "/api/workflows/wf/publish": code, response = fixture.publish_status, {"id": "wf", "name": "Stress-Test"} elif self.path == "/api/workflows/wf/execute": @@ -168,6 +168,11 @@ def test_own_lock_is_reused(self): self.assertEqual(0, self.run_python("stress-test/launch-40x.py")) self.assertFalse(any(path.endswith("/lock") for _, path, _ in self.calls)) + def test_publish_sends_the_version_it_read(self): + self.assertEqual(0, self.run_python("stress-test/launch-40x.py")) + publishes = [body for _, path, body in self.calls if path.endswith("/publish")] + self.assertEqual([3], [body["expectedVersion"] for body in publishes]) + def test_foreign_lock_and_failed_publish_never_launch(self): for owner, publish_status in (("another-user", 200), (None, 400)): self.owner, self.publish_status = owner, publish_status @@ -230,6 +235,7 @@ def test_demo_seed_publishes_every_new_workflow(self): creates = [body for method, path, body in self.calls if method == "POST" and path == "/api/workflows"] publishes = [body for _, path, body in self.calls if path.endswith("/publish")] self.assertEqual(4, len(creates)) + self.assertTrue(all(body.pop("expectedVersion") == 1 for body in publishes)) self.assertEqual(creates, publishes) self.assertTrue(all(isinstance(body["definitionJson"], str) for body in creates)) diff --git a/src/NodePilot.Api/Controllers/WorkflowEditingController.cs b/src/NodePilot.Api/Controllers/WorkflowEditingController.cs index 0e2b4767..6669e3eb 100644 --- a/src/NodePilot.Api/Controllers/WorkflowEditingController.cs +++ b/src/NodePilot.Api/Controllers/WorkflowEditingController.cs @@ -459,6 +459,12 @@ public async Task> Publish( if (await RequireWorkflowAccessAsync(workflow, NodePilot.Core.Interfaces.ResourceOp.Edit, ct) is { } d6) return d6; if (await EnsureWriteLockAsync(workflow, ct) is { } locked) return locked; + const string publishConflictMessage = + "Workflow was published concurrently. Reload the workflow and retry publish."; + // The compare-and-swap below also matches on workflow.Version, so this check is atomic. + if (request.ExpectedVersion != workflow.Version) + return await LostCompareAndSwapAsync(id, publishConflictMessage, ct); + var sizeError = ValidateDefinitionJson(request.DefinitionJson); if (sizeError is not null) return sizeError; if (NodePilot.Api.Security.WorkflowActivationValidation.Validate(request.DefinitionJson) is { } activationError) @@ -479,8 +485,6 @@ public async Task> Publish( // Publish through the shared compare-and-swap (see CompareAndSwapWorkflowAsync); the // setter chain additionally enables the workflow and releases the edit lock. - const string publishConflictMessage = - "Workflow was published concurrently. Reload the workflow and retry publish."; int updated; try { diff --git a/src/NodePilot.Api/Controllers/WorkflowsController.cs b/src/NodePilot.Api/Controllers/WorkflowsController.cs index 140d42f2..e01c4f66 100644 --- a/src/NodePilot.Api/Controllers/WorkflowsController.cs +++ b/src/NodePilot.Api/Controllers/WorkflowsController.cs @@ -511,6 +511,8 @@ public async Task Update(Guid id, UpdateWorkflowRequest request, if (await RequireWorkflowAccessAsync(workflow, ResourceOp.Edit, ct) is { } denied) return denied; if (await EnsureWriteLockAsync(workflow, ct) is { } locked) return locked; + // The guarded update below also matches on workflow.Version, so this check is atomic. + if (request.ExpectedVersion != workflow.Version) return await WorkflowMutationConflictAsync(id, ct); var lockOwnerId = this.GetCurrentUserId(); var sizeError = ValidateDefinitionJson(request.DefinitionJson); diff --git a/src/NodePilot.Api/Dtos/WorkflowDtos.cs b/src/NodePilot.Api/Dtos/WorkflowDtos.cs index 0a767a71..b7e34ad3 100644 --- a/src/NodePilot.Api/Dtos/WorkflowDtos.cs +++ b/src/NodePilot.Api/Dtos/WorkflowDtos.cs @@ -11,7 +11,15 @@ namespace NodePilot.Api.Dtos; /// chosen folder before persisting. /// public record CreateWorkflowRequest(string Name, string? Description, string DefinitionJson, Guid? FolderId = null); -public record UpdateWorkflowRequest(string Name, string? Description, string DefinitionJson); +/// +/// Body for PUT /api/workflows/{id}. is the workflow version +/// the client's draft is based on; a mismatch returns 409 so a stale draft never replaces a newer save. +/// +public record UpdateWorkflowRequest( + string Name, + string? Description, + string DefinitionJson, + [property: System.Text.Json.Serialization.JsonRequired] int ExpectedVersion); /// /// Body for PUT /api/workflows/{id}/concurrency-limit. Null means unlimited. @@ -183,4 +191,8 @@ public record WorkflowCapabilities(bool CanRead, bool CanRun, bool CanEdit, bool /// — publish is "save current draft + enable + unlock" in one atomic transaction, so it /// needs the full editable surface. /// -public record PublishWorkflowRequest(string Name, string? Description, string DefinitionJson); +public record PublishWorkflowRequest( + string Name, + string? Description, + string DefinitionJson, + [property: System.Text.Json.Serialization.JsonRequired] int ExpectedVersion); diff --git a/src/NodePilot.Cli/Api/Dtos/WorkflowDtos.cs b/src/NodePilot.Cli/Api/Dtos/WorkflowDtos.cs index 62555d9d..6b0c7c53 100644 --- a/src/NodePilot.Cli/Api/Dtos/WorkflowDtos.cs +++ b/src/NodePilot.Cli/Api/Dtos/WorkflowDtos.cs @@ -65,8 +65,8 @@ public sealed record WorkflowCapabilities( public sealed record CreateWorkflowRequest( string Name, string? Description, string DefinitionJson, Guid? FolderId = null); -public sealed record UpdateWorkflowRequest(string Name, string? Description, string DefinitionJson); -public sealed record PublishWorkflowRequest(string Name, string? Description, string DefinitionJson); +public sealed record UpdateWorkflowRequest(string Name, string? Description, string DefinitionJson, int ExpectedVersion); +public sealed record PublishWorkflowRequest(string Name, string? Description, string DefinitionJson, int ExpectedVersion); public sealed record SetWorkflowConcurrencyLimitRequest(int? MaxConcurrentExecutions); public sealed record RollbackRequest(string? Reason); diff --git a/src/NodePilot.Cli/Commands/Workflow/WorkflowLifecycleCommands.cs b/src/NodePilot.Cli/Commands/Workflow/WorkflowLifecycleCommands.cs index 8c5c4d8d..9af0542e 100644 --- a/src/NodePilot.Cli/Commands/Workflow/WorkflowLifecycleCommands.cs +++ b/src/NodePilot.Cli/Commands/Workflow/WorkflowLifecycleCommands.cs @@ -188,7 +188,7 @@ protected override async Task RunAsync(CommandContext _, WorkflowPublishSet var api = ClientFactory.Create(session); var w = await WorkflowResolver.ResolveAsync(api, settings.IdOrName, ct); - var req = new PublishWorkflowRequest(settings.Name ?? w.Name, settings.Description ?? w.Description, json); + var req = new PublishWorkflowRequest(settings.Name ?? w.Name, settings.Description ?? w.Description, json, w.Version); var published = await api.PublishWorkflowAsync(w.Id, req, ct); writer.Success($"Workflow [bold]{Markup.Escape(published.Name)}[/] published (Version {published.Version}, Enabled)."); return ExitCodes.Success; diff --git a/src/NodePilot.Mcp/Api/Dtos/Dtos.cs b/src/NodePilot.Mcp/Api/Dtos/Dtos.cs index ece99905..d1c1e83b 100644 --- a/src/NodePilot.Mcp/Api/Dtos/Dtos.cs +++ b/src/NodePilot.Mcp/Api/Dtos/Dtos.cs @@ -80,8 +80,8 @@ public sealed record WorkflowCapabilities( public sealed record CreateWorkflowRequest( string Name, string? Description, string DefinitionJson, Guid? FolderId = null); -public sealed record UpdateWorkflowRequest(string Name, string? Description, string DefinitionJson); -public sealed record PublishWorkflowRequest(string Name, string? Description, string DefinitionJson); +public sealed record UpdateWorkflowRequest(string Name, string? Description, string DefinitionJson, int ExpectedVersion); +public sealed record PublishWorkflowRequest(string Name, string? Description, string DefinitionJson, int ExpectedVersion); public sealed record SetWorkflowConcurrencyLimitRequest(int? MaxConcurrentExecutions); public sealed record RollbackRequest(string? Reason); diff --git a/src/NodePilot.Mcp/Tools/WorkflowEditTools.cs b/src/NodePilot.Mcp/Tools/WorkflowEditTools.cs index a7c8dec1..796ec47f 100644 --- a/src/NodePilot.Mcp/Tools/WorkflowEditTools.cs +++ b/src/NodePilot.Mcp/Tools/WorkflowEditTools.cs @@ -327,10 +327,10 @@ private async Task PersistAsync( { if (publish) { - var r = await ApiErrorMapper.Guard(() => _api.PublishWorkflowAsync(wf.Id, new PublishWorkflowRequest(name, description, definitionJson), ct)); + var r = await ApiErrorMapper.Guard(() => _api.PublishWorkflowAsync(wf.Id, new PublishWorkflowRequest(name, description, definitionJson, wf.Version), ct)); return new { published = true, workflowId = r.Id, version = r.Version, enabled = r.IsEnabled, notes }; } - await ApiErrorMapper.Guard(() => _api.UpdateWorkflowAsync(wf.Id, new UpdateWorkflowRequest(name, description, definitionJson), ct)); + await ApiErrorMapper.Guard(() => _api.UpdateWorkflowAsync(wf.Id, new UpdateWorkflowRequest(name, description, definitionJson, wf.Version), ct)); return new { saved = true, mode = "draft", workflowId = wf.Id, notes }; } diff --git a/src/nodepilot-docs-ui/content/de/api/endpoints.md b/src/nodepilot-docs-ui/content/de/api/endpoints.md index 25509697..8cfd33c2 100644 --- a/src/nodepilot-docs-ui/content/de/api/endpoints.md +++ b/src/nodepilot-docs-ui/content/de/api/endpoints.md @@ -69,18 +69,18 @@ curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflow curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X PUT "$NP/api/workflows/21f1c0d4-..." \ -H 'Content-Type: application/json' \ -d '{ "name": "Deploy App v2", "description": "updated", - "definitionJson": "{\"nodes\":[...],\"edges\":[...]}" }' -i # 204 + "definitionJson": "{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 3 }' -i # 204 # Publish (atomar), Duplicate, Cancel-All curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../publish" \ -H 'Content-Type: application/json' \ -d '{ "name": "Deploy App", "description": null, - "definitionJson": "{\"nodes\":[...],\"edges\":[...]}" }' + "definitionJson": "{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 4 }' curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../duplicate" curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../cancel-all" # {"total":3,"signalled":2} ``` -409-Lock-Konflikt-Body: `{"message":"Workflow is already locked by alice.","lockedByUserName":"alice","lockedAt":"..."}`. 409-Version-Konflikt: `{"code":"workflow_version_conflict","currentVersion":4}`. +409-Lock-Konflikt-Body: `{"message":"Workflow is already locked by alice.","lockedByUserName":"alice","lockedAt":"..."}`. 409-Version-Konflikt: `{"code":"workflow_version_conflict","currentVersion":4}`. `PUT` und `publish` verlangen `expectedVersion`, die Version, auf der der Entwurf basiert; weicht sie von der gespeicherten ab, antwortet der Server mit diesem 409 und überschreibt nichts. ## Versionen & Contract diff --git a/src/nodepilot-docs-ui/content/de/api/workflow-control.md b/src/nodepilot-docs-ui/content/de/api/workflow-control.md index a94e18b5..b9051a7f 100644 --- a/src/nodepilot-docs-ui/content/de/api/workflow-control.md +++ b/src/nodepilot-docs-ui/content/de/api/workflow-control.md @@ -55,7 +55,7 @@ Workflows haben einen per-User Edit-Lock (`CheckedOutByUserId` + `CheckedOutAt`) |---|---| | `POST /lock` | Atomar `IsEnabled=false` + Lock-Fields setzen. 409 wenn schon gelockt. | | `POST /unlock` | Lock-Fields auf null. `IsEnabled` bleibt unverändert. | -| `POST /publish` | Atomar: Save + `IsEnabled=true` + Unlock. Die übergebene Definition wird vorher validiert: ein schwaches Webhook-HMAC-Secret liefert `400 weak_webhook_hmac_secret`, eine für Quartz ungültige Cron-Expression liefert `400 invalid_cron_expression`. | +| `POST /publish` | Atomar: Save + `IsEnabled=true` + Unlock. Wie `PUT` verlangt es `expectedVersion`; ein veralteter Entwurf liefert `409 workflow_version_conflict`. Die übergebene Definition wird vorher validiert: ein schwaches Webhook-HMAC-Secret liefert `400 weak_webhook_hmac_secret`, eine für Quartz ungültige Cron-Expression liefert `400 invalid_cron_expression`. | | `POST /force-unlock` | Admin-only. Bricht fremden Lock. | ## UX-Flow @@ -148,13 +148,13 @@ curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflow curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X PUT "$NP/api/workflows/21f1c0d4-..." \ -H 'Content-Type: application/json' \ -d '{ "name":"Deploy App", "description":"wip", - "definitionJson":"{\"nodes\":[...],\"edges\":[...]}" }' -i # 204 + "definitionJson":"{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 3 }' -i # 204 # Publish — atomar Save + Enable + Unlock. Workflow ist wieder Productive. curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../publish" \ -H 'Content-Type: application/json' \ -d '{ "name":"Deploy App", "description":null, - "definitionJson":"{\"nodes\":[...],\"edges\":[...]}" }' + "definitionJson":"{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 4 }' # Exit ohne Publish: nur Unlock — Workflow bleibt Disabled curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../unlock" diff --git a/src/nodepilot-docs-ui/content/en/api/endpoints.md b/src/nodepilot-docs-ui/content/en/api/endpoints.md index 77de05d0..7e16293b 100644 --- a/src/nodepilot-docs-ui/content/en/api/endpoints.md +++ b/src/nodepilot-docs-ui/content/en/api/endpoints.md @@ -68,18 +68,18 @@ curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflow curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X PUT "$NP/api/workflows/21f1c0d4-..." \ -H 'Content-Type: application/json' \ -d '{ "name": "Deploy App v2", "description": "updated", - "definitionJson": "{\"nodes\":[...],\"edges\":[...]}" }' -i # 204 + "definitionJson": "{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 3 }' -i # 204 # Publish (atomic), duplicate, cancel-all curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../publish" \ -H 'Content-Type: application/json' \ -d '{ "name": "Deploy App", "description": null, - "definitionJson": "{\"nodes\":[...],\"edges\":[...]}" }' + "definitionJson": "{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 4 }' curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../duplicate" curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../cancel-all" # {"total":3,"signalled":2} ``` -The 409 lock-conflict body: `{"message":"Workflow is already locked by alice.","lockedByUserName":"alice","lockedAt":"..."}`. A 409 version conflict: `{"code":"workflow_version_conflict","currentVersion":4}`. +The 409 lock-conflict body: `{"message":"Workflow is already locked by alice.","lockedByUserName":"alice","lockedAt":"..."}`. A 409 version conflict: `{"code":"workflow_version_conflict","currentVersion":4}`. `PUT` and `publish` require `expectedVersion`, the version the draft is based on; when it differs from the stored one the server answers with this 409 and overwrites nothing. ## Versions & contract diff --git a/src/nodepilot-docs-ui/content/en/api/workflow-control.md b/src/nodepilot-docs-ui/content/en/api/workflow-control.md index 2335e651..666a9f8c 100644 --- a/src/nodepilot-docs-ui/content/en/api/workflow-control.md +++ b/src/nodepilot-docs-ui/content/en/api/workflow-control.md @@ -53,7 +53,7 @@ Workflows have a per-user edit lock (`CheckedOutByUserId` + `CheckedOutAt`). Mut |---|---| | `POST /lock` | Atomically sets `IsEnabled=false` + the lock fields. 409 if already locked. | | `POST /unlock` | Sets the lock fields to null. `IsEnabled` stays as it is. | -| `POST /publish` | Atomically: save + `IsEnabled=true` + unlock. The submitted definition is validated first: a weak webhook HMAC secret returns `400 weak_webhook_hmac_secret`, an invalid Quartz cron returns `400 invalid_cron_expression`. | +| `POST /publish` | Atomically: save + `IsEnabled=true` + unlock. Like `PUT` it requires `expectedVersion`; a stale draft returns `409 workflow_version_conflict`. The submitted definition is validated first: a weak webhook HMAC secret returns `400 weak_webhook_hmac_secret`, an invalid Quartz cron returns `400 invalid_cron_expression`. | | `POST /force-unlock` | Admin only. Breaks someone else's lock. | ## The UX flow @@ -146,13 +146,13 @@ curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflow curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X PUT "$NP/api/workflows/21f1c0d4-..." \ -H 'Content-Type: application/json' \ -d '{ "name":"Deploy App", "description":"wip", - "definitionJson":"{\"nodes\":[...],\"edges\":[...]}" }' -i # 204 + "definitionJson":"{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 3 }' -i # 204 # Publish — atomically save + enable + unlock. The workflow is productive again. curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../publish" \ -H 'Content-Type: application/json' \ -d '{ "name":"Deploy App", "description":null, - "definitionJson":"{\"nodes\":[...],\"edges\":[...]}" }' + "definitionJson":"{\"nodes\":[...],\"edges\":[...]}", "expectedVersion": 4 }' # Exit without publishing: unlock only — the workflow stays disabled curl -s -b cookie.jar -H "X-CSRF-Token: $(csrf_token)" -X POST "$NP/api/workflows/21f1c0d4-.../unlock" diff --git a/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx b/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx index f52627b0..a5066302 100644 --- a/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx +++ b/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx @@ -21,10 +21,14 @@ vi.mock('../../stores/confirmStore', async (importOriginal) => { }); import { confirmDialog } from '../../stores/confirmStore'; -vi.mock('../../api/client', () => ({ - api: { put: vi.fn(), post: vi.fn() }, -})); -import { api } from '../../api/client'; +vi.mock('../../api/client', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, api: { put: vi.fn(), post: vi.fn() } }; +}); +import { api, ApiError } from '../../api/client'; + +vi.mock('../../stores/toastStore', () => ({ toast: { error: vi.fn(), success: vi.fn() } })); +import { toast } from '../../stores/toastStore'; import { useWorkflowPersistence } from '../../hooks/useWorkflowPersistence'; import { useCanvasConnect } from '../../hooks/useCanvasConnect'; @@ -76,7 +80,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { canvasRef: { current: null }, screenToFlowPosition: (position) => position }); return { persistence, connect }; }, { wrapper: makeWrapper() }); - act(() => { result.current.persistence.syncFromServer('Saved workflow'); }); + act(() => { result.current.persistence.syncFromServer('Saved workflow', 4); }); act(() => { if (operation === 'quick-connect') result.current.connect.setQuickConnect({ fromNodeId: 'step-1', fromHandleId: null, screenX: 0, screenY: 0, flowPosition: { x: 20, y: 20 }, @@ -109,7 +113,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { { wrapper, initialProps: { label: 'revision-a' } }, ); - act(() => { expect(result.current.syncFromServer('Initial name')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('Initial name', 4)).toBe(true); }); act(() => { result.current.markDirty(); result.current.save(); }); await waitFor(() => expect(api.put).toHaveBeenCalledTimes(1)); expect(vi.mocked(api.put).mock.calls[0][1]).toMatchObject({ @@ -126,7 +130,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { }); // The stale success must neither mark the newer edit clean nor let a refetch replace it. expect(result.current.isDirty).toBe(true); - expect(result.current.syncFromServer('Stale server name')).toBe(false); + expect(result.current.syncFromServer('Stale server name', 4)).toBe(false); await act(async () => second.resolve({})); await waitFor(() => expect(result.current.isDirty).toBe(false)); @@ -143,7 +147,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { { wrapper, initialProps: { label: 'published-snapshot' } }, ); - act(() => { expect(result.current.syncFromServer('Initial name')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('Initial name', 4)).toBe(true); }); act(() => { result.current.rename('Published name'); result.current.publish(); }); await waitFor(() => expect(api.post).toHaveBeenCalledTimes(1)); rerender({ label: 'newer-local-revision' }); @@ -151,7 +155,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { await act(async () => publish.resolve({})); await waitFor(() => expect(result.current.isDirty).toBe(true)); - expect(result.current.syncFromServer('Stale server name')).toBe(false); + expect(result.current.syncFromServer('Stale server name', 4)).toBe(false); expect(result.current.name).toBe('Newer local name'); expect(vi.mocked(api.post).mock.calls[0][1]).toMatchObject({ name: 'Published name', @@ -159,10 +163,55 @@ describe('useWorkflowPersistence — revision-safe requests', () => { }); }); + it('sends the synced server version and advances it after each successful save', async () => { + vi.mocked(api.put).mockResolvedValue(undefined); + const { result } = renderPersistence(); + act(() => { expect(result.current.syncFromServer('Initial', 7)).toBe(true); }); + + act(() => { result.current.markDirty(); result.current.save(); }); + await waitFor(() => expect(result.current.isDirty).toBe(false)); + act(() => { result.current.markDirty(); result.current.save(); }); + await waitFor(() => expect(api.put).toHaveBeenCalledTimes(2)); + + expect(vi.mocked(api.put).mock.calls.map(([, body]) => (body as { expectedVersion: number }).expectedVersion)) + .toEqual([7, 8]); + }); + + it('publishes with the version the draft is based on', async () => { + vi.mocked(api.post).mockResolvedValue({ version: 6 }); + const { result } = renderPersistence(); + act(() => { expect(result.current.syncFromServer('Initial', 5)).toBe(true); }); + act(() => result.current.publish()); + await waitFor(() => expect(api.post).toHaveBeenCalledTimes(1)); + expect(vi.mocked(api.post).mock.calls[0][1]).toMatchObject({ expectedVersion: 5 }); + }); + + it('keeps a stale draft dirty and stops autosave after a version conflict', async () => { + vi.useFakeTimers(); + try { + vi.mocked(api.put).mockRejectedValue( + new ApiError('Workflow was updated concurrently.', 409, 'workflow_version_conflict')); + const { result } = renderPersistence(); + act(() => { expect(result.current.syncFromServer('Initial', 1)).toBe(true); }); + act(() => result.current.markDirty()); + + await act(async () => { await vi.advanceTimersByTimeAsync(5000); }); + expect(api.put).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith(expect.stringContaining('saved elsewhere')); + expect(result.current.isDirty).toBe(true); + + act(() => result.current.markDirty()); + await act(async () => { await vi.advanceTimersByTimeAsync(15000); }); + expect(api.put).toHaveBeenCalledTimes(1); + // A newer server state is adopted only when the editor syncs again (reload). + expect(result.current.syncFromServer('Newer elsewhere', 2)).toBe(false); + } finally { vi.useRealTimers(); } + }); + it('applies one async graph token at most once and exposes it to a same-tick Save', async () => { vi.mocked(api.put).mockResolvedValue({}); const { result } = renderPersistence(); - act(() => { expect(result.current.syncFromServer('Initial')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('Initial', 4)).toBe(true); }); const token = result.current.beginAsyncGraphEdit(); act(() => { @@ -189,11 +238,11 @@ describe('useWorkflowPersistence — revision-safe requests', () => { }), { wrapper, initialProps: { workflowId: 'wf-1' } }, ); - act(() => { expect(result.current.syncFromServer('First')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('First', 4)).toBe(true); }); const staleToken = result.current.beginAsyncGraphEdit(); rerender({ workflowId: 'wf-2' }); - act(() => { expect(result.current.syncFromServer('Second')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('Second', 4)).toBe(true); }); act(() => { expect(result.current.applyAsyncGraphEdit(staleToken!, [editableNode('stale-layout')], [])).toBe(false); }); @@ -204,7 +253,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { const publish = deferred(); vi.mocked(api.post).mockImplementationOnce(() => publish.promise); const { result } = renderPersistence(); - act(() => { expect(result.current.syncFromServer('Initial')).toBe(true); }); + act(() => { expect(result.current.syncFromServer('Initial', 4)).toBe(true); }); const token = result.current.beginAsyncGraphEdit(); act(() => result.current.publish()); @@ -226,15 +275,15 @@ describe('useWorkflowPersistence — revision-safe requests', () => { }), { wrapper: makeWrapper(), initialProps: { workflowId: 'wf-1' } }, ); - act(() => { result.current.syncFromServer('First'); result.current.markDirty(); result.current.save(); }); + act(() => { result.current.syncFromServer('First', 4); result.current.markDirty(); result.current.save(); }); await waitFor(() => expect(api.put).toHaveBeenCalledTimes(1)); act(() => result.current.publish()); // In-app navigation may continue after the user confirms discarding the old draft. rerender({ workflowId: 'wf-2' }); - act(() => { result.current.syncFromServer('Second'); }); + act(() => { result.current.syncFromServer('Second', 4); }); if (returnToOriginal) { rerender({ workflowId: 'wf-1' }); - act(() => { result.current.syncFromServer('First reopened'); }); + act(() => { result.current.syncFromServer('First reopened', 4); }); } await act(async () => saving.resolve({})); await waitFor(() => expect(result.current.isPublishing).toBe(false)); @@ -246,7 +295,7 @@ describe('useWorkflowPersistence — revision-safe requests', () => { vi.mocked(api.put).mockImplementationOnce(() => saving.promise); vi.mocked(api.post).mockResolvedValue({}); const { result, unmount } = renderPersistence(); - act(() => { result.current.syncFromServer('First'); result.current.markDirty(); result.current.save(); }); + act(() => { result.current.syncFromServer('First', 4); result.current.markDirty(); result.current.save(); }); await waitFor(() => expect(api.put).toHaveBeenCalledTimes(1)); act(() => result.current.publish()); unmount(); diff --git a/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts b/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts index f4b259e1..bd4a28b7 100644 --- a/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts +++ b/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts @@ -3,7 +3,7 @@ import { useMutation, useQueryClient } from '@tanstack/react-query'; import { useBlocker } from 'react-router'; import { useTranslation } from 'react-i18next'; import type { Node, Edge } from '@xyflow/react'; -import { api } from '../api/client'; +import { api, ApiError } from '../api/client'; import type { Workflow } from '../types/api'; import { stripRuntimeDefinition } from '../lib/workflowDefinitionSanitizer'; import { confirmDialog } from '../stores/confirmStore'; @@ -26,9 +26,15 @@ interface WorkflowSnapshot { name: string; description: string; definitionJson: string; + /** Server version the draft is based on; a mismatch makes the server answer 409. */ + expectedVersion: number; }; } +function isVersionConflict(err: unknown): boolean { + return err instanceof ApiError && err.status === 409 && err.code === 'workflow_version_conflict'; +} + export interface AsyncGraphEditToken { readonly workflowId: string; readonly generation: number; @@ -49,7 +55,11 @@ export function useWorkflowPersistence({ const [isDirty, setIsDirty] = useState(false); const [isSaving, setIsSaving] = useState(false); const [isPublishQueued, setIsPublishQueued] = useState(false); + // Set when the server rejected the draft as stale. Autosave stops so the draft is kept and + // the conflict is not retried every few seconds; a reload adopts the newer server state. + const [hasVersionConflict, setHasVersionConflict] = useState(false); const revisionRef = useRef(0); + const baseVersionRef = useRef(0); const dirtyRef = useRef(false); const savingRef = useRef(false); const publishRef = useRef(false); @@ -78,6 +88,15 @@ export function useWorkflowPersistence({ useLayoutEffect(() => () => { draftGenerationRef.current += 1; }, []); + const reportSaveError = (err: unknown) => { + if (isVersionConflict(err)) { + setHasVersionConflict(true); + toast.error(t('editor:versionConflict')); + return; + } + toast.error(t('common:saveFailed', { message: (err as Error).message })); + }; + const updateDirty = useCallback((value: boolean) => { dirtyRef.current = value; setIsDirty(value); @@ -93,14 +112,19 @@ export function useWorkflowPersistence({ name: draft.name, description: draft.description, definitionJson: JSON.stringify(stripRuntimeDefinition(draft.graph)), + expectedVersion: baseVersionRef.current, }, }; }, []); const saveMutation = useMutation({ mutationFn: (snapshot: WorkflowSnapshot) => api.put(`/workflows/${snapshot.workflowId}`, snapshot.body), - onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['workflows'] }); }, - onError: (err) => toast.error(t('common:saveFailed', { message: (err as Error).message })), + onSuccess: (_response, snapshot) => { + if (draftRef.current.workflowId === snapshot.workflowId) + baseVersionRef.current = snapshot.body.expectedVersion + 1; + queryClient.invalidateQueries({ queryKey: ['workflows'] }); + }, + onError: (err) => reportSaveError(err), }); const saveLoopRef = useRef | null>(null); @@ -139,14 +163,15 @@ export function useWorkflowPersistence({ }, [captureSnapshot, saveMutation, updateDirty]); const publishMutation = useMutation({ - mutationFn: (snapshot: WorkflowSnapshot) => api.post(`/workflows/${snapshot.workflowId}/publish`, snapshot.body), - onSuccess: (_response, snapshot) => { + mutationFn: (snapshot: WorkflowSnapshot) => api.post(`/workflows/${snapshot.workflowId}/publish`, snapshot.body), + onSuccess: (response, snapshot) => { + if (draftRef.current.workflowId === snapshot.workflowId) baseVersionRef.current = response.version; queryClient.invalidateQueries({ queryKey: ['workflows'] }); queryClient.invalidateQueries({ queryKey: ['workflow', snapshot.workflowId] }); if (draftRef.current.workflowId === snapshot.workflowId && revisionRef.current === snapshot.revision) updateDirty(false); }, - onError: (err) => toast.error(t('common:saveFailed', { message: (err as Error).message })), + onError: (err) => reportSaveError(err), onSettled: () => { publishRef.current = false; setIsPublishQueued(false); @@ -156,11 +181,11 @@ export function useWorkflowPersistence({ // Autosave five seconds after the newest edit. A running save performs its own follow-up. const autosaveTimer = useRef | null>(null); useEffect(() => { - if (!isDirty || isSaving || isPublishQueued || suspendAutoSave || !workflowId) return; + if (!isDirty || isSaving || isPublishQueued || hasVersionConflict || suspendAutoSave || !workflowId) return; if (autosaveTimer.current) clearTimeout(autosaveTimer.current); autosaveTimer.current = setTimeout(() => { void saveLatest().catch(() => undefined); }, 5000); return () => { if (autosaveTimer.current) clearTimeout(autosaveTimer.current); }; - }, [isDirty, isSaving, isPublishQueued, suspendAutoSave, nodes, edges, name, workflowId, saveLatest]); + }, [isDirty, isSaving, isPublishQueued, hasVersionConflict, suspendAutoSave, nodes, edges, name, workflowId, saveLatest]); useEffect(() => { const handler = (e: BeforeUnloadEvent) => { if (isDirty) e.preventDefault(); }; @@ -271,12 +296,14 @@ export function useWorkflowPersistence({ * Returns whether the page may atomically adopt the server name and DefinitionJson. Lifecycle * refetches during a save/publish or while dirty must not replace the locally owned canvas. */ - const syncFromServer = useCallback((serverName: string): boolean => { + const syncFromServer = useCallback((serverName: string, serverVersion: number): boolean => { const isNewWorkflow = syncedWorkflowIdRef.current !== workflowId; if (!isNewWorkflow && (dirtyRef.current || savingRef.current || publishRef.current)) return false; syncedWorkflowIdRef.current = workflowId; if (isNewWorkflow) revisionRef.current = 0; + baseVersionRef.current = serverVersion; + setHasVersionConflict(false); draftRef.current = { ...draftRef.current, name: serverName }; setName(serverName); updateDirty(false); diff --git a/src/nodepilot-ui/src/i18n/locales/de/editor.json b/src/nodepilot-ui/src/i18n/locales/de/editor.json index 7447fc14..8b383adf 100644 --- a/src/nodepilot-ui/src/i18n/locales/de/editor.json +++ b/src/nodepilot-ui/src/i18n/locales/de/editor.json @@ -137,6 +137,7 @@ "unsavedChanges": "Du hast nicht gespeicherte Änderungen.", "discardChanges": "Änderungen verwerfen?", "discardChangesConfirm": "Du hast nicht gespeicherte Änderungen. Wirklich verwerfen?", + "versionConflict": "Der Workflow wurde inzwischen an anderer Stelle gespeichert. Dein Entwurf bleibt erhalten, wird aber nicht mehr automatisch gespeichert. Lade den Workflow neu, um den aktuellen Stand zu sehen.", "fitView": "An Ansicht anpassen", "zoomIn": "Vergrößern", "zoomOut": "Verkleinern", diff --git a/src/nodepilot-ui/src/i18n/locales/en/editor.json b/src/nodepilot-ui/src/i18n/locales/en/editor.json index af34a22a..a5c4b19c 100644 --- a/src/nodepilot-ui/src/i18n/locales/en/editor.json +++ b/src/nodepilot-ui/src/i18n/locales/en/editor.json @@ -137,6 +137,7 @@ "unsavedChanges": "You have unsaved changes.", "discardChanges": "Discard changes?", "discardChangesConfirm": "You have unsaved changes. Really discard?", + "versionConflict": "This workflow was saved elsewhere in the meantime. Your draft is kept but no longer saved automatically. Reload the workflow to see the current version.", "fitView": "Fit view", "zoomIn": "Zoom in", "zoomOut": "Zoom out", diff --git a/src/nodepilot-ui/src/pages/WorkflowEditorPage.tsx b/src/nodepilot-ui/src/pages/WorkflowEditorPage.tsx index 75eec85b..6e9209fb 100644 --- a/src/nodepilot-ui/src/pages/WorkflowEditorPage.tsx +++ b/src/nodepilot-ui/src/pages/WorkflowEditorPage.tsx @@ -356,7 +356,7 @@ function WorkflowEditorInner() { useEffect(() => { if (workflow) { - if (!syncFromServer(workflow.name)) return; + if (!syncFromServer(workflow.name, workflow.version)) return; resetPasteCount(); pushRecentWorkflow(workflow.id); try { diff --git a/tests/NodePilot.Api.Tests/Controllers/WorkflowPortabilityTests.cs b/tests/NodePilot.Api.Tests/Controllers/WorkflowPortabilityTests.cs index 71a17edf..2710589d 100644 --- a/tests/NodePilot.Api.Tests/Controllers/WorkflowPortabilityTests.cs +++ b/tests/NodePilot.Api.Tests/Controllers/WorkflowPortabilityTests.cs @@ -109,7 +109,7 @@ public async Task UnresolvedReferences_AreNotRetained_AndCannotBeEnabled(string (await h.Workflows.Enable(saved.Id, Ct)).Should().BeOfType(); saved.CheckedOutByUserId = Guid.Parse("00000000-0000-0000-0000-000000000001"); await db.SaveChangesAsync(Ct); - var publish = await h.Editing.Publish(saved.Id, new(saved.Name, null, saved.DefinitionJson), Ct); + var publish = await h.Editing.Publish(saved.Id, new(saved.Name, null, saved.DefinitionJson, saved.Version), Ct); publish.Result.Should().BeOfType(); saved.DefinitionJson = saved.DefinitionJson.Replace(Guid.Empty.ToString(), Guid.NewGuid().ToString()); WorkflowResourceReferences.UnresolvedError(saved.DefinitionJson).Should().BeNull(); diff --git a/tests/NodePilot.Api.Tests/Controllers/WorkflowsControllerTests.cs b/tests/NodePilot.Api.Tests/Controllers/WorkflowsControllerTests.cs index e2a43686..ffd91885 100644 --- a/tests/NodePilot.Api.Tests/Controllers/WorkflowsControllerTests.cs +++ b/tests/NodePilot.Api.Tests/Controllers/WorkflowsControllerTests.cs @@ -499,7 +499,7 @@ public async Task Update_Exists_Returns204_IncrementsVersion() await db.SaveChangesAsync(); var h = NewController(db); - var request = new UpdateWorkflowRequest("Updated", "New desc", "{\"updated\":true}"); + var request = new UpdateWorkflowRequest("Updated", "New desc", "{\"updated\":true}", ExpectedVersion: 1); // Act var result = await h.Workflows.Update(workflow.Id, request, CancellationToken.None); @@ -537,7 +537,7 @@ public async Task Update_InvalidEdgeReference_Returns400_AndLeavesWorkflowUnchan var result = await h.Workflows.Update( workflow.Id, - new UpdateWorkflowRequest("Updated", null, invalidDefinition), + new UpdateWorkflowRequest("Updated", null, invalidDefinition, ExpectedVersion: 1), CancellationToken.None); result.Should().BeOfType(); @@ -553,7 +553,7 @@ public async Task Update_NotFound_Returns404() // Arrange var db = CreateContext(); var h = NewController(db); - var request = new UpdateWorkflowRequest("Updated", null, "{}"); + var request = new UpdateWorkflowRequest("Updated", null, "{}", ExpectedVersion: 1); // Act var result = await h.Workflows.Update(Guid.NewGuid(), request, CancellationToken.None); @@ -842,7 +842,7 @@ public async Task Update_SnapshotsOldDefinition_AndEmitsAudit() await h.Workflows.Update( workflow.Id, - new UpdateWorkflowRequest("Original v2", "updated", OneNodeDef), + new UpdateWorkflowRequest("Original v2", "updated", OneNodeDef, ExpectedVersion: 1), CancellationToken.None); // Live row advanced. @@ -876,7 +876,7 @@ public async Task Update_StoresHistoricDefinitionEncrypted_AndAuthorizedEditorRe await h.Workflows.Update( workflow.Id, - new UpdateWorkflowRequest("Legacy v2", null, """{"nodes":[],"edges":[]}"""), + new UpdateWorkflowRequest("Legacy v2", null, """{"nodes":[],"edges":[]}""", ExpectedVersion: 1), CancellationToken.None); db.ChangeTracker.Clear(); @@ -919,7 +919,7 @@ public async Task Update_WhenVersionSnapshotAlreadyExists_Returns409() var result = await h.Workflows.Update( workflow.Id, - new UpdateWorkflowRequest("Updated", null, OneNodeDef), + new UpdateWorkflowRequest("Updated", null, OneNodeDef, ExpectedVersion: 1), CancellationToken.None); result.Should().BeOfType(); diff --git a/tests/NodePilot.Api.Tests/Controllers/WorkflowsEditLockTests.cs b/tests/NodePilot.Api.Tests/Controllers/WorkflowsEditLockTests.cs index 798f1c19..f82b15d9 100644 --- a/tests/NodePilot.Api.Tests/Controllers/WorkflowsEditLockTests.cs +++ b/tests/NodePilot.Api.Tests/Controllers/WorkflowsEditLockTests.cs @@ -131,7 +131,7 @@ public async Task Update_WithoutAnyLock_Returns423() await db.SaveChangesAsync(); var h = NewHarness(db); - var result = await h.Workflows.Update(w.Id, new UpdateWorkflowRequest("X", null, "{}"), CancellationToken.None); + var result = await h.Workflows.Update(w.Id, new UpdateWorkflowRequest("X", null, "{}", ExpectedVersion: 1), CancellationToken.None); var obj = result.Should().BeOfType().Subject; obj.StatusCode.Should().Be(StatusCodes.Status423Locked); @@ -147,7 +147,7 @@ public async Task Update_WithForeignLock_Returns423() await db.SaveChangesAsync(); var h = NewHarness(db); // caller is OwnerId - var result = await h.Workflows.Update(w.Id, new UpdateWorkflowRequest("X", null, "{}"), CancellationToken.None); + var result = await h.Workflows.Update(w.Id, new UpdateWorkflowRequest("X", null, "{}", ExpectedVersion: 1), CancellationToken.None); var obj = result.Should().BeOfType().Subject; obj.StatusCode.Should().Be(StatusCodes.Status423Locked); @@ -186,7 +186,7 @@ await db.Workflows.Where(x => x.Id == w.Id) var h = NewHarness(db, audit, authz: authz); var result = await h.Workflows.Update( - w.Id, new UpdateWorkflowRequest("must-not-land", null, "{}"), CancellationToken.None); + w.Id, new UpdateWorkflowRequest("must-not-land", null, "{}", ExpectedVersion: 1), CancellationToken.None); result.Should().BeOfType(); db.ChangeTracker.Clear(); @@ -513,7 +513,7 @@ public async Task Publish_AsOwner_AtomicallySavesEnablesAndUnlocks() var result = await h.Editing.Publish( w.Id, - new PublishWorkflowRequest("Renamed", "new desc", newDef), + new PublishWorkflowRequest("Renamed", "new desc", newDef, ExpectedVersion: 1), CancellationToken.None); result.Result.Should().BeOfType(); @@ -529,6 +529,88 @@ public async Task Publish_AsOwner_AtomicallySavesEnablesAndUnlocks() audit.Calls.Should().ContainSingle(c => c.Action == "WORKFLOW_PUBLISHED"); } + [Fact] + public async Task Update_FromStaleTab_Returns409AndKeepsTheEarlierSave() + { + var db = CreateContext(); + await SeedUserAsync(db, OwnerId, "alice"); + var w = NewWorkflow(enabled: false, lockedBy: OwnerId); + db.Workflows.Add(w); + await db.SaveChangesAsync(); + var h = NewHarness(db); + const string tabADefinition = """{"nodes":[{"id":"log","type":"activity","position":{"x":0,"y":0},"data":{"activityType":"log","config":{"message":"from tab A"}}}],"edges":[]}"""; + + // Both tabs loaded version 1. Tab A saves first, tab B then saves its old snapshot. + var tabA = await h.Workflows.Update( + w.Id, new UpdateWorkflowRequest("W", null, tabADefinition, ExpectedVersion: 1), CancellationToken.None); + var tabB = await h.Workflows.Update( + w.Id, new UpdateWorkflowRequest("Renamed in tab B", null, "{}", ExpectedVersion: 1), CancellationToken.None); + + tabA.Should().BeOfType(); + var conflict = tabB.Should().BeOfType().Subject; + conflict.Value.Should().BeEquivalentTo(new { code = "workflow_version_conflict", currentVersion = 2 }); + db.ChangeTracker.Clear(); + var saved = (await db.Workflows.FindAsync(w.Id))!; + saved.DefinitionJson.Should().Be(tabADefinition); + saved.Name.Should().Be("W"); + saved.Version.Should().Be(2); + (await db.WorkflowVersions.CountAsync(v => v.WorkflowId == w.Id)).Should().Be(1); + } + + [Fact] + public async Task Update_WithStaleVersionAndForeignLock_Returns423() + { + var db = CreateContext(); + await SeedUserAsync(db, OtherUserId, "bob"); + var w = NewWorkflow(lockedBy: OtherUserId); + w.Version = 3; + db.Workflows.Add(w); + await db.SaveChangesAsync(); + + var result = await NewHarness(db).Workflows.Update( + w.Id, new UpdateWorkflowRequest("X", null, "{}", ExpectedVersion: 1), CancellationToken.None); + + result.Should().BeOfType().Which.StatusCode.Should().Be(StatusCodes.Status423Locked); + } + + [Fact] + public async Task Publish_FromStaleDraft_Returns409AndLeavesWorkflowLockedAndUnchanged() + { + var db = CreateContext(); + await SeedUserAsync(db, OwnerId, "alice"); + var w = NewWorkflow(enabled: false, lockedBy: OwnerId); + w.Version = 2; + db.Workflows.Add(w); + await db.SaveChangesAsync(); + var audit = new CapturingAuditWriter(); + + var result = await NewHarness(db, audit).Editing.Publish( + w.Id, new PublishWorkflowRequest("Stale", null, """{"nodes":[],"edges":[]}""", ExpectedVersion: 1), + CancellationToken.None); + + result.Result.Should().BeOfType() + .Which.Value.Should().BeEquivalentTo(new { code = "workflow_version_conflict", currentVersion = 2 }); + db.ChangeTracker.Clear(); + var saved = (await db.Workflows.FindAsync(w.Id))!; + saved.Name.Should().Be("W"); + saved.Version.Should().Be(2); + saved.IsEnabled.Should().BeFalse(); + saved.CheckedOutByUserId.Should().Be(OwnerId); + audit.Calls.Should().NotContain(c => c.Action == "WORKFLOW_PUBLISHED"); + } + + [Theory] + [InlineData(typeof(UpdateWorkflowRequest))] + [InlineData(typeof(PublishWorkflowRequest))] + public void SaveRequests_WithoutExpectedVersion_DoNotDeserialize(Type requestType) + { + var act = () => System.Text.Json.JsonSerializer.Deserialize( + """{"name":"W","description":null,"definitionJson":"{}"}""", requestType, + new System.Text.Json.JsonSerializerOptions(System.Text.Json.JsonSerializerDefaults.Web)); + + act.Should().Throw().WithMessage("*expectedVersion*"); + } + [Fact] public async Task Publish_WeakHmacWebhookSecret_IsRejectedAndRemainsLocked() { @@ -551,7 +633,7 @@ public async Task Publish_WeakHmacWebhookSecret_IsRejectedAndRemainsLocked() var result = await NewHarness(db).Editing.Publish( workflow.Id, - new PublishWorkflowRequest("Unsafe", null, weakHmacDefinition), + new PublishWorkflowRequest("Unsafe", null, weakHmacDefinition, ExpectedVersion: 1), CancellationToken.None); var badRequest = result.Result.Should().BeOfType().Subject; @@ -582,7 +664,7 @@ public async Task Publish_WhenVersionSnapshotAlreadyExists_Returns409() var result = await h.Editing.Publish( w.Id, - new PublishWorkflowRequest("Renamed", "new desc", "{}"), + new PublishWorkflowRequest("Renamed", "new desc", "{}", ExpectedVersion: 1), CancellationToken.None); result.Result.Should().BeOfType(); @@ -610,7 +692,7 @@ public async Task Publish_UniqueViolationWithNonEnglishProviderMessage_Returns40 .Options); var result = await NewHarness(db).Editing.Publish( - w.Id, new PublishWorkflowRequest("Renamed", null, "{}"), CancellationToken.None); + w.Id, new PublishWorkflowRequest("Renamed", null, "{}", ExpectedVersion: 1), CancellationToken.None); result.Result.Should().BeOfType(); } @@ -643,7 +725,7 @@ public async Task Publish_WithoutLock_Returns423() var h = NewHarness(db); var result = await h.Editing.Publish( w.Id, - new PublishWorkflowRequest("X", null, "{}"), + new PublishWorkflowRequest("X", null, "{}", ExpectedVersion: 1), CancellationToken.None); var obj = result.Result.Should().BeOfType().Subject; diff --git a/tests/NodePilot.Cli.Tests/Api/NodePilotApiClientTests.cs b/tests/NodePilot.Cli.Tests/Api/NodePilotApiClientTests.cs index 018d6c15..77daa613 100644 --- a/tests/NodePilot.Cli.Tests/Api/NodePilotApiClientTests.cs +++ b/tests/NodePilot.Cli.Tests/Api/NodePilotApiClientTests.cs @@ -198,7 +198,7 @@ public async Task PublishWorkflowAsync_PostsBody() .RespondWith(Response.Create().WithStatusCode(200).WithBody(SampleSingleBody(id, "Build"))); var w = await _client.PublishWorkflowAsync(id, - new PublishWorkflowRequest("Build", "desc", "{\"nodes\":[]}"), CancellationToken.None); + new PublishWorkflowRequest("Build", "desc", "{\"nodes\":[]}", ExpectedVersion: 1), CancellationToken.None); w.Id.Should().Be(id); } @@ -210,7 +210,7 @@ public async Task PublishWorkflowAsync_Forbidden_ThrowsForbidden() .RespondWith(Response.Create().WithStatusCode(403)); var ex = await Assert.ThrowsAsync(() => - _client.PublishWorkflowAsync(id, new PublishWorkflowRequest("x", null, "{}"), CancellationToken.None)); + _client.PublishWorkflowAsync(id, new PublishWorkflowRequest("x", null, "{}", ExpectedVersion: 1), CancellationToken.None)); ex.IsForbidden.Should().BeTrue(); } diff --git a/tests/NodePilot.Engine.Tests/LoadTests/LoadHarnessContractTests.cs b/tests/NodePilot.Engine.Tests/LoadTests/LoadHarnessContractTests.cs index da47f2c9..33156474 100644 --- a/tests/NodePilot.Engine.Tests/LoadTests/LoadHarnessContractTests.cs +++ b/tests/NodePilot.Engine.Tests/LoadTests/LoadHarnessContractTests.cs @@ -58,9 +58,10 @@ public async Task Seeding_PublishesEveryWorkflowBeforeReturningExecutableIds() { var id = Guid.NewGuid(); created.Add(id, (name, definition)); - return Json(new { id }); + return Json(new { id, version = 1 }); } Assert.EndsWith("/publish", request.RequestUri.AbsolutePath); + Assert.Equal(1, body.RootElement.GetProperty("expectedVersion").GetInt32()); var workflowId = Guid.Parse(request.RequestUri.Segments[^2].Trim('/')); Assert.Equal(created[workflowId], (name, definition)); // A parent may only become executable once its named child is published. diff --git a/tests/NodePilot.LoadTests/NodePilotApiClient.cs b/tests/NodePilot.LoadTests/NodePilotApiClient.cs index ad1f897a..7510755c 100644 --- a/tests/NodePilot.LoadTests/NodePilotApiClient.cs +++ b/tests/NodePilot.LoadTests/NodePilotApiClient.cs @@ -44,19 +44,21 @@ public async Task LoginAsync(string username, string password, CancellationToken _http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", _token); } - public async Task CreateWorkflowAsync(string name, string definitionJson, CancellationToken ct = default) + public async Task<(Guid Id, int Version)> CreateWorkflowAsync(string name, string definitionJson, CancellationToken ct = default) { // Mirror of CreateWorkflowRequest(Name, Description, DefinitionJson) var req = new { name, description = (string?)null, definitionJson }; using var resp = await _http.PostAsJsonAsync("api/workflows", req, ct); resp.EnsureSuccessStatusCode(); using var doc = await JsonDocument.ParseAsync(await resp.Content.ReadAsStreamAsync(ct), cancellationToken: ct); - return doc.RootElement.GetProperty("id").GetGuid(); + return (doc.RootElement.GetProperty("id").GetGuid(), doc.RootElement.GetProperty("version").GetInt32()); } - public async Task PublishWorkflowAsync(Guid id, string name, string definitionJson, CancellationToken ct = default) + public async Task PublishWorkflowAsync( + Guid id, string name, string definitionJson, int expectedVersion, CancellationToken ct = default) { - using var resp = await _http.PostAsJsonAsync($"api/workflows/{id}/publish", new { name, definitionJson }, ct); + using var resp = await _http.PostAsJsonAsync( + $"api/workflows/{id}/publish", new { name, definitionJson, expectedVersion }, ct); resp.EnsureSuccessStatusCode(); } diff --git a/tests/NodePilot.LoadTests/Seeder.cs b/tests/NodePilot.LoadTests/Seeder.cs index dfbf0020..73ab5b2d 100644 --- a/tests/NodePilot.LoadTests/Seeder.cs +++ b/tests/NodePilot.LoadTests/Seeder.cs @@ -15,8 +15,8 @@ public static async Task SeedAsync(NodePilotApiClient client, LoadTe async Task CreatePublishedAsync(string name, string definition) { - var id = await client.CreateWorkflowAsync(name, definition, ct); - await client.PublishWorkflowAsync(id, name, definition, ct); + var (id, version) = await client.CreateWorkflowAsync(name, definition, ct); + await client.PublishWorkflowAsync(id, name, definition, version, ct); return id; } diff --git a/tests/NodePilot.Mcp.Tests/Tools/WorkflowEditToolsTests.cs b/tests/NodePilot.Mcp.Tests/Tools/WorkflowEditToolsTests.cs index 2fd7f3b3..d6f780fc 100644 --- a/tests/NodePilot.Mcp.Tests/Tools/WorkflowEditToolsTests.cs +++ b/tests/NodePilot.Mcp.Tests/Tools/WorkflowEditToolsTests.cs @@ -61,6 +61,7 @@ public async Task PublishWorkflow_RestoresRealSecret_NotTheAgentsMask() var body = api.Server.LogEntries.Last(e => e.RequestMessage!.Path.EndsWith("/publish")).RequestMessage!.Body ?? ""; body.Should().Contain("real-secret-value"); // restored from the current version body.Should().NotContain("\"***\""); // the mask was NOT written over the real secret + body.Should().Contain("\"expectedVersion\":1"); // the version the merge was based on } [Fact]