Skip to content

Scatter every-N-minutes schedules to reduce concurrent overlap #42524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 6 commits into from
Jun 30, 2026
Merged

Scatter every-N-minutes schedules to reduce concurrent overlap #42524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2396b05
Scatter every-N-minutes schedules with workflow-derived offset
Copilot Jun 30, 2026
eade16d
Address review feedback: guard, comments, stronger offset-range test
Copilot Jun 30, 2026
d3b7890
docs(adr): add draft ADR-42524 for every-N-minutes schedule scattering
github-actions[bot] Jun 30, 2026
4221eea
Plan: fix checkScattered assertions and fuzz test FUZZY field count
Copilot Jun 30, 2026
b08c92d
fix: strengthen checkScattered assertions and update fuzz test for EV…
Copilot Jun 30, 2026
def9834
Merge branch 'main' into copilot/update-compilation-every-n-minutes
pelikhan Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/issue-monster.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/pr-sous-chef.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions docs/adr/42524-scatter-every-n-minutes-schedules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# ADR-42524: Scatter Every-N-Minutes Schedules via Fuzzy Token Pipeline

**Date**: 2026-06-30
**Status**: Draft
**Deciders**: Unknown

---

### Context

Workflows using "every N minutes" natural-language schedule syntax (e.g., `every 10 minutes`, `every 5m`) were compiled directly to standard cron `*/N * * * *` expressions. Because all workflows sharing the same interval produce the same cron pattern, they all fire simultaneously, creating predictable load spikes at minute boundaries. The codebase already contained a fuzzy-scatter pipeline (`ScatterSchedule`) that resolves intermediate `FUZZY:*` tokens to deterministically offset cron expressions for hourly and weekly schedules. Minute-interval schedules were not included in this pipeline, leaving a gap in the thundering-herd mitigation strategy.

### Decision

We will extend the existing fuzzy-scatter pipeline to cover minute-interval schedules. The schedule parser now emits `FUZZY:EVERY_MINUTE/N * * * *` instead of `*/N * * * *` for "every N minutes" inputs. A new `handleEveryMinute` handler in the scatter layer resolves this token to `M/N * * * *`, where `M = stableHash(workflowIdentifier, N) ∈ [0, N-1]`, preserving the period while distributing start minutes across the clock face. The offset is deterministic per workflow identifier so recompilation produces the same cron without drift. Raw cron expressions (`*/N * * * *` typed directly by users) bypass scatter as before.

### Alternatives Considered

#### Alternative 1: Keep `*/N * * * *` and accept simultaneous firing

The simplest option: do nothing. Workflows with the same interval continue to fire at the same minute. This was rejected because simultaneous firing creates measurable load spikes, and the infrastructure to mitigate it already exists for other schedule types — extending it to minute intervals is low-cost and consistent.

#### Alternative 2: Apply a random offset at compile time

Assign a random offset `M ∈ [0, N-1]` when compiling the schedule, storing the result as a plain cron expression. This avoids the intermediate `FUZZY:` token. It was rejected because a random offset changes on every recompilation, causing schedule drift — a workflow would silently shift its firing time whenever its definition is reprocessed. The deterministic hash approach used in the decision ensures the same workflow always produces the same cron, which is safer for audit trails and change detection.

#### Alternative 3: Offset by a fixed per-repo value rather than per-workflow hash

Use a single fixed offset derived from the repository rather than the workflow identifier. This was rejected because all workflows in the same repo would still cluster together at the same offset, merely shifting the load spike rather than distributing it.

### Consequences

#### Positive
- Workflows sharing a minute interval are now spread across the clock face, reducing simultaneous firing and the associated load spikes.
- The offset is deterministic — same workflow identifier always produces the same cron, so recompilation is idempotent and auditable.
- The fix reuses the existing `stableHash` and `ScatterSchedule` infrastructure, keeping the implementation surface small and consistent with the hourly/weekly scatter approach.

#### Negative
- The compiled cron output changes from the intuitive `*/N` form to the less-familiar `M/N` step-with-start form. Both are semantically equivalent but the latter may surprise users reading raw cron values.
- Existing stored cron expressions (`*/N * * * *`) generated before this change are not retroactively updated; workflows will only receive a scattered offset on their next recompilation.

#### Neutral
- The intermediate `FUZZY:EVERY_MINUTE/N` token must now be understood by any tooling that inspects or stores cron expressions mid-pipeline.
- Tests for minute-interval schedules now assert offset-in-range (`[0, N-1]`) rather than exact cron strings, which is more robust to hash-function changes but loses exact value pinning (except in the cross-platform stability test).

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
27 changes: 27 additions & 0 deletions pkg/parser/schedule_fuzzy_scatter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ func ScatterSchedule(fuzzyCron, workflowIdentifier string) (string, error) {
handleDaily,
handleHourlyWeekdays,
handleHourly,
handleEveryMinute,
handleWeeklyAround,
handleWeeklySpecific,
handleWeekly,
Expand Down Expand Up @@ -325,6 +326,32 @@ func handleHourly(fuzzyCron, workflowIdentifier string) (string, bool, error) {
return result, true, nil
}

// handleEveryMinute scatters "FUZZY:EVERY_MINUTE/N * * * *" to "M/N * * * *",
// where M is a deterministic offset in [0, N-1] derived from the workflow identifier.
// This ensures that concurrent "every N minutes" workflows start on different minutes,
// distributing execution across the clock face and reducing simultaneous load spikes.
func handleEveryMinute(fuzzyCron, workflowIdentifier string) (string, bool, error) {

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] No dedicated TestScatterScheduleEveryMinute was added to schedule_fuzzy_scatter_test.go, yet every other handler (e.g. TestScatterScheduleHourly) has its own test function that verifies the result is a valid cron expression and checks the output structure.

💡 Suggested test skeleton 
func TestScatterScheduleEveryMinute(t *testing.T) {
    tests := []struct {
        name               string
        fuzzyCron          string
        workflowIdentifier string
        expectError        bool
    }{
        {
            name:               "valid every 10 minutes",
            fuzzyCron:          "FUZZY:EVERY_MINUTE/10 * * * *",
            workflowIdentifier: "workflow-a.md",
        },
        {
            name:               "interval zero causes error not panic",
            fuzzyCron:          "FUZZY:EVERY_MINUTE/0 * * * *",
            workflowIdentifier: "workflow-a.md",
            expectError:        true,
        },
        {
            name:               "negative interval causes error",
            fuzzyCron:          "FUZZY:EVERY_MINUTE/-1 * * * *",
            workflowIdentifier: "workflow-a.md",
            expectError:        true,
        },
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := ScatterSchedule(tt.fuzzyCron, tt.workflowIdentifier)
            if tt.expectError {
                if err == nil {
                    t.Errorf("expected error, got result: %s", result)
                }
                return
            }
            if !IsCronExpression(result) {
                t.Errorf("ScatterSchedule returned invalid cron: %s", result)
            }
            // Verify offset is in [0, N-1]
            fields := strings.Fields(result)
            parts := strings.SplitN(fields[0], "/", 2)
            offset, _ := strconv.Atoi(parts[0])
            interval, _ := strconv.Atoi(parts[1])
            if offset < 0 || offset >= interval {
                t.Errorf("offset %d out of [0, %d): %s", offset, interval, result)
            }
        })
    }
}

@copilot please address this.

const prefix = "FUZZY:EVERY_MINUTE/"
if !strings.HasPrefix(fuzzyCron, prefix) {
return "", false, nil
}
// parseHourlyInterval extracts the integer N from a "PREFIX/N ..." pattern;

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The inline comment explaining why parseHourlyInterval is reused adds noise without adding value — the function name and parameter types make the intent clear at the call site. Drop it (or keep just the error-message doc, which is already captured in the parameter strings).

@copilot please address this.

// it is reused here because the parsing logic is identical.
interval, err := parseHourlyInterval(fuzzyCron, prefix, "invalid fuzzy every-minute pattern", "invalid interval in fuzzy every-minute pattern")

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseHourlyInterval is misnamed for this context and couples the minute handler to future hourly-specific changes.

💡 Details

The function name parseHourlyInterval implies it is for hourly scheduling. If a future maintainer adds hourly-specific validation to it (e.g., clamping the max interval to 23 for hours-in-a-day semantics), handleEveryMinute would silently inherit that constraint and reject valid minute intervals > 23.

The comment already acknowledges this is a workaround ("parsing logic is identical"), which signals the abstraction is wrong rather than the reuse being safe.

Suggested fix: Extract a generic parseStepInterval(fuzzyCron, prefix string) (int, error) helper and have both handleHourly and handleEveryMinute call it. This makes the intent clear and prevents accidental cross-domain coupling.

if err != nil {
return "", true, err
}
if interval < 1 {

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The interval < 1 guard is the critical safety net preventing a panic in stableHash(workflowIdentifier, 0) (Go panics on integer mod-zero). It has no test coverage, so it could be silently removed in a future refactor.

💡 Why this matters

stableHash does h.Sum32() % uint32(modulo). If modulo == 0, this is a runtime divide-by-zero panic. The parser itself validates interval >= 1 upstream (line 225 of schedule_parser.go), so well-formed input can never reach this guard. But ScatterSchedule is a public API and can be called directly.

Add a test case with FUZZY:EVERY_MINUTE/0 * * * * to TestScatterScheduleEveryMinute (see the adjacent comment) to lock in this behaviour.

@copilot please address this.

return "", true, fmt.Errorf("invalid interval in fuzzy every-minute pattern: interval must be >= 1, got %d", interval)

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing upper-bound validation for interval

Only interval < 1 is rejected. There is no guard for interval >= 60.

For N >= 60, the cron step in the minute field exceeds the field's range (0–59), so the schedule fires at most once per hour regardless of N — not once per N minutes. For example, 21/60 * * * * fires only at minute 21 (21+60=81 > 59); 35/90 * * * * fires only at minute 35. The desired period is silently lost.

Consider adding:

if interval >= 60 {
    return "", true, fmt.Errorf("invalid interval in fuzzy every-minute pattern: interval must be < 60, got %d (use 'every N hours' for larger intervals)", interval)
}

@copilot please address this.

}
// stableHash returns a value in [0, interval-1], so offset is always a valid
// starting minute that preserves the N-minute period.
offset := stableHash(workflowIdentifier, interval)

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: invalid cron minute field when N >= 60

The comment on lines 347–348 claims "offset is always a valid starting minute", but this is incorrect for interval >= 60. stableHash returns a value in [0, interval-1], so when interval = 90 the offset can be 60–89 — all invalid cron minute values (cron minute fields only accept 0–59).

Confirmed examples (FNV-1a hash, same algorithm as stableHash):

  • deploy.md + N=90 → offset 63 → 63/90 * * * * ❌
  • lint.md + N=90 → offset 61 → 61/90 * * * * ❌
  • workflow-b.md + N=120 → offset 82 → 82/120 * * * * ❌

Fix: cap the hash modulo at 60 so the offset always lands in [0, 59]:

offset := stableHash(workflowIdentifier, min(interval, 60))

Or add an explicit upper-bound guard rejecting interval >= 60 (see next comment).

@copilot please address this.

result := fmt.Sprintf("%d/%d * * * *", offset, interval)
scheduleFuzzyScatterLog.Printf("FUZZY:EVERY_MINUTE/%d scattered: offset=%d, result=%s", interval, offset, result)
return result, true, nil
}

func handleWeeklyAround(fuzzyCron, workflowIdentifier string) (string, bool, error) {
const prefix = "FUZZY:WEEKLY_AROUND:"
if !strings.HasPrefix(fuzzyCron, prefix) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/parser/schedule_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func formatShortDurationCron(interval int, unit string, hasWeekdaysSuffix bool)
if hasWeekdaysSuffix {
return "", errors.New("minute intervals with 'on weekdays' are not supported")
}
return fmt.Sprintf("*/%d * * * *", interval), nil
return fmt.Sprintf("FUZZY:EVERY_MINUTE/%d * * * *", interval), nil

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty workflowIdentifier now silently produces unresolvable FUZZY cron output — this is a silent regression for any caller that compiles schedules without setting a workflow identifier.

💡 Details and suggested fix

Before this PR, every N minutes always emitted a valid cron expression (*/N * * * *), even when no workflowIdentifier was set. After this PR it emits FUZZY:EVERY_MINUTE/N * * * *, which is only resolved to a real cron by ScatterSchedule — and ScatterSchedule is only called when c.workflowIdentifier != "":

// schedule_preprocessing.go:46
if parser.IsFuzzyCron(parsedCron) && c.workflowIdentifier != "" {
    // scatter happens
}
// else: raw FUZZY token is written to the output YAML unchanged

Any caller that invokes the parser without SetWorkflowIdentifier, or uses ParseSchedule directly without the preprocessing pipeline, now gets FUZZY:EVERY_MINUTE/10 * * * * as the cron string — rejected by any real cron scheduler. Previously those callers got */10 * * * *: valid, deterministic, immediately schedulable.

Note: TestSchedulePreprocessingShorthandOnString always calls compiler.SetWorkflowIdentifier("test-workflow.md"), so this regression path is not covered by any test.

Suggested fix: Emit the FUZZY token from the parser as today, but inside preprocessScheduleFields fall back to */N * * * * (not the FUZZY token) when workflowIdentifier is empty rather than leaving the unresolved FUZZY string in the output.

case "h":
Comment on lines 283 to 287
return formatHourlyIntervalCron(interval, hasWeekdaysSuffix), nil
case "d":
Expand All @@ -303,7 +303,7 @@ func formatLongIntervalCron(interval int, unit string, hasWeekdaysSuffix bool) (
if hasWeekdaysSuffix {
return "", errors.New("minute intervals with 'on weekdays' are not supported")
}
return fmt.Sprintf("*/%d * * * *", interval), nil
return fmt.Sprintf("FUZZY:EVERY_MINUTE/%d * * * *", interval), nil
case "hours":
return formatHourlyIntervalCron(interval, hasWeekdaysSuffix), nil
case "days":
Expand Down
25 changes: 16 additions & 9 deletions pkg/parser/schedule_parser_fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,18 +381,25 @@ func FuzzScheduleParser(f *testing.F) {

// 4. Validate cron expression format if successful
if err == nil && cron != "" {
// Allow fuzzy schedules (FUZZY:*) which have 4 fields
// Allow fuzzy schedules (FUZZY:*) which have 4 fields, except
// FUZZY:EVERY_MINUTE/N which carries all 5 cron fields.
if strings.HasPrefix(cron, "FUZZY:") {
// Fuzzy schedules have the format:
// - "FUZZY:DAILY * * *" (4 fields)
// - "FUZZY:HOURLY/N * * *" (4 fields)
// - "FUZZY:DAILY_AROUND:HH:MM * * *" (4 fields but with colon-separated time in first field)
// - "FUZZY:DAILY_BETWEEN:START_H:START_M:END_H:END_M * * *" (4 fields with 4 colon-separated values in first field)
fields := strings.Fields(cron)
if len(fields) != 4 {
t.Errorf("ParseSchedule returned invalid fuzzy cron format with %d fields (expected 4): %q for input: %q", len(fields), cron, input)
if strings.HasPrefix(cron, "FUZZY:EVERY_MINUTE/") {
// "FUZZY:EVERY_MINUTE/N * * * *" — 5 fields
if len(fields) != 5 {
t.Errorf("ParseSchedule returned invalid FUZZY:EVERY_MINUTE format with %d fields (expected 5): %q for input: %q", len(fields), cron, input)
}
} else {
// All other fuzzy schedules have the format:
// - "FUZZY:DAILY * * *" (4 fields)
// - "FUZZY:HOURLY/N * * *" (4 fields)
// - "FUZZY:DAILY_AROUND:HH:MM * * *" (4 fields but with colon-separated time in first field)
// - "FUZZY:DAILY_BETWEEN:START_H:START_M:END_H:END_M * * *" (4 fields with 4 colon-separated values in first field)
if len(fields) != 4 {
t.Errorf("ParseSchedule returned invalid fuzzy cron format with %d fields (expected 4): %q for input: %q", len(fields), cron, input)
}
}

// For FUZZY:DAILY_AROUND, validate the time format
if strings.HasPrefix(cron, "FUZZY:DAILY_AROUND:") {
// Extract the time part from FUZZY:DAILY_AROUND:HH:MM
Expand Down
30 changes: 30 additions & 0 deletions pkg/parser/schedule_parser_stability_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,36 @@ func TestScatterScheduleCrossPlatformConsistency(t *testing.T) {
workflowIdentifier: "workflow-a.md",
expectedCron: "21 8 * * 1",
},
{
name: "every 10 minutes - workflow-a.md",
fuzzyCron: "FUZZY:EVERY_MINUTE/10 * * * *",

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing stability pin for N=1 (every 1 minute). With interval=1, stableHash(id, 1) always returns 0 (any integer mod 1 = 0), so every workflow produces 0/1 * * * * — scatter provides no distribution benefit. This is correct behavior, but the absence of a pinned test (and an explanation) could surprise future maintainers.

💡 Suggested addition 
{
    name:               "every 1 minute - any workflow (no scatter, mod-1 always 0)",
    fuzzyCron:          "FUZZY:EVERY_MINUTE/1 * * * *",
    workflowIdentifier: "workflow-a.md",
    expectedCron:       "0/1 * * * *", // stableHash(_, 1) == 0 for all identifiers
},

A comment in the test body explaining why this always yields 0/1 would also help.

@copilot please address this.

workflowIdentifier: "workflow-a.md",
expectedCron: "1/10 * * * *",
},
{
name: "every 10 minutes - workflow-b.md",
fuzzyCron: "FUZZY:EVERY_MINUTE/10 * * * *",
workflowIdentifier: "workflow-b.md",
expectedCron: "2/10 * * * *",
},
{
name: "every 5 minutes - workflow-a.md",
fuzzyCron: "FUZZY:EVERY_MINUTE/5 * * * *",
workflowIdentifier: "workflow-a.md",
expectedCron: "1/5 * * * *",
},
{
name: "every 30 minutes - workflow-a.md",
fuzzyCron: "FUZZY:EVERY_MINUTE/30 * * * *",
workflowIdentifier: "workflow-a.md",
expectedCron: "21/30 * * * *",
},
{
name: "every 15 minutes - workflow-b.md",
fuzzyCron: "FUZZY:EVERY_MINUTE/15 * * * *",
workflowIdentifier: "workflow-b.md",
expectedCron: "7/15 * * * *",
},
}

for _, tt := range tests {
Expand Down
12 changes: 6 additions & 6 deletions pkg/parser/schedule_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,19 +640,19 @@ func TestParseSchedule(t *testing.T) {
{
name: "every 10 minutes",
input: "every 10 minutes",
expectedCron: "*/10 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/10 * * * *",
expectedOrig: "every 10 minutes",
},
{
name: "every 5 minutes",
input: "every 5 minutes",
expectedCron: "*/5 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/5 * * * *",
expectedOrig: "every 5 minutes",
},
{
name: "every 30 minutes",
input: "every 30 minutes",
expectedCron: "*/30 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/30 * * * *",
expectedOrig: "every 30 minutes",
},
{
Expand Down Expand Up @@ -684,7 +684,7 @@ func TestParseSchedule(t *testing.T) {
{
name: "every 30m",
input: "every 30m",
expectedCron: "*/30 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/30 * * * *",
expectedOrig: "every 30m",
},
{
Expand Down Expand Up @@ -969,13 +969,13 @@ func TestParseSchedule(t *testing.T) {
{
name: "interval at minimum - 5m",
input: "every 5m",
expectedCron: "*/5 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/5 * * * *",
expectedOrig: "every 5m",
},
{
name: "interval at minimum - 5 minutes",
input: "every 5 minutes",
expectedCron: "*/5 * * * *",
expectedCron: "FUZZY:EVERY_MINUTE/5 * * * *",
expectedOrig: "every 5 minutes",
},

Expand Down
69 changes: 59 additions & 10 deletions pkg/workflow/schedule_preprocessing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package workflow

import (
"fmt"
"strconv"
"strings"
"testing"
)
Expand Down Expand Up @@ -216,7 +217,7 @@ func TestSchedulePreprocessingShorthandOnString(t *testing.T) {
frontmatter: map[string]any{
"on": "every 10 minutes",
},
expectedCron: "*/10 * * * *",
checkScattered: true, // Scattered minute-interval schedule
expectWorkflowDispatch: true,
},
{
Expand Down Expand Up @@ -342,6 +343,20 @@ func TestSchedulePreprocessingShorthandOnString(t *testing.T) {
fields := strings.Fields(actualCron)
if len(fields) != 5 {
t.Errorf("expected 5 fields in cron expression, got %d: %s", len(fields), actualCron)
return
}
// If the minute field uses M/N start/step syntax, verify that the
// offset is a valid integer in [0, N-1]. A non-integer offset (e.g.
// "*" from an unscattered "*/N") is itself a failure.
minuteField := fields[0]
if parts := strings.SplitN(minuteField, "/", 2); len(parts) == 2 {

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The offset bounds-check block (lines 348–356) is duplicated verbatim in TestSchedulePreprocessing around line 552. Extract into a shared helper to keep the invariant in one place.

💡 Suggested helper 
// assertValidScatteredMinuteCron verifies that actualCron is a valid 5-field
// cron expression whose minute field uses M/N syntax with M in [0, N-1].
func assertValidScatteredMinuteCron(t *testing.T, actualCron string) {
    t.Helper()
    if strings.HasPrefix(actualCron, "FUZZY:") {
        t.Errorf("expected scattered cron, got fuzzy: %s", actualCron)
        return
    }
    fields := strings.Fields(actualCron)
    if len(fields) != 5 {
        t.Errorf("expected 5 fields in cron expression, got %d: %s", len(fields), actualCron)
        return
    }
    if parts := strings.SplitN(fields[0], "/", 2); len(parts) == 2 {
        offset, offsetErr := strconv.Atoi(parts[0])
        interval, intervalErr := strconv.Atoi(parts[1])
        if offsetErr == nil && intervalErr == nil && (offset < 0 || offset >= interval) {
            t.Errorf("offset %d is not in [0, %d): %s", offset, interval, actualCron)
        }
    }
    t.Logf("Successfully scattered schedule to: %s", actualCron)
}

Then replace both checkScattered blocks with assertValidScatteredMinuteCron(t, actualCron).

@copilot please address this.

offset, offsetErr := strconv.Atoi(parts[0])
interval, intervalErr := strconv.Atoi(parts[1])
if offsetErr != nil || intervalErr != nil {
t.Errorf("minute field %q is not a valid M/N expression: %s", minuteField, actualCron)
} else if offset < 0 || offset >= interval {
t.Errorf("offset %d is not in [0, %d): %s", offset, interval, actualCron)
}
}
t.Logf("Successfully scattered schedule to: %s", actualCron)
} else if tt.expectedCron != "" {
Expand All @@ -355,11 +370,13 @@ func TestSchedulePreprocessingShorthandOnString(t *testing.T) {

func TestSchedulePreprocessing(t *testing.T) {
tests := []struct {
name string
frontmatter map[string]any
expectedCron string
expectedError bool
errorSubstring string
name string
frontmatter map[string]any
workflowIdentifier string
expectedCron string
checkScattered bool
expectedError bool
errorSubstring string
}{
{
name: "daily schedule",
Expand Down Expand Up @@ -398,7 +415,8 @@ func TestSchedulePreprocessing(t *testing.T) {
},
},
},
expectedCron: "*/10 * * * *",
workflowIdentifier: "test-workflow.md",
checkScattered: true,
},
{
name: "existing cron expression unchanged",
Expand Down Expand Up @@ -469,7 +487,8 @@ func TestSchedulePreprocessing(t *testing.T) {
"schedule": "every 10 minutes",
},
},
expectedCron: "*/10 * * * *",
workflowIdentifier: "test-workflow.md",
checkScattered: true,
},
{
name: "shorthand string format - existing cron",
Expand All @@ -495,6 +514,9 @@ func TestSchedulePreprocessing(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
compiler := NewCompiler()
if tt.workflowIdentifier != "" {
compiler.SetWorkflowIdentifier(tt.workflowIdentifier)
}
err := compiler.preprocessScheduleFields(tt.frontmatter, "", "")

if tt.expectedError {
Expand All @@ -519,8 +541,35 @@ func TestSchedulePreprocessing(t *testing.T) {
firstSchedule := scheduleArray[0].(map[string]any)
actualCron := firstSchedule["cron"].(string)

if actualCron != tt.expectedCron {
t.Errorf("expected cron '%s', got '%s'", tt.expectedCron, actualCron)
if tt.checkScattered {
// Should be scattered to a valid cron (not fuzzy)
if strings.HasPrefix(actualCron, "FUZZY:") {
t.Errorf("expected scattered cron, got fuzzy: %s", actualCron)
}
fields := strings.Fields(actualCron)
if len(fields) != 5 {
t.Errorf("expected 5 fields in cron expression, got %d: %s", len(fields), actualCron)
return
}
// If the minute field uses the M/N start/step syntax, verify
// that the offset is a valid integer in [0, N-1] so the period
// is preserved. A non-integer offset (e.g. "*" from an
// unscattered "*/N") is itself a failure.
minuteField := fields[0]
if parts := strings.SplitN(minuteField, "/", 2); len(parts) == 2 {
offset, offsetErr := strconv.Atoi(parts[0])
interval, intervalErr := strconv.Atoi(parts[1])
if offsetErr != nil || intervalErr != nil {
t.Errorf("minute field %q is not a valid M/N expression: %s", minuteField, actualCron)
} else if offset < 0 || offset >= interval {
t.Errorf("offset %d is not in [0, %d): %s", offset, interval, actualCron)
}
}
t.Logf("Successfully scattered schedule to: %s", actualCron)
} else if tt.expectedCron != "" {
if actualCron != tt.expectedCron {
t.Errorf("expected cron '%s', got '%s'", tt.expectedCron, actualCron)
}
}
})
}
Expand Down
Loading