Repository navigation
fix(backend): late corrections reach a running result, and finalize failures always free the worker - #295
fix(backend): late corrections reach a running result, and finalize failures always free the worker#295obvious-autobuild[bot] wants to merge 2 commits into
Conversation
…aises A background finalize worker that failed wrote the failure to the capture record and emitted the terminal event under the finish lock, then cleared its slot and settled its Future only after that write returned. When the publication itself raised - a transient state outage makes put_capture fail - the exception escaped the thread with the slot busy and the Future unsettled: later finalize retries answered processing forever with no worker ever starting again, and drain_finalize hung waiting on a Future nobody would settle. The failure write keeps its lock; the slot release and the Future settlement moved into a finally path, so the worker's original error always reaches the client and a finalize retry starts a new worker. Regression: test_failure_publication_error_releases_the_worker_slot drives both the work and its publication into an outage, asserts the drained error is the original one, the slot is free, and a retried finalize runs the orchestrator to a terminal capture state. Co-authored-by: Sam Gu <sgu07966@gmail.com>
…esult on_confirmation left a pending or running run alone on the assumption its result stage had not consumed the corrected reads yet. The window is real: verdict_ready is decided before the result stage finishes uploading, so a confirmation can arrive after the stage hydrated its inputs. The hook saved the corrected reads and returned; the run closed done with the pre-correction meter, and a replayed confirmation short-circuited upstream at the route's fingerprint check, so nothing ever recomposed the served payload. The hook now records a pending-correction marker when the running stages are criteria or result (earlier arrivals are applied by the reads stage itself), and the coordinator consumes the marker before marking the run done - resetting criteria and result once through the same reset the done-run recomposition uses. The consumed marker keeps a repeated finish or a replayed hook call from resetting twice, and the done-path recomposition's stale-marker delete uses the blob namespace, matching how stateio put_json stores it. Regression: test_correction_during_live_result_stage_recomposes_once gates the result stub after it reads its inputs, lands a correction and its replay in that window, and asserts one recomposition, a consumed marker, and the corrected meter in the served payload; test_confirmation_replay_recomposes_once pins the route's replay short-circuit to a single recomposition. Co-authored-by: Sam Gu <sgu07966@gmail.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add BaseScanning to your team's repositories. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1df5b44158
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| finally: | ||
| _clear_slot(capture_id) | ||
| future.set_exception(exc) |
There was a problem hiding this comment.
If another finalize request arrives while this worker is handling the exception, _kick sets the slot's again flag and returns a 202. This unconditional _clear_slot then clears that flag without starting another worker, so when failure publication also failed the capture can remain in processing with no worker and the request that was meant to retry it is lost. Preserve the queued retry or explicitly kick a new worker after releasing the slot.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [High impact] Fixes worker slot release and state publication in finalize failure path. Not ready to merge: correction recovery can lose pending work, and both earlier correction findings remain unresolved.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| live = False | ||
| for stage in ("criteria", "result"): | ||
| rec = services.state.get_stage(run.run_id, stage) # type: ignore[attr-defined] | ||
| if rec is not None and rec.status == "running": | ||
| live = True | ||
| if not live: | ||
| return | ||
| put_json( | ||
| services.state, # type: ignore[attr-defined] | ||
| pending_correction_key(run.run_id), | ||
| {"captureId": capture_id, "recordedAt": services.clock.iso()}, # type: ignore[attr-defined] |
There was a problem hiding this comment.
After on_confirmation reads a running run, the coordinator can finish the result and check for a marker before _record_pending_correction writes it. The hook then either finds no running stage and returns, or writes a marker that later ticks will never consume because the run is already done.
The saved answer never reaches the result, and the route skips repeated confirmations. Make recording the correction and closing the run share one lock or an atomic state change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/api.py
Line: 275-285
Comment:
**Accepted corrections can stay stale**
After `on_confirmation` reads a running run, the coordinator can finish the result and check for a marker before `_record_pending_correction` writes it. The hook then either finds no running stage and returns, or writes a marker that later ticks will never consume because the run is already done.
The saved answer never reaches the result, and the route skips repeated confirmations. Make recording the correction and closing the run share one lock or an atomic state change.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if self._consume_pending_correction(run): | ||
| return |
There was a problem hiding this comment.
The first live pass returns here before _verify_final_event records final_event_seq. If that pass emitted verdict_ready before the confirmation, the recomposed pass emits another final event. At the second finish, _verify_final_event sees two finals with no saved sequence, emits result_no_event, and forces the capture to manual_review.
Unlike the completed-run path, this path has no first close to record the sequence. Account for the first pass's final event before resetting, and test that live recomposition does not emit a false failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/coordinator.py
Line: 613-614
Comment:
**Successful corrections can report failure**
The first live pass returns here before `_verify_final_event` records `final_event_seq`. If that pass emitted `verdict_ready` before the confirmation, the recomposed pass emits another final event. At the second finish, `_verify_final_event` sees two finals with no saved sequence, emits `result_no_event`, and forces the capture to `manual_review`.
Unlike the completed-run path, this path has no first close to record the sequence. Account for the first pass's final event before resetting, and test that live recomposition does not emit a false failure.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptileai review this at plus |
| self.s.state.delete_blob(key) | ||
| reset_criteria_and_result(self.s, run) |
There was a problem hiding this comment.
_consume_pending_correction deletes the marker before saving either stage reset. If the coordinator stops, or the first state write fails, in that gap, both stages remain done and the run remains running. The sweeper later resumes the run, finds no marker, and closes it with the old result. Repeated confirmations skip the hook, so they cannot repair it.
Keep the correction pending until the reset is saved, with recovery for partial resets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/orchestrator/coordinator.py
Line: 594-595
Comment:
**Corrections vanish during recovery**
`_consume_pending_correction` deletes the marker before saving either stage reset. If the coordinator stops, or the first state write fails, in that gap, both stages remain `done` and the run remains `running`. The sweeper later resumes the run, finds no marker, and closes it with the old result. Repeated confirmations skip the hook, so they cannot repair it.
Keep the correction pending until the reset is saved, with recovery for partial resets.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.This branch has not been deployed
2 participants
fix(backend): late corrections reach a running result, and finalize failures always free the worker
What changed
Two background races closed by Greptile review on PR #293, both in the backend's capture → result pipeline:
A correction accepted while the result stage is mid-flight never reached the served verdict.
on_confirmationleft a pending or running run alone, on the assumption its result stage had not consumed the corrected reads yet. The window is real:verdict_readyis decided before the result stage finishes uploading, so a confirmation can arrive after the stage hydrated its inputs. The hook saved the corrected reads and returned; the run closed done with the pre-correction meter, and a replayed confirmation short-circuited upstream at the route's fingerprint check, so nothing ever recomposed the served payload. Now the hook records a pending-correction marker when the running stages are criteria or result (earlier arrivals are applied by the reads stage itself), and the coordinator consumes the marker before marking the run done — resetting criteria and result once through the same reset the done-run recomposition uses. The consumed marker keeps a repeated finish or a replayed hook call from resetting twice.A finalize worker whose failure publication itself raised stranded the capture on processing forever. The worker wrote the failure to the capture record and emitted the terminal event under the finish lock, then cleared its slot and settled its Future only after that write returned. When the publication itself raised — a transient state outage makes
put_capturefail — the exception escaped the thread with the slot busy and the Future unsettled: later finalize retries answeredprocessingforever with no worker ever starting again, anddrain_finalizehung on a Future nobody would settle. The failure write keeps its lock; the slot release and the Future settlement moved into afinallypath, so the worker's original error always reaches the client and a finalize retry starts a new worker.Why
Both paths promise the same thing — an accepted correction or a failed finalize is reflected in what
GET /resultandPOST /finalizeanswer next — and both broke it in the same way: the bookkeeping that records the outcome could itself fail and take the cleanup with it. The fix keeps each cleanup unconditional while leaving the concurrency controls (finish lock, slot guard, fingerprint short-circuit) exactly where they were.How it works
sequenceDiagram participant R as Correction POST participant H as on_confirmation participant C as Coordinator Note over C: result stage running, inputs already hydrated R->>H: corrected meter 12345 H->>C: pending-correction marker (live run) Note over C: result finishes done → _maybe_finish C->>C: consume marker, reset criteria+result once C->>R: served result carries 12345One correction, one recomposition: the marker is deleted when consumed, the reset reuses the done-run path's
reset_criteria_and_result, and the second pass's extra final event is already covered by thefinal_event_seqthe first close recorded.Verification
All checks run against head
1df5b441, backend env installed withuv sync --frozen --extra api:test_correction_during_live_result_stage_recomposes_oncegates the result stub after it reads its inputs, lands a correction and its replay inside that window, and asserts one recomposition (read from the event log — the coordinator recomputes spawn attempts, so attempt counters cannot show a re-run), a consumed marker, and the corrected meter in the served payload. Fails on the pre-fix source (run closed done with meter000000000).test_confirmation_replay_recomposes_oncepins the route's fingerprint short-circuit to a single recomposition over the real FastAPI app with the committed mini-arkit packet.test_failure_publication_error_releases_the_worker_slotdrives both the finalize work and its failure publication into an outage, asserts the drained error is the original one, the slot is free, and a retried finalize runs the orchestrator to a terminal capture state. Fails on the pre-fix source (unhandled thread exception, future never settled).pytest server/orchestrator/tests server/api/tests server/common/tests— 432 passed.make teston head: 4 failed / 1509 passed / 19 skipped / 14 errors in 706s — every failure pre-exists on the alpha base. The 4 failures areserver/eval/tests/test_gates.py(the expired SD-96 evaluation window) and reproduce identically on a pristine969d4e93worktree (4 failed / 90 passed in that module); the 14 errors aretests/test_capture_cold_restart.pycollection failures (the knownload_receiptimport gap). Zero new failures from this branch.make lint: green —ruff check/ruff format --checkclean, import-lint contracts 5 kept / 0 broken.Risk
The live-window fix adds one coordinator pass over criteria and result per pending marker; the marker is consumed exactly once and only recorded when a correction arrives during the narrow running window, so normal runs see no extra work. The finalize change only reorders cleanup that previously ran after the publication; the publication stays under the finish lock, so the concurrent-retry ordering the lock protects is unchanged.
Checklist
Human author: Sam Gu (sgu07966@gmail.com)
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact