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]