Repository navigation
fix(runner): use os.CreateTemp and mutex serialization for atomic resume on Windows #2616
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package runner | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "sync" | ||
|
|
||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| var atomicResumeMutex sync.Mutex | ||
|
|
||
| // syncDir attempts to fsync the directory containing targetPath on POSIX platforms. | ||
| func syncDir(dirPath string) error { | ||
| if runtime.GOOS == "windows" { | ||
| return nil | ||
| } | ||
| d, err := os.Open(dirPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer d.Close() | ||
| return d.Sync() | ||
| } | ||
|
|
||
| // SaveAtomic writes data to a temporary file in targetPath's directory, syncs to disk, and atomically renames it over targetPath. | ||
| func SaveAtomic(targetPath string, data []byte) error { | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| atomicResumeMutex.Lock() | ||
| defer atomicResumeMutex.Unlock() | ||
|
|
||
| dir := filepath.Dir(targetPath) | ||
| tmpFile, err := os.CreateTemp(dir, "httpx-resume-*.tmp") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tmpName := tmpFile.Name() | ||
| defer os.Remove(tmpName) | ||
|
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -u
base=bdc5c33aefb1315831e9731ea41943eb25c1c552
head=001e68a3f7466ccd57291a1d9541b484ccbacc8c
printf '%s\n' '--- target source at reviewed head ---'
git show "$head:runner/atomic_resume.go" | nl -ba
printf '%s\n' '--- PR diff for target ---'
git diff --unified=12 "$base" "$head" -- runner/atomic_resume.go
printf '%s\n' '--- errcheck and golangci references at reviewed head ---'
git grep -n -i -E 'errcheck|golangci-lint' "$head" -- . ':!vendor' || test "$?" -eq 1
printf '%s\n' '--- candidate lint and CI configuration files ---'
git ls-tree -r --name-only "$head" | grep -E '(^|/)(\.golangci[^/]*|Makefile|makefile|\.github/workflows/[^/]+|Taskfile[^/]*|\.gitlab-ci\.yml|magefile\.go)$' || trueRepository: projectdiscovery/httpx Length of output: 5623 🤖 get_repo_knowledge executed:
Length of output: 1288 🏁 Script executed: #!/bin/bash
set -u
head=001e68a3f7466ccd57291a1d9541b484ccbacc8c
for path in .github/workflows/build-test.yml Makefile; do
printf '\n--- %s ---\n' "$path"
git show "$head:$path" | nl -ba
done
printf '\n--- tracked lint configuration files ---\n'
git ls-tree -r --name-only "$head" | grep -Ei '(^|/)(\.golangci[^/]*|golangci[^/]*|\.errcheck[^/]*)$' || true
printf '\n--- root tracked files ---\n'
git ls-tree --name-only "$head"Repository: projectdiscovery/httpx Length of output: 3141 🌐 Web query:
💡 Result: 🏁 Script executed: python3 - <<'PY'
import json
import urllib.request
url = "https://api.github.com/repos/projectdiscovery/actions/contents/golangci-lint/v2?ref=v1"
request = urllib.request.Request(url, headers={"Accept": "application/vnd.github+json", "User-Agent": "review-inspector"})
try:
with urllib.request.urlopen(request, timeout=20) as response:
entries = json.load(response)
except Exception as exc:
print(f"GitHub API request failed: {type(exc).__name__}: {exc}")
raise SystemExit(0)
for entry in entries:
print(f"{entry['type']} {entry['path']}")
if entry["type"] == "file" and entry["name"] in {"action.yml", "action.yaml", "README.md"}:
content_url = entry.get("download_url")
if content_url:
print(f"--- {entry['path']} ---")
req = urllib.request.Request(content_url, headers={"User-Agent": "review-inspector"})
try:
with urllib.request.urlopen(req, timeout=20) as response:
print(response.read().decode("utf-8", "replace"))
except Exception as exc:
print(f"Fetch failed: {type(exc).__name__}: {exc}")
PYRepository: projectdiscovery/httpx Length of output: 1468 🌐 Web query:
💡 Result: Make the ignored cleanup errors explicit.
Suggested fix- defer os.Remove(tmpName)
+ defer func() { _ = os.Remove(tmpName) }()
if _, err := tmpFile.Write(data); err != nil {
- tmpFile.Close()
+ _ = tmpFile.Close()
return err
}
if err := tmpFile.Sync(); err != nil {
- tmpFile.Close()
+ _ = tmpFile.Close()
return err
}🧰 Tools🪛 golangci-lint (2.13.2)[error] 38-38: Error return value of (errcheck) 🤖 Prompt for AI Agents |
||
|
|
||
| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| return err | ||
| } | ||
| if err := tmpFile.Sync(); err != nil { | ||
| tmpFile.Close() | ||
| return err | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := os.Rename(tmpName, targetPath); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return syncDir(dir) | ||
| } | ||
|
|
||
| // SaveResumeConfigAtomic serializes the resume config and writes it using SaveAtomic. | ||
| func (r *Runner) SaveResumeConfigAtomic() error { | ||
| if r.options == nil || r.options.resumeCfg == nil { | ||
| return nil | ||
| } | ||
| var resumeCfg ResumeCfg | ||
| resumeCfg.Index = r.options.resumeCfg.currentIndex | ||
| resumeCfg.ResumeFrom = r.options.resumeCfg.current | ||
| data, err := yaml.Marshal(resumeCfg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return SaveAtomic(DefaultResumeFile, data) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| package runner | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "sync" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestSaveAtomic(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
| targetPath := filepath.Join(tempDir, "test_resume.cfg") | ||
|
|
||
| // 1. Basic atomic write | ||
| data := []byte("resume_index: 42\nresume_from: example.com\n") | ||
| err := SaveAtomic(targetPath, data) | ||
| require.NoError(t, err) | ||
|
|
||
| readData, err := os.ReadFile(targetPath) | ||
| require.NoError(t, err) | ||
| require.Equal(t, data, readData) | ||
|
|
||
| // 2. Overwrite atomically | ||
| newData := []byte("resume_index: 100\nresume_from: target.org\n") | ||
| err = SaveAtomic(targetPath, newData) | ||
| require.NoError(t, err) | ||
|
|
||
| readData, err = os.ReadFile(targetPath) | ||
| require.NoError(t, err) | ||
| require.Equal(t, newData, readData) | ||
|
|
||
| // 3. Concurrent saves | ||
| var wg sync.WaitGroup | ||
| for i := 0; i < 10; i++ { | ||
| wg.Add(1) | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| payload := []byte("concurrent_data") | ||
| _ = SaveAtomic(targetPath, payload) | ||
|
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Check every concurrent save result. If one 🤖 Prompt for AI Agents |
||
| }(i) | ||
| } | ||
| wg.Wait() | ||
|
|
||
| finalData, err := os.ReadFile(targetPath) | ||
| require.NoError(t, err) | ||
| require.Equal(t, []byte("concurrent_data"), finalData) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: projectdiscovery/httpx
Length of output: 283
🏁 Script executed:
Repository: projectdiscovery/httpx
Length of output: 4419
🏁 Script executed:
Repository: projectdiscovery/httpx
Length of output: 5996
🏁 Script executed:
Repository: projectdiscovery/httpx
Length of output: 270
🏁 Script executed:
Repository: projectdiscovery/httpx
Length of output: 2354
Handle the
d.Closeerrcheck finding.The CI lint job runs
golangci-lint. Explicitly discard the deferred close error.Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 golangci-lint (2.13.2)
[error] 23-23: Error return value of
d.Closeis not checked(errcheck)
🤖 Prompt for AI Agents