From fbce52b207c7969037805f07c5dc39a571c7397f Mon Sep 17 00:00:00 2001 From: Shai Asher Date: Mon, 24 Nov 2025 13:44:58 +0200 Subject: [PATCH 1/2] fix: remove continue-on-error from critical CI steps Previously, the CI workflow had several critical short-circuits that would hide failures: 1. go vet had continue-on-error: true (line 56) - This silently ignored all vet failures - Removed to catch real issues 2. Tests had continue-on-error: true (line 84) - This was VERY problematic - test failures were ignored - Tests would appear to pass even when failing - Removed to ensure test failures fail the CI 3. ci-success job had flawed logic (lines 250-253) - Allowed both success AND failure for vet - Now properly checks that all jobs succeeded - Added detailed output showing each job result 4. Improved ci-success validation - Added echo statements to show all job results - Uses a FAILED flag to accumulate failures - Clear error messages for each failed job - Better visibility into what failed These changes ensure that: - Test failures always fail the CI - Vet issues are not silently ignored - CI accurately reports the build status - No more hidden failures in the pipeline The previous continue-on-error flags were added for known assembly bugs but they prevented discovering NEW bugs and regressions. --- .github/workflows/ci.yml | 69 +++++++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8b7c003..b8dc238 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: exit 1 fi - # Run go vet (allow to fail due to known assembly issues) + # Run go vet vet: name: Go Vet runs-on: ubuntu-latest @@ -53,7 +53,6 @@ jobs: - name: Run go vet run: go vet ./... - continue-on-error: true # Run tests on multiple platforms test: @@ -81,7 +80,6 @@ jobs: - name: Run tests run: go test -v -race -count=1 ./... - continue-on-error: true # Allow failures due to known assembly bugs - name: Generate coverage if: matrix.os == 'ubuntu-latest' @@ -224,7 +222,7 @@ jobs: go install golang.org/x/vuln/cmd/govulncheck@latest govulncheck ./... - # All checks must pass (Windows tests allowed to fail) + # All checks must pass ci-success: name: CI Success runs-on: ubuntu-latest @@ -242,37 +240,64 @@ jobs: - name: Check required jobs run: | # Check if any required job failed + echo "Checking CI job results..." + echo "format-check: ${{ needs.format-check.result }}" + echo "vet: ${{ needs.vet.result }}" + echo "test: ${{ needs.test.result }}" + echo "test-arm64: ${{ needs.test-arm64.result }}" + echo "lint: ${{ needs.lint.result }}" + echo "build-matrix: ${{ needs.build-matrix.result }}" + echo "verify-assembly: ${{ needs.verify-assembly.result }}" + echo "security: ${{ needs.security.result }}" + echo "" + + FAILED=0 + if [ "${{ needs.format-check.result }}" != "success" ]; then - echo "Format check failed" - exit 1 + echo "❌ Format check failed or was skipped" + FAILED=1 fi - # Vet allowed to fail due to known assembly issues - if [ "${{ needs.vet.result }}" != "success" ] && [ "${{ needs.vet.result }}" != "failure" ]; then - echo "Vet check did not complete" - exit 1 + + if [ "${{ needs.vet.result }}" != "success" ]; then + echo "❌ Vet check failed or was skipped" + FAILED=1 fi + if [ "${{ needs.test.result }}" != "success" ]; then - echo "Tests failed" - exit 1 + echo "❌ Tests failed or were skipped" + FAILED=1 fi + if [ "${{ needs.test-arm64.result }}" != "success" ]; then - echo "ARM64 tests failed" - exit 1 + echo "❌ ARM64 tests failed or were skipped" + FAILED=1 fi + if [ "${{ needs.lint.result }}" != "success" ]; then - echo "Lint failed" - exit 1 + echo "❌ Lint failed or was skipped" + FAILED=1 fi + if [ "${{ needs.build-matrix.result }}" != "success" ]; then - echo "Build matrix failed" - exit 1 + echo "❌ Build matrix failed or was skipped" + FAILED=1 fi + if [ "${{ needs.verify-assembly.result }}" != "success" ]; then - echo "Assembly verification failed" - exit 1 + echo "❌ Assembly verification failed or was skipped" + FAILED=1 fi + if [ "${{ needs.security.result }}" != "success" ]; then - echo "Security scan failed" + echo "❌ Security scan failed or was skipped" + FAILED=1 + fi + + if [ $FAILED -eq 1 ]; then + echo "" + echo "❌ CI checks failed!" exit 1 fi - echo "All CI checks passed!" + + echo "" + echo "✅ All CI checks passed!" From 5d125ee95fae4a2055ede954810483c7e6e07198 Mon Sep 17 00:00:00 2001 From: Shai Asher Date: Mon, 24 Nov 2025 14:06:34 +0200 Subject: [PATCH 2/2] fix: correct assembly function signatures for go vet Fix two assembly issues that were revealed after removing continue-on-error: 1. Fixed bucketLookupAVX2 frame size - Changed from $0-40 to $0-33 - go vet correctly calculated the argument size: * []byte = 24 bytes (ptr + len + cap) * byte = 1 byte * bool return = 1 byte * Total with padding = 33 bytes 2. Disabled unused processBatchXXHashAVX2 assembly - Added "unused" build tag to batch_avx2_amd64.s - The Go declaration is commented out in batch_amd64.go - Prevents "missing Go declaration" error from go vet - Added clear documentation on how to re-enable These issues were previously hidden by continue-on-error: true in the CI workflow. Now that we properly fail on vet errors, these needed to be fixed. All tests pass and go vet is clean. --- internal/hash/xxhash/batch_avx2_amd64.s | 8 ++++++-- internal/lookup/bucket_lookup_avx2_amd64.s | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/hash/xxhash/batch_avx2_amd64.s b/internal/hash/xxhash/batch_avx2_amd64.s index d6eb934..ba62dfb 100644 --- a/internal/hash/xxhash/batch_avx2_amd64.s +++ b/internal/hash/xxhash/batch_avx2_amd64.s @@ -1,5 +1,9 @@ -//go:build amd64 -// +build amd64 +//go:build amd64 && unused +// +build amd64,unused + +// NOTE: This file is currently disabled because processBatchXXHashAVX2 is not used. +// The Go declaration is commented out in batch_amd64.go. +// To enable, uncomment the function declaration and remove the "unused" build tag. #include "textflag.h" diff --git a/internal/lookup/bucket_lookup_avx2_amd64.s b/internal/lookup/bucket_lookup_avx2_amd64.s index 789e9a2..38a1b7d 100644 --- a/internal/lookup/bucket_lookup_avx2_amd64.s +++ b/internal/lookup/bucket_lookup_avx2_amd64.s @@ -3,7 +3,7 @@ #include "textflag.h" // func bucketLookupAVX2(fingerprints []byte, target byte) bool -TEXT ·bucketLookupAVX2(SB), NOSPLIT, $0-40 +TEXT ·bucketLookupAVX2(SB), NOSPLIT, $0-33 MOVQ fingerprints_base+0(FP), AX // AX = pointer to fingerprints MOVQ fingerprints_len+8(FP), CX // CX = length MOVBQZX target+24(FP), DX // DX = target fingerprint