fix(lock): replace invalid SET NX GET with SET NX + GET

[ruromero]

Summary

• Rewrite RedisLockService following the single-instance Redlock pattern
• Switch from high-level RedisDataSource to RedisAPI for protocol-level access with proper return values

Problems fixed

• tryAcquire: Used setGet (SET NX EX GET) which Redis rejects — GET cannot be combined with NX
• release: Used non-atomic GET + DEL — if the lock expires between the two calls, another instance's lock gets deleted

Changes

• Acquire: Single SET key value NX EX ttl via RedisAPI.set(), checking the OK response atomically
• Release: Lua script via EVAL for atomic owner-check-and-delete, preventing race conditions on lock expiry

References

• Redis Distributed Locks (Redlock)
• Redis SET command — NX/GET incompatibility

Test plan

• All 48 existing tests pass (including HardenedImageProviderTest)
• Deploy to stage and verify HardenedImageProvider refresh completes without Redis errors
• Verify concurrent lock acquisition across replicas

Fixes #645

🤖 Generated with Claude Code

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Reworks Redis distributed lock acquisition to avoid an invalid SET NX GET usage by separating the SET and GET operations, changing the lock acquisition check semantics accordingly.

Sequence diagram for updated RedisLockService.tryAcquire lock acquisition

sequenceDiagram
  participant Caller
  participant RedisLockService
  participant Redis

  Caller->>RedisLockService: tryAcquire(key, ttl)
  activate RedisLockService
  RedisLockService->>Redis: SET key instanceId NX EX ttl
  Redis-->>RedisLockService: [status reply]
  RedisLockService->>Redis: GET key
  Redis-->>RedisLockService: value
  RedisLockService-->>Caller: value == instanceId
  deactivate RedisLockService

Loading

File-Level Changes

Change	Details	Files
Fix Redis lock acquisition to avoid invalid SET NX GET syntax by splitting into SET NX EX followed by GET and adjusting the success condition.	Replace single setGet call that attempted SET with NX and EX plus GET with a plain SET using NX and EX options. Add a separate GET call after SET to read the current lock value. Change the acquisition condition from checking for a null previous value to verifying that the stored value matches this instance's ID.	src/main/java/io/github/guacsec/trustifyda/integration/lock/RedisLockService.java

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#645	Replace RedisLockService.tryAcquire use of setGet (SET ... NX EX GET) with a valid sequence using set (SET ... NX EX) followed by get, to avoid the Redis syntax error.	✅
#645	Preserve correct distributed lock semantics in RedisLockService.tryAcquire by verifying that the current instance owns the lock after attempting acquisition.	✅

Possibly linked issues

• RedisLockService.tryAcquire fails with Redis syntax error #645: PR changes RedisLockService.tryAcquire from SET NX EX GET to SET NX EX plus GET, fixing the syntax error.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• You can avoid the extra round-trip GET by checking the return value of lockCommands.set(...) (e.g., whether it returned "OK" or null) to determine lock acquisition instead of issuing a separate get.
• The new logic treats tryAcquire as successful if the current value already equals instanceId, even when NX prevented a new set; if the intent is to report only fresh acquisitions, consider basing the return value solely on the SET outcome.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- You can avoid the extra round-trip GET by checking the return value of `lockCommands.set(...)` (e.g., whether it returned `"OK"` or null) to determine lock acquisition instead of issuing a separate `get`.
- The new logic treats `tryAcquire` as successful if the current value already equals `instanceId`, even when `NX` prevented a new set; if the intent is to report only fresh acquisitions, consider basing the return value solely on the `SET` outcome.

## Individual Comments

### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/integration/lock/RedisLockService.java" line_range="52-53" />
<code_context>
   public boolean tryAcquire(String key, Duration ttl) {
-    String previous = lockCommands.setGet(key, instanceId, new SetArgs().nx().ex(ttl));
-    return previous == null;
+    lockCommands.set(key, instanceId, new SetArgs().nx().ex(ttl));
+    return instanceId.equals(lockCommands.get(key));
   }

</code_context>
<issue_to_address>
**issue (bug_risk):** Re-acquiring an existing lock held by the same instance will now incorrectly return true without refreshing the TTL.

With this change, if this instance already holds the lock, `SET NX` becomes a no-op but `GET` still returns `instanceId`, so `tryAcquire` now returns `true` without refreshing the TTL. Previously, `setGet(...nx...)` would have returned the existing value and caused `tryAcquire` to return `false`. This means callers can think they’ve re-acquired/extended the lock when the TTL is unchanged, allowing the lock to expire earlier than expected and be acquired by another instance. If you want the previous behavior, either treat “already held by this instance” as a failure or explicitly refresh the TTL in that case.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.19%. Comparing base (0d846da) to head (0217d79).

Files with missing lines	Patch %	Lines
.../trustifyda/integration/lock/RedisLockService.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #646   +/-   ##
=========================================
  Coverage     57.19%   57.19%           
  Complexity      878      878           
=========================================
  Files            92       92           
  Lines          5021     5021           
  Branches        691      690    -1     
=========================================
  Hits           2872     2872           
  Misses         1841     1841           
  Partials        308      308           

Flag	Coverage Δ
integration-tests	57.19% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../trustifyda/integration/lock/RedisLockService.java	0.00% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[a-oren]

LGTM