[OPIK-7159][OPIK-7029] [BE] fix: address post-merge review (Redis-independent status writes; configurable reaper lookback)#7512
Merged
Conversation
Two fixes from thiagohora's review of the merged PR #7428: - OptimizationService.update(): only acquire the distributed lock for a metadata update (the read-modify-write that can drop keys under a concurrent partial update). Status-only and name-only writes now persist lock-free, so a Redis blip can no longer 500 the worker's mark_completed / mark_error callback and leave a run non-terminal for the reaper to later mislabel ERROR. The worker already retries a metadata-carrying status write as a metadata-less one, and that fallback now persists lock-free. Adds OptimizationServiceUpdateLockTest covering both paths. - Stalled-run reaper: make the scan lookback margin (was a hardcoded Duration.ofDays(7) in OptimizationDAO) a validated, env-overridable config key (optimizationStalledReaper.lookbackMargin / OPTIMIZATION_STALLED_REAPER_LOOKBACK_MARGIN), like the sibling timeouts. Threaded through job -> service -> DAO; tests updated. Verified locally: backend compiles (JDK 25); reaper + new lock tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
2 tasks
Contributor
⏱️ pre-commit per-hook timing
⏭️ 40 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…with a Redis-blip fallback Adversarial code review of the previous "lock only when metadata != null" variant found it de-serialized concurrent status/name writers: a rename racing a status write (or the worker racing the reaper) could drop one column via the UPDATE_BY_ID INSERT...SELECT, and a dropped terminal status strands a finished run non-terminal for the reaper to later mislabel ERROR. Take the per-id lock for every update again (restoring serialization), but fall back to a lock-free apply on a lock-ACQUISITION RedisException so a Redis blip still cannot 500 the worker's mark_completed / mark_error callback or the reaper's own ERROR write (thiagohora's original [medium]). Only the acquisition surfaces RedisException, so the action's own 404/409 still propagate and it runs exactly once. Test rewritten to cover both the serialized and Redis-down fallback paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… review nits Address the re-review of PR #7512: - [high] Cancellation must not use the Redis-blip lock-free fallback. The worker learns of a cancel only from the Redis signal it polls (CancellationMonitor, no DB-status polling), so persisting CANCELLED while Redis is down would report success to the user while the run keeps executing to timeout and its terminal callback is dropped by the terminal-overwrite guard. update() now returns the lock-guarded write directly for CANCELLED (hard-fail), and only falls back lock-free for the writes that don't need Redis to reach anyone (worker mark_completed/mark_error, reaper ERROR, rename). - test: use the shared PodamFactoryUtils.newPodamFactory() fixture; add a name-only serialization case and a cancellation-hard-fail case; fix a comment typo. Verified: backend compiles (JDK 25); 4/4 lock tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
reviewed
Jul 18, 2026
…kend testing guide .agents/skills/opik-backend/testing.md prescribes camelCase test names (the methodUnderTest + scenario shape), not snake/double-underscore (review: baz). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… the Redis-blip fallback Per thiagohora's review: locks are lightweight and should protect every state change against data loss. Serialize all updates under the per-id lock and let a Redis outage surface the error rather than falling back to a lock-free write. This removes the earlier fallback's subtlety: the onErrorResume(RedisException -> action) re-subscribed applyUpdate, which was safe only while applyUpdate never surfaced a RedisException (a non-local invariant a future edit could break into a double write / double analytics event), and it sanctioned a lock-free lost-update path. A run left non-terminal by a failed write is covered by the stalled-run reaper, so protecting against data loss is the better trade. Also drops the CANCELLED special-case, now unnecessary. Test simplified to: every write serializes under the lock; a lock-acquisition failure surfaces the error. Verified: backend compiles (JDK 25); 3/3 lock tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
approved these changes
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Follow-up to #7428 (merged), addressing @thiagohora's post-merge review. Two backend fixes:
1.
OptimizationService.update()no longer hard-depends on Redis for every status change ([medium]).The distributed lock is now acquired only for a metadata update — the read-modify-write (
getById→ merge → insert) that can drop keys under a concurrent partial update. Status-only and name-only writes persist lock-free, so a Redis blip can no longer 500 the worker'smark_completed/mark_errorcallback (or the reaper's own ERROR write) and leave a run non-terminal for the reaper to later mislabelERROR— the exact failure this feature exists to prevent. The worker already retries a metadata-carrying status write as a metadata-less one on failure, and that fallback now persists lock-free.2. Reaper scan lookback margin is now configurable.
Was a hardcoded
Duration.ofDays(7)inOptimizationDAO; now a validated, env-overridable config keyoptimizationStalledReaper.lookbackMargin(OPTIMIZATION_STALLED_REAPER_LOOKBACK_MARGIN, default7d), alongside the siblinginitializedTimeout/runningTimeout/lockDuration. Threaded through job → service → DAO so ops can tune the scan floor (and skip-index pruning) without a redeploy.Change checklist
lookbackMarginadded as a validated, env-overridable config keyOptimizationServiceUpdateLockTestcovers both the locked (metadata) and lock-free (status-only) pathsIssues
OPIK-7159, OPIK-7029 — post-merge review follow-up to #7428.
Testing
OptimizationServiceUpdateLockTest(new): asserts a status-only update does not touchLockService, and a metadata update is serialized under it.OptimizationStalledReaperConfigTest,OptimizationStalledReaperJobTestpass with the newlookbackMarginfield.Documentation
Config documented inline in
config.yml(default, env var, and the reaper-downtime tradeoff). No user-facing docs affected.