Fix automatic review cost accounting stalls - #47
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e498e8937d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with orchestrator_store_lock( | ||
| state_dir=state_dir, | ||
| name=REVIEW_COSTS_LOCK_NAME, | ||
| timeout_seconds=REVIEW_COSTS_LOCK_TIMEOUT_SECONDS, |
There was a problem hiding this comment.
When a review finishes while costs --all is running, this synchronous completion path waits on the same global review-cost-accounting lock that cmd_costs holds throughout the full history and SQLite scan. Because the timeout is 600 seconds, an otherwise completed review can appear stalled for up to ten minutes; the best-effort delta should use a short/non-blocking acquisition and rely on the canonical-history rebuild when the lock is busy.
Useful? React with 👍 / 👎.
| raw = json.dumps(stable, sort_keys=True, separators=(",", ":"), default=str) | ||
| return f"fallback:{blake2s(raw.encode('utf-8'), digest_size=16).hexdigest()}" |
There was a problem hiding this comment.
When two independent finalized runs lack all four preferred identifiers and happen to have the same selected fields—such as repeated gate failures with identical slot, variant, empty usage, elapsed time, and output—this content hash assigns them the same ID. After the first record stores that ID, _add_record and the incremental delta discard the second run, undercounting sessions and any matching token/cost contribution; the fallback needs a persisted per-run or per-attempt identity rather than only run contents.
Useful? React with 👍 / 👎.
b8672e0 to
2c250c5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c250c548b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metadata, | ||
| normalized_cwd, | ||
| is_legacy, | ||
| ) = _cached_review_cost_row(state_dir=state_dir, review_cwd=review_cwd) |
There was a problem hiding this comment.
When an asynchronous or resumed review started for branch A finishes after the worktree has switched to branch B, the record still identifies A through task_id/task_id_hint, but this lookup selects or creates the cache row using B's current Git metadata and never applies the branch predicate used by collect_review_cost_rows. The delta consequently charges B even though a canonical collection excludes that record from B, corrupting the cached ledger; skip records whose task ID differs from the current branch or derive the target row from the recorded branch.
Useful? React with 👍 / 👎.
2c250c5 to
5935acc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5935acc73f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| *, state_dir: Path, review_cwd: Path | ||
| ) -> tuple[ReviewCostRow | None, Path, Path, dict[str, str], str, bool]: | ||
| normalized_cwd = str(normalize_review_cwd_value(review_cwd) or "") | ||
| metadata = _metadata_for_cwd(normalized_cwd) |
There was a problem hiding this comment.
When Arena is enabled and gh pr view is slow or unreachable, this metadata lookup runs synchronously while apply_review_cost_delta_best_effort holds the global accounting lock, and _current_pr_number can wait for its 10-second subprocess timeout. The completed review therefore still stalls, while every concurrent automatic delta uses a zero-second lock timeout and is silently dropped until a manual costs --all rebuild. Preserve the cached PR number or defer GitHub metadata refresh to the explicit rebuild path.
Useful? React with 👍 / 👎.
5935acc to
15e91ca
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Incident
Automatic review cost refresh detached
review_suite_arena.py costs --cd ...after many lifecycle actions. That command took the crash-prone directorycost-reports.lockfor up to 600 seconds, then rescanned all round files and the Codex SQLite database. A stale lock directory made every detached job wait ten minutes even though Arena is disabled by default.Fix
arena.enabled=false.costs --cdis cache-only and points stale/absent rows tocosts --all.review-cost-accounting.lock; remove the detached launcher and non-spend refreshes from grade, signoff close, and GitHub anchoring.Recovery
Canonical review and gate records remain the source of truth.
costs --allremains the explicit full-history repair path and refreshes implementation estimates. The derived cache is best effort across process crashes or a busy accounting lock; the automatic path never waits behind a rebuild.costs --allrepairs any skipped delta. The PR does not claim exactly-once cache semantics.Performance evidence
All mutations used disposable temp state. Live state was passed only to
collect_review_cost_rowsin read-only measurements. GitHub PR lookup was disabled in collector timings withREVIEW_SUITE_COST_SKIP_GH_PR_VIEW=1.7a2f087: disposable stalecost-reports.lock+ real detached launcher returned in 6.720 ms, spawned one child, and the child was still blocked after 2 s; source timeout is 600 s. The child was terminated after observation.costs --allcollection phase) on live read-only state: 37.317 s for 2,919 rows/24,946 reviewer sessions. It intentionally remains the slow recovery command.Validation
pytest -n 0on cost, Arena, gate, GitHub, and orchestrator canonical suites: 243 passed.rvw_2e59d994, complete with clean local and anchored GitHub verdicts on15e91ca.