fix(durable): enforce lifecycle progress invariants - #148
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough원격 배치 상태가 Changes원격 배치 진행률 불변성
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_remote_batch_progress_invariant.py (1)
94-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftSQL 문자열 검사만으로 upsert 동작을 검증하지 마세요.
Line 94-118은 한 번만 호출하므로 INSERT 경로만 실행합니다.
_Cursor.execute는 SQL을 기록할 뿐 PostgreSQL을 실행하지 않으며, 충돌한 기존 행도 모델링하지 않습니다. 따라서 이 테스트는 guard 문자열의 존재만 검증합니다.기존 값과 신규 값의 누적 progress가 total을 초과할 때 UPDATE가 거부되는지, 경계값에서는 UPDATE가 수행되는지 PostgreSQL 통합 테스트 또는 conflict-aware 테스트 하네스로 검증하세요.
근거: 이 테스트의
_Cursor.execute구현과 단일 persistence 호출 경로입니다.🤖 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/test_remote_batch_progress_invariant.py` around lines 94 - 118, Replace the SQL-string-only assertion in test_upsert_guards_combined_monotonic_progress_invariant with a conflict-aware PostgreSQL integration test or harness that models an existing row and executes the ON CONFLICT UPDATE path. Verify updates are rejected when the merged completed and failed progress exceeds the merged total, and accepted at the boundary; retain the test’s existing monotonic-progress setup and symbols.
🤖 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 `@pg_llm_batch/db.py`:
- Around line 390-394: Update the request-count aggregation around
_provider_count and the SQL upsert guard so missing or invalid total values are
distinguished from an explicitly known total of zero. Ensure progress updates
are persisted when no known total exists, while retaining the inconsistency
check for valid totals and preserving invalid-total-to-zero behavior with
regression coverage.
---
Nitpick comments:
In `@tests/test_remote_batch_progress_invariant.py`:
- Around line 94-118: Replace the SQL-string-only assertion in
test_upsert_guards_combined_monotonic_progress_invariant with a conflict-aware
PostgreSQL integration test or harness that models an existing row and executes
the ON CONFLICT UPDATE path. Verify updates are rejected when the merged
completed and failed progress exceeds the merged total, and accepted at the
boundary; retain the test’s existing monotonic-progress setup and symbols.
🪄 Autofix
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 Plus
Run ID: 5b5f4e36-a04f-4a83-a0f1-4a7f76c34251
📒 Files selected for processing (2)
pg_llm_batch/db.pytests/test_remote_batch_progress_invariant.py
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pg_llm_batch/db.py (1)
587-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value내부 전용 필드 제거를 한 곳으로 모으세요.
두 공개 래퍼가 각각
total_requests_known을 제거합니다. 새 공개 래퍼가 추가되면 제거를 누락할 위험이 있습니다._persist_remote_batch_state가 공개 스냅샷을 만들어 반환하도록 하거나, 공용 헬퍼로 정리 로직을 분리하세요.♻️ 정리 헬퍼 예시
+_INTERNAL_SNAPSHOT_FIELDS = ("total_requests_known",) + + +def _public_remote_batch_snapshot(snapshot: Dict[str, Any]) -> Dict[str, Any]: + """Return one snapshot without internal-only persistence fields.""" + for field in _INTERNAL_SNAPSHOT_FIELDS: + snapshot.pop(field, None) + return snapshotAlso applies to: 601-610
🤖 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 `@pg_llm_batch/db.py` at line 587, 두 공개 래퍼에 분산된 `total_requests_known` 제거 로직을 `_persist_remote_batch_state`가 공개 스냅샷을 반환하도록 통합하거나 공용 정리 헬퍼로 추출하세요. 각 래퍼에서는 개별 `snapshot.pop` 호출을 제거하고 중앙화된 경로를 사용해 새 공개 래퍼에서도 내부 전용 필드가 항상 제외되도록 하세요.
🤖 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 `@docker/postgres/init/02_schema.sql`:
- Around line 152-165: Update the backfill around total_requests_known in
docker/postgres/init/02_schema.sql (152-165) and pg_llm_batch/schema.sql
(152-165) so rows where completed_requests + failed_requests exceeds
total_requests have their progress corrected or are explicitly reported for
migration, rather than only being marked FALSE. Apply identical logic in both
files and keep their schema contents synchronized.
- Around line 191-207: In docker/postgres/init/02_schema.sql lines 191-207 and
pg_llm_batch/schema.sql lines 191-207, update the migration around
ck_llm_remote_batch_jobs_known_progress so existing violating rows are corrected
before validation, then run VALIDATE CONSTRAINT only when
pg_constraint.convalidated is FALSE; apply the same conditional repair and
validation logic in both schema files.
In `@pg_llm_batch/db.py`:
- Around line 516-532: Update the upsert flow around the `ON CONFLICT DO UPDATE`
execution to inspect `cur.rowcount` after `cur.execute`. When the progress guard
skips the update, do not return the unpersisted snapshot as if it succeeded;
instead reread and return the stored job row, or explicitly mark the returned
snapshot as not applied, using the existing job lookup/serialization symbols in
the surrounding function.
In `@tests/test_integration_pg.py`:
- Around line 163-245: Update persist_remote_batch_state and its PostgreSQL
upsert guard so skipped updates return the existing persisted snapshot rather
than the rejected input snapshot. Extend the skipped-update cases for
known_batch_id and sparse_batch_id to capture and assert the method’s return
value, matching the database state verified by _read_remote_progress; preserve
the existing behavior for accepted updates.
---
Nitpick comments:
In `@pg_llm_batch/db.py`:
- Line 587: 두 공개 래퍼에 분산된 `total_requests_known` 제거 로직을
`_persist_remote_batch_state`가 공개 스냅샷을 반환하도록 통합하거나 공용 정리 헬퍼로 추출하세요. 각 래퍼에서는 개별
`snapshot.pop` 호출을 제거하고 중앙화된 경로를 사용해 새 공개 래퍼에서도 내부 전용 필드가 항상 제외되도록 하세요.
🪄 Autofix
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 Plus
Run ID: 4c65bb9b-a229-4a55-aaa9-6bbd8184cb54
📒 Files selected for processing (5)
docker/postgres/init/02_schema.sqlpg_llm_batch/db.pypg_llm_batch/schema.sqltests/test_integration_pg.pytests/test_remote_batch_progress_invariant.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_remote_batch_progress_invariant.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pg_llm_batch/db.py`:
- Around line 568-584: Update the stored-state SELECT projection in
pg_llm_batch/db.py lines 568-584 to include total_requests_known immediately
after total_requests, matching _REMOTE_BATCH_STATE_FIELDS order. Update
tests/test_remote_batch_progress_invariant.py lines 138-156 to add the
corresponding total_requests_known value to stored_row.
🪄 Autofix
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 Plus
Run ID: 2ab2f9a1-982e-4f21-b52b-693023f31c88
📒 Files selected for processing (5)
docker/postgres/init/02_schema.sqlpg_llm_batch/db.pypg_llm_batch/schema.sqltests/test_integration_pg.pytests/test_remote_batch_progress_invariant.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_integration_pg.py
- pg_llm_batch/schema.sql
- docker/postgres/init/02_schema.sql
1 participant
Data-integrity gap
Issue #135 identified two durable lifecycle defects: a single provider observation could carry
completed + failed > total, and independent monotonicGREATEST(...)updates could combine individually valid observations into an impossible stored projection.The branch now also carries the smallest repository-owned repair lane for a current required-Strix finding on
db.apply_schema(): the low-level helper still accepts an arbitrary caller-selected SQL file path even though the supported CLI always applies the packaged schema and the helper docstring itself describes packaged-schema authority. This is being handled test-first on this already-authoritativedb.pybranch rather than creating a competing writer.Test-first implementation
4a8ba6f138bbddf6f27c7d4e1a01e59d725708b3added focused regressions for same-observation inconsistency and the combined monotonic SQL invariant.c0f278d584fae33f5fab4001119f8aa8be8f1982validates explicit provider counts before PostgreSQL access and adds one atomicON CONFLICT ... DO UPDATE ... WHEREguard so monotonic completed/failed enrichment cannot exceed the monotonic total.total_requests_knownas persistence-internal evidence, reports skipped guarded updates by rereading the durable row, and makes package/container schema mirrors migration-safe.apply_schema(..., schema_path)remains possible before the narrow authority fix.The bounded lifecycle contract preserves sparse provider observations, tenant/RLS identity, terminal-state monotonicity, public return shape, and existing schema identity. The schema-application hardening does not add release authority or change the supported
init-dbCLI target:python -m pg_llm_batch init-dbalready callsdb.apply_schema(dsn)without a path override.Current exact gate boundary
At the latest inspection, exact-head repository CI, Security Scan, SAST Semgrep, Release Acceptance, coverage evidence, and required review/runtime checks were terminal-success except required Strix. Strix's quick scan experienced provider 429s but ultimately emitted one substantive HIGH finding: arbitrary caller-controlled
schema_pathcan select SQL text executed by Psycopg. Repository inspection confirmsapply_schemais not exported frompg_llm_batch.__init__, the CLI exposes no schema-path argument, and repository documentation describes packaged-schema application. The finding is therefore being treated as a valid least-authority defect rather than dismissed as infrastructure noise.Merge boundary
Keep Draft while the new RED→GREEN schema-authority repair and exact-head gates are incomplete. Promote only when the unchanged exact head is mergeable against the then-live protected base, every required current-head workflow/security/coverage/package/provenance/release gate is terminal-success, review threads are clear, and live rules are satisfied. Queued, pending, cancelled, skipped-required, absent, neutral-required, stale, predecessor, synthetic, status-only, author-only, infrastructure-only, rate-limited, or failed evidence is not acceptance.
Refs #135.