[Sub] 영상 워커 서비스 기본 경로 구현#124
Open
chowon442 wants to merge 3 commits into
Open
Conversation
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough이 PR은 Cloud Tasks HTTP push로 비디오 잡을 실행하는 워커 스택을 추가합니다. Settings 필드 및 앱 라우팅을 업데이트하고, SegmentProgress 이벤트를 도입하며, VideoJobRepository에 lease 수명주기와 lease-검증형 진행/종료 메서드를 추가하고, VideoWorkerRunner 및 /jobs/run HTTP 핸들러와 관련 테스트를 구현합니다. Changes워커 서비스 통합
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Labels
Suggested Reviewers
🚥 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 docstrings
🧪 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.
요약
워커 runner·lease·cancel poll·progress 연동은 설계(§3.4/§3.6)와 단위 테스트 방향이 잘 맞습니다. 다만 /jobs/run HTTP 경계 인증 부재, lease 획득 실패 시 200 ack, progress write의 lease 미검증은 배포 전에 손보는 편이 안전합니다.
[문제 1]
- 심각도: Critical
- 범주: 보안
- 근거:
http_handler.py의POST /jobs/run에 Cloud Tasks OIDC(또는 동등한 invoker 검증)가 없고,main.py에서 API 앱과 동일 라우터로 노출됩니다. 설계(video-generation-design.md§3.4)는 “OIDC 토큰 (Cloud Tasks → 워커)”를 전제합니다. ingress가/jobs/*까지 열려 있으면job_id만 알면 임의 워커 실행·리소스 소모가 가능합니다. - 수정안: FastAPI dependency로 OIDC JWT audience/issuer 검증을 추가하거나, 워커 전용 서비스/ingress에서
/jobs/run만 제한합니다. 최소한 배포 전 체크리스트에 “퍼블릭 API와 워커 라우트 분리”를 명시하세요.
[문제 2]
- 심각도: High
- 범주: 정확성
- 근거:
acquire_lease가None이면 상태와 무관하게SKIPPED+ HTTP 200을 반환합니다. 다른 인스턴스가 살아 있는 lease를 쥔RUNNING잡에 Cloud Tasks 재배달이 오면, 워커는 작업 없이 2xx로 ack하고 큐 재시도가 끝납니다. 선행 워커가 비정상 종료했는데 lease가 아직 stale이 아니면(기본 480s), 잡이RUNNING에 오래 남을 수 있습니다(lazy sweep 전까지). - 수정안:
get(job_id)후 분기하세요.TERMINAL또는 이미 처리 완료 → 200SKIPPED.RUNNING+ live lease(다른 holder, stale 아님) → 503 등 재시도 유도.QUEUED등 재시도 가능 상태는 별도 정책 검토.
[문제 3]
- 심각도: High
- 범주: 정확성
- 근거:
PostgresVideoJobRepository.update_progress/InMemoryVideoJobRepository.update_progress의 UPDATE가lease_holder_instance_id를 검사하지 않습니다. self-fence·lease 탈취 후에도 이전 워커의 in-flightupdate_progress가 새 holder의 stage/progress를 덮을 수 있습니다. - 수정안: 워커 경로용
update_progress에lease_holder_instance_id = :instance_id조건을 추가하거나, repository에instance_id를 받는 variant를 두고_WorkerProgressWriter에서 전달하세요.
[문제 4]
- 심각도: Medium
- 범주: 정확성
- 근거: 파이프라인 성공 직후
heartbeat_lease실패 시SELF_FENCED로 200 반환하고finalize/artifact_object_key저장을 건너뜁니다. 의도된 self-fence이지만, 후속 워커·lazy detection까지 중복 실행·지연 복구 비용이 큽니다. - 수정안: (추가 확인 필요) lease 소유가 확인된 경우에만 finalize하거나, finalize와 release를 단일 트랜잭션으로 묶고 실패 시 503으로 Cloud Tasks 재시도를 허용하는 방안을 검토하세요.
[문제 5]
- 심각도: Medium
- 범주: 유지보수성
- 근거:
create_video_worker_runner는debug=True이고database_url이 비어 있으면InMemoryVideoJobRepository를 씁니다. 워커 프로세스에DEBUG가 잘못 켜지면 인스턴스 간 상태가 공유되지 않습니다. - 수정안: 워커 팩토리에서는 in-memory fallback을 막고,
database_url필수로 fail-fast 하세요.
리스크(짧게): 환불은 task 2.07 범위로 보이며 이번 PR에서 누락된 것은 문서와 일치합니다. sync_cancel_event의 job당 get() 폴링은 segment/stage마다 DB 부하가 될 수 있어(성능) 후속 최적화 여지가 있습니다.
Sent by Cursor Automation: Chowon Reviewer
| return runner | ||
|
|
||
|
|
||
| @router.post("/run", response_model=RunVideoJobResponse) |
There was a problem hiding this comment.
[Critical · 보안] POST /jobs/run에 OIDC/invoker 검증이 없습니다. 설계(§3.4)는 Cloud Tasks → 워커 OIDC를 전제하고, main.py에서 API와 같은 앱에 라우터가 붙어 있습니다.
수정안: OIDC JWT 검증 dependency 추가, 또는 워커 전용 서비스·ingress로 /jobs/run만 격리하세요.
| skipped = await self._repository.get(job_id) | ||
| return VideoWorkerRunResult( | ||
| job_id=job_id, | ||
| status=VideoWorkerRunStatus.SKIPPED, |
There was a problem hiding this comment.
[High · 정확성] acquire_lease 실패 시 항상 SKIPPED + HTTP 200입니다. 다른 워커가 live lease를 쥔 RUNNING 잡에 Cloud Tasks 재배달이 오면 2xx ack 후 큐 재시도가 멈춰, 선행 워커 비정상 종료 + lease 미만료 시 잡이 RUNNING에 묶일 수 있습니다.
수정안: terminal/이미 완료 → 200 SKIPPED; RUNNING + live foreign lease → 503 등 재시도 유도.
|
|
||
| async def update_progress( | ||
| self, | ||
| job_id: str, |
There was a problem hiding this comment.
[High · 정확성] update_progress UPDATE에 lease_holder_instance_id 조건이 없습니다. lease 탈취/self-fence 이후 이전 워커의 in-flight progress write가 새 holder 상태를 덮을 수 있습니다.
수정안: WHERE ... AND lease_holder_instance_id = %(instance_id)s (또는 워커 전용 API)로 holder 일치 시에만 갱신하세요.
| attempt_id=active_attempt_id, | ||
| ) | ||
|
|
||
| if await self._repository.heartbeat_lease(job_id, instance_id=self._instance_id) is None: |
There was a problem hiding this comment.
[Medium · 정확성] 파이프라인 성공 직후 heartbeat_lease 실패 시 finalize/artifact 저장 없이 SELF_FENCED + 200입니다. 의도된 fence일 수 있으나 중복 실행·복구 지연 비용이 큽니다.
수정안: (추가 확인) finalize+release를 트랜잭션으로 묶거나, 이 경로에서 503 재시도 정책 검토.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/features/video/jobs/repository.py`:
- Around line 247-250: The lease liveness check in acquire_lease relies on
progress_updated_at but request_cancel also updates that same field, which can
prevent stale-lease takeover; change request_cancel so it does not modify
progress_updated_at (either stop updating it or introduce and update a separate
cancel_requested_at/timestamp field) and update any code that reads/writes
progress_updated_at accordingly (check functions acquire_lease and
request_cancel and the related logic around the second occurrence noted in the
diff) to ensure lease liveness and cancel signaling are separated.
In `@src/proovy_agent/features/video/worker/http_handler.py`:
- Around line 47-59: Update the _get_video_worker_runner function signature to
declare a return type of VideoWorkerRunner and add the corresponding import for
VideoWorkerRunner at the top of http_handler.py; specifically, change def
_get_video_worker_runner(request: Request): to def
_get_video_worker_runner(request: Request) -> VideoWorkerRunner: and import
VideoWorkerRunner (used by create_video_worker_runner) so type checkers see the
correct return type.
In `@src/proovy_agent/features/video/worker/runner.py`:
- Around line 127-130: The completed branch in the event handling currently only
calls await self.sync_cancel_event() but doesn't act on it, so a cancel right at
the final stage can be ignored; modify the logic inside the runner (the
event.status handling where self.sync_cancel_event() is called) so that when
event.status == "completed" and await self.sync_cancel_event() is True you
immediately raise asyncio.CancelledError (mirroring the started branch
behavior), ensuring cancelled requests are honored at the final stage.
🪄 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: 4a118afc-b7dc-4519-b00c-5fb22d92c37e
📒 Files selected for processing (15)
src/proovy_agent/app/main.pysrc/proovy_agent/common/config.pysrc/proovy_agent/features/video/jobs/repository.pysrc/proovy_agent/features/video/pipeline/__init__.pysrc/proovy_agent/features/video/pipeline/stage_context.pysrc/proovy_agent/features/video/pipeline/stages/render.pysrc/proovy_agent/features/video/pipeline/stages/tts.pysrc/proovy_agent/features/video/worker/__init__.pysrc/proovy_agent/features/video/worker/factory.pysrc/proovy_agent/features/video/worker/http_handler.pysrc/proovy_agent/features/video/worker/runner.pytasks/phase-2-video-async-infra/04-worker-service.mdtests/app/test_video_worker_http_handler.pytests/features/video/test_worker_factory.pytests/features/video/test_worker_runner.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
🏷️ PR 타입
📝 작업 내용
features/video/worker/에 Cloud Tasks HTTP push용/jobs/runhandler와VideoWorkerRunner를 추가했습니다.video_jobs저장소 계약에 lease acquire / heartbeat / release 경로를 추가해 live lease 거부, stale lease 탈취, self-fence를 검증할 수 있게 했습니다.cancel_requested확인을 추가해 running job이canceledterminal 상태로 남도록 했습니다.review로 갱신하고 구현/검증 완료 항목을 체크했습니다.📸 스크린샷
전체 테스트 참고:
실패 14건은 이번 변경 범위 밖의 기존 OCR 테스트 기대값 불일치입니다.
tests/ocr_node/test_math_postprocessor.py는 수학 기호 LaTeX 변환 기대값과 현재 출력이 다르고,tests/ocr_node/test_models.py는OCROptions가 현재flash/gpt4o-mini만 허용하지만 테스트는sonnet/opus를 기대합니다.✅ 체크리스트
📎 기타 참고사항
Summary by CodeRabbit
새로운 기능
테스트
문서