Repository navigation
fix(reliability): serialize same-job artifact lifecycle work #350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a66a5c9
test(lifecycle): require bounded job serialization
seonghobae 914b7e5
fix(lifecycle): add bounded artifact job lock registry
seonghobae d2f679c
Merge branch 'main' into fix/lifecycle-lock-registry-main-20260811
opencode-agent[bot] 994c446
test(reliability): require one lifecycle lock registry
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
81 changes: 81 additions & 0 deletions
81
src/main/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistry.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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| package com.clearfolio.viewer.lifecycle; | ||
|
|
||
| import java.util.Objects; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.locks.ReentrantLock; | ||
| import java.util.function.Supplier; | ||
|
|
||
| import org.springframework.stereotype.Component; | ||
|
|
||
| /** | ||
| * Provides fixed-memory, process-local serialization for one document lifecycle. | ||
| * | ||
| * <p>Conversion and deletion must not write or remove bytes for the same | ||
| * permanently reserved job identifier at the same time. The registry hashes | ||
| * identifiers onto a fixed number of lock stripes, so memory use remains | ||
| * bounded while unrelated jobs may proceed concurrently unless they share a | ||
| * stripe.</p> | ||
| * | ||
| * <p>This reference registry protects one JVM. Multi-instance deployments must | ||
| * provide an equivalent distributed generation fence, object-version | ||
| * precondition, or transactional outbox consumer guarantee.</p> | ||
| */ | ||
| @Component | ||
| public final class ArtifactLifecycleLockRegistry { | ||
|
|
||
| private static final int DEFAULT_STRIPE_COUNT = 256; | ||
| private static final ArtifactLifecycleLockRegistry SHARED = | ||
| new ArtifactLifecycleLockRegistry(DEFAULT_STRIPE_COUNT); | ||
|
|
||
| private final ReentrantLock[] locks; | ||
|
|
||
| /** | ||
| * Creates a registry with the default fixed number of lock stripes. | ||
| */ | ||
| public ArtifactLifecycleLockRegistry() { | ||
| this(DEFAULT_STRIPE_COUNT); | ||
| } | ||
|
|
||
| ArtifactLifecycleLockRegistry(int stripeCount) { | ||
| if (stripeCount <= 0) { | ||
| throw new IllegalArgumentException("stripeCount must be positive"); | ||
| } | ||
| this.locks = new ReentrantLock[stripeCount]; | ||
| for (int index = 0; index < stripeCount; index++) { | ||
| locks[index] = new ReentrantLock(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the process-wide registry used by standalone constructors. | ||
| * | ||
| * @return shared fixed-memory lifecycle lock registry | ||
| */ | ||
| public static ArtifactLifecycleLockRegistry shared() { | ||
| return SHARED; | ||
| } | ||
|
|
||
| /** | ||
| * Executes one action while holding the stripe for a conversion job. | ||
| * | ||
| * <p>The lock is always released, including when the supplied action throws. | ||
| * The action's return value or runtime exception is passed through unchanged.</p> | ||
| * | ||
| * @param jobId permanently reserved conversion-job identifier | ||
| * @param action work that reads or mutates metadata and artifact bytes | ||
| * @param <T> action return type | ||
| * @return value returned by the action | ||
| * @throws NullPointerException when the identifier or action is absent | ||
| */ | ||
| public <T> T withJobLock(UUID jobId, Supplier<T> action) { | ||
| UUID requiredJobId = Objects.requireNonNull(jobId, "jobId"); | ||
| Supplier<T> requiredAction = Objects.requireNonNull(action, "action"); | ||
| ReentrantLock lock = locks[Math.floorMod(requiredJobId.hashCode(), locks.length)]; | ||
| lock.lock(); | ||
| try { | ||
| return requiredAction.get(); | ||
| } finally { | ||
| lock.unlock(); | ||
| } | ||
| } | ||
| } | ||
115 changes: 115 additions & 0 deletions
115
src/test/java/com/clearfolio/viewer/lifecycle/ArtifactLifecycleLockRegistryTest.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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package com.clearfolio.viewer.lifecycle; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertSame; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import java.util.UUID; | ||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * Verifies fixed-memory process-local serialization for artifact lifecycles. | ||
| */ | ||
| class ArtifactLifecycleLockRegistryTest { | ||
|
|
||
| @Test | ||
| void sameJobCannotEnterTwoArtifactLifecyclesConcurrently() throws Exception { | ||
| ArtifactLifecycleLockRegistry registry = ArtifactLifecycleLockRegistry.shared(); | ||
| UUID jobId = UUID.fromString("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); | ||
| CountDownLatch firstEntered = new CountDownLatch(1); | ||
| CountDownLatch releaseFirst = new CountDownLatch(1); | ||
| CountDownLatch secondStarted = new CountDownLatch(1); | ||
| CountDownLatch secondEntered = new CountDownLatch(1); | ||
| AtomicReference<Thread> secondThread = new AtomicReference<>(); | ||
| ExecutorService executor = Executors.newFixedThreadPool(2); | ||
| try { | ||
| Future<String> first = executor.submit(() -> registry.withJobLock(jobId, () -> { | ||
| firstEntered.countDown(); | ||
| await(releaseFirst); | ||
| return "first"; | ||
| })); | ||
| assertTrue(firstEntered.await(5, TimeUnit.SECONDS)); | ||
|
|
||
| Future<String> second = executor.submit(() -> { | ||
| secondThread.set(Thread.currentThread()); | ||
| secondStarted.countDown(); | ||
| return registry.withJobLock(jobId, () -> { | ||
| secondEntered.countDown(); | ||
| return "second"; | ||
| }); | ||
| }); | ||
|
|
||
| assertTrue(secondStarted.await(5, TimeUnit.SECONDS)); | ||
| assertTrue(awaitBlockedOnLock(secondThread.get())); | ||
| assertFalse(secondEntered.await(200, TimeUnit.MILLISECONDS)); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| releaseFirst.countDown(); | ||
|
|
||
| assertEquals("first", first.get(5, TimeUnit.SECONDS)); | ||
| assertEquals("second", second.get(5, TimeUnit.SECONDS)); | ||
| assertTrue(secondEntered.await(5, TimeUnit.SECONDS)); | ||
| } finally { | ||
| releaseFirst.countDown(); | ||
| executor.shutdownNow(); | ||
| assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void actionFailureReleasesStripeForFollowingWork() { | ||
| ArtifactLifecycleLockRegistry registry = ArtifactLifecycleLockRegistry.shared(); | ||
| UUID jobId = UUID.randomUUID(); | ||
| IllegalStateException expected = new IllegalStateException("controlled failure"); | ||
|
|
||
| IllegalStateException actual = assertThrows( | ||
| IllegalStateException.class, | ||
| () -> registry.withJobLock(jobId, () -> { | ||
| throw expected; | ||
| }) | ||
| ); | ||
|
|
||
| assertSame(expected, actual); | ||
| assertEquals("recovered", registry.withJobLock(jobId, () -> "recovered")); | ||
| } | ||
|
|
||
| @Test | ||
| void constructionAndNullInputsFailClosed() { | ||
| ArtifactLifecycleLockRegistry registry = ArtifactLifecycleLockRegistry.shared(); | ||
|
|
||
| assertEquals(0, ArtifactLifecycleLockRegistry.class.getConstructors().length); | ||
| assertSame(registry, ArtifactLifecycleLockRegistry.shared()); | ||
| assertThrows(NullPointerException.class, () -> registry.withJobLock(null, () -> "unused")); | ||
| assertThrows(NullPointerException.class, () -> registry.withJobLock(UUID.randomUUID(), null)); | ||
| } | ||
|
|
||
| private static boolean awaitBlockedOnLock(Thread thread) { | ||
| long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); | ||
| while (System.nanoTime() < deadline) { | ||
| Thread.State state = thread.getState(); | ||
| if (state == Thread.State.WAITING || state == Thread.State.BLOCKED) { | ||
| return true; | ||
| } | ||
| Thread.onSpinWait(); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private static void await(CountDownLatch latch) { | ||
| try { | ||
| if (!latch.await(5, TimeUnit.SECONDS)) { | ||
| throw new IllegalStateException("latch timeout"); | ||
| } | ||
| } catch (InterruptedException exception) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new IllegalStateException("interrupted", exception); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
단일 잠금 레지스트리 인스턴스를 보장하십시오.
@Component인스턴스,SHARED인스턴스, 그리고 기본 생성자는 서로 다른ReentrantLock[]를 만듭니다. 같은jobId라도 호출자가 서로 다른 접근 경로를 사용하면 동일한 스트라이프를 잠그지 않습니다. 그러면 변환과 삭제가 동시에 아티팩트 상태를 변경할 수 있습니다.하나의 생성 경로만 유지하십시오. Spring singleton을 유일한 레지스트리로 사용하거나,
SHARED인스턴스를 Spring bean으로 등록하고 독립 생성 경로를 제거하십시오.shared()JavaDoc의 “standalone constructors” 설명도 현재 동작과 일치하지 않습니다.Also applies to: 35-36, 49-56
🤖 Prompt for AI Agents