Bound CHASM scheduler request IDs - #11240
Conversation
71e241e to
afa4950
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964f336470
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
964f336 to
f1903ae
Compare
f1903ae to
4999eb8
Compare
|
Why this request-ID representation change is safe:
The change is consequently a bounded representation change for future scheduler-generated IDs, not a rewrite of existing buffered state or a source of duplicate starts. |
| } | ||
|
|
||
| // Keep request IDs bounded and deterministic even when schedule IDs are long. | ||
| scheduleIDUUID := uuid.NewSHA1(uuid.Nil, []byte(scheduleID)) |
There was a problem hiding this comment.
Not super sure I understand, wouldn't hashing the entire thing be the goal? What it if the NS is really long?
I am not sure if we need the requestID to be human-readable, but if we don't, then what about:
return uuid.NewSHA1(uuid.Nil, []byte(fmt.Sprintf(
"sched-%s-%s-%s-%d-%d-%d",
backfillID,
namespaceID,
scheduleID,
conflictToken,
nominal.UnixMilli(),
actual.UnixMilli(),
)))
There was a problem hiding this comment.
I think the only places we'd see this is in tdbg execution dumps of the chasms schedule or workflows started by a schedule.
I'd also like to keep auto vs backfiller id since the boolean marking a buffered start as manual is not displayed in tdbg output when false. Having the backfiller is also help with identifying which backfiller a buffered start came from.
| return fmt.Sprintf( | ||
| "sched-%s-%s-%s-%d-%d-%d", | ||
| "sched-%s-%s", | ||
| backfillID, |
There was a problem hiding this comment.
if you want to be completionist, it might be worth capping the backfill ID as well at a max length so that user-defined inputs can't exceed the field size.
Resolves a conflict in addBackfiller from two overlapping changes: - temporalio#11240 moved GenerateBackfillerID from common/schedules to chasm/lib/scheduler/internal, so the call site now uses schedulerinternal.GenerateBackfillerID. - This branch removes the LastProcessedTime seeding from addBackfiller, which was the only timestamppb use in backfiller.go, so that import is dropped. Both sides kept: the new import path plus the unseeded BackfillerState and the comment explaining why the watermark must stay unset. temporalio#11161 (generator buffer reserve) also landed in the meantime and reworks allowedBufferedStarts, but leaves Execute's buffer-full early return intact -- it still increments Attempt via defer without writing a watermark -- so the premise of this fix is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Generate request IDs as
sched-<auto|backfill-id>-<UUIDv5>. The UUID covers the namespace ID, schedule ID, conflict token, nominal time, and actual time.This keeps generated IDs within the 255-byte SQL request-ID column while retaining the useful auto/backfill marker.
Validation
go test -tags test_dep ./chasm/lib/scheduler/...make lint-code