Skip to content

Fix replace() to register unregistered functions#207

Merged
chrisguidry merged 1 commit intomainfrom
fix-replace-registration
Nov 24, 2025
Merged

Fix replace() to register unregistered functions#207
chrisguidry merged 1 commit intomainfrom
fix-replace-registration

Conversation

@chrisguidry
Copy link
Copy Markdown
Owner

When calling docket.replace() with an unregistered function, the method wasn't registering it with the docket. This caused tasks to be scheduled successfully but fail during execution when the worker couldn't find the function in the task registry.

The Issue

The replace() method was missing the else: self.register(function) clause that add() has. This meant that while the Lua scheduling script correctly scheduled the task (acting like an add when the key doesn't exist), the Python function object wasn't available for the worker to execute.

This became apparent when testing whether replace() on a non-existent key acts as a no-op, an error, or like an add(). The Lua script showed it should act like add(), but the missing registration caused execution failures.

The Fix

Added the missing registration logic to replace() (lines 444-445 in src/docket/docket.py) to match add()'s behavior:

if isinstance(function, str):
    function = self.tasks[function]
else:
    self.register(function)  # ← Added this

This makes replace() truly idempotent and safe to call regardless of whether a task with that key already exists.

Testing

  • Added test_replace_without_existing_task_acts_like_add() to verify that replace() on a non-existent key schedules and executes correctly
  • All existing tests pass
  • The key leak checker confirmed proper TTL handling after the fix

🤖 Generated with Claude Code

When calling `docket.replace()` with an unregistered function, the method
wasn't registering it with the docket. This caused tasks to be scheduled
successfully but fail during execution when the worker couldn't find the
function in the task registry.

The issue existed because `replace()` was missing the `else: self.register(function)`
clause that `add()` has. This meant that while the Lua scheduling script
correctly scheduled the task (acting like an add when the key doesn't exist),
the Python function object wasn't available for the worker to execute.

Added the missing registration logic to match `add()`'s behavior, making
`replace()` truly idempotent and safe to call regardless of whether a task
with that key already exists.

Also added a test to verify that `replace()` on a non-existent key schedules
and executes the task correctly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Nov 24, 2025

Claude finished @chrisguidry's task —— View job


Code Review for PR #207: Fix replace() to register unregistered functions

Thank you for this fix! I've reviewed the changes and here's my feedback:

✅ Review Summary

The PR correctly addresses the issue where docket.replace() wasn't registering unregistered functions, making it inconsistent with docket.add(). The fix is minimal, correct, and aligns the behavior of both methods.

Review Checklist

  • Examine the code changes in detail
  • Verify consistency with add() method
  • Review test case completeness
  • Check for edge cases and potential issues
  • Assess performance and security implications
  • Verify test coverage requirements

Code Quality & Best Practices ✅

The implementation is clean and follows the existing patterns:

src/docket/docket.py:444-445

else:
    self.register(function)

This exactly mirrors the logic in the add() method (lines 360-361), ensuring consistent behavior between both methods. The fix is minimal and focused.

Test Coverage ✅

The new test test_replace_without_existing_task_acts_like_add() is well-designed:

  • Tests the specific bug scenario (replace on non-existent key)
  • Verifies the task executes successfully with correct arguments
  • Follows existing test patterns and naming conventions
  • Complements existing replace tests which cover the case when a task already exists

Potential Issues & Edge Cases ✅

None identified. The change handles all scenarios correctly:

  • ✅ String function references via task registry lookup
  • ✅ Callable functions get registered before scheduling
  • ✅ The Lua scheduling script already handles replace-as-add semantics
  • ✅ No memory leaks - the key leak checker validates proper TTL handling

Performance Considerations ✅

No performance impact:

  • Registration is idempotent (re-registering is a no-op)
  • Adds minimal overhead only when needed
  • Consistent with add() method's behavior

Security Considerations ✅

No security concerns:

  • No new attack vectors introduced
  • Function registration follows existing security patterns
  • Input validation remains unchanged

Documentation Consideration 📝

Minor suggestion: The docstring for replace() could be updated to explicitly mention that it registers unregistered functions, matching the behavior of add(). However, this is not critical as the behavior is now intuitive and consistent.

Conclusion

LGTM! 👍 This is a well-crafted fix that correctly addresses the issue. The code is clean, the test coverage is appropriate, and the change makes the API more consistent and predictable. The fix ensures replace() is truly idempotent and safe to use regardless of prior state.

Great work on identifying and fixing this subtle but important bug!

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c14d794) to head (97521bd).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #207   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        48           
  Lines         7505      7513    +8     
  Branches       395       395           
=========================================
+ Hits          7505      7513    +8     
Flag Coverage Δ
python-3.10 100.00% <100.00%> (ø)
python-3.11 99.01% <100.00%> (+<0.01%) ⬆️
python-3.12 100.00% <100.00%> (ø)
python-3.13 100.00% <100.00%> (ø)
python-3.14 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/docket/docket.py 100.00% <100.00%> (ø)
tests/test_fundamentals.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

📚 Documentation has been built for this PR!

You can download the documentation directly here:
https://github.com/chrisguidry/docket/actions/runs/19642540293/artifacts/4663313792

@chrisguidry chrisguidry merged commit bbfa452 into main Nov 24, 2025
25 checks passed
@chrisguidry chrisguidry deleted the fix-replace-registration branch November 24, 2025 18:17
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.

2 participants