feat(durable): define generation-fenced conversion attempt record - #371
Open
seonghobae wants to merge 5 commits into
Open
feat(durable): define generation-fenced conversion attempt record#371seonghobae wants to merge 5 commits into
seonghobae wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough변환 시도의 영속 상태와 불변 레코드를 추가했다. 레코드는 생성 입력을 검증하고 작업·세대·lease 권한을 확인한다. 성공 또는 실패 상태로 종료하며, 동일한 종료 결과는 멱등적으로 처리한다. 관련 동작을 JUnit 테스트로 검증한다. Changes변환 시도 생명주기
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
seonghobae
marked this pull request as ready for review
August 11, 2026 02:08
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/clearfolio/viewer/durable/ConversionAttemptRecord.java (1)
129-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueJavaDoc에 null 후보 동작을 명시하세요.
authorizes는candidateJobId또는candidateLeaseId가 null이면false를 반환합니다. 이 동작은 필드가 항상 non-null이기 때문에 성립합니다. 현재 JavaDoc은 이 계약을 기술하지 않습니다. 향후 비교 순서를 바꾸는 리팩터링이 NPE를 유발할 수 있습니다. 계약을 문서로 고정하세요.📝 제안 변경
* `@param` candidateJobId candidate permanently reserved job identifier * `@param` candidateGeneration candidate lifecycle generation * `@param` candidateLeaseId candidate worker-lease identifier - * `@return` true only when job, generation, and lease all match exactly + * `@return` true only when job, generation, and lease all match exactly; + * false when a candidate identifier is null */🤖 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/main/java/com/clearfolio/viewer/durable/ConversionAttemptRecord.java` around lines 129 - 144, Update the JavaDoc for ConversionAttemptRecord.authorizes to explicitly state that it returns false when candidateJobId or candidateLeaseId is null, while retaining the existing exact-match contract for non-null candidates.src/test/java/com/clearfolio/viewer/durable/ConversionAttemptRecordTest.java (1)
136-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value예외 메시지를 함께 검증하세요.
finish는 서로 다른 두 원인으로IllegalArgumentException을 던집니다. 하나는 비종료 상태이고, 다른 하나는 완료 시각이 청구 시각보다 앞선 경우입니다. 현재 테스트는 예외 타입만 확인합니다. 두 메시지가 뒤바뀌어도 테스트는 통과합니다. 메시지를 함께 검증하면 실패 원인을 정확히 고정할 수 있습니다.♻️ 제안 변경
- assertThrows(IllegalArgumentException.class, - () -> record.finish(ConversionAttemptState.CLAIMED, claimedAt)); - assertThrows(IllegalArgumentException.class, - () -> record.finish(ConversionAttemptState.SUCCEEDED, claimedAt.minusNanos(1L))); + assertEquals("terminalState must be terminal", + assertThrows(IllegalArgumentException.class, + () -> record.finish(ConversionAttemptState.CLAIMED, claimedAt)).getMessage()); + assertEquals("completionTime must not precede claimedAt", + assertThrows(IllegalArgumentException.class, + () -> record.finish(ConversionAttemptState.SUCCEEDED, claimedAt.minusNanos(1L))) + .getMessage());동일한 방식을
claimFailsClosedForMissingOrNonPositiveAuthority의generation과attempt검증에도 적용할 수 있습니다.🤖 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/test/java/com/clearfolio/viewer/durable/ConversionAttemptRecordTest.java` around lines 136 - 139, Update the assertions in ConversionAttemptRecordTest around finish so each IllegalArgumentException also verifies the expected message for the non-terminal state and the completion timestamp preceding claimedAt. Apply the same message-assertion pattern to generation and attempt validation in claimFailsClosedForMissingOrNonPositiveAuthority, preserving the existing exception-type checks.
🤖 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/main/java/com/clearfolio/viewer/durable/ConversionAttemptRecord.java`:
- Around line 129-144: Update the JavaDoc for ConversionAttemptRecord.authorizes
to explicitly state that it returns false when candidateJobId or
candidateLeaseId is null, while retaining the existing exact-match contract for
non-null candidates.
In
`@src/test/java/com/clearfolio/viewer/durable/ConversionAttemptRecordTest.java`:
- Around line 136-139: Update the assertions in ConversionAttemptRecordTest
around finish so each IllegalArgumentException also verifies the expected
message for the non-terminal state and the completion timestamp preceding
claimedAt. Apply the same message-assertion pattern to generation and attempt
validation in claimFailsClosedForMissingOrNonPositiveAuthority, preserving the
existing exception-type checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aec4837-552e-481c-8a65-aa6651979aa2
📒 Files selected for processing (3)
src/main/java/com/clearfolio/viewer/durable/ConversionAttemptRecord.javasrc/main/java/com/clearfolio/viewer/durable/ConversionAttemptState.javasrc/test/java/com/clearfolio/viewer/durable/ConversionAttemptRecordTest.java
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.
Objective
Advance issue #312 with a path-disjoint durable attempt-state primitive. One claimed conversion attempt must bind immutable job/generation/attempt/lease identity, and once that attempt reaches a terminal outcome a stale or contradictory completion must not rewrite it.
Test-first state
This Draft starts intentionally RED at exact test-only head
66bcbe6f74dcad76d358b2979ba7e1a202b960ccon protectedmain55d7ae8647208e301f282350f076eeddaba61d11.ConversionAttemptRecordTestrequires exact attempt/job/generation/lease authority, positive generation and attempt numbers, persisted claim/completion timestamps, exact publication fencing, monotonic terminal outcomes, idempotent exact terminal replay, and fail-closed invalid input. The production types do not exist at this head, so Maven test compilation is expected to fail at that exact boundary.Scope
New durable-domain attempt state/record plus focused tests only. This does not persist attempts, claim a broker message, replace the process-local worker, wire PR #367's lease object, schedule PR #369 retries, publish artifacts, implement cancellation, or change HTTP acceptance. It is deliberately separate from the current idempotency writer and from the outbox/retry/cancellation PR paths.
Acceptance
Observe exact-head RED -> smallest immutable production implementation -> exact-head
mvn -B --no-transfer-progress verifywith zero missed owned production coverage/public Javadocs -> CI/Security Scan/SAST/fuzz -> current reviews/threads/live-base refetch. Keep Draft until GREEN exact-head evidence exists. Independent write-authorized approval remains a separate protected-merge gate.Summary by CodeRabbit
새로운 기능
버그 수정