Make background workers safe across control-plane replicas

[gciavarrini]

Make deferred-deletion cleanup safe when multiple control-plane instances share the same Postgres database.

Each replica leases SCHEDULED rows before publishing to the agent, so only one instance drives a given deletion at a time.

• Add deletion_claimed_until and ClaimPendingDeletions (optimistic on
  SQLite, FOR UPDATE SKIP LOCKED on Postgres)
• Wire the cleanup scheduler to claim before processing
• Document JetStream durable load-sharing for the status response consumer
• Add a subsystem test with control-plane-2 in compose

Provider health-check claiming is out of scope here: main uses the agent heartbeat monitor (MarkStaleUnavailable), which is already a single atomic update.

Fixes

https://redhat.atlassian.net/browse/FLPATH-4630

[qodo-code-review]

PR Summary by Qodo

Lease deferred deletions across control-plane replicas

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Lease scheduled deletions before replicas publish agent cleanup requests.
• Support PostgreSQL locking and optimistic SQLite claims with automatic lease recovery.
• Validate single-publisher behavior using a two-replica subsystem stack.

Diagram

graph TD
  R["CP Replicas"] --> S["Cleanup Scheduler"] --> C["Claim Store"] --> D[("Shared Postgres")]
  C --> P["Delete Publisher"] --> N["NATS JetStream"] --> A["Agent"]
  A --> N --> U["Durable Consumer"] --> D

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Control-plane leader election

• ➕ Prevents all background-worker duplication through a single active scheduler.
• ➕ Avoids adding per-row claim state.
• ➖ Serializes unrelated work behind one replica.
• ➖ Requires leader lifecycle and failover coordination.
• ➖ Reduces availability during election transitions.

2. PostgreSQL advisory locks

• ➕ Avoids adding a lease column to the service instance model.
• ➕ Provides database-coordinated mutual exclusion.
• ➖ Introduces PostgreSQL-specific lock and connection semantics.
• ➖ Provides poor parity with SQLite-based tests.
• ➖ Crash recovery and lock ownership are harder to observe operationally.

3. JetStream deletion work queue

• ➕ Uses native durable load-sharing and redelivery.
• ➕ Decouples scheduler execution from individual control-plane replicas.
• ➖ Requires redesigning deletion state and retry orchestration.
• ➖ Creates additional consistency concerns between database state and queued work.
• ➖ Substantially expands the scope of this fix.

Recommendation: DB-backed row leasing is the best fit because PostgreSQL remains the shared source of truth, replicas can process independent rows concurrently, and expired claims recover after worker failure. Leader election would over-serialize cleanup, while advisory locks and queue-based orchestration add portability or consistency costs.

Files changed (9) +355 / -36

Bug fix (4) +113 / -16

scheduler.goClaim deferred deletions before scheduler processing +12/-2

Claim deferred deletions before scheduler processing

• The cleanup scheduler now leases scheduled deletion rows for five minutes before processing them. Claim failures are logged separately from listing failures, making multi-replica execution database-coordinated rather than leader-based.

internal/sp/cleanup/scheduler.go

service_type_instance.goPersist deferred-deletion lease expiration +5/-4

Persist deferred-deletion lease expiration

• The service instance model gains a deletion_claimed_until timestamp used to exclude actively leased deletion rows from other replicas.

internal/sp/store/model/service_type_instance.go

dialect.goAdd PostgreSQL dialect detection +9/-0

Add PostgreSQL dialect detection

• Introduces a helper for selecting PostgreSQL-specific claiming behavior while retaining a portable fallback for SQLite and other dialects.

internal/sp/store/resource_manager/dialect.go

service_instance.goImplement atomic deletion row claiming +87/-10

Implement atomic deletion row claiming

• Adds ClaimPendingDeletions with PostgreSQL FOR UPDATE SKIP LOCKED and an optimistic conditional-update fallback. Claim state is initialized or cleared during deletion scheduling, retries, failures, and retry resets.

internal/sp/store/resource_manager/service_instance.go

Tests (3) +196 / -0

service_instance_test.goTest deletion claim exclusivity and expiration +50/-0

Test deletion claim exclusivity and expiration

• Adds store tests covering exclusion of already claimed rows, reclaiming expired leases, and enforcement of claim limits.

internal/sp/store/resource_manager/service_instance_test.go

ha_workers_test.goVerify HA deferred-deletion publishing +128/-0

Verify HA deferred-deletion publishing

• Adds a subsystem test that counts agent deletion requests across two replicas, verifies one cleanup publication per scheduler tick, acknowledges the deletion, and confirms tombstone visibility behavior.

test/subsystem/sp/ha_workers_test.go

setup_test.goRequire both control-plane replicas for HA tests +18/-0

Require both control-plane replicas for HA tests

• Adds configurable addressing for the second control-plane and a health-check helper that fails fast unless both replicas are reachable.

test/subsystem/sp/setup_test.go

Documentation (1) +4 / -0

consumer.goDocument durable consumer load-sharing semantics +4/-0

Document durable consumer load-sharing semantics

• Package documentation explains that replicas bind the same JetStream durable consumer and load-share status events without leader election.

internal/sp/consumer/consumer.go

Other (1) +42 / -20

docker-compose.yamlAdd a second control-plane test replica +42/-20

Add a second control-plane test replica

• Extracts shared control-plane environment settings into a YAML anchor and adds a second replica sharing PostgreSQL and NATS. Scheduler and health intervals are shortened for HA subsystem scenarios.

test/subsystem/sp/docker-compose.yaml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Replicas can publish the same deletion ✓ Resolved

Description

IncrementDeletionRetry clears deletion_claimed_until immediately after processOne publishes a
delete request, including after a successful publish, while the deletion remains SCHEDULED.
Because claim eligibility accepts a null lease, another replica's scheduler can reclaim the row
before the asynchronous agent acknowledgement arrives, publish the same delete request, and
increment the retry count again.

Code

internal/sp/store/resource_manager/service_instance.go[R432-435]

+			"retry_count":            gorm.Expr("retry_count + 1"),
+			"last_deletion_attempt":  now,
+			"deletion_claimed_until": nil,
		})

Evidence

The scheduler initially claims each eligible row for five minutes, but calls
IncrementDeletionRetry immediately after every publish attempt while the status remains
SCHEDULED awaiting asynchronous acknowledgement. That update sets the lease to null, and both
claim implementations accept scheduled rows whose claim is null or expired, so the row becomes
immediately eligible to other replicas instead of remaining excluded for the five-minute lease.

internal/sp/cleanup/scheduler.go[90-105]
internal/sp/cleanup/scheduler.go[151-166]
internal/sp/store/resource_manager/service_instance.go[396-435]
internal/sp/consumer/response_consumer.go[307-325]
internal/sp/store/resource_manager/service_instance.go[371-393]
internal/sp/store/resource_manager/service_instance.go[426-435]
internal/sp/cleanup/scheduler.go[151-165]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`IncrementDeletionRetry` releases a deletion's lease immediately after publishing while the row remains `SCHEDULED`. This allows another control-plane replica to reclaim and republish the same deletion before the asynchronous agent acknowledgement arrives, defeating the database claim and potentially incrementing retry accounting again.

## Fix Focus Areas
- internal/sp/store/resource_manager/service_instance.go[426-435]
- internal/sp/cleanup/scheduler.go[151-166]

## Recommended Fix
Do not clear `deletion_claimed_until` as part of recording a publish attempt or retry accounting. Preserve the claim until deletion is finalized or the worker intentionally abandons it, and make claim release or renewal atomic with the corresponding deletion state transition; rely on lease expiry for recovery after a crashed worker, or, if retries must occur sooner, add an explicit atomically maintained next-attempt or ownership mechanism that keeps the row excluded until the intended retry time.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Old requests block newer deletions ✓ Resolved

Description

ClaimPendingDeletions converts the scheduler's zero limit to 100 and always orders by the oldest
request, while successful attempts clear their claim without changing the SCHEDULED status. If
those first 100 requests receive no acknowledgments, every cycle selects them again; with unlimited
retries newer rows are never attempted, and with the default retry limit they wait through up to ten
cycles per older batch.

Code

internal/sp/store/resource_manager/service_instance.go[R362-364]

+	if limit <= 0 {
+		limit = 100
+	}

Evidence

Both claim implementations use ascending request time plus a hard limit, and retry accounting
immediately restores eligibility by nulling the claim. The scheduler only marks a row failed after
its configured retry threshold, while a non-positive threshold disables that check, proving that the
oldest batch can monopolize all future selections.

internal/sp/store/resource_manager/service_instance.go[358-380]
internal/sp/store/resource_manager/service_instance.go[396-423]
internal/sp/store/resource_manager/service_instance.go[426-435]
internal/sp/cleanup/scheduler.go[142-165]
internal/app/config.go[84-86]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The bounded oldest-first query repeatedly reclaims the same unacknowledged rows, preventing later scheduled deletions from entering a batch.

## Fix Focus Areas
- internal/sp/store/resource_manager/service_instance.go[361-364]
- internal/sp/store/resource_manager/service_instance.go[375-380]
- internal/sp/store/resource_manager/service_instance.go[426-435]

## Recommended Fix
Track when each deletion is next eligible and exclude attempted rows until that retry time, while allowing later scheduled rows into subsequent batches. Preserve deterministic ordering among currently eligible rows and add coverage with more than 100 unacknowledged deletions across multiple cycles.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Some deletions stall for five minutes ✓ Resolved

Description

ProcessPendingDeletions leases the whole batch before processing, but transient agent lookup
failures and context cancellation return without releasing those claims. Those rows cannot be
retried on the next one-minute cleanup cycle and instead remain unavailable until the fixed
five-minute lease expires, including rows never reached before the ten-second cycle timeout.

Code

internal/sp/cleanup/scheduler.go[R93-94]

+	now := time.Now()
+	pending, err := s.store.ServiceTypeInstance().ClaimPendingDeletions(ctx, now, now.Add(deletionClaimTTL), 0)

Evidence

The scheduler claims as many as 100 rows at once, then may return when its context is canceled;
agent lookup errors also return without any state update. The lease is five minutes, while
production defaults bound a cycle to ten seconds and schedule cycles every minute, so these paths
demonstrably skip several intended cycles.

internal/sp/cleanup/scheduler.go[22-24]
internal/sp/cleanup/scheduler.go[78-87]
internal/sp/cleanup/scheduler.go[100-105]
internal/sp/cleanup/scheduler.go[132-140]
internal/app/config.go[84-86]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Claims survive transient early exits and cancellation, delaying affected deletions until lease expiry even though the scheduler says they will retry on the next cycle.

## Fix Focus Areas
- internal/sp/cleanup/scheduler.go[93-105]
- internal/sp/cleanup/scheduler.go[132-140]
- internal/sp/store/resource_manager/service_instance.go[358-368]

## Recommended Fix
Add ownership-aware claim release and release any claimed row that exits before a retry or terminal transition. Avoid claiming more rows than the cycle can process, or explicitly release the unprocessed remainder when the cycle context ends.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

Review mode: 🧠 Deep: This is a concurrency-sensitive, cross-cutting change spanning database claiming, scheduler behavior, JetStream consumers, migrations/models, and multi-replica integration tests, with many independent logic sites where subtle defects could be missed in one pass.

Tip of the day

💡 Did you know, you can keep summaries lean with Findings visible per group, which tucks the rest behind a View link

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[jordigilh]

The five-minute deletion lease remains the only duplicate-dispatch guard after a successful publish, but it is not renewed while the asynchronous agent deletion is in flight. If the agent takes longer than five minutes to acknowledge, another replica can reclaim and republish the deletion. Each Publisher.publish call generates a fresh JetStream message ID, so the later publish is not deduplicated. Please renew the lease until acknowledgment or use a stable operation identifier/idempotent dispatch mechanism.