feat: implement delivery retry schedule

[alexluong]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
outpost-docs	Ready	Preview	Comment	Oct 19, 2025 4:22am
outpost-website	Ready	Preview	Comment	Oct 19, 2025 4:22am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

[alexluong]

@claude can you please review this PR, thanks?

[claude]

Claude finished @alexluong's task —— View job

---

Code Review for PR: feat: implement delivery retry schedule

Overall Assessment

This PR implements a flexible retry schedule feature for delivery retries, allowing users to specify custom retry delays instead of only using exponential backoff. The implementation is well-structured, thoroughly tested, and production-ready. The code follows best practices and maintains backward compatibility.

Strengths:

• Clean abstraction with the ScheduledBackoff type
• Comprehensive test coverage for all edge cases
• Proper validation and configuration handling
• Good documentation updates
• Backward compatibility maintained

Minor Issues:

• One typo in a comment (see details below)
• Documentation could be slightly clearer about schedule length determining max retries

1. Core Implementation (backoff/backoff.go)

File: internal/backoff/backoff.go:49-66

The ScheduledBackoff implementation is solid and handles edge cases well:

Positive aspects:

• Good handling of empty schedules (returns 0)
• Smart fallback behavior when retries exceed schedule length (returns last value)
• Clear documentation explaining the behavior

Minor comment typo:
Line 7 has a typo: numeber should be number

// Duration accepts the numeber of times the operation has been retried.

Suggestion: Consider documenting why returning the last value for attempts beyond the schedule is the desired behavior. This is reasonable, but worth explicitly noting in the docstring.

2. Test Coverage (backoff/backoff_test.go)

File: internal/backoff/backoff_test.go:91-139

Excellent test coverage for the new ScheduledBackoff type:

Well-tested scenarios:

• ✅ Custom schedule with multiple values
• ✅ Beyond-schedule behavior (returns last value)
• ✅ Empty schedule edge case
• ✅ Single element schedule

Test quality: The tests are clear, comprehensive, and follow good testing practices with table-driven tests.

3. Configuration Integration (config/config.go)

File: internal/config/config.go:78-80, 155-157, 372-387

The configuration integration is well-designed:

Positive aspects:

• Clear field naming and descriptions (line 78)
• Good default value (empty slice) to maintain backward compatibility (line 155)
• Smart logic in GetRetryBackoff() to prioritize scheduled backoff over exponential (lines 374-386)

Observation on line 380:

return &backoff.ScheduledBackoff{Schedule: schedule}, c.RetryMaxLimit

When using scheduled backoff, c.RetryMaxLimit is returned but isn't actually used since the schedule length determines the max retries. This could be confusing. Consider:
a. Returning len(c.RetrySchedule) explicitly here, OR
b. Adding a comment explaining that the validation step ensures RetryMaxLimit equals the schedule length

Looking at validation.go:176-180, I see the validation already sets RetryMaxLimit to match the schedule length, which is good! This makes the return value consistent. Consider adding a comment in GetRetryBackoff() to reference this validation behavior.

4. Validation Logic (config/validation.go)

File: internal/config/validation.go:175-182

The validation function is smart and handles the interaction between retry_schedule and retry_max_limit correctly:

Strengths:

• Automatically overrides retry_max_limit when schedule is provided (line 179)
• Simple and clear logic
• Properly integrated into the validation chain (line 48)

Potential improvement: The function name validateRetryConfiguration suggests validation, but it also mutates config by overriding RetryMaxLimit. Consider renaming to something like validateAndNormalizeRetryConfiguration to make the mutation more explicit, or add a comment explaining this behavior.

5. Configuration Tests (config/config_retry_test.go)

File: internal/config/config_retry_test.go

Excellent dedicated test file for retry configuration:

Well-covered scenarios:

• ✅ Default empty retry schedule
• ✅ YAML retry schedule overrides max limit
• ✅ Environment variable precedence
• ✅ Backward compatibility with retry_interval_seconds
• ✅ Mixed configuration scenarios

Test quality: Very thorough and covers the critical behavior where schedule length determines max retries (lines 39-40, 54-55, 83-84).

6. Documentation (docs/pages/references/configuration.mdx)

File: docs/pages/references/configuration.mdx

Documentation updates are good:

Lines 75, 117-118, 530-537: Clear descriptions of the new configuration options.

Suggestions for clarity:
a. Line 75: Consider making it more explicit that the schedule length determines the number of retries:

"Ignored if retry_schedule is provided. Note: When using retry_schedule, the number of retries is determined by the schedule length."

b. Line 118: The description is good but could emphasize the override behavior more clearly:

"...Schedule length automatically becomes the max number of retries, overriding retry_max_limit."

These are minor suggestions; the current documentation is functional.

7. Delivery Service Integration (delivery/delivery.go)

File: internal/services/delivery/delivery.go:142-154

The integration into the delivery service is clean:

Line 142: cfg.GetRetryBackoff() is called to retrieve both the backoff strategy and max limit.

Positive aspects:

• Clean abstraction - the delivery service doesn't need to know about schedule vs exponential backoff
• Configuration logic is properly encapsulated in the config package

No issues found in this integration.

---

Summary

This is a high-quality implementation that successfully adds flexible retry scheduling to Outpost. The code is:

• ✅ Well-tested with comprehensive coverage
• ✅ Backward compatible
• ✅ Properly documented
• ✅ Cleanly integrated

Recommendations:
a. Fix the typo in backoff.go:7 (numeber → number)
b. Consider adding a comment in GetRetryBackoff() referencing the validation behavior
c. Optional: Enhance documentation clarity around schedule length determining max retries

Verdict: Approve with minor suggestions. The implementation is production-ready, and the suggestions above are optional improvements for clarity.

---

Job Run | Branch: retry-schedule