fix(java): guard JobRec reads with the backend monitor - #569
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults 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
ChangesIn-memory backend updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
d6bf428 to
bcc09a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@sdks/java/test-support/src/main/java/org/byteveda/taskito/test/InMemoryQueueBackend.java`:
- Around line 116-123: Complete the JobRec monitor contract by making
setProgress use the backend monitor while mutating JobRec.progress, and
synchronize listArchivedAfterJson on that same monitor when reading job fields
through jobView. Preserve the existing synchronized access patterns in
getJobJson and related methods so all multi-field JobRec reads and writes share
one lock.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b145ba9b-c2fd-4927-b771-167bb5ba78c2
📒 Files selected for processing (2)
sdks/java/test-support/src/main/java/org/byteveda/taskito/test/InMemoryQueueBackend.javasdks/java/test-support/src/test/java/org/byteveda/taskito/test/InMemoryPubSubTest.java
A poller could see claimNext's half-applied expiry: status cancelled, error still null.
bcc09a0 to
e3a064b
Compare
InMemoryPubSubTest.publishExpiryCancelsDeliveriesThatOutliveItfails intermittently in CI (seen on the run for #568, which touches no Java source):Line 284 asserts the status and passed; line 285 asserts
errorand failed. So the job was observed cancelled, without its error.Cause
jobsis aConcurrentHashMap, so the lookup is safe — but aJobRec's fields are plain mutable state. Every writer already mutates them under the backend monitor (claimNext,cancel,onComplete,onFail,onCancel, …). The readers that snapshot a job did not take it.claimNextexpires a job in three separate writes:awaitJobpollsgetJobJson, which was unsynchronized, so it could build its view between the first and third write — status alreadycancelled, error still null. Exactly the observed failure.Fix
getJobJson,getResult,listJobsJsonandlistJobsAfterJsonnow take the same monitor the writers hold, making the "JobRec fields are guarded by the backend monitor" invariant true on both sides. All four are pure in-memory reads with no outward calls, so this adds no deadlock risk.Stats helpers are deliberately left alone:
countreads a single field per job, so it has no multi-field snapshot to tear — a momentarily stale count is inherent to an unsynchronized aggregate.Second commit closes three
InMemoryQueueBackendinstances inInMemoryPubSubTestwith try-with-resources, matching the idiom the rest of the file already uses. Those tests start no workers, so the leak was benign, but it was flagged and the fix is free.Verification
:test-support:testgreen across five--reruninvocations,:test-support:spotlessCheckclean. Local runs cannot reproduce the original flake — it passed locally before the fix too — so the argument for the fix rests on the memory-model reasoning above, not on a red-to-green local repro.Summary by CodeRabbit