[Feat] 크레딧 그래프 배선#114
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthrough이 PR은 Planner의 실행 전 plan 홀드를 예약하고 런타임에서 활성 홀드를 추적해 예외 시 정리하며, CreditSettler가 동기 비용을 finalize 하도록 Postgres 기반 클라이언트·비용 산정·플래너 preflight·엔드포인트 수명주기·테스트를 통합합니다. Changes크레딧 그래프 배선: 홀드/정산 통합
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
요약
크레딧 hold → 동기 작업 finalize → SSE 정산 흐름과 retry preflight·contextvar 배선은 방향이 맞고 테스트도 핵심 경로를 잘 덮습니다. 다만 plan hold 추정이 core_solver의 가변 과금과 고정 가정으로 어긋나, all-or-nothing hold의 실효가 약해질 수 있습니다. 그래프 중단 시 hold 해제는 TTL-on-read(20분)에 의존하므로 설계상 허용 범위이나, 부분 실행·멀티턴에서는 추가 확인이 필요합니다.
[문제 1]
- 심각도: High
- 범주: 정확성
- 근거:
credit_pricing._estimate_solve_cost는 verify/explain LLM을 각 1회, code_generate/code_execute를 각 1회로 가정합니다. 반면core_solver는 Phase 1 반복(_MAX_ITERATIONS=5)에 따라llm_call_count,codegen_count,execute_count가 가변적으로credit_log에 누적됩니다. hold는 고정 추정치로 잡히고finalize_hold는 실제sync_cost전액을 balance에서 차감하며 hold 금액 상한을 검사하지 않습니다(태스크 문서의 “초과 허용”과 일치). 그 결과 hold가 실제보다 작으면 예약(hold) 없이 추가 차감이 발생할 수 있고, 사용자 입장에서는 “N cr 필요”로 거절됐어야 할 요청이 실행 후 더 많이 청구되는 경로가 생깁니다. - 수정안: (a) hold 추정에
core_solver와 동일한 상한(예: max iteration × 단가)을 반영하거나, (b)finalize_hold전에sync_cost > hold.amount이면CreditHoldAmountExceededError로 처리·release_hold후 사용자 오류를 반환하거나, (c) 최소한MODEL_CREDIT_COST/_MODEL_COST를 단일 모듈로 통합해 추정·실청구가 같은 테이블을 참조하게 하세요. 단기적으로는 (a)+(c) 조합이 리스크 대비 변경 폭이 작습니다.
[문제 2]
- 심각도: Medium
- 범주: 정확성 / 보안
- 근거:
_preflight_video_retry에서video_job_client가None이면 검증 없이 통과합니다(client is None: return). DB/ledger는 있으나 video client만 미주입·장애인 경우,video_retry요청이 hold 없이 planner LLM까지 진행될 수 있습니다. - 수정안: retry 액션이면 client가 없을 때도
invalid_input(또는 503)로_raise_preflight_error처리하세요. dev/테스트용으로 ledger를 끄는 경우만 명시적 설정 플래그로 preflight skip을 허용하는 편이 안전합니다.
[문제 3]
- 심각도: Medium
- 범주: 정확성
- 근거:
_reserve_plan_hold가state.hold_id가 있으면 금액·plan 변경 없이 기존 ID를 그대로 재사용합니다. 체크포인트에 pending hold가 남은 멀티턴/재시도 시나리오에서 새 plan 비용과 hold amount가 어긋날 수 있습니다. (LangGraph 입력 merge가 매 턴hold_id를 초기화하는지는 호출 경로에 따라 달라 추가 확인 필요) - 수정안: 턴 시작 시 새 hold가 필요하면
hold_id를 명시적으로None으로 리셋하거나, 기존 hold의plan_id/amount와 현재estimate_plan_hold_amount를 비교해 불일치 시release_hold후 재 hold 하세요.
[문제 4]
- 심각도: Low
- 범주: 성능
- 근거:
PostgresCreditLedgerClient가hold/finalize_hold/release_hold마다AsyncConnection.connect로 새 연결을 엽니다. solve 한 번에 planner hold + settler finalize로 최소 2회 연결이 생깁니다. - 수정안: lifespan에서 connection pool(또는
AsyncConnectionPool)을 주입하고 client가 pool에서 conn을 빌려 쓰도록 바꾸면 됩니다. 당장은 기능상 문제는 없으나 트래픽 증가 시 우선 개선 후보입니다.
잔여 리스크 (수정 필수 아님): 그래프가 credit_settler 전에 예외 종료되면 pending hold는 TTL(기본 1200s) 만료까지 가용 잔액을 묶습니다. 설계 문서(TTL-on-read)와 일치하나, 장시간 실패 시 UX 이슈는 모니터링 대상입니다. 영상 10cr는 hold에 포함되나 동기 정산에서 제외되는 것은 후속 Task 2.06 범위로 보이며, 이번 PR 범위에서는 의도된 분리로 이해했습니다.
Sent by Cursor Automation: Chowon Reviewer
| explanation_mode: Literal["full", "brief"], | ||
| ) -> Decimal: | ||
| model_cost = MODEL_CREDIT_COST.get(selected_model, MODEL_CREDIT_COST["flash"]) | ||
| llm_call_count = Decimal("1") + (Decimal("1") if explanation_mode == "full" else Decimal("0")) |
There was a problem hiding this comment.
[High · 정확성] _estimate_solve_cost는 LLM/code 호출을 각 1회로 고정하지만, core_solver는 iteration에 따라 llm_call_count·codegen_count·execute_count가 늘 수 있습니다. hold가 작게 잡히면 finalize_hold가 예약액을 넘는 sync_cost를 그대로 차감할 수 있어, all-or-nothing hold의 의미가 약해집니다. 상한 반영 또는 sync_cost > hold.amount 가드 검토를 권합니다.
|
|
||
| client = _get_video_job_client() | ||
| if client is None: | ||
| return |
There was a problem hiding this comment.
[Medium · 정확성/보안] video_job_client가 None이면 retry preflight가 전부 스킵됩니다. retry 요청이면 client 부재 시에도 거절(또는 명시적 dev-only bypass)하는 편이 hold 전 검증 의도와 맞습니다.
| planner_credit: CreditEntry, | ||
| ) -> str | None: | ||
| if state.hold_id: | ||
| return state.hold_id |
There was a problem hiding this comment.
[Medium · 정확성] 기존 hold_id가 있으면 재추정·재 hold 없이 반환합니다. 체크포인트에 남은 pending hold와 새 plan 비용이 다를 때 정산 불일치 가능성이 있어, 턴 경계에서 hold 리셋/재 hold 조건을 한번 더 확인해 주세요.
| reserved: float | None = None | ||
| refunded: float | None = None | ||
|
|
||
| if state.hold_id: |
There was a problem hiding this comment.
[확인 부탁 · Medium] 그래프 에러 시 hold 누수 가능성
이 finalize_hold 경로가 hold를 해제하는 사실상 유일한 지점이고 그래프 끝(credit_settler)에서만 실행되는 것 같은데요. planner가 hold를 잡은 뒤 core_solver 등 중간 노드가 예외를 던지면 credit_settler가 실행되지 않아 hold가 PENDING으로 남고, 사용자 크레딧이 TTL 만료까지 묶이지 않을까요?
실패한 요청을 바로 재시도할 때 잔액이 줄어 있는 UX가 우려됩니다. _run 에러 경로(solve.py)나 그래프 finally에서 release_hold를 호출하는 방안은 검토되셨는지 확인 부탁드립니다. (TTL 자동 해제는 되지만 ~20분 윈도우라서요.)
| *, | ||
| plan_id: str | None = None, | ||
| ) -> CreditHold: | ||
| async with await AsyncConnection.connect(self._database_url) as conn: |
There was a problem hiding this comment.
[확인 부탁 · Medium] 연산마다 새 커넥션 — 풀링 검토
hold/finalize/release가 각각 AsyncConnection.connect로 새 커넥션을 여는 구조라, 한 요청에서 hold + finalize만으로도 커넥션이 2개 이상 생성됩니다. 동시 요청이 늘면 커넥션 churn/고갈이 걱정되는데, checkpointer처럼 풀(또는 공유 커넥션)을 재사용하는 방향은 어떨지 확인 부탁드립니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/proovy_agent/app/api/v1/solve.py (1)
80-97:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift종료 시 solve 태스크와 공유 리소스의 수명을 맞춰 주세요.
여기서 만든
_run()은 요청과 분리돼 계속 실행되는데,src/proovy_agent/app/main.py의lifespan()은 shutdown 시credit_ledger_client와 Daytona를 바로 닫습니다. 그 상태에서 태스크가 hold를 가진 채 취소되거나 닫힌 클라이언트로finalize_hold()/release_hold()를 호출하면 크레딧이 만료 시점까지 묶일 수 있습니다. 앱 종료 전에 in-flight solve 태스크를 cancel+await한 뒤 공유 클라이언트를 닫고,CancelledError경로에서도_release_active_credit_hold()를 호출하도록 정리하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proovy_agent/app/api/v1/solve.py` around lines 80 - 97, The solve background task (_run()) outlives the request and may try to use shared clients after shutdown; update shutdown flow to cancel and await in-flight solve tasks before closing shared clients (see _active_tasks, task created for _run()), and ensure the CancelledError handler in _run() calls _release_active_credit_hold(credit_ledger_client, state.user_id) so any held credits are released even when the task is cancelled; adjust src/proovy_agent/app/main.py lifespan() to iterate over _active_tasks, cancel and await them prior to closing credit_ledger_client/Daytona.
🧹 Nitpick comments (2)
src/proovy_agent/graph/credit_pricing.py (1)
37-53: ⚡ Quick winPlan hold 추정 누락 우려 완화: PlanStep.action 범위로는 general_chat/image 미포함
estimate_plan_hold_amount에서solve/video만 누적해도,PlanStep.action(및_StepInput.action,PlanStepView.action)이Literal["solve", "video", "pdf"]로 고정되어 있어general_chat/image액션이 계획에 들어올 수 없습니다. 다만GENERAL_CHAT_COST·IMAGE_COST는credit_pricing.py에서만 보이므로 실제 정산 경로에서 쓰이는지(데드 코드 여부) 점검이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proovy_agent/graph/credit_pricing.py` around lines 37 - 53, estimate_plan_hold_amount currently only accumulates costs for actions "solve", "pdf", and "video" but PlanStep.action may include other action types (e.g., "general_chat", "image"), and GENERAL_CHAT_COST and IMAGE_COST exist in this module; update estimate_plan_hold_amount to handle these additional actions by adding branches for "general_chat" (add GENERAL_CHAT_COST) and "image" (add IMAGE_COST), ensuring you still use credit_log_amount(accrued_log) and _estimate_solve_cost(selected_model, explanation_mode) for solve, or if those constants are truly unused elsewhere, add a note/remove dead code after confirming via search; refer to estimate_plan_hold_amount, PlanStep.action, GENERAL_CHAT_COST, IMAGE_COST, credit_log_amount, _estimate_solve_cost, PDF_COST, VIDEO_FLAT_COST.tests/app/test_solve_endpoint.py (1)
63-109: 💤 Low value
_FakeCreditLedger/_hold헬퍼가tests/graph/test_credit_graph_wiring.py와 거의 동일하게 중복됩니다.두 파일에서 동일한 스텁을 유지보수해야 하므로 공용
conftest.py픽스처나 테스트 유틸로 추출하는 것을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/app/test_solve_endpoint.py` around lines 63 - 109, The _FakeCreditLedger class and _hold helper are duplicated across tests; extract them into a shared test fixture or helper module (e.g., conftest.py or a test_utils module) and update tests to import/use the shared fixture instead of redefining _FakeCreditLedger and _hold; ensure the exported fixture preserves the same constructor signature, async methods (hold, finalize_hold, release_hold) and return values (CreditHold instances with the same fields) so existing tests referencing _FakeCreditLedger and _hold continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/proovy_agent/graph/nodes/credit_settler.py`:
- Around line 55-59: The AIMessage call in credit_settler.py uses
metadata={"display":"system"}, which is not a LangChain 1.0 standard field;
update the AIMessage(...) invocation to pass the hint via response_metadata
(e.g., response_metadata={"display":"system"}) or additional_kwargs if you need
non-standard keys, keeping the message text (f"총 {float(sync_cost)}cr 사용")
intact and ensuring any downstream code that reads that hint reads
response_metadata instead of metadata.
In `@src/proovy_agent/graph/nodes/planner.py`:
- Around line 222-224: The hold cleanup (checking state.hold_id and calling
await ledger.release_hold(state.user_id, UUID(state.hold_id)) catching
CreditLedgerError/ValueError) must run before _preflight_video_retry, because
failed retry validation currently bypasses the release and leaves a stale hold;
move that release block ahead of the call to _preflight_video_retry or extract
it into a helper (e.g., _ensure_release_hold(state, ledger)) and invoke it
immediately before retry validation so old holds are always released even when
_preflight_video_retry fails.
- Around line 222-224: The current suppress(ValueError, CreditLedgerError)
around await ledger.release_hold(state.user_id, UUID(state.hold_id)) swallows
all ledger errors and can leave a prior hold PENDING, causing duplicate holds or
spurious credit_exhausted; change this to only ignore the parsing error
(ValueError) and handle ledger errors explicitly: remove CreditLedgerError from
the suppress, catch only narrow ledger exceptions such as HoldNotFound or
HoldAlreadyReleased (or the ledger method's specific "not found/already
released" exception classes) to treat as non-fatal (optionally log), and let
other CreditLedgerError variants propagate or be handled separately so failures
abort the flow; update references to state.hold_id, ledger.release_hold,
UUID(...) and CreditLedgerError accordingly and add logging for the caught
narrow exceptions.
In `@tests/graph/test_credit_graph_wiring.py`:
- Around line 321-336: The test
test_planner_works_without_ledger_client_for_unit_tests can accidentally create
a Postgres ledger because _reserve_plan_hold uses
current_credit_ledger_client.get() or create_credit_ledger_client() which falls
back to planner_module.settings.database_url; to fix it, in the test set
planner_module.settings.database_url to an empty string (e.g. via monkeypatch)
before calling planner_module.planner so the fallback path is disabled and
hold_id remains None—ensure you patch planner_module.settings.database_url in
this test (and restore/keep isolated) similar to the sibling tests that force
the no-database path.
---
Outside diff comments:
In `@src/proovy_agent/app/api/v1/solve.py`:
- Around line 80-97: The solve background task (_run()) outlives the request and
may try to use shared clients after shutdown; update shutdown flow to cancel and
await in-flight solve tasks before closing shared clients (see _active_tasks,
task created for _run()), and ensure the CancelledError handler in _run() calls
_release_active_credit_hold(credit_ledger_client, state.user_id) so any held
credits are released even when the task is cancelled; adjust
src/proovy_agent/app/main.py lifespan() to iterate over _active_tasks, cancel
and await them prior to closing credit_ledger_client/Daytona.
---
Nitpick comments:
In `@src/proovy_agent/graph/credit_pricing.py`:
- Around line 37-53: estimate_plan_hold_amount currently only accumulates costs
for actions "solve", "pdf", and "video" but PlanStep.action may include other
action types (e.g., "general_chat", "image"), and GENERAL_CHAT_COST and
IMAGE_COST exist in this module; update estimate_plan_hold_amount to handle
these additional actions by adding branches for "general_chat" (add
GENERAL_CHAT_COST) and "image" (add IMAGE_COST), ensuring you still use
credit_log_amount(accrued_log) and _estimate_solve_cost(selected_model,
explanation_mode) for solve, or if those constants are truly unused elsewhere,
add a note/remove dead code after confirming via search; refer to
estimate_plan_hold_amount, PlanStep.action, GENERAL_CHAT_COST, IMAGE_COST,
credit_log_amount, _estimate_solve_cost, PDF_COST, VIDEO_FLAT_COST.
In `@tests/app/test_solve_endpoint.py`:
- Around line 63-109: The _FakeCreditLedger class and _hold helper are
duplicated across tests; extract them into a shared test fixture or helper
module (e.g., conftest.py or a test_utils module) and update tests to import/use
the shared fixture instead of redefining _FakeCreditLedger and _hold; ensure the
exported fixture preserves the same constructor signature, async methods (hold,
finalize_hold, release_hold) and return values (CreditHold instances with the
same fields) so existing tests referencing _FakeCreditLedger and _hold continue
to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3713a4c0-578c-4581-b243-afb9fd9e81ba
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
pyproject.tomlsrc/proovy_agent/app/api/v1/solve.pysrc/proovy_agent/app/main.pysrc/proovy_agent/features/credits/__init__.pysrc/proovy_agent/features/credits/service.pysrc/proovy_agent/graph/agents/core_solver/agent.pysrc/proovy_agent/graph/credit_pricing.pysrc/proovy_agent/graph/nodes/credit_settler.pysrc/proovy_agent/graph/nodes/planner.pysrc/proovy_agent/graph/runtime.pytasks/phase-2-video-async-infra/03-credit-graph-wiring.mdtests/app/test_health.pytests/app/test_solve_endpoint.pytests/graph/test_credit_graph_wiring.py
3 participants
📌 관련 이슈
🏷️ PR 타입
📝 작업 내용
credit_exhaustedSSE 오류로 실행 전 all-or-nothing 거절hold_id를 state에 저장해 이후 정산 노드가 같은 hold row를 사용retry_source_job_idpreflight를 hold 전에 수행finalize_hold로 capturecredit_settled.reserved/refundedpayload에 반영📸 스크린샷
✅ 체크리스트
📎 기타 참고사항
@pytest.mark.postgres테스트는DATABASE_URL미설정으로 기존 정책대로 skip되었습니다.tests/pdf_node/test_pdf_node.py::test_pdf_node_basicreturn-not-none.Summary by CodeRabbit
새로운 기능
개선 사항
테스트