fix: make provider batch apply and undo crash-safe#1336
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough新增 Provider 批量应用的 PostgreSQL 持久化账本，支持幂等应用、预映像一致性校验、结果重放与撤销，并接入 action、repository、API 错误映射、本地化文案及测试体系。 ChangesProvider 批量应用账本
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a durable ledger for provider batch apply operations by adding the provider_batch_apply_operations table and implementing transaction-backed batch apply, replay, and undo mechanisms. Feedback on these changes includes replacing JSON.stringify with a robust deep equality helper in isProviderBatchPreimageValueEqual to avoid false-positive stale states from object key ordering, and removing the inline DELETE from findProviderBatchApplyOperation to keep the query read-only and compatible with read-replicas.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/unit/repository/provider-batch-preimage-cas.test.ts (1)
24-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议在 Mock 数据中补充
isEnabled字段以提升测试健壮性目前的
makeLockedProvider没有返回isEnabled属性，这意味着在执行“returns stale when provider enablement changed after preview”测试时，系统会将undefined（当前值）与false（预期值）进行对比。虽然测试确实能因此返回stale并通过，但这属于非预期的巧合。建议显式加上isEnabled: true以对齐真实的Provider数据结构。💡 建议的修改
function makeLockedProvider(id: number, groupTag: string | null) { return { id, name: `Provider-${id}`, url: "https://api.example.com/v1", key: "sk-test", providerVendorId: 1, providerType: "codex", + isEnabled: true, groupTag, priority: 1, weight: 100,🤖 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/unit/repository/provider-batch-preimage-cas.test.ts` around lines 24 - 40, Update the makeLockedProvider test fixture to include an explicit isEnabled: true property, matching the real Provider structure and ensuring enablement-change assertions compare defined boolean values.
🤖 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/actions/providers.ts`:
- Around line 2515-2538: Update the undo flow around undoProviderBatchOperation
so post-commit side effects such as providerPatchUndoStore.delete, cache
broadcasting, or circuit-breaker handling are isolated in error handling that
records failures without propagating them as undo failures. Once the database
restoration succeeds, preserve the successful response and revertedCount even if
token cleanup or other post-commit processing throws, while retaining conflict
and expired handling.
- Around line 2025-2035: Update the newly added error branches in the relevant
provider action flows, including the idempotency-conflict and stale-preview
returns, to expose only stable errorCode and errorParams values instead of
hardcoded Chinese error text. Ensure the presentation boundary translates these
codes with next-intl for zh-CN, zh-TW, en, ja, and ru; leave existing
user-facing strings in the file unchanged and keep the broader Server Action
i18n migration separate.
- Around line 1634-1640: 调整 providers 更新流程中 undoValues 的构造，使其仅记录后续 updateGroups
实际应用的字段；复用 2360-2401 附近按 provider 类型筛选和跳过字段的逻辑，不要直接遍历所有 rows 写入撤销快照。保留 values
的现有构造行为，并确保撤销时不会恢复未实际应用的字段。
- Around line 2130-2149: 在批量调用 forceCloseCircuitState 前，针对每个 providerId
读取并确认当前熔断器阈值仍表示禁用状态，不要仅依据
operation.postCommitEffects.nextCircuitBreakerFailureThreshold 的旧值执行关闭；仅对当前仍禁用的
provider 调用 forceCloseCircuitState，并保留现有失败告警处理。
- Line 83: 将 providers 导入列表中的 updateProvidersBatch
从类型导入改为值导入，确保调用该函数的运行时代码能够正确解析；保留其他仅用于类型的导入不变。
In `@tests/unit/actions/providers-apply-engine.test.ts`:
- Line 11: 删除 providers-apply-engine.test.ts 模块作用域内重复的 redisStore const
声明，仅保留一份共享声明，确保同一作用域不再出现重复标识符并恢复测试编译。
In `@tests/unit/actions/providers-patch-actions-contract.test.ts`:
- Around line 12-20: 为 apply ledger mock 抽取共享的 ApplyLedgerResult 类型，具体声明
applyResult 及其 undoToken、operationId 字段，并将其复用于
tests/unit/actions/providers-patch-actions-contract.test.ts（12-20）和
tests/unit/actions/providers-undo-engine.test.ts（14-22）的 result 类型，避免嵌套属性被推断为
unknown。
---
Nitpick comments:
In `@tests/unit/repository/provider-batch-preimage-cas.test.ts`:
- Around line 24-40: Update the makeLockedProvider test fixture to include an
explicit isEnabled: true property, matching the real Provider structure and
ensuring enablement-change assertions compare defined boolean values.
🪄 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: f7b427a0-8ac7-4855-ab34-a95efc1b1f55
📒 Files selected for processing (19)
drizzle/0109_nice_shriek.sqldrizzle/meta/0109_snapshot.jsondrizzle/meta/_journal.jsonpackage.jsonsrc/actions/providers.tssrc/app/api/v1/resources/providers/handlers.tssrc/drizzle/schema.tssrc/lib/provider-batch-patch-error-codes.tssrc/repository/provider.tstests/README.mdtests/configs/provider-batch-ledger-postgres.config.tstests/integration/provider-batch-ledger-postgres.test.tstests/tsconfig.provider-batch-ledger.jsontests/unit/actions/providers-apply-engine.test.tstests/unit/actions/providers-patch-actions-contract.test.tstests/unit/actions/providers-preview-engine.test.tstests/unit/actions/providers-undo-engine.test.tstests/unit/repository/provider-batch-ledger.test.tstests/unit/repository/provider-batch-preimage-cas.test.ts
![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: c92d84915a
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The durable PostgreSQL ledger design is sound: claim insertion, complete-preimage validation under FOR UPDATE, effective-subset writes, and the applying->applied status transition all live in a single transaction, so a stuck applying row is impossible under normal PostgreSQL semantics (commit is all-or-nothing). Atomic undo (row restoration + token consumption in one transaction), single-consumption previews via unique indexes, and sensitive-URL stripping from durable payloads are all correctly implemented and well-tested. The 60s preview TTL is safely shorter than the 24h ledger TTL, so a ledger row can never outlive its preview and be re-consumed.
PR Size: XL
- Lines changed: 7732 (7446 additions, 286 deletions)
- Files changed: 20
Split suggestions (informational; the PR is cohesive as-is): if future iterations grow this surface further, the durable-ledger repository layer (applyProviderBatchOperationIfUnchanged, undoProviderBatchOperation, findProviderBatchApplyOperation), the actions-layer orchestration (applyProviderBatchPatch/undoProviderPatch rewiring + runProviderBatchPostCommitEffects), and the migration/schema could land as separate PRs to shrink the blast radius of each.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (parameterized queries, fingerprint regex-validated, idempotencyKey length-capped at 128)
- Error handling - Clean (all post-commit effects in the apply path are try/caught + logged; preimage mismatches surface PREVIEW_STALE)
- Type safety - Clean
- Documentation accuracy - Clean (comments match behavior, including the deliberately-cautious "applying" note)
- Test coverage - Strong (62 unit tests + PostgreSQL integration suite covering concurrency, rollback, replay, atomic undo, and failed-undo rollback)
- Code clarity - Good
Automated review by Claude AI
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/provider-batch-patch-error-codes.ts (1)
12-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win将敏感字段集合设为只读
导出的
Set仍可被其他模块调用clear()或add()；一旦被意外修改，proxyUrl或mcpPassthroughUrl可能重新进入持久化撤销载荷，违反本 PR 的脱敏保证。至少将其声明为ReadonlySet<string>，让 TypeScript 阻止后续调用方修改集合。建议修改
-export const SENSITIVE_PROVIDER_BATCH_UNDO_KEYS = new Set(["proxyUrl", "mcpPassthroughUrl"]); +export const SENSITIVE_PROVIDER_BATCH_UNDO_KEYS: ReadonlySet<string> = new Set([ + "proxyUrl", + "mcpPassthroughUrl", +]);🤖 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/lib/provider-batch-patch-error-codes.ts` at line 12, Update SENSITIVE_PROVIDER_BATCH_UNDO_KEYS to expose a ReadonlySet<string> type, preventing consumers from calling mutating Set methods while preserving the existing sensitive keys and membership behavior.
🤖 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.
Nitpick comments:
In `@src/lib/provider-batch-patch-error-codes.ts`:
- Line 12: Update SENSITIVE_PROVIDER_BATCH_UNDO_KEYS to expose a
ReadonlySet<string> type, preventing consumers from calling mutating Set methods
while preserving the existing sensitive keys and membership behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4de314e9-3b10-46b3-914c-3ae95053d150
📒 Files selected for processing (21)
messages/en/settings/providers/batchEdit.jsonmessages/en/settings/providers/batchTest.jsonmessages/ja/settings/providers/batchEdit.jsonmessages/ja/settings/providers/batchTest.jsonmessages/ru/settings/providers/batchEdit.jsonmessages/ru/settings/providers/batchTest.jsonmessages/zh-CN/settings/providers/batchEdit.jsonmessages/zh-CN/settings/providers/batchTest.jsonmessages/zh-TW/settings/providers/batchEdit.jsonmessages/zh-TW/settings/providers/batchTest.jsonsrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/batch-edit/provider-batch-dialog.tsxsrc/app/[locale]/settings/providers/_components/batch-test/batch-test-dialog.tsxsrc/app/api/v1/resources/providers/handlers.tssrc/lib/provider-batch-error-i18n.tssrc/lib/provider-batch-patch-error-codes.tssrc/repository/provider.tstests/unit/actions/provider-undo-edit.test.tstests/unit/actions/providers-patch-actions-contract.test.tstests/unit/actions/providers-undo-engine.test.tstests/unit/repository/provider-batch-preimage-cas.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/unit/actions/provider-undo-edit.test.ts
- src/app/api/v1/resources/providers/handlers.ts
- tests/unit/actions/providers-undo-engine.test.ts
- tests/unit/actions/providers-patch-actions-contract.test.ts
- tests/unit/repository/provider-batch-preimage-cas.test.ts
- src/repository/provider.ts
- src/actions/providers.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf74ff8588
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d81bb210d8
ℹ️ 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".
d81bb21 to
39ed2a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0db39a9ef
ℹ️ 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".
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
What changed
Related
Why
The existing preview/apply flow could validate against fresh state outside the write transaction. Concurrent provider edits or repeated execution of the same preview could therefore overwrite newer values or perform the same approved operation more than once.
The previous undo flow also consumed its token before restoring provider rows, leaving a crash window where the token was permanently consumed but the provider changes were not reverted.
Behavior
Validation
bun run typecheckgit diff --checkGreptile Summary
This PR hardens the provider batch preview/apply/undo pipeline by replacing the previous in-memory undo snapshot store with a durable PostgreSQL ledger, making the flow resilient to process crashes and concurrent edits.
applyingtoapplied— all or nothing.FOR UPDATE, applies provider restoration, and marks the undo token consumed in the same commit, eliminating the prior crash window.claimKey/previewToken/payload replay the committed result from the ledger; sensitive proxy URLs are stripped from the durable undo preimage; expired-row cleanup is moved outside the critical transaction to avoid blocking provider locks on large backlogs.Confidence Score: 5/5
The core transactional correctness is solid — provider locks are acquired in a consistent order, the preimage CAS check is complete, and undo token consumption is committed atomically with provider restoration. The two findings are quality concerns rather than functional defects on any live path.
The apply and undo transactions are both correctly structured, the shared SENSITIVE_PROVIDER_BATCH_UNDO_KEYS constant resolves the prior duplicate-literal drift risk, and all new repository status values are properly handled in the action layer. The exported-but-unused consumeProviderBatchUndo is a cleanup concern, not a runtime defect; the missing DB-level CHECK on the status column is a defence-in-depth gap, not a broken invariant.
src/repository/provider.ts — the exported consumeProviderBatchUndo function should be reviewed for removal. src/drizzle/schema.ts — the status column would benefit from a CHECK constraint to match the project's use of pgEnum elsewhere.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant A as Action Layer participant L as Ledger (PG) participant P as Providers (PG) Note over C,P: Apply path C->>A: applyProviderBatchPatch(claimKey, previewToken, payload) A->>L: findProviderBatchApplyOperation alt replay or conflict L-->>A: "replay | idempotency_conflict | preview_consumed" A-->>C: cached result or 409 else not_found A->>L: "BEGIN TX INSERT ledger status=applying" L-->>A: inserted A->>P: SELECT FOR UPDATE ORDER BY id A->>P: validate preimage CAS A->>P: UPDATE effective providers A->>L: "UPDATE ledger status=applied" A->>L: COMMIT A-->>C: 200 operationId undoToken end Note over C,P: Undo path (durable) C->>A: undoProviderPatch(undoToken, operationId) A->>L: findProviderBatchUndoOperation A->>L: BEGIN TX SELECT FOR UPDATE ledger row A->>P: UPDATE providers to preimage A->>L: "UPDATE ledger undo_consumed_at=now" A->>L: COMMIT A-->>C: 200 revertedCountReviews (5): Last reviewed commit: "Merge dev into provider batch safety" | Re-trigger Greptile