Skip to content

Cover held ordering locks in cfworkers - #1019

Open
seongmin36 wants to merge 1 commit into
fedify-dev:mainfrom
seongmin36:test/cfworkers-process-message-false
Open

Cover held ordering locks in cfworkers#1019
seongmin36 wants to merge 1 commit into
fedify-dev:mainfrom
seongmin36:test/cfworkers-process-message-false

Conversation

@seongmin36

Copy link
Copy Markdown
Contributor

Closes #879.

WorkersMessageQueue.processMessage() returns shouldProcess: false when an ordering key lock is already held, which is what tells a queue handler to retry the message instead of running it out of order. src/mod.test.ts covered the lock being taken and released, but never the already-held case.

test/mq.test.ts already covers that branch, but it stubs get() to return a lock unconditionally, so the lock key itself is never exercised. This test takes the lock with a real processMessage() call and then sends a second message for the same ordering key without releasing it, which is the state a queue handler is in while the first message is still in flight. That also proves the key processMessage() writes is the one it later reads back.

Checks

  • mise run check-each cfworkers — passes.
  • vitest run — 44 tests pass across 3 files.
  • Confirmed the test is not vacuous: replacing the lock check in processMessage() with if (false) makes it fail with expected true to be false, and it passes again once reverted.

Rebased onto main after #1016 touched the same file; both tests coexist.

No changelog fragment, since this is test-only work; the commit carries Changelog: none.

AI disclosure

Per AI_POLICY.md: I wrote the test myself. Claude Code (claude-opus-5) helped diagnose why my first attempt asserted against the lock-acquire path instead of the lock-exists path, and ran the checks listed above. The commit carries the Assisted-by: Claude Code:claude-opus-5 trailer.

src/mod.test.ts covered the ordering lock being taken and released, but
never the case where processMessage() finds one already held and returns
shouldProcess: false.  test/mq.test.ts covers that branch by stubbing
get() to return a lock unconditionally; this test takes the lock with a
real processMessage() call and sends a second message for the same key
without releasing it, so it also proves the key that processMessage()
writes is the one it later reads back.

Closes fedify-dev#879

Changelog: none
Assisted-by: Claude Code:claude-opus-5
@netlify

netlify Bot commented Aug 30, 2026

Copy link
Copy Markdown

Deploy Preview for fedify-json-schema canceled.

Name Link
🔨 Latest commit 26b91a7
🔍 Latest deploy log https://app.netlify.com/projects/fedify-json-schema/deploys/6a9474b85fac530008bbd5d9

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 38149dc9-80aa-4798-b714-08b2174516ce

📥 Commits

Reviewing files that changed from the base of the PR and between 81416ae and 26b91a7.

📒 Files selected for processing (1)
  • packages/cfworkers/src/mod.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Added test coverage for WorkersMessageQueue.processMessage() when a second message uses an existing ordering lock. The test verifies shouldProcess: false and confirms that no message or release callback is returned.

Changes

Queue ordering lock behavior

Layer / File(s) Summary
Duplicate ordering key test
packages/cfworkers/src/mod.test.ts
The test verifies that the first message is processed and the second message with the same ordering key is rejected without a payload or release callback.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 26b91

This test-only change adds coverage for held ordering-key locks without changing production behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: 2chanhaeng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: adding coverage for held ordering locks in cfworkers.
Description check ✅ Passed The description explains the test change, expected behavior, linked issue, validation results, and test-only scope.
Linked Issues check ✅ Passed The PR satisfies issue #879 by testing that a second message with the same ordering key returns shouldProcess: false while the lock remains held.
Out of Scope Changes check ✅ Passed The changes are limited to the requested test in packages/cfworkers/src/mod.test.ts. No unrelated code or public API changes are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test locked queue messages in @fedify/cfworkers

1 participant