Skip to content

fix(noema): bound orchestrator health cancellation latency - #670

Merged
seonghobae merged 5 commits into
mainfrom
fix/orchestrator-health-cancel-liveness-20260912
Sep 12, 2026
Merged

fix(noema): bound orchestrator health cancellation latency#670
seonghobae merged 5 commits into
mainfrom
fix/orchestrator-health-cancel-liveness-20260912

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Purpose

Close a Noema-owned consumer-side liveness gap in verifyOrchestratorHealthz without moving provider/model routing or contextual-orchestrator service authority into Noema.

Hosted RED → causal repair

Protected main@643106a6c6db859a612854052d1d209354f97fee awaited cancellation cleanup after an already-decisive health-response size violation in two paths: advertised Content-Length > 65,536 and streamed body overflow. A hostile or wedged upstream body could therefore keep the Noema health preflight pending even though fail-closed rejection had already been decided.

Test-only exact 73d25fcf93e458242830ee6498fbe97fcd176532 adds bounded hostile failsafes for both cancellation paths and requires reader-lock release to remain independent. Because same-branch concurrency twice cancelled historical runs before an unambiguous result, Draft RED-only evidence lane #671 was created at that same exact commit. Hosted CI 34685998987 passed exact checkout, toolchain, base/lock control, install and typecheck, then failed specifically at release tests. This is the unambiguous executable RED; #671 contains no production repair.

The first source repair made cancellation completion best-effort. Review then found a second causal gap: a cancellation implementation may throw synchronously before returning a promise, which would replace the primary oversize classification and, for the reader path, could skip lock release. Additional hostile tests bind both synchronous-throw paths.

Exact c19682d1df245cdb869d8074fa8913ab91ef03f3 catches synchronous cleanup faults as well as asynchronous rejection, never waits for cancellation completion after the size decision is final, and releases the consumed reader lock independently. Final exact 14b4c9a46692df0084a03b9a3b2ae4acf1178a83 adds the required public JSDoc for the shared hostile-test failsafe helper without changing runtime behavior. Base→head is behind 0 and limited to scripts/lib/orchestrator-gateway.mjs plus test/orchestrator-gateway-stream-bound.test.ts.

Exact-head GREEN

On 14b4c9a46692df0084a03b9a3b2ae4acf1178a83:

A fresh exact-head direct review found no actionable issue, and the PR has zero review threads. CodeRabbit's substantive review covered c19682d…; its JSDoc-only delta attempt was rate-limited and is not represented as independent approval.

Boundary

No provider selection, model routing, credential ownership, outbound/destination authority, quarantine/security authority, or contextual-orchestrator server behavior is added to Noema. The change is limited to Noema's already-authorized health-response consumer cleanup semantics.

Successor evidence

#671 is a RED-only evidence PR whose sole valid delta is an ancestor of this PR and remains in the current test file. It must not be merged. After this PR is normally merged, #671 may be closed only as a verified complete-successor case, preserving its hosted RED evidence by reference.

Merge authority

No gate weakening, force push, destructive rebase or self-approval. Merge requires exact head 14b4c9a46692df0084a03b9a3b2ae4acf1178a83, fresh unchanged protected base 643106a6c6db859a612854052d1d209354f97fee, four terminal SUCCESS gates, and zero unresolved threads.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c1369a7e-6d02-4b25-ae59-8585c8023749

📥 Commits

Reviewing files that changed from the base of the PR and between 643106a and c19682d.

📒 Files selected for processing (2)
  • scripts/lib/orchestrator-gateway.mjs
  • test/orchestrator-gateway-stream-bound.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

헬스 응답의 취소 작업을 대기하지 않는 방식으로 변경합니다. 취소 실패가 원래의 크기 초과 또는 읽기 오류를 대체하지 않도록 합니다. 중단 및 동기 예외 상황을 테스트합니다.

Changes

헬스 응답 취소 처리

Layer / File(s) Summary
취소 작업의 비동기 처리
scripts/lib/orchestrator-gateway.mjs
응답 본문과 스트림 리더의 cancel() 호출을 기다리지 않습니다. 취소 오류는 빈 catch 블록으로 처리합니다.
취소 실패 동작 검증
test/orchestrator-gateway-stream-bound.test.ts
취소가 해결되지 않거나 동기적으로 예외를 발생시켜도 원래 오류가 유지되고 리더가 해제되는지 검증합니다.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c1968

Oversized health responses retain their fail-closed rejection even when cancellation stalls or fails, with reader locks released independently. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 verifyOrchestratorHealthz의 취소 지연을 제한하는 핵심 변경을 정확하고 간결하게 설명합니다.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/orchestrator-health-cancel-liveness-20260912

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae added bug Something isn't working priority: high High-priority or P1 work labels Sep 12, 2026 — with ChatGPT Codex Connector

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head direct review on 14b4c9a46692df0084a03b9a3b2ae4acf1178a83: no actionable finding. The two production changes are limited to best-effort cancellation after a final oversize decision; synchronous and asynchronous cleanup faults cannot replace the primary error, and reader-lock release remains independent. The hostile stalled-cancellation and synchronous-throw cases cover both streamed overflow and advertised Content-Length paths. The final delta only documents the shared bounded test helper. Base is protected main@643106a6c6db859a612854052d1d209354f97fee, behind 0; all four exact-head hosted gates are terminal SUCCESS and there are zero review threads. This COMMENT records direct review evidence and is not self-approval.

@seonghobae
seonghobae merged commit d42dcf5 into main Sep 12, 2026
18 checks passed
@seonghobae
seonghobae deleted the fix/orchestrator-health-cancel-liveness-20260912 branch September 12, 2026 09:55
seonghobae added a commit that referenced this pull request Sep 12, 2026
seonghobae added a commit that referenced this pull request Sep 12, 2026
seonghobae added a commit that referenced this pull request Sep 12, 2026
…alth-cancel-liveness-authority-20260912

docs(noema): converge #670 health cancellation authority
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: high High-priority or P1 work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant