Cover the ordering lock release in cfworkers - #1004
Conversation
processMessage() hands back a release() callback after it takes an ordering key lock, and the point of that callback is to delete the lock so the next message for the same key can run. Nothing checked it, so a release() that quietly stopped deleting would have stalled every ordered queue without failing a test. Rather than only asserting the key is gone, the test pushes a second message through the same ordering key and expects it to be processed, which is the behaviour users actually depend on. MockKvNamespace.get() threw unless it was called in JSON mode, while processMessage() reads the lock as a plain string, so the mock now returns the raw value for an untyped get(). See fedify-dev#880 Changelog: none Assisted-by: Claude Code:claude-opus-5
✅ Deploy Preview for fedify-json-schema canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change updates mock KV retrieval for raw values and adds coverage for releasing ordered message locks before processing the next message. ChangesCFWorkers behavior tests
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized test-only change verifies ordering-lock release behavior and confirms that a subsequent message can proceed; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
2 participants
Closes #880.
processMessage()hands back arelease()callback once it takes an orderingkey lock, and nothing in the suite checked that calling it actually deletes the
lock. A
release()that quietly stopped deleting would stall every orderedqueue and no test would notice.
The test takes the lock through
processMessage(), asserts the KV entry exists,calls
release(), then asserts it is gone. It also pushes a second messagethrough the same ordering key afterwards and expects it to be processed —
asserting that a key vanished from a mock felt like thin evidence on its own,
and "the next message can run" is the behaviour that actually matters here.
One supporting change:
MockKvNamespace.get()threw unless it was called inJSON mode, but
processMessage()reads the lock as a plain string. It nowreturns the raw value for an untyped
get()and parses only whentype === "json", which leaves the existingWorkersKvStoretests untouched.Checks
mise run check-each cfworkers— passesmise run test-each cfworkers— 42 tests pass across 3 filesrelease()into a no-op makes itfail with
expected true to be false, and it passes again once reverted.No changelog fragment, since this is test-only work; the commit carries
Changelog: none.AI disclosure
Per AI_POLICY.md: this was written with Claude Code (claude-opus-5). It
drafted the test and ran the checks listed above. The commit carries the
Assisted-by: Claude Code:claude-opus-5trailer.That verification is done: the diff has been reviewed,
test-eachandcheck-eachre-run on this branch, and the mutation check above repeated toconfirm the test fails when
release()stops deleting the lock.Also: CONTRIBUTING asks contributors to comment on an issue and wait to be
assigned. I have opened this as a draft rather than commented, so if you would
rather hand #880 to someone else, say the word and I will close it.