fix(reliability): reconcile artifact lifecycle lock on current main - #439
fix(reliability): reconcile artifact lifecycle lock on current main#439seonghobae wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 41 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)
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 |
|
Exact-current-head review request for @opencode-agent review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Repository identity =='
git rev-parse HEAD
git merge-base HEAD 961f44a19c4ced87f8062b3ab9842a9a08d05235
git diff --name-status 961f44a19c4ced87f8062b3ab9842a9a08d05235...HEAD
echo '== Changed Java files =='
fd -t f 'ArtifactLifecycleLockRegistry.*\.java$' .
echo '== AST outlines =='
while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done < <(fd -t f 'ArtifactLifecycleLockRegistry.*\.java$' .)
echo '== Exact diff =='
git diff --find-renames --find-copies --unified=80 \
961f44a19c4ced87f8062b3ab9842a9a08d05235...HEAD -- \
'*ArtifactLifecycleLockRegistry*.java'
echo '== Registry references =='
rg -n -C 4 --glob '*.java' \
'\bArtifactLifecycleLockRegistry\b|\bwithLock\s*\(|\bexecute\s*\(' . || trueLength of output: 1649 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='d7e3e6e7b54bdc70a05f18ff09338b9ab54aaeed'
main_file='src/main/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistry.java'
test_file='src/test/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistryTest.java'
echo '== Production outline =='
ast-grep outline "$main_file" --items all
echo '== Test outline =='
ast-grep outline "$test_file" --items all
echo '== Exact production and test diff =='
git diff --unified=100 "$base...$head" -- "$main_file" "$test_file"
echo '== Production file =='
cat -n "$main_file"
echo '== Focused test file =='
cat -n "$test_file"
echo '== Non-test registry references =='
rg -n -C 6 --glob '*.java' --glob '!src/test/**' \
'\bArtifactLifecycleLockRegistry\b' src
echo '== Registry method call sites =='
rg -n -C 4 --glob '*.java' \
'\.(withLock|execute|runWithLock|lock)\s*\(' src || trueLength of output: 26314 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Lifecycle and artifact-related Java files =='
fd -t f -e java . src/main | rg -i '(artifact|lifecycle|conversion|document|delete|cleanup|storage)'
echo '== Candidate operation declarations and calls =='
rg -n -i -C 5 --glob '*.java' --glob '!src/test/**' \
'\b(delete|remove|cleanup|convert|conversion|upload|write|store|save)\w*\s*\(' src/main
echo '== Artifact byte and object-storage references =='
rg -n -i -C 5 --glob '*.java' --glob '!src/test/**' \
'(artifact|object.?storage|blob|s3|bucket|fileSystem|filesystem|deleteObject|putObject)' src/main
echo '== Existing synchronization and lifecycle lock mechanisms =='
rg -n -C 5 --glob '*.java' --glob '!src/test/**' \
'(synchronized\b|ReentrantLock|ReadWriteLock|Semaphore|Lock\b|mutex|lifecycle)' src/mainLength of output: 201344 🏁 Script executed: #!/bin/bash
set -euo pipefail
service='src/main/java/com/clearfolio/viewer/service/DefaultDocumentConversionService.java'
worker='src/main/java/com/clearfolio/viewer/service/DefaultConversionWorker.java'
store='src/main/java/com/clearfolio/viewer/artifact/ArtifactStore.java'
echo '== Service outline =='
ast-grep outline "$service" --items all
echo '== Worker outline =='
ast-grep outline "$worker" --items all
echo '== Artifact store outline =='
ast-grep outline "$store" --items all
echo '== Service artifact deletion and storage contexts =='
rg -n -C 18 \
'(deletePdf|artifactStore\.(putPdf|deletePdf)|\bdelete\s*\(|\bremove\s*\()' "$service"
echo '== Worker artifact write contexts =='
rg -n -C 18 \
'(artifactStore\.(putPdf|deletePdf)|\bprocess\s*\(|\bconvert\w*\s*\()' "$worker"
echo '== All main-source withJobLock invocations =='
rg -n -C 4 --glob '*.java' \
'\bwithJobLock\s*\(' src/mainLength of output: 13018
As a result, conversion and deletion can still overlap for the same job. The new lock registry is correct in isolation, but it has no runtime effect. Wrap each relevant artifact lifecycle mutation with The exact head and the two-file scope match the requested base and head. The singleton construction, fixed 256-stripe storage, null handling, and
|
|
@opencode-agent review Review exact head Review read-only; preserve independent reviewer identity. Do not mutate the branch, synthesize author approval, weaken protection, or merge. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='961f44a19c4ced87f8062b3ab9842a9a08d05235'
head='d7e3e6e7b54bdc70a05f18ff09338b9ab54aaeed'
main_file='src/main/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistry.java'
test_file='src/test/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistryTest.java'
echo '== Exact commit identity =='
git rev-parse HEAD
git cat-file -e "${head}^{commit}"
git merge-base "$head" "$base"
git diff --name-status "$base...$head"
echo '== Changed-source outlines =='
ast-grep outline "$main_file" --items all
ast-grep outline "$test_file" --items all
echo '== Exact diff =='
git diff --unified=100 "$base...$head" -- "$main_file" "$test_file"
echo '== Production implementation =='
git show "${head}:${main_file}" | cat -n
echo '== Focused tests =='
git show "${head}:${test_file}" | cat -n
echo '== Production registry references and lock invocations =='
git grep -n -C 5 -E '\bArtifactLifecycleLockRegistry\b|\bwithJobLock[[:space:]]*\(' "$head" -- 'src/main/**/*.java' || true
echo '== Lifecycle mutation candidates =='
git grep -n -C 8 -E '\b(deletePdf|putPdf|deleteJob|performDefaultConversion|seedPdfPassthroughArtifact)[[:space:]]*\(' \
"$head" -- 'src/main/**/*.java' || trueLength of output: 68208
Artifact lifecycle mutations remain outside the lock domain:
Therefore, conversion and deletion can still overlap for one job. The new registry has no production effect. The registry implementation itself meets the requested isolated checks:
Wrap the relevant lifecycle operations with
|
|
Exact-current-head independent review request for The two-file process-local lifecycle-lock slice is mergeable; CI, Security Scan, SAST Semgrep, and fuzz are terminal-success; unresolved inline findings are zero. The implementation exposes one private construction path and one process-wide fixed-memory registry, preventing disjoint lock domains while guaranteeing release after action failure. Protected @opencode-agent review Review this exact head read-only. Do not mutate the branch, synthesize author approval, weaken protection, or merge. |
Rate Limit Exceeded
|
1 participant
Objective
Replace stale #350 with the same bounded process-local artifact lifecycle serialization primitive directly on current protected
main, and remove the split-lock-domain defect discovered on the replacement head.Exact current-base identity
961f44a19c4ced87f8062b3ab9842a9a08d05235;d5069a3212007166fc40cf8ecfb4e59bfe5e0a42;d7e3e6e7b54bdc70a05f18ff09338b9ab54aaeed;ArtifactLifecycleLockRegistry.javaand its focused test.Test-first repair evidence
The initial replacement's exact-head CI failed one of 511 tests:
constructionAndNullInputsFailClosedexpected no public constructor but the class exposed a public constructor and Spring@Component. That allowed dependency injection and standalone callers to construct disjoint registries, defeating same-job mutual exclusion across those call paths.The corrected implementation now has one private construction path, one process-wide fixed-memory registry, no Spring component registration, 256 lock stripes, fail-closed null handling, and unconditional lock release after action failure.
Exact-head gates
On
d7e3e6e7b54bdc70a05f18ff09338b9ab54aaeed:Scope boundary
This protects one JVM only. It is not durable or distributed fencing and does not replace object-version preconditions, transactional outbox semantics, lease generations, or cross-instance deletion coordination.
Merge gate
Keep the exact head unchanged. Require a fresh current-head automated review, qualifying independent non-author formal approval, all live branch-protection rules, and zero valid unresolved findings. Auto-merge may act only after those conditions are satisfied; predecessor evidence does not transfer.