Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 25 additions & 0 deletions .github/workflows/gocacheprog-test-reusable.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
name: gocacheprog Tests (Reusable)

on:
workflow_call:

permissions:
contents: read

jobs:
test:
name: gocacheprog Unit Tests
runs-on: arc-runner-set
timeout-minutes: 10
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
persist-credentials: false
- uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0
env:
HOME: /tmp
with:
go-version-file: contrib/ci/gocacheprog/go.mod
cache: false
- name: Run tests
run: cd contrib/ci/gocacheprog && go test -race -count=1 ./...
15 changes: 15 additions & 0 deletions .github/workflows/gocacheprog-test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
name: gocacheprog Tests

on:
pull_request:
branches:
- main
- release-4.22
paths:
- contrib/ci/gocacheprog/**

jobs:
test:
uses: openshift/hypershift/.github/workflows/gocacheprog-test-reusable.yaml@main

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== File: .github/workflows/gocacheprog-test.yaml =="
if [ -f ".github/workflows/gocacheprog-test.yaml" ]; then
  nl -ba .github/workflows/gocacheprog-test.yaml | sed -n '1,160p'
else
  echo "Missing file .github/workflows/gocacheprog-test.yaml"
fi

echo
echo "== Look for local reusable workflow file =="
ls -la .github/workflows | head -n 200 || true
echo
fd -H "gocacheprog-test-reusable\.yaml" .github/workflows || true

echo
echo "== Search for references to gocacheprog-test-reusable.yaml =="
rg -n "gocacheprog-test-reusable\.yaml(@|$)" .github/workflows -S || true

Repository: openshift/hypershift

Length of output: 177


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE=".github/workflows/gocacheprog-test.yaml"
echo "== File: $FILE =="
if [ -f "$FILE" ]; then
  cat -n "$FILE" | sed -n '1,220p'
else
  echo "Missing $FILE"
fi

echo
echo "== Listing .github/workflows (first 200 entries) =="
ls -la .github/workflows | head -n 200 || true

echo
echo "== Find local reusable workflow candidates =="
fd -H "gocacheprog-test-reusable\.yaml$" .github/workflows || true
fd -H "gocacheprog-test-reusable" .github/workflows || true

echo
echo "== Search for references to gocacheprog-test-reusable.yaml =="
rg -n "gocacheprog-test-reusable\.yaml(@|$)" .github/workflows -S || true

echo
echo "== Show any uses: .../gocacheprog-test-reusable... lines =="
rg -n "uses:\s*openshift/hypershift/\.github/workflows/gocacheprog-test-reusable\.yaml@" .github/workflows -S || true

Repository: openshift/hypershift

Length of output: 2943


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE=".github/workflows/gocacheprog-test-reusable.yaml"
echo "== File: $FILE =="
if [ -f "$FILE" ]; then
  cat -n "$FILE" | sed -n '1,220p'
else
  echo "Missing $FILE"
fi

echo
echo "== Search for tag/mutable refs in reusable workflow =="
rg -n "uses:\s*[^ ]+@((main)|(master)|(v[0-9]+)|(\w+))\b" .github/workflows/gocacheprog-test-reusable.yaml -S || true

echo
echo "== Search specifically for `@main` in reusable workflow =="
rg -n "`@main`\b" .github/workflows/gocacheprog-test-reusable.yaml -S || true

Repository: openshift/hypershift

Length of output: 1217


Pin reusable workflow reference to an immutable revision (avoid @main).

In .github/workflows/gocacheprog-test.yaml, the job calls openshift/hypershift/.github/workflows/gocacheprog-test-reusable.yaml@main (mutable), so CI won’t necessarily run the same reusable workflow revision deterministically. Use a local workflow reference (or a full commit SHA) instead.

🔒 Proposed fix
-    uses: openshift/hypershift/.github/workflows/gocacheprog-test-reusable.yaml@main
+    uses: ./.github/workflows/gocacheprog-test-reusable.yaml
🧰 Tools
🪛 zizmor (1.25.2)

[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/gocacheprog-test.yaml at line 13, The workflow currently
references the reusable workflow as
"openshift/hypershift/.github/workflows/gocacheprog-test-reusable.yaml@main",
which is mutable; update that reference to an immutable revision by replacing
"`@main`" with a specific commit SHA (e.g. "@<commit-sha>") or point to a local
workflow file (e.g. use a relative path to a vendored copy) so CI runs a
deterministic reusable workflow revision.

permissions:
contents: read
25 changes: 23 additions & 2 deletions contrib/ci/gocacheprog/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func handlePut(req *request, rwDir string) response {
if err := os.MkdirAll(filepath.Dir(dPath), 0o777); err != nil {
return response{ID: req.ID, Err: err.Error()}
}
if err := os.WriteFile(dPath, req.Body, 0o666); err != nil {
if err := writeFileAtomic(dPath, req.Body); err != nil {
return response{ID: req.ID, Err: err.Error()}
}

Expand All @@ -187,10 +187,31 @@ func handlePut(req *request, rwDir string) response {
req.BodySize,
time.Now().UnixNano(),
)
if err := os.WriteFile(aPath, []byte(entry), 0o666); err != nil {
if err := writeFileAtomic(aPath, []byte(entry)); err != nil {
return response{ID: req.ID, Err: err.Error()}
}

return response{ID: req.ID, DiskPath: dPath}
}

// writeFileAtomic writes data to a temporary file in the same directory
// then renames it to the target path. This prevents concurrent readers
// from seeing a truncated file.
func writeFileAtomic(path string, data []byte) error {
tmp, err := os.CreateTemp(filepath.Dir(path), ".tmp-*")
if err != nil {
return err
}
tmpName := tmp.Name()
if _, err := tmp.Write(data); err != nil {
tmp.Close()
os.Remove(tmpName)
return err
}
if err := tmp.Close(); err != nil {
os.Remove(tmpName)
return err
}
return os.Rename(tmpName, path)
}

61 changes: 61 additions & 0 deletions contrib/ci/gocacheprog/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path/filepath"
"sync"
"sync/atomic"
"testing"
"time"
)
Expand Down Expand Up @@ -419,3 +420,63 @@ func TestConcurrentPutAndGet(t *testing.T) {
}
wg.Wait()
}

func TestConcurrentPutSameOutputID(t *testing.T) {
t.Parallel()
rwDir := t.TempDir()
outputID := mustDecodeHex(t, "7777777777777777")
body := []byte("shared output content that all writers agree on")

// Seed one entry so GETs can find it while PUTs overwrite the data file.
seedAction := mustDecodeHex(t, fmt.Sprintf("%016x", 2000))
seedReq := &request{
ID: 0, Command: "put",
ActionID: seedAction, OutputID: outputID,
Body: body, BodySize: int64(len(body)),
}
if resp := handlePut(seedReq, rwDir); resp.Err != "" {
t.Fatalf("seed put: %s", resp.Err)
}

// Interleave PUTs (different ActionIDs, same OutputID) with GETs that
// read the data file via DiskPath. Without atomic writes, a PUT's
// O_TRUNC would momentarily zero the file, causing a reader to see
// truncated/empty content.
var wg sync.WaitGroup
var badReads atomic.Int64
for i := range 100 {
wg.Add(2)
go func() {
defer wg.Done()
actionID := mustDecodeHex(t, fmt.Sprintf("%016x", i+2000))
putReq := &request{
ID: int64(i), Command: "put",
ActionID: actionID, OutputID: outputID,
Body: body, BodySize: int64(len(body)),
}
if resp := handlePut(putReq, rwDir); resp.Err != "" {
t.Errorf("put %d error: %s", i, resp.Err)
}
}()
go func() {
defer wg.Done()
getReq := &request{ID: int64(i + 5000), Command: "get", ActionID: seedAction}
resp := handleGet(getReq, "", rwDir)
if resp.Miss {
return
}
data, err := os.ReadFile(resp.DiskPath)
if err != nil {
return
}
Comment on lines +463 to +471

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Count GET misses and read errors as test failures.

Right now this only increments badReads when the bytes differ, so the test still passes if concurrent PUTs make handleGet return a miss or make DiskPath unreadable. Those are part of the failure mode this test is supposed to catch.

🔧 Suggested fix
 		go func() {
 			defer wg.Done()
 			getReq := &request{ID: int64(i + 5000), Command: "get", ActionID: seedAction}
 			resp := handleGet(getReq, "", rwDir)
 			if resp.Miss {
+				badReads.Add(1)
 				return
 			}
 			data, err := os.ReadFile(resp.DiskPath)
 			if err != nil {
+				badReads.Add(1)
 				return
 			}
 			if string(data) != string(body) {
 				badReads.Add(1)
 			}
 		}()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
getReq := &request{ID: int64(i + 5000), Command: "get", ActionID: seedAction}
resp := handleGet(getReq, "", rwDir)
if resp.Miss {
return
}
data, err := os.ReadFile(resp.DiskPath)
if err != nil {
return
}
go func() {
defer wg.Done()
getReq := &request{ID: int64(i + 5000), Command: "get", ActionID: seedAction}
resp := handleGet(getReq, "", rwDir)
if resp.Miss {
badReads.Add(1)
return
}
data, err := os.ReadFile(resp.DiskPath)
if err != nil {
badReads.Add(1)
return
}
if string(data) != string(body) {
badReads.Add(1)
}
}()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contrib/ci/gocacheprog/main_test.go` around lines 463 - 471, The test
currently ignores GET misses and file read errors; update the loop where getReq
is created and handleGet is called (see getReq, handleGet, resp.Miss, and
os.ReadFile) so that if resp.Miss is true or os.ReadFile returns an error you
increment badReads (or the test failure counter) just like when the bytes
differ, and do not silently return; this ensures misses and read errors are
counted as test failures.

if string(data) != string(body) {
badReads.Add(1)
}
}()
}
wg.Wait()

if n := badReads.Load(); n > 0 {
t.Errorf("got %d reads with corrupted data from concurrent PUT/GET on same OutputID", n)
}
}
Loading