argoproj / argoproj/argo-workflows
Multi-controller sync: `CheckWorkflowExistence` deletes other controllers' pending queue entries
- Dominant language
- Go
- Stars
- 17k
- Forks
- 3.7k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 138
Description
### Pre-requisites
- [x] I have double-checked my configuration
- [x] I have tested with the `:latest` image tag (i.e. `quay.io/argoproj/workflow-controller:latest`) and can confirm the issue still exists on `:latest`. If not, I have explained why, **in detail**, in my description below.
- [x] I have searched existing issues and could not find a match for this bug
- [x] I'd like to contribute the fix myself (see [contributing guide](https://github.com/argoproj/argo-workflows/blob/main/docs/CONTRIBUTING.md))
### What happened? What did you expect to happen?
## Summary
When several controllers share a sync database, each controller's `CheckWorkflowExistence` garbage collector deletes the pending (`held=false`) state rows of every other controller, once a minute. The GC lists holder and waiter keys from the shared state table but checks them against the local informer only, so another controller's workflows always look deleted. `RemoveFromQueue` has no controller condition, so the delete goes through.
Held rows survive because `ReleaseHeld` filters on `controller`, so mutual exclusion still works. Lock handover and queue ordering do not.
Present on `main` (checked at e8414b593) and in the 3.7+ database sync code.
## Where it happens
`CheckWorkflowExistence` (`workflow/sync/sync_manager.go`) runs every minute and does, for each database lock:
1. `getCurrentHolders` / `getCurrentPending` query `sync_state` by lock name only (`GetCurrentState`, `util/sync/db/queries.go`), so the results include rows from every controller sharing the database.
2. Each key is checked with `workflowExists`, which is backed by the local cluster's informer. A workflow running under another controller is never there.
3. For each "missing" workflow it calls `release()` and `removeFromQueue()`. `release()` is a no-op on foreign rows because `ReleaseHeld` includes `controller = `. `RemoveFromQueue` (`util/sync/db/queries.go`) deletes by name and key only, so the foreign pending row is removed.
The GC predates the database backend. For in-memory locks every key it sees is local, so the informer check was valid. The shared state table broke that assumption.
## Impact
- **Missed handover.** Controller A deletes controller B's pending row, then A's workflow releases the lock. `notifyWaiters` reads an empty queue and wakes nobody. B's periodic `probeWaiting` reads the same table and also finds nothing. The row only comes back when something else reconciles B's workflow (informer resync, by default around 20 minutes), because re-adding happens inside `TryAcquire`. The lock sits free with a waiter parked on it.
- **Queue jumping.** `queueOrdered` picks the next holder from the rows that exist at that moment. Each controller only ever evicts other controllers' rows, since its own workflows pass the informer check. Under contention the controller that holds the lock tends to hand it to its own waiters, even when a waiter on another controller has been queued longer.
- **Churn.** Waiting workflows on other controllers have their row deleted every minute and re-inserted on their next reconcile.
## Reproduction sketch
Two controllers with distinct `controllerName` values sharing one sync database. Workflow A on controller 1 holds a database mutex; workflow B on controller 2 waits for it. Watch `sync_state`: B's pending row disappears within a minute (deleted by controller 1) and reappears when B reconciles. If A releases while the row is gone, B is not notified and can wait until the next informer resync.
## Suggested fix
Add a `controllerName` condition to `RemoveFromQueue`, mirroring `ReleaseHeld`. The existing callers (`Release`, `ReleaseAll`, the GC) only ever pass local keys, so that change preserves behavior.
## Note
Correctness here leans entirely on the `controller` filter in `ReleaseHeld`. If two controllers are configured with the same `controllerName` (nothing validates this, and the field defaults to empty), the GC deletes the other controller's held rows too and a database mutex can be held twice. That may deserve its own issue; happy to file it.
## Note
Found while investigating a cross-DC report of a database mutex being held by two workflows at once.
Happy for someone else to pick this up
### Version(s)
3.7+
### Paste a minimal workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflow that uses private images.
```YAML
N/A
```
### Logs from the workflow controller
```text
N/A
```
### Logs from in your workflow's wait container
```text
N/A
```
Contributor guide
Research direction
Start with CheckWorkflowExistence in workflow/sync/sync_manager.go and the GetCurrentState and RemoveFromQueue queries in util/sync/db/queries.go. Reproduce the two-controller case while watching sync_state, then verify that garbage collection preserves another controller's pending queue row and lock handover still works.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- backend-api-design, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100