[OPIK-7159] fix: recover stuck INITIALIZED optimization runs#7428
Conversation
Optimization runs could stall in INITIALIZED when the Python subprocess never advanced their status. Adds a 3-layer recovery: - backend: OptimizationStalledReaperJob + config marks stale runs as errored - python worker: mark-error on subprocess failure, structured studio errors - frontend: stall / error UI (RunErrorPanel) on the optimization page Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📋 PR Linter Failed❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the |
github-actions
Bot
added
python
Backend Tests - Integration Group 31 tests 0 ✅ 0s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 11 tests 0 ✅ 1s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Unit Tests1 910 tests 1 858 ✅ 1m 17s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
⏱️ pre-commit per-hook timing
⏭️ 38 skipped (no matching files changed)
|
Backend Tests - Integration Group 21 tests 0 ✅ 1s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1115 tests 7 ✅ 10s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 614 tests 6 ✅ 10s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1017 tests 8 ✅ 11s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 89 tests 0 ✅ 7s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1239 tests 31 ✅ 32s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1423 tests 14 ✅ 53s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1510 tests 0 ✅ 8s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 1687 tests 79 ✅ 45s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 48 tests 0 ✅ 27s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 58 tests 0 ✅ 24s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
Backend Tests - Integration Group 720 tests 12 ✅ 27s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| @Inject | ||
| public OptimizationStalledReaperJob(@NonNull OptimizationService optimizationService, | ||
| @NonNull LockService lockService, | ||
| @NonNull @Config("optimizationStalledReaper") OptimizationStalledReaperConfig config) { |
There was a problem hiding this comment.
Manual DI constructor breaks convention
OptimizationStalledReaperJob hand-writes an @Inject constructor for three final dependencies, so it drifts from the repo’s DI convention — should we switch to @RequiredArgsConstructor(onConstructor_ = @Inject) like AlertJob and .agents/skills/opik-backend/SKILL.md suggest?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/jobs/OptimizationStalledReaperJob.java
around lines 53-60, the class defines a manual @Inject constructor that only assigns
three final fields (OptimizationService, LockService, and the
@Config("optimizationStalledReaper") OptimizationStalledReaperConfig) and has no
additional setup. Refactor to follow the repo DI guideline by removing the handwritten
constructor and adding Lombok @RequiredArgsConstructor(onConstructor_ = @Inject) so the
constructor is generated automatically. If the repo’s convention (e.g., AlertJob in
the same package) uses a field-level @Config instead of a constructor parameter
annotation, move the @Config annotation to the config field accordingly so dependency
injection still binds the same config entry.
| @Override | ||
| @WithSpan | ||
| public Mono<Long> reconcileStalledStudioOptimizations(@NonNull Duration initializedTimeout, | ||
| @NonNull Duration runningTimeout, int batchSize) { | ||
| return optimizationDAO.findStalledStudioOptimizations(initializedTimeout, runningTimeout, batchSize) | ||
| // Sequential: stalled runs are rare and this keeps the reaper's DB/Redis footprint small. | ||
| .concatMap(stalled -> markStalledOptimizationAsError(stalled, initializedTimeout, runningTimeout)) | ||
| .reduce(0L, Long::sum); | ||
| } | ||
|
|
||
| /** | ||
| * Best-effort transition of a single stalled run to ERROR. Records the reason in the run's logs first |
There was a problem hiding this comment.
Reaper behavior untested
reconcileStalledStudioOptimizations now moves stalled Studio runs to ERROR and emits a system log before updating the record, but there’s no test for the public OptimizationStalledReaperJob path, so regressions in the thresholds, filtering, or log append call could slip through — should we add a focused regression test around findStalledStudioOptimizations that asserts the log sync and ERROR transition?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java around
lines 581-642, add regression tests for the new `reconcileStalledStudioOptimizations` +
`markStalledOptimizationAsError` behavior (DAO query → log sync via
`appendSystemLogLine` → update via `update(..., ERROR)` with the built reason, and
returning the correct transitioned count). Create a focused unit/integration test class
for the reaper path (the `OptimizationStalledReaperJob` entry point if it exists, or
directly exercising `reconcileStalledStudioOptimizations`) that mocks
`optimizationDAO.findStalledStudioOptimizations`, `optimizationDAO.getById`,
`logSyncService.appendSystemLogLine`, and asserts `update` is invoked only when the
current status is still in `CANCELLABLE_STATUSES`. Write the test as parameterized over
`initializedTimeout`/`runningTimeout` (or at least two statuses/threshold values) and
cover at least: (1) happy path where a RUNNING/INIT-stalled record becomes ERROR and one
log line is appended, and (2) skip path where the run is already
terminal/non-cancellable so it appends no log and returns 0; use Reactor `StepVerifier`
to assert the Mono result and verify mock interactions.
| for (let i = lines.length - 1; i >= 0; i--) { | ||
| if (ERROR_LINE_RE.test(lines[i])) return lines[i]; | ||
| if (lines[i].startsWith(SYSTEM_PREFIX)) { | ||
| return lines[i].slice(SYSTEM_PREFIX.length).trim() || null; | ||
| } |
There was a problem hiding this comment.
Missing structured error detail
According to the PR description, both the worker and stalled-run reaper append curated [System] lines, but this end-scan returns only the last one, so a stalled-run notice can overwrite earlier worker context and the UI drops user-facing error detail — should we collect every lines[i] that starts with SYSTEM_PREFIX before falling back to the generic string?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/OptimizationPage/runError.ts around lines 23-26, the
extractSystemNotice function currently scans backward and immediately returns the first
line starting with SYSTEM_PREFIX, which truncates earlier curated “[System]”
notices. Refactor extractSystemNotice to collect all matching system notice lines from
the log (after ANSI stripping/trim) and return them concatenated (e.g., joined with “;
”) so both the worker and reaper messages are surfaced in the UI. Keep the existing
fallback behavior to return null/GENERIC_RUN_ERROR when no SYSTEM_PREFIX lines are
present.
| record StalledOptimization(UUID id, String workspaceId, OptimizationStatus status) { | ||
| } |
There was a problem hiding this comment.
Internal DTO skips builder contract
StalledOptimization omits @Builder(toBuilder = true) and @NonNull on required fields, so findStalledStudioOptimizations() has to use the positional constructor and the DTO misses the backend's construction/null-safety convention — should we add those annotations, as .agents/skills/opik-backend/SKILL.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
62-63, update the internal carrier record `StalledOptimization` to follow the project
DTO/null-safety conventions: add Lombok `@Builder(toBuilder = true)` and mark its
required components (`id`, `workspaceId`, `status`) with `@NonNull`. Then, in the
`findStalledStudioOptimizations(...)` implementation around lines 1199-1202, replace the
positional `new StalledOptimization(...)` construction with the Lombok builder API so
required-field null-safety is enforced consistently with the rest of the backend.
| @Override | ||
| public Flux<StalledOptimization> findStalledStudioOptimizations(@NonNull Duration initializedTimeout, | ||
| @NonNull Duration runningTimeout, int limit) { | ||
| var details = "initializedTimeoutSeconds=%d, runningTimeoutSeconds=%d, limit=%d" | ||
| .formatted(initializedTimeout.toSeconds(), runningTimeout.toSeconds(), limit); | ||
| var template = FilterUtils.getSTWithLogComment(FIND_STALLED_STUDIO_OPTIMIZATIONS, | ||
| "find_stalled_studio_optimizations", "", "", details); | ||
| return Mono.from(connectionFactory.create()) | ||
| .flatMapMany(connection -> { | ||
| var statement = connection.createStatement(template.render()) | ||
| .bind("initialized_timeout_seconds", initializedTimeout.toSeconds()) | ||
| .bind("running_timeout_seconds", runningTimeout.toSeconds()) |
There was a problem hiding this comment.
Connection pool exhaustion
findStalledStudioOptimizations creates a raw R2DBC Connection with Mono.from(connectionFactory.create()) and never closes it, so the reaper job can drain the pool and later calls start failing or stalling — should we wrap the work in Mono.usingWhen(connectionFactory.create(), connection -> …, Connection::close) or close it in doFinally?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java around lines
1184-1203, the OptimizationDAOImpl#findStalledStudioOptimizations method creates a raw
R2DBC Connection via connectionFactory.create() but never closes it, which can drain the
connection pool. Refactor the reactive chain to manage the resource with
Mono.usingWhen(connectionFactory.create(), connection -> …, Connection::close) so the
connection is released on completion, error, or cancellation. Keep the existing template
rendering, bind calls, and row-to-StalledOptimization mapping unchanged, but ensure the
statement/flux execution is performed inside the usingWhen connection scope.
Backend Tests - Integration Group 1313 tests 0 ✅ 2m 20s ⏱️ For more details on these errors, see this check. Results for commit 06a3772. |
1 participant
Summary
Optimization runs could stall in
INITIALIZEDwhen the Python subprocess never advanced their status. This adds a 3-layer recovery:OptimizationStalledReaperJob+OptimizationStalledReaperConfigmark stale runs as errored.errors.py).RunErrorPanel) on the optimization page.Test plan
🤖 Generated with Claude Code