Skip to content

feat(archive): jitter request pacing and retry throttled fetches #16

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
IamCoder18 merged 6 commits into from
Oct 17, 2026
Merged

feat(archive): jitter request pacing and retry throttled fetches #16

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
a4cd313
feat(archive): jitter request pacing and retry throttled fetches
IamCoder18 Oct 16, 2026
ae70414
test(archive): satisfy revive unused-parameter in pacing test
IamCoder18 Oct 16, 2026
64f50a6
fix(archive): address review findings on pacing and retry
IamCoder18 Oct 16, 2026
704de68
fix(retry): keep DelayFor across defaults and preserve jitter on hints
IamCoder18 Oct 16, 2026
8633b5c
test(retry): pin Attempts so TestDelayForRaisesTheFloor actually bites
IamCoder18 Oct 17, 2026
509f688
style(retry): drop inline comment per AGENTS.md rule 6
IamCoder18 Oct 17, 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
20 changes: 20 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,26 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Added
- `archive update` spaces its D2L requests out instead of firing them as
fast as the network returns them. Each gap is a randomized duration in
`[N, 2N)` rather than a fixed `N`, so consecutive requests don't land in
an evenly-spaced pattern. Tunable with `--request-interval`; the default
is 50ms and `--request-interval 0` disables pacing entirely. Because the
gap is measured from the previous request's start, a slow response
already satisfies the floor and costs no extra wall-clock.
- `archive update` now retries throttled and temporarily-failing requests
(HTTP 429 and 5xx) for up to 3 attempts with exponential backoff, jitter,
and `Retry-After` support in both delta-seconds and HTTP-date form.
A `Retry-After` hint is capped at 60s so a hostile or broken value cannot
stall a pass. Permanent failures (404, 400) and expired sessions are
still not retried.

### Fixed
- `archive update` leaked a connection per TOC fetch and per course-list
page: the response body was decoded but never closed. Retries made those
leaks multiply, so they are closed now.

## [0.3.0] - 2026-10-15

### Added
Expand Down
34 changes: 21 additions & 13 deletions cmd/archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ var (
archiveUpdateNoBodies bool
archiveUpdateNoRefresh bool
archiveUpdateWait bool
archiveUpdateInterval time.Duration

archiveFindExt string
archiveFindType string
Expand Down Expand Up @@ -99,6 +100,7 @@ func init() {
archiveUpdateCmd.Flags().BoolVar(&archiveUpdateNoBodies, "no-bodies", false, "Skip file-body downloads (metadata only)")
archiveUpdateCmd.Flags().BoolVar(&archiveUpdateNoRefresh, "no-auto-refresh", false, "Do not re-run login if the saved session has expired")
archiveUpdateCmd.Flags().BoolVar(&archiveUpdateWait, "wait", false, "Block instead of failing when another archive run holds the lock")
archiveUpdateCmd.Flags().DurationVar(&archiveUpdateInterval, "request-interval", archive.DefaultRequestInterval, "Floor on the gap between D2L requests; the real gap is randomized above it, so 0 disables pacing entirely")

archiveFindCmd.Flags().StringVar(&archiveFindExt, "ext", "", "Filter by URL extension (e.g. pdf, docx)")
archiveFindCmd.Flags().StringVar(&archiveFindType, "type", "", "Filter by topic type (File, Link)")
Expand Down Expand Up @@ -252,6 +254,10 @@ var archiveUpdateCmd = &cobra.Command{
}

func runArchiveUpdate(args []string) error {
if archiveUpdateInterval < 0 {
return fmt.Errorf("--request-interval must not be negative (got %s); pass 0 to disable pacing", archiveUpdateInterval)
}

abs, err := filepath.Abs(archiveDir)
if err != nil {
return err
Expand Down Expand Up @@ -292,13 +298,14 @@ func runArchiveUpdate(args []string) error {
scope := mergeScope(args, archiveCourses)
start := time.Now()
result, err := archive.Run(archive.Options{
Root: abs,
Jar: ens.Jar,
Verbose: archiveVerbose,
Quiet: archiveJSON || archiveQuiet,
CourseIDs: scope,
Progress: progressAdapter(hud),
DownloadFiles: !archiveUpdateNoBodies,
Root: abs,
Jar: ens.Jar,
Verbose: archiveVerbose,
Quiet: archiveJSON || archiveQuiet,
CourseIDs: scope,
Progress: progressAdapter(hud),
DownloadFiles: !archiveUpdateNoBodies,
RequestInterval: &archiveUpdateInterval,
})
if err != nil {
return err
Expand Down Expand Up @@ -360,12 +367,13 @@ func runArchiveUpdateDryRun(args []string, abs string) error {
scope := mergeScope(args, archiveCourses)
start := time.Now()
result, err := archive.Diff(archive.Options{
Root: abs,
Jar: ens.Jar,
Verbose: archiveVerbose,
Quiet: archiveJSON || archiveQuiet,
CourseIDs: scope,
DownloadFiles: !archiveUpdateNoBodies,
Root: abs,
Jar: ens.Jar,
Verbose: archiveVerbose,
Quiet: archiveJSON || archiveQuiet,
CourseIDs: scope,
DownloadFiles: !archiveUpdateNoBodies,
RequestInterval: &archiveUpdateInterval,
})
if err != nil {
return err
Expand Down
50 changes: 33 additions & 17 deletions internal/archive/archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,14 @@ type Options struct {
CourseIDs []string // when non-empty, restrict to these course ids
Now func() time.Time
DownloadFiles bool // when true, also download each File topic's underlying bytes and hash-store them in blobs/<sha256>.bin (default: false for backwards-compat)
// RequestInterval is the floor on the gap between consecutive outbound
// requests; the real gap is randomized above it. Nil means
// DefaultRequestInterval; an explicit 0 disables pacing altogether.
RequestInterval *time.Duration

// pacer is the pass-wide rate limiter, created from RequestInterval on
// first use so one Run/Diff shares a single budget across all courses.
pacer *pacer
}

// baseURL returns the effective D2L base, defaulting to ua.D2LBase.
Expand Down Expand Up @@ -522,6 +530,7 @@ func Run(opts Options) (Result, error) {
if opts.Jar == nil {
return Result{}, fmt.Errorf("archive: nil cookie jar")
}
opts.pacer = opts.limiter()
now := opts.Now
if now == nil {
now = func() time.Time { return time.Now().UTC() }
Expand Down Expand Up @@ -563,7 +572,7 @@ func Run(opts Options) (Result, error) {
if opts.Progress != nil {
opts.Progress(ProgressEvent{Phase: "list"})
}
listed, err := listAllCoursesAt(opts.Jar, opts.baseURL())
listed, err := listAllCoursesAt(opts, opts.baseURL())
if err != nil {
return Result{}, fmt.Errorf("archive: list courses: %w", err)
}
Expand Down Expand Up @@ -652,6 +661,7 @@ func Diff(opts Options) (DiffResult, error) {
if opts.Jar == nil {
return DiffResult{}, fmt.Errorf("archive: nil cookie jar")
}
opts.pacer = opts.limiter()

result := DiffResult{Root: opts.Root}
idx, err := LoadIndex(opts.Root)
Expand Down Expand Up @@ -680,7 +690,7 @@ func Diff(opts Options) (DiffResult, error) {
if opts.Progress != nil {
opts.Progress(ProgressEvent{Phase: "list"})
}
listed, err := listAllCoursesAt(opts.Jar, opts.baseURL())
listed, err := listAllCoursesAt(opts, opts.baseURL())
if err != nil {
return result, fmt.Errorf("diff: list courses: %w", err)
}
Expand Down Expand Up @@ -733,7 +743,7 @@ func Diff(opts Options) (DiffResult, error) {
// index, fetch the TOC, classify every topic, return the counts. No body
// fetches, no index writes.
func diffCourse(opts Options, c Course) (CourseDiff, error) {
toc, err := fetchTocFor(opts.baseURL(), c.OrgUnitId, opts.Jar)
toc, err := opts.fetchToc(opts.limiter(), c.OrgUnitId)
if err != nil {
return CourseDiff{}, err
}
Expand Down Expand Up @@ -806,7 +816,7 @@ func archiveCourse(opts Options, c Course, now func() time.Time) (courseStats, e
func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats, error) {
var s courseStats
transportFails := 0
toc, err := fetchTocFor(opts.baseURL(), c.OrgUnitId, opts.Jar)
toc, err := opts.fetchToc(opts.limiter(), c.OrgUnitId)
if err != nil {
return s, err
}
Expand Down Expand Up @@ -873,7 +883,7 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats,
Reason: q.reason,
Stale: s.stale,
})
body, ferr := fetchTopicFor(opts.baseURL(), opts.Jar, c.OrgUnitId, q.topicID)
body, ferr := opts.fetchTopic(opts.limiter(), c.OrgUnitId, q.topicID)
if ferr != nil {
if errors.Is(ferr, errSessionExpired) {
return abortWithIndex(ferr)
Expand Down Expand Up @@ -931,7 +941,7 @@ func archiveCourseV2(opts Options, c Course, now func() time.Time) (courseStats,
if bodyURL == "" {
continue
}
bodyBytes, contentType, berr := downloadFileBody(opts.Jar, bodyURL, opts.baseURL())
bodyBytes, contentType, berr := opts.fetchBody(opts.limiter(), bodyURL)
if berr != nil {
// The metadata for a topic whose reason was new/modified is
// about to be recorded as current, but its body is not. Clear
Expand Down Expand Up @@ -1061,7 +1071,8 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string,
if len(msg) > 200 {
msg = msg[:200] + "..."
}
return nil, "", fmt.Errorf("download %d %s for %s: %s", res.StatusCode, res.Status, rawURL, msg)
return nil, "", newStatusError(res,
fmt.Sprintf("download %d %s for %s: %s", res.StatusCode, res.Status, rawURL, msg))
}
// An expired session redirects the download to the D2L login page,
// which answers 200 with HTML; that page must never be hashed as a
Expand All @@ -1072,7 +1083,7 @@ func downloadFileBody(jar *cookiejar.Jar, rawURL, base string) ([]byte, string,
}
body, err := io.ReadAll(res.Body)
if err != nil {
return nil, "", err
return nil, "", &transportError{err}
}
return body, res.Header.Get("Content-Type"), nil
}
Expand Down Expand Up @@ -1116,13 +1127,15 @@ func fetchTocFor(base, orgUnitID string, jar *cookiejar.Jar) (content.TocRespons
Headers: map[string]string{"Accept": "application/json"},
})
if err != nil {
return content.TocResponse{}, err
return content.TocResponse{}, &transportError{err}
}
defer func() { _ = res.Body.Close() }()
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
if res.StatusCode != 200 {
if res.StatusCode == 401 || res.StatusCode == 403 {
return content.TocResponse{}, errSessionExpired
}
return content.TocResponse{}, fmt.Errorf("D2L API returned %d %s for %s", res.StatusCode, res.Status, res.Request.URL)
return content.TocResponse{}, newStatusError(res,
fmt.Sprintf("D2L API returned %d %s for %s", res.StatusCode, res.Status, res.Request.URL))
}
var toc content.TocResponse
if err := json.NewDecoder(res.Body).Decode(&toc); err != nil {
Expand All @@ -1145,14 +1158,15 @@ func fetchTopicFor(base string, jar *cookiejar.Jar, orgUnitID string, topicID in
if res.StatusCode == 401 || res.StatusCode == 403 {
return nil, errSessionExpired
}
return nil, fmt.Errorf("D2L API returned %d %s for topic %d", res.StatusCode, res.Status, topicID)
return nil, newStatusError(res,
fmt.Sprintf("D2L API returned %d %s for topic %d", res.StatusCode, res.Status, topicID))
}
if ct := res.Header.Get("Content-Type"); strings.Contains(ct, "text/html") || ua.IsLoginURL(res.Request.URL.String()) {
return nil, errSessionExpired
}
body, err := httpclient.BodyBytes(res)
if err != nil {
return nil, err
return nil, &transportError{err}

@sourcery-ai sourcery-ai Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium · Malformed redirects look like network outages

When five consecutive topic or file-body requests receive a malformed redirect response, fetchTopicFor and downloadFileBody wrap every FollowRedirects error as a transportError, including non-network errors such as a redirect with no Location. archiveCourseV2 then counts these as network failures and aborts after five consecutive malformed responses, instead of continuing past per-topic errors.

Wrap only errors known to come from the network transport as transportError, leaving redirect and request-construction errors unclassified as transport failures.

Also at internal/archive/archive.go:1086.

Prompt for AI agents 
In `internal/archive/archive.go` at line 1169:

**Malformed redirects look like network outages**

When five consecutive topic or file-body requests receive a malformed redirect response, `fetchTopicFor` and `downloadFileBody` wrap every `FollowRedirects` error as a `transportError`, including non-network errors such as a redirect with no `Location`. `archiveCourseV2` then counts these as network failures and aborts after five consecutive malformed responses, instead of continuing past per-topic errors.

Wrap only errors known to come from the network transport as `transportError`, leaving redirect and request-construction errors unclassified as transport failures.

Also at `internal/archive/archive.go:1086`.

@IamCoder18 IamCoder18 Oct 16, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Pre-existing, and left alone here.

fetchTopicFor and downloadFileBody wrapped FollowRedirects errors as transportError before this branch (verified against main), so five malformed redirects already tripped the consecutive-transport-failure abort.

I did extend the pattern to fetchTocFor and fetchCoursesPage, where it has no practical effect — those errors are fatal to the pass regardless, so nothing is miscounted.

A correct fix is to only wrap errors that actually came from the transport (errors.As for *url.Error, which is what client.Do returns) and let redirect-construction errors through unclassified. That changes the error taxonomy with existing tests attached, so I'd rather do it as its own change. Want me to?

@sourcery-ai sourcery-ai Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes—please handle it as a separate follow-up. Since the topic and file FollowRedirects wrapping predates this PR, I agree it shouldn’t block this change; thanks for checking against main.

}
out, err := prettyJSON(body)
if err != nil {
Expand Down Expand Up @@ -1348,13 +1362,13 @@ type d2lCoursesResponse struct {
Courses []d2lCourse `json:"Courses"`
}

func listAllCoursesAt(jar *cookiejar.Jar, base string) ([]Course, error) {
func listAllCoursesAt(opts Options, base string) ([]Course, error) {
var all []Course
next := coursesAPIURLFor(base)
pageNum := 0
for next != "" {
pageNum++
data, err := fetchCoursesPage(jar, next)
data, err := opts.fetchCoursesPage(opts.limiter(), next)
if err != nil {
return all, err
}
Expand All @@ -1377,21 +1391,23 @@ func listAllCoursesAt(jar *cookiejar.Jar, base string) ([]Course, error) {
func fetchCoursesPage(jar *cookiejar.Jar, rawURL string) (d2lCoursesResponse, error) {
res, err := httpclient.FollowRedirects(rawURL, jar, httpclient.FetchOptions{})
if err != nil {
return d2lCoursesResponse{}, err
return d2lCoursesResponse{}, &transportError{err}
}
defer func() { _ = res.Body.Close() }()
if res.StatusCode != 200 {
if res.StatusCode == 401 || res.StatusCode == 403 {
return d2lCoursesResponse{}, errSessionExpired
}
return d2lCoursesResponse{}, fmt.Errorf("D2L my-courses API returned %d %s", res.StatusCode, res.Status)
return d2lCoursesResponse{}, newStatusError(res,
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
fmt.Sprintf("D2L my-courses API returned %d %s", res.StatusCode, res.Status))
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
ct := res.Header.Get("Content-Type")
if !strings.Contains(ct, "application/json") && !strings.Contains(ct, "text/json") {
return d2lCoursesResponse{}, fmt.Errorf("expected JSON, got %s", ct)
}
body, err := httpclient.BodyBytes(res)
if err != nil {
return d2lCoursesResponse{}, err
return d2lCoursesResponse{}, &transportError{err}
}
var out d2lCoursesResponse
if err := json.Unmarshal(body, &out); err != nil {
Expand Down
Loading
Loading