Skip to content

Improve scheduling management and add examples#47

Merged
scottf merged 2 commits into
mainfrom
scheduling-example-and-improve
May 14, 2026
Merged

Improve scheduling management and add examples#47
scottf merged 2 commits into
mainfrom
scheduling-example-and-improve

Conversation

@scottf
Copy link
Copy Markdown
Collaborator

@scottf scottf commented May 13, 2026

No description provided.

@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer Bot commented May 13, 2026

Claude finished @scottf's task in 2m 13s —— View job


PR Review

  • Gather context and read CLAUDE.md
  • Fetch PR diff
  • Review ScheduleManagement.java changes
  • Review new example files
  • Review test changes
  • Post inline comments and final summary

Summary

The PR is well-structured. The split of publishAndCancelSchedule(..., boolean) into two clearly-named methods (publishAndCancelSchedule / publishAndCancelScheduleIfExists) is a good API improvement — the boolean flag was ambiguous at call sites. Tests are updated consistently.

One bug and two items worth knowing about:

Bug

  • ScheduleBasicsAlternate.java line 106 — copy-paste error: the report(...) label says SCHEDULE_PREFIX + "now" for the "every" schedule. Left an inline suggestion to fix it.

Breaking API change

  • publishAndCancelSchedule(jsm, sched, tgt, data, headers, boolean) no longer exists. Any callers outside this repo will break silently at compile time. If this is still pre-1.0 / unreleased that's fine; otherwise a deprecation bridge or release note is warranted.

Inherent TOCTOU in cancelSchedule(jsm, subject, stream)

  • The sequence lookup (getLastMessage) and the delete (deleteMessage) are two separate server round-trips. If a recurring schedule fires between them, the delete hits the old (already-replaced) sequence and returns NOT_FOUND, leaving the schedule still active. This is hard to fix without the publishAndCancelSchedule atomic mechanism, but callers of cancelSchedule should know it is best-effort on actively firing schedules. The Javadoc doesn't currently mention this limitation.

No other correctness, security, or performance issues found.

@scottf
Copy link
Copy Markdown
Collaborator Author

scottf commented May 14, 2026

@claude please re-review with these notes

  • Bug Fixed.
  • No api change, this is unreleased.
  • TOCTOU, Your understanding is not correct. The schedule sequence never changes because the schedule message subject is never added to.

@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer Bot commented May 14, 2026

Claude finished @scottf's task in 48s —— View job


Re-review

  • Read CLAUDE.md
  • Read ScheduleManagement.java to understand the TOCTOU concern
  • Verify the bug fix in ScheduleBasicsAlternate.java
  • Post updated review

All three points addressed — the PR looks good to merge.

Bug fixedScheduleBasicsAlternate.java line 106 now correctly reads "SCHEDULING " + SCHEDULE_PREFIX + "every". Confirmed.

No API change — since this is unreleased, no deprecation bridge is needed. Point noted and withdrawn.

TOCTOU — original concern was wrong. I assumed the schedule subject could receive additional publishes after the initial schedule was created, which would invalidate the sequence retrieved by getLastMessage. That's not how ADR-51 works. The schedule message is published once to the schedule subject and lives there unchanged; the server fires the schedule by publishing to the target subject, never back to the schedule subject. The sequence retrieved in getScheduleSequence (line 338) is therefore stable for the lifetime of the schedule, making the two-step getLastMessagedeleteMessage in cancelSchedule(jsm, subject, stream) fully correct and race-free.

No issues found. The API split into publishAndCancelSchedule / publishAndCancelScheduleIfExists remains a clean improvement, the examples are well-structured, and the test updates are consistent.

@scottf scottf merged commit 8747877 into main May 14, 2026
1 check passed
@scottf scottf deleted the scheduling-example-and-improve branch May 14, 2026 12:06
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.

1 participant