Skip to content

Fix panic when max_retries/timeout/priority/delay is set without idempotency_key#41

Merged
alob-mtc merged 1 commit intomainfrom
fix/idempotency-key-panic
Nov 19, 2025
Merged

Fix panic when max_retries/timeout/priority/delay is set without idempotency_key#41
alob-mtc merged 1 commit intomainfrom
fix/idempotency-key-panic

Conversation

@alob-mtc
Copy link
Owner

@alob-mtc alob-mtc commented Nov 19, 2025

Description

Fixes a panic that occurs when max_retries, timeout, priority, or delay is set, but idempotency_key is not provided.

Problem

The code at line 1258 was calling unwrap() on self.idempotency_key without checking if it was Some. This caused a panic when:

  • Any of priority, max_retries, timeout, or delay was set
  • But idempotency_key was None

Solution

Changed the code to use Option::map() instead of unwrap(), which properly handles the case where idempotency_key is None. The idempotency_key field in ActivityOption is already optional, so this change aligns the code with that design.

Changes

  • Replaced unwrap() with Option::map() to safely handle None values
  • The idempotency key is now only formatted and set if it's provided
  • Other options can now be set independently without requiring an idempotency key

Testing

Related Issues

Closes #40

Summary by CodeRabbit

Refactor

  • Optimized internal idempotency key handling logic for improved code maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

…potency_key

- Changed idempotency_key handling to use Option::map instead of unwrap()
- Now properly handles cases where other options are set but idempotency_key is None
- Fixes issue #40
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

The PR refactors idempotency_key transformation in src/runner/runner.rs by replacing an unsafe unwrap() call with Option.map, eliminating a panic that occurred when activity options like max_retries, timeout, priority, or delay were set without providing an idempotency_key.

Changes

Cohort / File(s) Summary
Bug fix: Safe idempotency_key handling
src/runner/runner.rs
Replaced unconditional unwrap() on self.idempotency_key with Option.map to safely transform the key into a (formatted_key, OnDuplicate) tuple, preventing panics when other activity options are set without an idempotency key

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Area of focus: Verify that the Option.map closure correctly preserves the original formatting logic (format!("{}-{}", key, activity_type)) and handles both Some and None cases as intended for ActivityOption construction
  • Validation: Confirm that the fix resolves the panic condition described in the linked issue without introducing new edge cases

Possibly related PRs

  • PR #26: Directly modifies the same idempotency_key construction in src/runner/runner.rs; this fix likely complements or supersedes that PR's changes

Poem

🐰 An unwrap that panicked in the night,
Has met its match—Option.map sets things right!
No more crashes when priorities align,
The key transforms safely, one line at a time. ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the panic that occurs when options like max_retries/timeout/priority/delay are set without idempotency_key, which matches the core issue and solution in the changeset.
Linked Issues check ✅ Passed The code change replaces unwrap() with Option::map() to make idempotency_key optional, directly addressing issue #40's root cause and expected behavior that idempotency_key should be optional when creating ActivityOption.
Out of Scope Changes check ✅ Passed The changes are tightly scoped to fixing the panic in src/runner/runner.rs by modifying how idempotency_key is computed; no unrelated alterations to other functionality or files were detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/idempotency-key-panic

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25cd9f3 and 1e7fa29.

📒 Files selected for processing (1)
  • src/runner/runner.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: alob-mtc
Repo: alob-mtc/runnerq PR: 26
File: src/queue/queue.rs:491-510
Timestamp: 2025-11-08T21:23:07.278Z
Learning: In src/queue/queue.rs, the AllowReuseOnFailure behavior for idempotency (around lines 493-510) intentionally uses a non-atomic store_idempotency_record update after checking the existing activity status. This can allow race conditions where multiple threads could enqueue activities with the same idempotency key when the existing activity has Failed or DeadLetter status. The maintainer has accepted this behavior as it does not cause negative impact in their use case.
Learnt from: alob-mtc
Repo: alob-mtc/runnerq PR: 26
File: src/queue/queue.rs:475-485
Timestamp: 2025-11-08T21:09:18.838Z
Learning: In src/queue/queue.rs, the AllowReuse behavior for idempotency intentionally uses a non-atomic store_idempotency_record update (around lines 475-485), which can allow race conditions where multiple threads enqueue activities with the same idempotency key. The maintainer has accepted this behavior as it does not cause negative impact in their use case.
📚 Learning: 2025-11-08T21:23:07.278Z
Learnt from: alob-mtc
Repo: alob-mtc/runnerq PR: 26
File: src/queue/queue.rs:491-510
Timestamp: 2025-11-08T21:23:07.278Z
Learning: In src/queue/queue.rs, the AllowReuseOnFailure behavior for idempotency (around lines 493-510) intentionally uses a non-atomic store_idempotency_record update after checking the existing activity status. This can allow race conditions where multiple threads could enqueue activities with the same idempotency key when the existing activity has Failed or DeadLetter status. The maintainer has accepted this behavior as it does not cause negative impact in their use case.

Applied to files:

  • src/runner/runner.rs
📚 Learning: 2025-11-08T21:09:18.838Z
Learnt from: alob-mtc
Repo: alob-mtc/runnerq PR: 26
File: src/queue/queue.rs:475-485
Timestamp: 2025-11-08T21:09:18.838Z
Learning: In src/queue/queue.rs, the AllowReuse behavior for idempotency intentionally uses a non-atomic store_idempotency_record update (around lines 475-485), which can allow race conditions where multiple threads enqueue activities with the same idempotency key. The maintainer has accepted this behavior as it does not cause negative impact in their use case.

Applied to files:

  • src/runner/runner.rs
🧬 Code graph analysis (1)
src/runner/runner.rs (2)
examples/test_sse_events.rs (1)
  • activity_type (24-26)
src/activity/activity.rs (1)
  • activity_type (374-374)
🔇 Additional comments (1)
src/runner/runner.rs (1)

1258-1263: LGTM! Critical panic fix is correct and idiomatic.

The use of Option::map() elegantly resolves the panic described in issue #40. When self.idempotency_key is None, the map returns None directly; when it's Some((key, behavior)), it transforms and formats the key. This allows activity options like max_retries, timeout, priority, and delay to be set independently of the idempotency key.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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 and usage tips.

@alob-mtc alob-mtc merged commit 539db72 into main Nov 19, 2025
1 check passed
@alob-mtc alob-mtc deleted the fix/idempotency-key-panic branch November 19, 2025 22:54
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.

[BUG] Panic when max_retries/timeout/priority/delay is set without idempotency_key

1 participant