fix(ask): fence Ask settlement on the claim generation - #979
Conversation
Orphan recovery can flip a long-running job back to queued while the original worker still holds the answer. Settlement now compare-and-sets the claim's updated_at so the previous owner cannot overwrite a reclaimed row.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
Compare-and-set settlement now inspects PostgreSQL UPDATE 0 so a reclaimed Ask job is left with the new owner instead of being misread as a completed write.
Age-based orphan recovery could reclaim a job whose owner was still computing. Renew updated_at on the recovery interval, abort when the generation is lost, and keep the 600 s deadline until that heartbeat is independently approved.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
현재 heartbeat wrapper가 compute_global_ask_answer()를 별도 asyncio.create_task()로 분리했는데, 바깥 process_global_ask_job() task가 shutdown/user cancellation으로 취소될 때 finally에서 beater만 취소하고 worker task는 취소·await하지 않습니다. 기존 direct-await 경계에서는 native cancellation이 계산 작업으로 전파됐지만 이 head에서는 child operation이 detached 상태로 계속 실행될 수 있습니다. 이는 #975의 native-cancellation/liveness 경계와도 충돌합니다. 현재 timeout_source="shutdown" 테스트는 child coroutine이 스스로 CancelledError를 던지는 경우만 다뤄서 이 regression을 잡지 못합니다. 외부에서 wrapper/process task를 cancel했을 때 operation이 실제로 cancel되고 detached task가 남지 않는 realistic RED를 먼저 추가한 뒤, wrapper finally에서 미완료 worker를 cancel+gather하는 최소 causal fix가 필요합니다. 동시에 PR 본문의 “does not ... add liveness heartbeats”는 current diff와 불일치하므로 live authority에 맞게 고쳐야 합니다.
Owner-task cancellation left compute_global_ask_answer running as a detached child of the claim-heartbeat wrapper. Cancel and await that worker in finally, and remove the one-shot repair workflow whose RED now lives in the product test suite.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh exact-head repair review on 3dbba6b15733c6e517d70461446eb1049eba57bf: the queued one-shot run 34131105502 did not execute its RED or fix. It was admitted only after the branch had already advanced from workflow authority 3f0b63d... to this concurrent product commit, so its Verify authority before repair step correctly failed and all repair/test steps were skipped. I adopted the intervening delta instead of treating that failure as a product regression. The current source now cancels and gathers both the inner Ask worker and heartbeat in finally, which is the causal cleanup required by review 5132764499, and the temporary workflow is gone; however this commit has no exact-head backend GREEN because repository Tests are skipped while Draft. Keep the cancellation regression test and run current-head PostgreSQL-backed validation before any completion or merge claim.
Omitted ORCHESTRATOR_ANSWER_TIMEOUT_SECONDS now means no LineageWeave elapsed socket limit, matching post-chat's null transport default. An explicit finite value still has to stay below the 600 s job deadline, which remains until the heartbeat is independently approved.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Valid current-head runtime finding: the new 30 s claim heartbeat/CAS fencing now distinguishes a live owner from an orphan, but process_global_ask_job() still wraps the entire model-backed computation in asyncio.timeout(JOB_DEADLINE_SECONDS) with JOB_DEADLINE_SECONDS = 600. That terminates a healthy reasoning/stream/tool-call operation solely because elapsed wall time reached 600 s, despite the live claim continuing to renew. This conflicts with the current execution contract: model timeout defaults null; user cancellation, provider completion/failure, admin policy and owner-liveness loss must remain distinct; elapsed time alone must not terminate a healthy model-backed operation. The orphan-recovery threshold is also still derived from that hard deadline even though heartbeat freshness is now the authoritative liveness signal.
Repair boundary: first add a realistic test in which the operation remains live across the former 600 s boundary while the claim heartbeat continues, and require no failed settlement/cancellation from elapsed time alone. Then remove the LineageWeave hard execution timeout from the active operation path and make orphan reclaim depend on stale heartbeat/claim generation rather than a guaranteed deadline. Keep explicit deployment/admin timeout policy separate if one is introduced; do not copy provider timeout policy or invent a default. Preserve native cancellation, CAS settlement, bounded failure detail and recovery after an actually dead worker.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head ADR/operability finding: this branch now owns a material runtime decision—claim-generation CAS settlement, periodic liveness renewal, stale-owner reclaim semantics, native cancellation, and removal of elapsed-time-only execution termination—but its changed-file set contains no ADR. AGENTS.md makes ADRs normative and requires a decision record before architectural/runtime policy changes; issue #975 also requires the operability/recovery decision record with alternatives, rejected timer-only recovery, rollback, and exact evidence. Keep the existing elapsed-deadline RED as the behavior gate, then in the same causal repair allocate an unclaimed Proposed ADR for the ownership-fenced liveness invariant and make the operability/gap docs code-current. Do not mark the ADR Accepted before protected-main acceptance. No new provider/model policy belongs in that ADR.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Acceptance-gap finding on the same exact head: #975 explicitly requires the ownership/recovery invariant against real PostgreSQL claim/settlement transactions and says not to rely on synthetic sleep-only assertions. The changed test surface here still uses in-memory/mock pool connections; tests/test_global_ask_elapsed_deadline.py is useful RED for elapsed cancellation, but it cannot prove the SQL CAS/heartbeat/reclaim invariant. Add a PostgreSQL-backed regression using the repository's real 0165_global_ask_job.sql schema (the normal Tests job already provisions PostgreSQL): Worker A must hold/renew one running claim generation, recovery must not reclaim while that heartbeat is current; when liveness actually becomes stale/reclaimed, the prior owner must fail CAS settlement and the queued successor remains authoritative. Keep Valkey as a wake-up stub if needed—the database row is already the documented source of truth. This test belongs in the same causal repair before #975 can be treated as verified.
The 600 s asyncio timeout still cancelled a renewing Ask owner. Compute now runs under claim-generation liveness only. ADR 0370 records that Proposed policy. Orphan recovery uses three missed heartbeats. Provider TimeoutError stays unavailable, not a worker deadline. PostgreSQL-backed race evidence remains a follow-up.
Issue #975 requires the reclaim invariant against actual claim and settlement transactions, not only in-memory fakes. After 0001 and 0165 replay, the previous owner's compare-and-set is UPDATE 0 and the new owner settles.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head repair finding on fbe68078a325d760e82105b7175492176d29453d: the elapsed compute deadline has been removed from process_global_ask_job(), and the new real-PostgreSQL CAS regression is a valid intervening delta to adopt. However backend/app/config.py still carries GLOBAL_ASK_JOB_DEADLINE_SECONDS = 600 and rejects an explicit finite ORCHESTRATOR_ANSWER_TIMEOUT_SECONDS >= 600 solely because it must be below that removed worker deadline. global_ask_queue.py also keeps the dead deadline import/alias, and the orphan-recovery docstring still says a per-job deadline guarantees settlement. This preserves deleted timeout policy in configuration and documentation and contradicts this PR's stated separation of nullable transport timeout from heartbeat/claim liveness. Realistic RED: an explicit finite positive transport timeout such as 900 s must be accepted after the worker deadline is removed, while omitted/blank remains null and zero/negative/non-finite values remain rejected. Minimal repair: validate only finite-positive explicit transport timeout, remove the dead worker-deadline constant/import/alias and stale recovery wording, and keep heartbeat-grace orphan recovery/native cancellation/CAS settlement unchanged. Do not introduce provider policy or a new default.
Collection on aecb873 imported GLOBAL_ASK_JOB_DEADLINE_SECONDS after the constant was removed. load_settings now accepts an explicit 900 s transport timeout and rejects non-positive values. ADR 0370 Proposed no longer bounds that timeout below 600 s.
|
RCA on exact head Causal leftover: Local evidence (not a CI transfer): 36 targeted tests passed; 266 tests collected without ImportError; live PostgreSQL |
Tests run 34153018910 failed the public-docstring gate because the nested Ask heartbeat coroutine was named beat. Rename it _beat so the helper is not a public production definition.
|
RCA on exact head Repair on |
|
Exact-head Prior review findings on this stack are closed on this head: native cancellation of the inner worker, claim-generation CAS settlement, live heartbeat vs elapsed-time cancellation, real PostgreSQL reclaim ( Parent #974 ( Next unproven liveness edge, not changed on this GREEN head: if the heartbeat task ends while compute is still running without |
A heartbeat exception left compute running without renewals until the test timed out. Treat a finished heartbeat with a live worker as a lost claim so the owner is cancelled instead of settling.
|
Follow-up on GREEN |
Leftover-map validation #980 already allocated ADR 0370 for comparison axis-singular. Keep that number on the leftover stack and record Ask claim-generation liveness as Proposed ADR 0371.
|
Exact-head Repair finding vs leftover validation #980: that lane already allocated |
|
Exact-head |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
backend/app/config.py— API and service runtimebackend/app/global_ask_queue.py— API and service runtimebackend/app/main.py— API and service runtimebackend/tests/test_config.py— regression suitedocs/adr/0371-ask-claim-generation-liveness.md— operator or user guidancedocs/product-technical-gap-baseline.md— operator or user guidancetests/test_global_ask_claim_cancellation.py— regression suitetests/test_global_ask_elapsed_deadline.py— regression suitetests/test_global_ask_queue.py— regression suitetests/test_global_ask_transport_timeout.py— regression suitetests/test_schema.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend: config.py (3 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend: config.py (3 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Test: test_config.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_config.py"]
R2 --> V2["targeted test run"]
Evidence --> S3["Docs: 0371-ask-claim-generation-liveness.md (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: 0371-ask-claim-generation-liveness.md (2 files)"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_global_ask_claim_cancellation.py (5 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_global_ask_claim_cancellation.py (5 files)"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
fef48b14f302dd40e1fa83096810cc880a5c4c66 - Workflow run: 34173501051
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend: config.py (3 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend: config.py (3 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Test: test_config.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_config.py"]
R2 --> V2["targeted test run"]
Evidence --> S3["Docs: 0371-ask-claim-generation-liveness.md (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: 0371-ask-claim-generation-liveness.md (2 files)"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_global_ask_claim_cancellation.py (5 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_global_ask_claim_cancellation.py (5 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
Still exact-head |
seonghobae
added
bug
ChatGPT Codex Connector
|
Head advanced concurrently to |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Independent current-head review of 6662ea5df6f8f026b54a083677d3e0ee98d2d6d2 (not APPROVE; same-author reviews do not satisfy the protected-branch independent-approval rule).
Prior product findings on this stack are addressed in the current source:
- owner cancellation cancels and gathers the inner compute task and heartbeat (
tests/test_global_ask_claim_cancellation.py) - live compute is not terminated by elapsed wall time;
JOB_DEADLINE_SECONDS/GLOBAL_ASK_JOB_DEADLINE_SECONDSare gone - omitted/blank Ask socket timeout stays null; explicit 900 s is accepted (
backend/tests/test_config.py) - settlement is compare-and-set on claim
updated_at; stale-owner PostgreSQL proof is intests/test_schema.py - inflight renewal is drained before settlement; a failed renewal rejects the simultaneous answer
- Proposed ADR 0371 is present and not marked Accepted
Hosted evidence on this exact head: Frontend and Full test suite GREEN (34182446558). This is stacked on #974 (codex/ask-timeout-attribution-20260907 @ def15fc69). Do not merge to main from this child. #974 Tests/Frontend are GREEN; remaining #974 failures are noema-review, opencode-review, strix (6h), and CodeQL compatibility analysis — not the product suite. Parent-first protected merge, then retarget.
Unresolved (not treated as current-head product defects in this delta): the inherited three-heartbeat orphan ratio is still uncalibrated, as ADR 0371 already states. No self-approval, force-push, or bypass.
1 participant
Ask jobs could lose an already completed answer when a claim renewal committed just before completion: cancelling the renewal discarded its new generation, so compare-and-set settlement updated no row. A simultaneous renewal failure could also be ignored. Drain in-flight renewal before admitting success/failure, reject unconfirmed ownership, and preserve native cancellation of both tasks.
This PR also replaces the elapsed compute deadline with claim-generation fencing, retains nullable explicit transport timeout, and prevents reclaimed owners from settling. It is stacked on #974 (
def15fc691d4442c0d82103c1642147b1528d7be); merge that parent through protection before retargeting this child tomain. Do not merge into the feature base.Validation for exact source correction
06be667c01f930fefb41a5107bf21fceb81ba7e7(current head6662ea5df6f8f026b54a083677d3e0ee98d2d6d2, including the test-fixture repair):ContextualWisdomLab/disksagespelling is corrected in the PRD register.Fresh hosted checks are required on the final head; predecessor Tests GREEN does not transfer. Independent approval is absent. The inherited three-heartbeat reclaim ratio still lacks a validated failure-detector/capacity contract and remains an explicit acceptance gap. No model policy, local mathematical implementation, timeout heuristic or warning suppression is added by this correction. The gap baseline separates current implementation, authority, non-identifying runtime observations and historical snapshots.