-
[OPIK-7159][OPIK-7029] [BE] fix: address post-merge review (Redis-independent status writes; configurable reaper lookback) (#7512)
frostbyte_neo 发布于
2026-07-21 10:02:50 +00:00 - [OPIK-7159][OPIK-7029] fix: address post-merge review feedback
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
- [OPIK-7159][OPIK-7029] fix: serialize optimization update under lock 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
- [OPIK-7159][OPIK-7029] fix: keep cancellation on the hard-fail path + 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
- [OPIK-7159][OPIK-7029] test: camelCase lock-test method names per backend 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
- [OPIK-7159][OPIK-7029] refactor: lock every optimization update, drop 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
Co-authored-by: Claude Opus 4.8 (1M context) noreply@anthropic.com
下载附件