fix: preserve concurrent execution checkpoints - #1609
Merged
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (46)
📝 WalkthroughWalkthrough
Changes체크포인트 병합
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ExecutionManagerImpl
participant DrizzleExecutionStore
participant PostgreSQL
Caller->>ExecutionManagerImpl: checkpoint(id, key, value)
ExecutionManagerImpl->>DrizzleExecutionStore: mergeCheckpoint(id, key, value)
DrizzleExecutionStore->>PostgreSQL: JSONB key merge UPDATE
PostgreSQL-->>DrizzleExecutionStore: Updated execution
DrizzleExecutionStore-->>ExecutionManagerImpl: Execution
ExecutionManagerImpl-->>Caller: Execution
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
📊 Benchmark Results✅ All benchmarks passed
Updated: 2026-07-30T05:32:47.923Z · Commit: 660f222 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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
`@packages/create-croco-app/templates/saas/apps/api-server/src/inMemoryAdapters.ts`:
- Around line 404-416: Replace generic missing-execution Errors with
ExecutionProblems.notFound(...) across mergeCheckpoint in
packages/create-croco-app/templates/saas/apps/api-server/src/inMemoryAdapters.ts
and the affected test helpers in
packages/admin-ops/src/tests/RetryConsole.spec.ts,
packages/batch-core/src/tests/ChunkExecutor.spec.ts,
packages/batch-qstash/src/tests/InMemoryContinuationStore.ts,
packages/cli/src/tests/integration/jobs-e2e.spec.ts, and
packages/cli/src/tests/jobs.spec.ts; ensure InMemoryContinuationStore.required()
also uses this standardized problem.
In `@packages/execution-core/package.json`:
- Line 39: Update the esbuild dependency declaration in package.json from the
current ^0.20.2 range to a patched version range starting at ^0.25.0, preferably
the current ^0.28.1 release. Keep the dependency entry otherwise unchanged.
In `@packages/execution-core/src/libs/interfaces/ExecutionStore.ts`:
- Around line 58-67: Update the mergeCheckpoint contract documentation to add an
`@throws` entry stating that it throws an Error when the target execution does not
exist or the merge fails, matching the behavior documented for create, update,
appendLog, and delete. Keep the existing atomic merge and ordering guarantees
unchanged.
In `@packages/execution-drizzle/package.json`:
- Around line 43-46: Update the dependency entries in the package manifest so
vitest, pg, and `@types/pg` use the workspace catalog reference consistently with
drizzle-orm. Add their versions to the catalog configuration in
pnpm-workspace.yaml, then replace the hardcoded versions in the manifest with
catalog references.
In `@packages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.ts`:
- Around line 107-124: Update the checkpoint writer blocking flow around
blockedWrite and waitUntilCheckpointWriterBlocks so timeout failures always
settle blockedWrite before cleanup. Ensure the second client’s in-flight query
is completed or safely cancelled before secondClient.release(), while preserving
the existing commit/rollback behavior and successful blocked-write path.
- Around line 103-104: In the test around firstWrite and secondWrite, validate
that writes contains exactly the expected number of entries before indexing it,
instead of relying on as ExecutionCheckpointWrite assertions. Keep the
subsequent key checks unchanged so insufficient writes produce a clear assertion
failure rather than a TypeError.
In `@packages/execution-drizzle/src/tests/DrizzleExecutionStore.spec.ts`:
- Around line 730-733: In the test around mergeCheckpoint, replace the manual
Error guard for written.checkpoints with a Vitest
expect(expression).toBeDefined() assertion, then continue converting the
expression to SQL as before.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e1b917f6-97d9-4959-8409-c8f2ff092d9f
⛔ Files ignored due to path filters (2)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/atomic-execution-checkpoints.md.github/workflows/ci.ymldocs/problem-code-registry.jsonpackages/admin-ops/src/tests/RetryConsole.spec.tspackages/batch-core/src/tests/ChunkExecutor.spec.tspackages/batch-qstash/src/tests/InMemoryContinuationStore.tspackages/cli/src/tests/integration/jobs-e2e.spec.tspackages/cli/src/tests/jobs.spec.tspackages/create-croco-app/templates/saas/apps/api-server/src/inMemoryAdapters.tspackages/docs/src/content/docs/api/execution-core/src/classes/ExecutionProblems.mdpackages/docs/src/content/docs/api/execution-core/src/classes/ExecutionStore.mdpackages/docs/src/content/docs/api/execution-core/src/enumerations/ExecutionProblemCode.mdpackages/docs/src/content/docs/api/execution-core/src/functions/createExecutionCheckpointStoreConformanceSuite.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointConcurrencyResult.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceCase.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceOptions.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceSuite.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointWrite.mdpackages/docs/src/content/docs/api/execution-drizzle/src/classes/DrizzleExecutionStore.mdpackages/docs/src/content/docs/api/problems-core/src/variables/CROCO_PROBLEM_CODE_REGISTRY.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/execution-core/README.mdpackages/execution-core/package.jsonpackages/execution-core/src/index.tspackages/execution-core/src/libs/ExecutionManagerImpl.tspackages/execution-core/src/libs/ExecutionProblem.tspackages/execution-core/src/libs/conformance.tspackages/execution-core/src/libs/interfaces/ExecutionStore.tspackages/execution-core/src/tests/BrowserEntry.spec.tspackages/execution-core/src/tests/ExecutionContinuation.spec.tspackages/execution-core/src/tests/ExecutionManagerImpl.spec.tspackages/execution-drizzle/package.jsonpackages/execution-drizzle/src/libs/DrizzleExecutionStore.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.spec.tspackages/tasks-core/src/tests/TaskRunner.integration.spec.tspackages/workflow-core/src/tests/WorkflowCore.spec.tspublic-api-surface.snapshot.jsonscripts/workflow-verification-contract.mts
kang-heewon
force-pushed
the
fix/1596-atomic-checkpoints
branch
from
July 29, 2026 15:08
b49c18f to
2d80710
Compare
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
kang-heewon
force-pushed
the
fix/1596-atomic-checkpoints
branch
from
July 29, 2026 16:07
2d80710 to
13b6ff4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@packages/execution-core/src/tests/ExecutionContinuation.spec.ts`:
- Around line 54-57: Replace the generic Error thrown by
ExecutionStore.mergeCheckpoint when the execution ID is missing with
ExecutionProblems.notFound(...). Also update the unexpected-call mock path at
packages/execution-core/src/tests/ExecutionContinuation.spec.ts#L651-L653 to
throw the appropriate ExecutionProblems subclass, ensuring both sites throw only
RFC 7807 Problem-based errors.
In `@packages/execution-core/src/tests/ExecutionManagerImpl.spec.ts`:
- Around line 108-112: Update mergeCheckpoint in the test mock to throw the
appropriate missing-execution ExecutionProblem from the existing
ExecutionProblems factory or Problem subclass instead of a generic Error,
preserving the current not-found condition and message/context.
In `@packages/execution-drizzle/src/libs/DrizzleExecutionStore.ts`:
- Around line 279-285: Update mergeCheckpoint so its value input is restricted
to the checkpoint contract’s serializable type, or consistently reject
undefined, bigint, and cyclic values with the established ExecutionProblem
checkpoint-conformance Problem before persistence. Ensure JSON.stringify cannot
silently produce an empty object for undefined or leak a raw TypeError for
unsupported values.
In `@packages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.ts`:
- Line 29: Replace the direct Error throw in the row-lock timeout path of the
PostgreSQL checkpoint-writer test with Vitest’s assert.fail(...), preserving the
existing timeout message so the failure is reported as an explicit test
assertion.
- Around line 43-79: Recreate the DrizzleExecutionStore instance for every test:
keep pool and schema setup in beforeAll, but after truncating executions in
beforeEach assign store from a fresh DrizzleExecutionStore built with
drizzle(pool). This ensures createStore: () => store uses an isolated store
instance per test.
- Around line 1-8: DrizzleExecutionStore.postgres 테스트의 import 순서를 가이드라인에 맞게
정리하세요. `@croco/*` 패키지 import를 외부 패키지 import보다 앞에 배치하고, `Execution`과
`PoolClient`는 각각 value import에서 분리해 `import type` 그룹으로 이동하세요. 기존 value import와
테스트 동작은 변경하지 마세요.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ab15eed5-bb41-4f19-a808-9cacbc29f4fb
⛔ Files ignored due to path filters (2)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.changeset/atomic-execution-checkpoints.md.github/workflows/ci.ymldocs/problem-code-registry.jsonpackages/admin-ops/src/tests/RetryConsole.spec.tspackages/batch-core/src/tests/ChunkExecutor.spec.tspackages/batch-qstash/src/tests/InMemoryContinuationStore.tspackages/cli/src/tests/integration/jobs-e2e.spec.tspackages/cli/src/tests/jobs.spec.tspackages/create-croco-app/templates/saas/apps/api-server/src/inMemoryAdapters.tspackages/docs/src/content/docs/api/execution-core/src/classes/ExecutionProblems.mdpackages/docs/src/content/docs/api/execution-core/src/classes/ExecutionStore.mdpackages/docs/src/content/docs/api/execution-core/src/enumerations/ExecutionProblemCode.mdpackages/docs/src/content/docs/api/execution-core/src/functions/createExecutionCheckpointStoreConformanceSuite.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointConcurrencyResult.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceCase.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceOptions.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointStoreConformanceSuite.mdpackages/docs/src/content/docs/api/execution-core/src/type-aliases/ExecutionCheckpointWrite.mdpackages/docs/src/content/docs/api/execution-drizzle/src/classes/DrizzleExecutionStore.mdpackages/docs/src/content/docs/api/problems-core/src/variables/CROCO_PROBLEM_CODE_REGISTRY.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/execution-core/README.mdpackages/execution-core/package.jsonpackages/execution-core/src/index.tspackages/execution-core/src/libs/ExecutionManagerImpl.tspackages/execution-core/src/libs/ExecutionProblem.tspackages/execution-core/src/libs/conformance.tspackages/execution-core/src/libs/interfaces/ExecutionStore.tspackages/execution-core/src/tests/BrowserEntry.spec.tspackages/execution-core/src/tests/ExecutionContinuation.spec.tspackages/execution-core/src/tests/ExecutionManagerImpl.spec.tspackages/execution-drizzle/package.jsonpackages/execution-drizzle/src/libs/DrizzleExecutionStore.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.spec.tspackages/tasks-core/src/tests/TaskRunner.integration.spec.tspackages/workflow-core/src/tests/WorkflowCore.spec.tspublic-api-surface.snapshot.jsonscripts/tests/ci-workflow.spec.tsscripts/workflow-verification-contract.mts
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.ts (1)
87-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win두 번째 커넥션 획득 실패 시 첫 커넥션이 누수됩니다.
두
pool.connect()호출이try앞에 있으므로 Line 88에서 실패하면 Line 87에서 획득한firstClient가release()되지 않습니다. 커넥션 획득을try내부로 옮기고,finally에서 실제로 획득된 클라이언트만 조건부 release하세요.🤖 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 `@packages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.ts` around lines 87 - 94, Move the pool.connect calls in the contention test setup inside the existing try block, and track each client as it is acquired. In the finally block, conditionally release only clients that were successfully obtained, including firstClient when secondClient acquisition fails.
🤖 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 `@packages/execution-drizzle/src/libs/DrizzleExecutionStore.ts`:
- Around line 25-43: 정의되지 않은 unknown 체크포인트 계약으로 인해 저장소별 값 처리 방식이 달라집니다.
DrizzleExecutionStore의 serializeCheckpoint에 공통 JSON-safe 타입과 validator를 도입해 거부 및
정규화 정책을 명시하고,
packages/execution-drizzle/src/libs/DrizzleExecutionStore.ts:25-43에서 이를 사용하세요.
packages/execution-core/src/tests/ExecutionContinuation.spec.ts:55-64의 메모리 저장소에도
동일한 검증을 적용하고,
packages/execution-core/src/tests/ExecutionManagerImpl.spec.ts:109-122의
MockExecutionStore가 실제 어댑터와 동일한 값 경계를 재현하도록 수정하세요.
---
Outside diff comments:
In `@packages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.ts`:
- Around line 87-94: Move the pool.connect calls in the contention test setup
inside the existing try block, and track each client as it is acquired. In the
finally block, conditionally release only clients that were successfully
obtained, including firstClient when secondClient acquisition fails.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e9938919-e2da-4df1-a714-898649c7ebef
📒 Files selected for processing (8)
.changeset/atomic-execution-checkpoints.mdpackages/execution-core/src/tests/ExecutionContinuation.spec.tspackages/execution-core/src/tests/ExecutionManagerImpl.spec.tspackages/execution-drizzle/src/libs/DrizzleExecutionStore.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.postgres.spec.tspackages/execution-drizzle/src/tests/DrizzleExecutionStore.spec.tsscripts/create-croco-app-generated-smoke.mtsscripts/tests/create-croco-app-generated-smoke.spec.ts
kang-heewon
force-pushed
the
fix/1596-atomic-checkpoints
branch
from
July 29, 2026 18:07
a7f4205 to
a2814c8
Compare
kang-heewon
force-pushed
the
fix/1596-atomic-checkpoints
branch
2 times, most recently
from
July 30, 2026 04:20
b5356a3 to
e62017f
Compare
kang-heewon
force-pushed
the
fix/1596-atomic-checkpoints
branch
from
July 30, 2026 05:23
dc51a4f to
b7aeaea
Compare
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.
Outcome
Execution checkpoint writes now delegate to a store-level atomic key merge instead of a manager-side read-modify-write cycle. Concurrent writes to different checkpoint keys preserve both values, while same-key writes use the store serialization order and retain the last applied mutation.
The Drizzle adapter performs one PostgreSQL JSONB merge update. A shared, browser-compatible conformance suite now exercises in-memory and durable stores, and CI runs the real PostgreSQL contention cases.
Fixes #1596
Verification
pnpm --filter @croco/execution-core test— 93/93 passed, including browser bundle coveragepnpm --filter @croco/execution-drizzle test— 47 passed; 2 credential-gated PostgreSQL tests skippedpnpm problem-registry:check— 512/512 discoveries matched current trunkpnpm public-api:check— 114 package snapshots matchedpnpm changeset-required:check— affected publishable packages coveredpnpm check— 22 repository gates passed; one not applicable, and the benchmark gate was invalidated by concurrent machine load; its focused rerun passed all thresholdsReview gates
Residual risk
Custom
ExecutionStoreadapters must implementmergeCheckpoint()as a storage-level atomic operation. The conformance suite detects lost different-key updates, but deterministic caller-order semantics for concurrent same-key writes require a stronger adapter-specific version or compare-and-swap contract.Summary by CodeRabbit