sneak/secret
1
0
Fork 0
Code Issues 22 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Make script/test fail on flaky failures and enable -race (closes #32) #53

Open
clawbot wants to merge 1 commits from fix-script-test-race into next
pull from: fix-script-test-race
merge into: sneak:next
Conversation 0 Commits 1 Files Changed 2 +14 -1
clawbot commented 2026-08-09 07:04:25 +02:00
Collaborator
Copy Link

Closes #32.

What changed

script/test only. Its last two operative lines were:

go vet ./...
go test ./... || go test -v ./...

The verbose rerun was the last command, so its exit status became the script's. A test that failed on the first run and passed on the retry produced exit 0. The Dockerfile runs make test and CI runs script/cibuild which runs docker build, so that hole made flaky failures invisible across the whole repo.

Replaced with the pattern from REPO_POLICIES.md, verbatim:

go test -timeout 30s -race -cover ./... || \
    { echo "--- Rerunning with -v for details ---"; \
      go test -timeout 30s -race -v ./...; exit 1; }

The trailing exit 1 is the point: the first run already proved the tests are broken, so the rerun is diagnostic only and can never turn a failed run green. go vet ./... still runs first, unchanged. The script stays POSIX sh with set -eu, the existing CGO_ENABLED=1 export, and the repo-root cd idiom. No other script is touched and .golangci.yml is untouched. TODO.md gets one additive entry in the same commit.

Read this before merging: CI is red, deliberately

Enabling -race exposed a real conflict with the policy timing budget. make check does not pass on this branch, and nothing here has been bent to make it pass.

Measured in the CI-equivalent container — script/cibuild, i.e. docker build --ulimit memlock=-1:-1 ., which runs make test in the builder stage. The host is unusable for this measurement: its memlock hard limit is 8192 KB, so the memguard 10MB case aborts long before the suite can be timed.

make test layer, wall clock slowest package
main today, no -race 18.7s (~9s of it go vet + compile) internal/cli 9.6s
this branch, -race on fails internal/cli killed at the 30s timeout 
panic: test timed out after 30s
	running tests:
		TestImportSecretVariousSizes (5s)
		TestImportSecretVariousSizes/99MB_file (3s)
FAIL	git.eeqj.de/sneak/secret/internal/cli	30.809s
ok  	git.eeqj.de/sneak/secret/internal/secret	12.698s	coverage: 43.1%
ok  	git.eeqj.de/sneak/secret/internal/vault	15.076s	coverage: 62.8%
ok  	git.eeqj.de/sneak/secret/pkg/agehd	3.166s	coverage: 86.3%
ok  	git.eeqj.de/sneak/secret/pkg/bip85	1.329s	coverage: 87.2%

No data race was found. Every package that ran to completion is clean under the detector. The failure is pure slowness, not a hang and not a deadlock — the stack at the timeout sits in memguard/core.Wipe under cli.(*Instance).ImportSecret (internal/cli/secrets.go:579), making ordinary forward progress. TestAddSecretVariousSizes alone eats 24.19s of the 30s budget, with 99MB at 10.25s and 100MB minus 1 at 10.19s; TestImportSecretVariousSizes is the same size ladder over the import path, which is where the package runs out of clock. internal/cli needs roughly 55-60s under -race against 9.6s without.

Every available lever is one the issue explicitly forbids: raising the timeout papers over it (and the 20s budget would still be missed by 3x), t.Skip is a skip, and shrinking the sizes deletes boundary coverage for the real 100MB cap at internal/cli/secrets.go:245 and :515. Making the large-secret path genuinely faster is out of scope here. No small in-scope change closes a 6x gap, so this stops and reports rather than forcing one.

#52 has the full measurements and four options for a decision. That decision is yours; my read is the build-tag split, since it is the only option that gives up nothing. This PR should land together with whatever #52 concludes, not before it.

Also worth stating plainly: the clean -race result is not evidence that #34's missing file locking is harmless. The suite has no test that touches the vault from two goroutines at once, so there was nothing for the detector to catch. #34 stands entirely on its own.

Verification

  • make lint — clean, 0 issues.
  • make fmt-check — clean.
  • make test — fails as described above; the new pattern was confirmed working end to end, printing --- Rerunning with -v for details --- and exiting non-zero after the rerun.
  • script/cibuild — run on both main and this branch for the before/after numbers in the table.
Closes #32. ## What changed `script/test` only. Its last two operative lines were: ```sh go vet ./... go test ./... || go test -v ./... ``` The verbose rerun was the last command, so its exit status became the script's. A test that failed on the first run and passed on the retry produced exit 0. The `Dockerfile` runs `make test` and CI runs `script/cibuild` which runs `docker build`, so that hole made flaky failures invisible across the whole repo. Replaced with the pattern from `REPO_POLICIES.md`, verbatim: ```sh go test -timeout 30s -race -cover ./... || \ { echo "--- Rerunning with -v for details ---"; \ go test -timeout 30s -race -v ./...; exit 1; } ``` The trailing `exit 1` is the point: the first run already proved the tests are broken, so the rerun is diagnostic only and can never turn a failed run green. `go vet ./...` still runs first, unchanged. The script stays POSIX sh with `set -eu`, the existing `CGO_ENABLED=1` export, and the repo-root `cd` idiom. No other script is touched and `.golangci.yml` is untouched. `TODO.md` gets one additive entry in the same commit. ## Read this before merging: CI is red, deliberately Enabling `-race` exposed a real conflict with the policy timing budget. **`make check` does not pass on this branch**, and nothing here has been bent to make it pass. Measured in the CI-equivalent container — `script/cibuild`, i.e. `docker build --ulimit memlock=-1:-1 .`, which runs `make test` in the builder stage. The host is unusable for this measurement: its `memlock` hard limit is 8192 KB, so the memguard 10MB case aborts long before the suite can be timed. | | `make test` layer, wall clock | slowest package | |---|---|---| | `main` today, no `-race` | **18.7s** (~9s of it `go vet` + compile) | `internal/cli` 9.6s | | this branch, `-race` on | **fails** | `internal/cli` killed at the 30s timeout | ``` panic: test timed out after 30s running tests: TestImportSecretVariousSizes (5s) TestImportSecretVariousSizes/99MB_file (3s) FAIL git.eeqj.de/sneak/secret/internal/cli 30.809s ok git.eeqj.de/sneak/secret/internal/secret 12.698s coverage: 43.1% ok git.eeqj.de/sneak/secret/internal/vault 15.076s coverage: 62.8% ok git.eeqj.de/sneak/secret/pkg/agehd 3.166s coverage: 86.3% ok git.eeqj.de/sneak/secret/pkg/bip85 1.329s coverage: 87.2% ``` **No data race was found.** Every package that ran to completion is clean under the detector. The failure is pure slowness, not a hang and not a deadlock — the stack at the timeout sits in `memguard/core.Wipe` under `cli.(*Instance).ImportSecret` (`internal/cli/secrets.go:579`), making ordinary forward progress. `TestAddSecretVariousSizes` alone eats 24.19s of the 30s budget, with `99MB` at 10.25s and `100MB minus 1` at 10.19s; `TestImportSecretVariousSizes` is the same size ladder over the import path, which is where the package runs out of clock. `internal/cli` needs roughly 55-60s under `-race` against 9.6s without. Every available lever is one the issue explicitly forbids: raising the timeout papers over it (and the 20s budget would still be missed by 3x), `t.Skip` is a skip, and shrinking the sizes deletes boundary coverage for the real 100MB cap at `internal/cli/secrets.go:245` and `:515`. Making the large-secret path genuinely faster is out of scope here. No small in-scope change closes a 6x gap, so this stops and reports rather than forcing one. **#52 has the full measurements and four options for a decision.** That decision is yours; my read is the build-tag split, since it is the only option that gives up nothing. This PR should land together with whatever #52 concludes, not before it. Also worth stating plainly: the clean `-race` result is not evidence that #34's missing file locking is harmless. The suite has no test that touches the vault from two goroutines at once, so there was nothing for the detector to catch. #34 stands entirely on its own. ## Verification - `make lint` — clean, `0 issues.` - `make fmt-check` — clean. - `make test` — fails as described above; the new pattern was confirmed working end to end, printing `--- Rerunning with -v for details ---` and exiting non-zero after the rerun. - `script/cibuild` — run on both `main` and this branch for the before/after numbers in the table.
clawbot added the needs-checks label 2026-08-09 07:06:26 +02:00
clawbot self-assigned this 2026-08-09 07:06:26 +02:00
clawbot added this to the 1.0.0 milestone 2026-08-09 07:06:26 +02:00
clawbot changed target branch from main to next 2026-08-10 16:13:08 +02:00
clawbot added 1 commit 2026-08-10 16:13:08 +02:00
Make script/test fail on flaky failures and enable -race (closes #32)
Some checks failed
check / check (push) Failing after 1m45s
Details
3d615ef612 
script/test ended with `go test ./... || go test -v ./...`, so the
verbose rerun's exit status became the script's. A test that failed
once and passed on the retry produced exit 0, and since the Dockerfile
runs `make test` and CI runs script/cibuild, flaky failures were
invisible repo-wide.

Replace that with the pattern from REPO_POLICIES.md: run
`go test -timeout 30s -race -cover ./...`, and on failure rerun
verbosely and then exit 1, so the rerun is diagnostic only and can
never turn a failed run green. `go vet ./...` still runs first; the
script stays POSIX sh with set -eu, the CGO_ENABLED=1 export, and the
repo-root cd idiom.

Enabling -race surfaced a genuine timing conflict rather than a data
race: no package that ran to completion reported one, but internal/cli
now exceeds the 30s timeout because the 99MB/100MB boundary cases in
secrets_size_test.go are about 10x slower under race instrumentation.
Measured in the CI-equivalent container (script/cibuild, with the
memlock ulimit): 18.7s for the make test layer without -race, versus
internal/cli alone needing roughly 55-60s with it. Neither the flags
nor the tests were weakened to hide this; the conflict is filed as #52
for a decision.
Some checks failed
check / check (push) Failing after 1m45s
Details
This pull request has changes conflicting with the target branch.
  • TODO.md
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix-script-test-race:fix-script-test-race
git checkout fix-script-test-race
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-checks
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#53
Delete Branch "fix-script-test-race"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?