Scatter every-N-minutes schedules to reduce concurrent overlap#42524
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel already completed for PR #42524 in this workflow run (run ID 28463378307). Comment and APPROVE review were successfully submitted in the same run. Both safe-output tool limits are exhausted — no duplicate actions needed. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR updates gh-aw’s schedule compilation so every N minutes no longer compiles to */N * * * * (which causes synchronized starts). Instead, it routes minute-interval schedules through the existing fuzzy-scatter pipeline to produce deterministic per-workflow offsets like M/N * * * *, reducing concurrent overlap.
Changes:
- Emit
FUZZY:EVERY_MINUTE/N …for minute-interval schedules in the schedule parser. - Add a
handleEveryMinutefuzzy-scatter handler that mapsFUZZY:EVERY_MINUTE/N→M/N * * * *usingstableHash(workflowIdentifier, N). - Update/add tests to validate scattering behavior and cross-platform stability for
EVERY_MINUTE.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/schedule_preprocessing_test.go | Updates preprocessing tests to validate scattered cron output and minute-offset bounds. |
| pkg/parser/schedule_parser.go | Changes minute-interval parsing to emit a FUZZY schedule token for scattering. |
| pkg/parser/schedule_parser_test.go | Updates parser expectations to match the new FUZZY output for minute intervals. |
| pkg/parser/schedule_parser_stability_test.go | Adds pinned cross-platform expectations for scattered EVERY_MINUTE schedules. |
| pkg/parser/schedule_fuzzy_scatter.go | Adds handleEveryMinute to scatter minute intervals deterministically by workflow identifier. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
| @@ -343,6 +344,17 @@ | |||
| if len(fields) != 5 { | |||
| t.Errorf("expected 5 fields in cron expression, got %d: %s", len(fields), actualCron) | |||
| } | |||
| // If the minute field uses M/N start/step syntax, verify offset is in [0, N-1]. | |||
| fields := strings.Fields(actualCron) | ||
| if len(fields) != 5 { | ||
| t.Errorf("expected 5 fields in cron expression, got %d: %s", len(fields), actualCron) | ||
| } | ||
| // If the minute field uses the M/N start/step syntax, verify | ||
| // offset is within [0, N-1] so the period is preserved. | ||
| 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 { | ||
| if offset < 0 || offset >= interval { | ||
| t.Errorf("offset %d is not in [0, %d): %s", offset, interval, actualCron) | ||
| } | ||
| } | ||
| } |
| 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 "h": |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (118 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: Scatter every-N-minutes schedules
The overall approach is sound — routing every N minutes through the fuzzy-scatter pipeline is the right design, and the stableHash-based determinism is well-motivated. However, there is one blocking correctness bug and two related gaps that need to be addressed before merging.
🔴 Blocking: invalid cron minute field for N >= 60
stableHash(workflowIdentifier, interval) returns a value in [0, interval-1]. When interval >= 60, the offset can exceed 59 — the maximum valid cron minute value. Concrete counter-examples (verified against the actual FNV-1a implementation):
| Workflow | N | offset | Result | Valid? |
|---|---|---|---|---|
deploy.md |
90 | 63 | 63/90 * * * * |
❌ |
lint.md |
90 | 61 | 61/90 * * * * |
❌ |
workflow-b.md |
120 | 82 | 82/120 * * * * |
❌ |
The inline comment (lines 347–348) that claims "offset is always a valid starting minute" is therefore incorrect.
Two paths to fix (either works):
- Cap the hash modulo:
offset := stableHash(workflowIdentifier, min(interval, 60)) - Reject
interval >= 60with an error directing users toevery N hours
🟡 Related: test assertions don't catch the overflow
The checkScattered helper validates offset ∈ [0, N-1] but not offset < 60. No test exercises a workflow/interval combination where the offset would overflow 59, so the bug above would not be caught by the current test suite.
What looks good
- Handler placement in the dispatch list is correct; no prefix collisions with existing handlers.
parseHourlyIntervalreuse is appropriate with a clear explanatory comment.- Cross-platform stability pins for N=5/10/15/30 are well-chosen.
- Parser change (emitting
FUZZY:EVERY_MINUTE/Ninstead of*/N) is minimal and correct. - Test structure (
checkScatteredflag +workflowIdentifierfield) is a clean extension.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 73.1 AIC · ⌖ 7.58 AIC · ⊞ 4.8K
| } | ||
| // 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) |
There was a problem hiding this comment.
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.
| return "", true, err | ||
| } | ||
| if interval < 1 { | ||
| return "", true, fmt.Errorf("invalid interval in fuzzy every-minute pattern: interval must be >= 1, got %d", interval) |
There was a problem hiding this comment.
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.
| offset, offsetErr := strconv.Atoi(parts[0]) | ||
| interval, intervalErr := strconv.Atoi(parts[1]) | ||
| if offsetErr == nil && intervalErr == nil { | ||
| if offset < 0 || offset >= interval { |
There was a problem hiding this comment.
Incomplete assertion: offset < interval does not imply offset < 60
The check offset < 0 || offset >= interval validates that the offset is within [0, N-1], but for N >= 60 the offset can still exceed 59, producing an invalid cron minute field. The test will not catch offsets like 63, 75, or 82 when interval = 90.
Add a second guard:
if offset >= 60 {
t.Errorf("offset %d is >= 60, invalid cron minute value: %s", offset, actualCron)
}Also consider adding a checkScattered test case with interval >= 60 (e.g., "every 90 minutes" with a workflow ID that hashes above 59 for N=90, such as deploy.md) to exercise this boundary.
@copilot please address this.
🧪 Test Quality Sentinel — PR #42524Score: 77/100 📊 Metrics & ClassificationFile Summary
Test Classification
Score Breakdown
🔍 Per-test Analysis
Adds cross-platform stability pins for
Existing table rows updated from
One test case converted to
Struct extended with
|
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 93 AIC · ⌖ 8.15 AIC · ⊞ 6.5K
Comment /matt to run again
| // 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) { |
There was a problem hiding this comment.
[/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.
| if err != nil { | ||
| return "", true, err | ||
| } | ||
| if interval < 1 { |
There was a problem hiding this comment.
[/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.
| } | ||
| // If the minute field uses M/N start/step syntax, verify offset is in [0, N-1]. | ||
| minuteField := fields[0] | ||
| if parts := strings.SplitN(minuteField, "/", 2); len(parts) == 2 { |
There was a problem hiding this comment.
[/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.
| }, | ||
| { | ||
| name: "every 10 minutes - workflow-a.md", | ||
| fuzzyCron: "FUZZY:EVERY_MINUTE/10 * * * *", |
There was a problem hiding this comment.
[/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.
| if !strings.HasPrefix(fuzzyCron, prefix) { | ||
| return "", false, nil | ||
| } | ||
| // parseHourlyInterval extracts the integer N from a "PREFIX/N ..." pattern; |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
REQUEST_CHANGES — three blocking issues must be fixed before merge.
Blocking issues
1. Fuzz test CI will fail (see existing comment on schedule_parser.go:287): schedule_parser_fuzz_test.go hardcodes that all FUZZY: tokens have exactly 4 space-separated fields. The new FUZZY:EVERY_MINUTE/N * * * * has 5. This file was not updated in this PR, so any fuzz corpus entry that hits an every N minutes path will fail the fuzz test. Either adopt the existing 4-field convention (e.g., FUZZY:EVERY_MINUTE/N * * *) or update the fuzz test to explicitly allow this new 5-field variant.
2. Empty workflowIdentifier silently emits invalid cron (see inline comment on schedule_parser.go:286): Before this PR, every N minutes always produced valid cron (*/N * * * *). After this PR it produces a FUZZY token that is only resolved when c.workflowIdentifier != "". Callers that don't set an identifier — or that use ParseSchedule directly — now receive FUZZY:EVERY_MINUTE/10 * * * * as a literal cron string, which any cron scheduler will reject. This is an untested regression.
3. checkScattered assertion is a no-op for */N form (see existing comments on schedule_preprocessing_test.go:347 and :561): When minuteField = "*/10", strconv.Atoi("*") fails silently, the bounds check is skipped, and a non-scattered cron passes as if it were correctly scattered. A regression that reintroduced */10 * * * * would not be caught. Add if parts[0] == "*" { t.Errorf(...) } before the Atoi path.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 175.3 AIC · ⌖ 7.95 AIC · ⊞ 1.6K
Comment /review to run again
| 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 |
There was a problem hiding this comment.
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 unchangedAny 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.
| } | ||
| // parseHourlyInterval extracts the integer N from a "PREFIX/N ..." pattern; | ||
| // 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") |
There was a problem hiding this comment.
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.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ERY_MINUTE 5-field FUZZY - Return early after len(fields)!=5 check to avoid potential panic - Fail explicitly when minute field offset is non-numeric (catches unscattered */N) - Allow FUZZY:EVERY_MINUTE/N to have 5 fields in fuzz test (other FUZZY types remain 4-field) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. Three issues addressed:
Local |
🤖 PR Triage
Score breakdown: Impact 38/50 · Urgency 10/30 · Quality 3/20 Rationale: High-impact behavioral change to cron schedule compilation (all workflows using
|
|
🎉 This pull request is included in a new release. Release: |
every N minutesschedules compiled to*/N * * * *, causing all workflows with the same interval to fire simultaneously.Changes
pkg/parser/schedule_parser.go—formatShortDurationCron(m) andformatLongIntervalCron(minutes) now emitFUZZY:EVERY_MINUTE/N * * * *instead of*/N * * * *, routing minute-interval schedules through the existing fuzzy-scatter pipelinepkg/parser/schedule_fuzzy_scatter.go— newhandleEveryMinutehandler resolvesFUZZY:EVERY_MINUTE/NtoM/N * * * *, whereM = stableHash(workflowIdentifier, N)∈[0, N-1]; preserves the period while distributing start minutes across the clock faceoffset ∈ [0, N-1]; new cross-platform stability pins forFUZZY:EVERY_MINUTE/NBehavior
every 10 minutes*/10 * * * *1/10 * * * *every 5m*/5 * * * *2/5 * * * **/10 * * * *(raw cron)Offset is deterministic per
workflowIdentifier— same workflow always compiles to the same cron. Raw cron expressions bypass scatter as before.