fix(github): increase retry attempts and add jitter to backoff - #2764
Conversation
PR Summary by QodoFix GitHub retry backoff (5 retries + jitter) and URL skill dir naming
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Site previewPreview: https://40c3f7be-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:25 PM UTC · Completed 7:38 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Symlink path escape
|
| skillName := filepath.Base(forgeInfo.Path) | ||
| if skillName == "" || skillName == "." { | ||
| skillName = "tree" | ||
| } | ||
| namedPath := filepath.Join(filepath.Dir(treePath), skillName) | ||
| if namedPath != treePath { | ||
| // Idempotent: only create if it doesn't already exist. | ||
| if _, err := os.Lstat(namedPath); os.IsNotExist(err) { | ||
| if err := os.Symlink("tree", namedPath); err != nil { | ||
| return Dependency{}, "", fmt.Errorf("creating named symlink for %s: %w", field, err) | ||
| } | ||
| } | ||
| treePath = namedPath | ||
| } |
There was a problem hiding this comment.
2. Symlink path escape 🐞 Bug ⛨ Security
resolveSkillDirURL() uses filepath.Base(forgeInfo.Path) as a filesystem name without rejecting "..", so a URL path ending in "/.." makes namedPath resolve to the parent of the cache directory. This can create/overwrite a symlink outside the cache root and then return that escaped path as the resolved skill directory.
Agent Prompt
### Issue description
`resolveSkillDirURL()` derives `skillName := filepath.Base(forgeInfo.Path)` and uses it to build `namedPath := filepath.Join(filepath.Dir(treePath), skillName)`. If `forgeInfo.Path` is `".."` (possible because forge URL parsing does not sanitize path segments), then `namedPath` escapes to the parent directory and `os.Symlink("tree", namedPath)` attempts to create a link outside the cache directory.
### Issue Context
`forge.ParseForgeURL()` constructs `ForgeURLInfo.Path` directly from URL segments after `{ref}` without filtering `.`/`..` segments. The new symlink creation runs even on cache hits (no network call required).
### Fix Focus Areas
- internal/resolve/resolve.go[372-388]
- internal/forge/url.go[48-92]
### Implementation notes
- Validate the derived `skillName` before using it:
- Reject `".."` (and keep existing `""`/"." handling).
- Optionally reject any path that contains `".."` segments at all (safer).
- Additionally enforce containment: after computing `namedPath`, ensure it stays within `filepath.Dir(treePath)` (or within the cache root) using `filepath.Rel`/prefix checks on cleaned absolute paths.
- If invalid, skip creating the named symlink and keep `treePath` pointing at `<cache>/tree`.
- Add a unit test that uses a forge URL ending with `/tree/<ref>/..` (with a pre-populated cache entry) and asserts resolution does not write outside the cache directory.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsLow
Previous runReviewFindingsHigh
Medium
Low
Labels: PR modifies the GitHub forge client (retry/jitter) and skill resolution caching (symlinks), spanning the harness and skills components. |
| } | ||
| treePath = filepath.Join(cachePath, "tree") | ||
| } else { | ||
| fetchedAt = dirEntry.FetchTime |
There was a problem hiding this comment.
[low] undocumented-design
The symlink naming logic introduces new behavior with no issue or design context explaining why named symlinks are needed.
| "fmt" | ||
| "io" | ||
| "math" | ||
| "math/rand/v2" |
There was a problem hiding this comment.
[low] import-convention
Adds math/rand/v2 for jitter. Appropriate for non-cryptographic randomness but rare in this codebase outside e2e tests.
The GitHub API client was giving up after only 3 retries with deterministic backoff delays. When multiple e2e test runners hit secondary rate limits simultaneously, the identical retry timing caused thundering-herd effects that prolonged rate limiting. - Increase maxRetries from 3 to 5, giving ~5 minutes of patience for secondary rate limits (which use 60s+ base backoff) - Add jitter (50-100% of base delay) to desynchronize concurrent callers, matching the approach in github-api-csma.sh - Retry-After header values are still used exactly (no jitter) Signed-off-by: Robin Bender Ginn <rbenderg@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
f6374ba to
4e3bc5a
Compare
|
🤖 Finished Review · ✅ Success · Started 7:55 PM UTC · Completed 8:06 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 12:01 PM UTC · Completed 12:10 PM UTC |
Retro: PR #2764 —
|
Summary
maxRetriesfrom 3 to 5 in the GitHub API client, giving ~5 minutes of patience for secondary rate limits (60s+ base backoff per attempt)Retry-Afterheader values are still respected exactly (no jitter applied)Context
E2e tests were failing with
403 retryable error after 3 attempts on POST /orgs/halfsend-06/repos— the client gave up too quickly on secondary rate limits. The shell-side CSMA library (github-api-csma.sh) already uses 8 attempts with jitter; this brings the Go client closer to parity.Test plan
TestDo_MaxRetries5— verifies 5 attempts before exhaustionTestRetryDelay_HasJitter— verifies non-deterministic delays for 5xxTestRetryDelay_SecondaryRateLimit_HasJitter— verifies jitter on 403 backoffTestRetryDelay_RespectsRetryAfterHeader— verifies Retry-After is exactTestCreateOrUpdateFile_MaxRetriesExceededfor new retry countgo test ./internal/forge/github/passes🤖 Generated with Claude Code