feat(video): normalize async job resource state - #912
Conversation
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthrough비디오 작업 레지스트리가 소유권, 사용량, 제공자 상태를 별도 레코드로 정규화합니다. 최초 완전 사용량은 보존하고 최신 상태는 갱신합니다. 기존 소유자 레코드의 읽기 호환 경로와 Valkey 원자 저장을 추가합니다. Changes정규화된 비디오 작업
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR separates ownership, usage, and lifecycle records while preserving legacy reads and atomic usage recording. However, provider observations may be persisted before identity validation, delayed responses can overwrite newer lifecycle state, and partial registration or rollback can leave jobs difficult to recover; these risks should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant 호출자
participant VideoJobRegistry
participant ValkeyJsonMapping
호출자->>VideoJobRegistry: register 작업 제출
VideoJobRegistry->>ValkeyJsonMapping: VideoJobRecord 저장
VideoJobRegistry->>ValkeyJsonMapping: 최초 VideoJobUsage 저장
VideoJobRegistry->>ValkeyJsonMapping: VideoJobLifecycle 저장
호출자->>VideoJobRegistry: observe_provider_result 호출
VideoJobRegistry->>ValkeyJsonMapping: 최신 상태 저장
VideoJobRegistry->>ValkeyJsonMapping: 기존 완전 사용량 유지
VideoJobRegistry-->>호출자: 조합된 VideoJobOwner 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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 26 functions across 4 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Conflict inventory (fail-closed diagnostic). Exact PR head: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| reported_usage = self._provider_usage(provider_result) | ||
| # Core ownership is written first: a failure after provider acceptance | ||
| # must never leave the job without a gateway-addressable owner. | ||
| self._records[gateway_job_id] = record |
There was a problem hiding this comment.
🔴 Rolling upgrades hide new video jobs
During mixed-version deployments, register writes new jobs only in the normalized registry. Older replicas return 404 when callers poll those accepted jobs.
Prompt for agents
Make the video-job storage transition safe for mixed-version replicas sharing Valkey. New code currently writes only video_job_records, while pre-upgrade replicas read only video_job_owners, causing intermittent 404 responses during rolling deployment. Add an explicit compatibility strategy, such as temporary dual writes with first-write usage semantics or a deployment migration/read cutover protocol, and cover new-writer/old-reader behavior in tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _usage_document(self, gateway_job_id: str) -> dict[str, int] | None: | ||
| """Read one normalized usage row as the public token-count shape.""" | ||
| usage = self._usages.get(gateway_job_id) | ||
| if usage is None: | ||
| return None | ||
| return { | ||
| "prompt_tokens": usage.prompt_tokens, | ||
| "completion_tokens": usage.completion_tokens, | ||
| } |
There was a problem hiding this comment.
📝 Info: Split retention remains aligned
Each measured-job lookup reads ownership and usage, refreshing both Valkey hashes. Jobs without usage refresh only their ownership record.
Was this helpful? React with 👍 or 👎 to provide feedback.
Resolve conflicts in docs/architecture.md and docs/product-technical-gap-baseline.md (both additive continuation entries from independent PRs). Also fix the legacy-owner-payload doc wording flagged by CodeRabbit: observe_provider_result writes back a first-complete usage row for legacy video_job_owners records, so it is a read-and-update compatibility path, not read-only. Verified the flagged OpenRouter video-generation doc link and the 2026-08-29 KST slice date are both accurate on current head; no change needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
| except Exception: # noqa: BLE001 - ownership is already durably committed | ||
| # Returning the opaque gateway id is the recoverability boundary: | ||
| # the caller can poll the accepted provider job and retry the | ||
| # companion observation, while billing still receives the | ||
| # provider response through the coordinator's independent sink. | ||
| _LOGGER.warning( | ||
| "video usage persistence failed after ownership commit; " | ||
| "follow-up polling may retry" | ||
| ) |
There was a problem hiding this comment.
🟡 Failed storage loses first usage report
When initial usage storage fails, register returns without retaining the observed counts. A later poll can persist revised counts, diverging from billing.
Prompt for agents
Preserve the first complete usage report even when the companion video_job_usages write fails after ownership persistence. In contextual_orchestrator/video_jobs.py, register currently logs and returns, while server.py independently records the initial counts in the cost ledger. A later observe_provider_result call can therefore persist different counts as the job's first usage row. Add a recoverable durable representation or retry/outbox mechanism that keeps the initial counts associated with the gateway job without sacrificing the requirement to return its opaque ID after provider acceptance. Ensure later polling cannot replace that initial report and that registry-visible usage stays aligned with the deterministic ledger record.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _owner_from_legacy(self, owner: VideoJobOwner) -> VideoJobOwner: | ||
| """Join a retained legacy owner to the shared first-write usage row.""" | ||
| if owner.provider_usage is not None: | ||
| return owner | ||
| provider_usage = self._usage_document(owner.gateway_job_id) | ||
| if provider_usage is None: | ||
| return owner | ||
| return replace( | ||
| owner, | ||
| usage_measurement_status="measured", | ||
| provider_usage=provider_usage, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Legacy counts remain authoritative
Legacy embedded usage takes precedence over normalized rows. Historical counts cannot be revised, while empty legacy records can adopt the first atomic observation.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Status check (2026-08-30, this cycle): Re-checked CodeRabbit's three outstanding findings against this head — all already resolved, no action needed:
Generated by Claude Code |
# Conflicts: # docs/product-technical-gap-baseline.md
| if usage is not None and owner.provider_usage is None: | ||
| self._store_usage_if_absent( | ||
| owner.gateway_job_id, | ||
| VideoJobUsage( | ||
| prompt_tokens=usage["prompt_tokens"], | ||
| completion_tokens=usage["completion_tokens"], | ||
| observed_at=int(time.time()), | ||
| ), | ||
| ) | ||
| return self._owner_from_record(record) |
There was a problem hiding this comment.
🟡 Failed billing write never retries
If record_async_video_usage fails after usage persists, later polls skip it because usage already exists. The job remains absent from billing.
Prompt for agents
Decouple “usage has been observed” from “usage has been ledgered.” In contextual_orchestrator/video_jobs.py, observe_provider_result returns the persisted usage on every later poll, while server.py only invokes CostRoutingCoordinator.record_async_video_usage when the pre-poll owner had no usage. If that ledger call fails or drops after the usage row was committed, every later request sees previous_usage populated and never retries. Add durable ledger-completion state or safely invoke the idempotent deterministic ledger write whenever persisted usage is available, including subsequent polls. Preserve first-complete usage semantics and avoid changing the provider usage row.
Was this helpful? React with 👍 or 👎 to provide feedback.
#921) * docs(gap-baseline): record the sidecar preflight max_tokens root cause Fulfills the reference added in .github#1436's code comment (Devin flagged it as a missing baseline entry when it merely pointed at a not-yet-written one). Records the exact-evidence trail (downloaded strix-reports artifact from this repo's own PR #912 run) that this repo's PRs cannot fix directly since the sidecar is central-.github-owned infrastructure. * docs(gap-baseline): record the post-merge canary result for the sidecar fix .github#1436 merged (admin bypass, structurally deadlocked check — evidence on the PR). Re-queued opencode-review/noema-review/strix on this PR plus #911/#920 as the live canary: the specific max_tokens 502 symptom is confirmed fixed, but noema-review still failed with a distinct signature (bytez discovery 500 + preflight finding zero passing routes). Records what's confirmed, what's still open, and the working hypothesis (concurrent-run rate-limit contention) pending a clean re-observation. * docs(gap-baseline): correct the reasoning-starvation mechanism claim Devin flagged (on #921) that ModelClient._response_content returns successfully for any string content, including "", so the entry's "reasoning consumes the budget, content comes back empty" narrative doesn't match the code, and the generic error message quoted implies a narrower condition (non-string/absent content, reasoning falsy) than originally claimed. Verified against the code and the original strix-reports artifact (no raw provider payload was ever captured — sanitized by design), corrected the entry to state what's actually evidenced (budget mismatch reproduces, matching it fixes it) versus what was an unverified hypothesis, and noted the fix's own tests target a different, stricter function (the launcher's own preflight content check) that is unaffected by this correction. * docs(gap-baseline): replace the rate-limit hypothesis with confirmed evidence Downloaded and inspected the actual strix-reports artifact instead of continuing to speculate: the real causes were (1) noema-review/ opencode-review having zero visibility into per-route preflight rejection reasons, and (2) the gateway preflight's 30s curl timeout cutting off a route the routing probe had just proven healthy in 18s. Both fixed and RED/GREEN-tested in ContextualWisdomLab/.github#1440. * docs(gap-baseline): restate the retracted hypothesis instead of a dangling "above" Devin caught it on #921: the previous edit deleted the paragraph stating the rate-limit hypothesis while the replacement text still said "the hypothesis above," leaving nothing for that reference to point to. Restated it inline. * docs(gap-baseline): record the full incident timeline (checker tightened 8/27, gateway check broke 8/29) Investigated whether .github's opencode-review verdict-checker itself was defective, since it's been failing org-wide for days. It isn't: git history shows it was a rubber stamp (always exit 0) until 8/27, when it was correctly rewritten to require a real matching review. Two days later, 8/29's "exercise exact gateway readiness" commit introduced the end-to-end gateway check that shipped with the max_tokens:16 bug (#1436 fixed today). The now-strict checker collided with a newly-broken dispatch path, not a checker design flaw. Confirmed via #1246: last real opencode-agent review was 8/23, none since despite the head moving forward repeatedly. * docs(gap-baseline): fix two stale/inaccurate claims CodeRabbit flagged 1. Softened "will resolve on the scheduler's next pass" to not assert an outcome that hasn't been verified. 2. Removed the false implication that the routing probe's 10s per-candidate timeout says anything about the separate gateway curl timeout completing in time -- especially now stale since that curl timeout was raised 30s->120s in .github#1440 after being found too tight for real reasoning-model latency. --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
video_job_ownersrecords.Validation
python -m pytest -q— 2527 passed in 704.59sruff checkon changed Python filessemgrep --config p/python contextual_orchestrator/video_jobs.py— 0 findingspython -m compileall -q contextual_orchestratorgit diff --checkBased on protected
mainb21645116b352967e50fc497b87eb745b9cc8c61.Summary by CodeRabbit
새 기능
문서
버그 수정