OSAC-1273: acquire row lock in updatePoolCapacity to prevent counter drift

[akshaynadkarni]

Summary

OSAC-1273: Fix a race condition where the PublicIPPool.status.allocated counter becomes stale after deleting a PublicIP, making the pool undeletable.

Why

updatePoolCapacity() reads the pool record without a row lock, then writes the entire proto back with updated counters. A concurrent pool reconciler update can interleave and overwrite the counter change, leaving the pool with a stale allocated count. The pool delete precondition then fails with "N public IP(s) are still allocated" even when no IPs exist.

Testing

Deployed to edge22 and verified the full lifecycle:

# Create pool
grpcurl ... osac.private.v1.PublicIPPools/Create -> allocated: 0, available: 6

# Create IP
osac create publicip --name test-fix-ip --pool test-fix-pool -> ALLOCATED

# Pool shows allocated: 1, available: 5

# Delete IP
osac delete publicip test-fix-ip -> Deleted

# Pool now shows allocated: 0, available: 6 (FIXED, previously stayed at 1)

# Delete pool succeeds (previously failed with stale counter)
grpcurl ... osac.private.v1.PublicIPPools/Delete -> {}

Unit tests: 853/853 pass (ginkgo run ./internal/servers/).

Ticket

OSAC-1273

Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude

Summary by CodeRabbit

• Bug Fixes
  • Enhanced pool capacity update mechanism to improve data consistency during concurrent operations.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@akshaynadkarni: This pull request references OSAC-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

OSAC-1273: Fix a race condition where the PublicIPPool.status.allocated counter becomes stale after deleting a PublicIP, making the pool undeletable.

Why

updatePoolCapacity() reads the pool record without a row lock, then writes the entire proto back with updated counters. A concurrent pool reconciler update can interleave and overwrite the counter change, leaving the pool with a stale allocated count. The pool delete precondition then fails with "N public IP(s) are still allocated" even when no IPs exist.

Testing

Deployed to edge22 and verified the full lifecycle:

# Create pool
grpcurl ... osac.private.v1.PublicIPPools/Create -> allocated: 0, available: 6

# Create IP
osac create publicip --name test-fix-ip --pool test-fix-pool -> ALLOCATED

# Pool shows allocated: 1, available: 5

# Delete IP
osac delete publicip test-fix-ip -> Deleted

# Pool now shows allocated: 0, available: 6 (FIXED, previously stayed at 1)

# Delete pool succeeds (previously failed with stale counter)
grpcurl ... osac.private.v1.PublicIPPools/Delete -> {}

Unit tests: 853/853 pass (ginkgo run ./internal/servers/).

Ticket

OSAC-1273

Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 35f8e2ad-f6f2-48a3-9fc9-80211dd2ccd2

📥 Commits

Reviewing files that changed from the base of the PR and between 56a5161 and 34466cd.

📒 Files selected for processing (1)

• internal/servers/private_public_ips_server.go

---

Walkthrough

This PR adds pessimistic locking to the pool capacity update operation in updatePoolCapacity by enabling SetLock(true) on the PublicIPPoolDao read. This single-line change serializes concurrent access to prevent race conditions during allocated/available counter modifications.

Changes

Pool Capacity Update Locking

Layer / File(s)	Summary
Pool capacity update locking internal/servers/private_public_ips_server.go	updatePoolCapacity now calls SetLock(true) on the DAO query to acquire a lock on the PublicIPPool row before computing and persisting updated counters, serializing concurrent capacity adjustments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Risk Assessment

⚠️ Severity: Medium | Impact: High

Concurrency Risk Mitigated: This change addresses a race condition in the pool capacity update path. Without locking, concurrent calls to updatePoolCapacity can read stale allocated/available values, compute updates independently, and write conflicting counters back, resulting in lost updates and corrupted inventory state.

Lock Implementation Risk: The effectiveness of this fix depends on:

• Correct lock semantics in PublicIPPoolDao.SetLock() (row-level vs. table-level, transaction isolation level)
• No other code paths bypassing the lock during concurrent pool operations
• Deadlock prevention if other operations also acquire locks in different orders

Validation Required: Reviewers should verify that SetLock(true) is placed before the pool read (not after), that the lock is held through the update write, and that no other competing lock acquisition patterns exist in parallel code paths.

Possibly related PRs

• osac-project/fulfillment-service#466: Both PRs modify the pool capacity update locking behavior in updatePoolCapacity to prevent concurrent allocation conflicts through different locking strategies.

Suggested labels

do-not-merge/hold

Suggested reviewers

• jhernand
• adriengentil
• SiddarthR56

---

🔒 One lock to guard them all,
Capacity counts shall never fall,
Race conditions tamed with care,
Pool updates now are fair! ✨

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and specifically describes the main change: acquiring a row lock in updatePoolCapacity to prevent counter drift, which matches the core fix in the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	No hardcoded secrets found. The single-line change adds SetLock(true) to a DAO query—a legitimate database locking mechanism, not a secret or credential.
No-Weak-Crypto	✅ Passed	No weak crypto detected: no MD5, SHA1, DES, RC4, 3DES, Blowfish, or ECB usage. No custom crypto implementations or non-constant-time secret comparisons. Change is database locking only.
No-Injection-Vectors	✅ Passed	No injection vectors detected. SetLock(true) safely appends "FOR UPDATE" to parameterized SQL. poolID parameter uses parameterized queries via SetId() method.
Container-Privileges	✅ Passed	PR only modifies Go source code (adding SetLock for race condition fix), contains no container/K8s manifest changes. Check is not applicable to this database locking fix.
No-Sensitive-Data-In-Logs	✅ Passed	The PR introduces logging that only includes pool UUIDs and generic error objects—no passwords, tokens, keys, PII, session IDs, internal hostnames, or customer data are exposed in logs.
Ai-Attribution	✅ Passed	PR correctly attributes AI tool usage via "Assisted-by: Cursor/Claude" trailer in commit message. No Co-Authored-By trailer used for AI tool, which is correct. Red Hat attribution requirement met.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[akshaynadkarni]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akshaynadkarni, jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [akshaynadkarni,jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment