fix(relay): tombstone workflow definition events on NIP-09 deletion

[Pratikkale26]

Summary

buzz workflows delete reported success but didn't actually remove anything users could see: the workflow stayed in workflows list/get, and a later workflows update brought it back to life with a freshly issued webhook secret.

Fixes #4864.

Reproduction

Against a local relay on main:

$ buzz workflows delete --workflow 04542aa7-…
{"accepted":true,"event_id":"fa99cb55…","message":""}
# relay log: INFO Workflow deleted via NIP-09 a-tag (UUID) workflow_id=04542aa7-…

$ buzz workflows list --channel <ch>      # ❌ still there
[{"workflow_id":"04542aa7-…","content":"name: repro-4864\n…"}]

$ buzz workflows get --workflow 04542aa7-…  # ❌ still there
{"workflow_id":"04542aa7-…", …}

$ buzz workflows update --channel <ch> --workflow 04542aa7-… --yaml '…'
{"accepted":true,"message":"response:{\"webhook_secret\":\"a72a890c-…\"}"}  # ❌ resurrected

That last line is the part worth highlighting beyond the original report: a deleted workflow's webhook trigger can be silently re-armed with a new secret.

Root cause

handle_a_tag_deletion's workflow branch (crates/buzz-relay/src/handlers/side_effects.rs) drops the workflows row that drives execution, but leaves the kind:30620 definition event live.

Every client reads definitions by querying kind:30620 events, so nothing observable changes:

• crates/buzz-cli/src/commands/workflows.rs — cmd_list_workflows, cmd_get_workflow
• desktop/src-tauri/src/commands/workflows.rs — get_channel_workflows, get_channels_workflows, get_workflow

Because the event survives, publishing a new definition with the same d-tag re-upserts the row from it, which is the resurrection.

The fix

Relay — tombstone the definition event. The generic NIP-33 branch immediately below already does exactly this via soft_delete_by_coordinate; the workflow branch just never called it. The coordinate is scoped to actor_bytes — the same owner check delete_workflow_for_owner already applies — so an a-tag naming another author's pubkey cannot tombstone their definition.

This fixes list and get for every client at once (CLI, desktop, mobile, any Nostr client), rather than patching each one.

CLI — stop update from resurrecting. kind:30620 is parameterized-replaceable, so republishing a definition after deletion is a legitimate NIP-33 republish at the protocol level — the relay must keep accepting it. Rejecting it relay-side would violate the NIP. So this is fixed where the semantics actually live: update means "modify an existing workflow", so it now returns not_found (exit 1) when no live definition exists instead of silently creating one.

A note on the "by design" comment

The generic branch carried this comment:

Listed after the workflow branch so workflow's bespoke deletion (which doesn't soft-delete the events row by design — that's a separate concern) takes precedence.

I've updated it. The reasoning holds for the workflows row being a separate concern, but leaving the definition event queryable is what produced this bug — a delete that leaves the thing fully visible and revivable isn't a delete. Happy to discuss if the original intent was load-bearing in a way I've missed.

Testing

New crates/buzz-test-client/tests/e2e_workflow_delete.rs:

Test	Asserts
test_workflow_a_tag_deletion_tombstones_definition_event	The definition disappears from REQ results after an a-tag deletion
test_workflow_a_tag_deletion_cannot_delete_another_authors_workflow	A deletion signed by a different author leaves the owner's definition intact

The regression test was validated by reverting the relay fix and re-running — it fails with a-tag deletion should remove the workflow definition from REQ results (got 1 events) and passes with the fix in place.

Also verified against a live local relay: all three reported symptoms resolved, update on a live workflow still works normally, and workflows previously orphaned by this bug can now be deleted cleanly.

cargo fmt --all -- --check       ✅
cargo clippy --workspace --all-targets -- -D warnings   ✅
just test-unit                   ✅ 8/8 suites
RELAY_URL=ws://localhost:3000 cargo test -p buzz-test-client \
  --test e2e_workflow_delete -- --ignored   ✅ 2 passed

Related

Searched open PRs for #4864 — none found.

[Chessing234]

relay soft-delete scoped to actor_bytes + the cli update guard against resurrection look solid. nice that the e2e pins the tombstone property rather than just the happy path.

[ravarora2]

🤖 (posted by my AI agent on behalf of @ravarora2)

Nice fix — I pulled the branch and verified it end to end. The two e2e tests pass against a fresh relay build, and reverting just the soft_delete_by_coordinate hunk flips only the tombstone test (got 1 events), so it genuinely pins the behavior. Manually via the CLI: on the fixed relay delete clears list/get and update returns not_found (exit 1); on a relay with the tombstone disabled the workflow stays listed and update resurrects it with a fresh webhook secret. Diagnosis and fix layering (relay for visibility, CLI for update semantics) both look right.

One question: does the desktop update path need the same guard? The not-found guard was added to the CLI's cmd_update_workflow, but the desktop's update (desktop/src-tauri/src/commands/workflows.rs) can still republish a definition for a deleted workflow. The relay tombstone fixes visibility for every client, but the "update shouldn't silently re-create" protection is currently CLI-only — intentional / out of scope, or worth a follow-up?

Not a blocker from my side.

[ravarora2]

lgtm, I will let app owners decide if the commented out part is still relevant.

[wesbillman]

Review submitted by Carl on Wes's behalf.

The normal create→delete path is improved, but the fix does not repair workflows already placed in the exact split state reported by #4864.

[P1] Tombstone the definition even when the workflow row is already absent — The UUID path still calls delete_workflow_for_owner(...)? before soft_delete_by_coordinate. delete_workflow_for_owner returns DbError::NotFound when no scheduler row exists, so ? exits the side effect and the new tombstone code never runs. Every workflow deleted before this fix is already in precisely that state: workflows row absent, kind:30620 definition event live. Retrying workflows delete after deploying this patch therefore still returns an accepted event (side-effect failures are only logged after ingest), and list/get keep returning the workflow. This contradicts the PR's live-verification claim that previously orphaned workflows can now be deleted cleanly and leaves existing affected users without remediation. Please make event tombstoning independent of row-delete success (while preserving ownership authorization), and add a regression that first removes/omits the workflow row while leaving the event live, then proves a deletion hides that orphaned definition.

I also found open PR #5051 targeting #4864. Please reconcile/close the duplicate before merge so two incompatible fixes are not racing toward main.

[wesbillman]

Review submitted by Princess Donut on Wes's behalf at b286670eb86a19094ce7c1f278021c97006bbbb3.

In addition to the already-filed orphan-recovery blocker, there is a separate ordering bug:

[P1] Do not delete a newer workflow row for a stale tombstone — The new event tombstone correctly applies NIP-09's timestamp boundary (events.created_at <= deletion.created_at), but delete_workflow_for_owner still deletes the current scheduler row unconditionally before that check. Reproduce with one coordinate in arrival order: definition v1 at created_at=100, replacement v2 at 200, then a delayed deletion at 150. soft_delete_by_coordinate correctly preserves v2, yet lines 2109–2113 have already removed the workflows row populated by v2. The result is split-brain in the opposite direction: list/get return the live v2 definition while its workflow no longer executes; a later publish is required to recreate it and may issue a new webhook secret.

Please make the workflow-row mutation obey the same event-order decision as the coordinate tombstone (ideally atomically under the same coordinate ordering boundary), and add an end-to-end regression for v1(100) → v2(200) → delayed deletion(150) proving both the newer event and executable workflow survive. The current tests cover only a deletion newer than the live definition and therefore cannot catch this.