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

script/test silently passes on flaky failures, and runs without -race or -timeout #32

New Issue
Open
opened 2026-08-09 03:37:31 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:37:31 +02:00
Collaborator
Copy Link

Found auditing the repo against REPO_POLICIES.md. Present on main and on the pending golangci-v2.12.2 branch alike. This is the highest-severity non-code finding in the audit, because it degrades the gate that every other item on this milestone is verified by.

The defect

script/test in full, the operative lines:

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

Three separate policy violations in one line, one of which is a live correctness hole.

1. A flaky test yields a green build. The rerun is the last command in the script, so its exit status becomes the script's exit status. A test that fails on the first run and passes on the retry produces exit 0. Policy calls this exact failure mode out by name:

> The exit 1 ensures the target always fails after a rerun — the first run already proved the tests are broken, so the build must not pass even if a flaky test happens to succeed on the second attempt. The rerun exists solely for diagnostic output.

The blast radius is everything: the Dockerfile runs make test, .gitea/workflows/check.yml runs script/cibuild which runs docker build, so a flaky failure anywhere in the suite is invisible in CI. For a tool whose tests cover key derivation, encryption round-trips, and unlocker behavior, "the test failed once and we shipped it" is not an acceptable outcome.

2. No -race. Policy's Go pattern specifies -race. Its absence matters concretely here: issue #34 concerns missing file locking and non-atomic writes in internal/vault/secrets.go. Race detection is precisely the instrument that would catch regressions in that area, and it is switched off. CGO_ENABLED=1 is already exported by this script, so -race will work with no further change.

3. No -timeout 30s and no -cover. Policy:

> make test must complete in under 20 seconds. Add a 30-second timeout in the Makefile.

Without it, a test that deadlocks on a lock or blocks on a terminal read hangs the CI runner until the job-level timeout kills it, with no indication which test hung.

Definition of done

  • script/test runs go test -timeout 30s -race -cover ./... first.

  • On failure it reruns verbosely and then exits non-zero, exactly per the policy pattern:

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

  • go vet ./... continues to run first, unchanged.

  • The script stays POSIX sh, keeps set -eu, keeps the existing CGO_ENABLED=1 export and the repo-root cd idiom.

  • make check is green with the new script, and the full suite still finishes in under 20 seconds with -race enabled — race instrumentation is a real slowdown, so this must be measured, not assumed.

  • If enabling -race or -timeout 30s makes any existing test fail or time out, that is a genuine bug this change has uncovered. Fix the underlying code or the test — do not weaken the flags, do not add skips, and report what was found on this issue.

  • TODO.md updated in the same commit.

Implementation requirements

  • Do not touch any other script in the same commit.
  • The memlock ulimit requirement is real and pre-existing: the suite needs --ulimit memlock=-1:-1 (see script/cibuild) for the memguard test. Verify locally under the same conditions CI uses rather than only on the host.
  • Report the measured wall-clock of the suite with -race on, in a comment here. If it exceeds 20 seconds, do not silently raise the timeout — say so and stop, because that is a policy conflict that needs a decision rather than a workaround.
Found auditing the repo against `REPO_POLICIES.md`. Present on `main` and on the pending `golangci-v2.12.2` branch alike. This is the highest-severity non-code finding in the audit, because it degrades the gate that every other item on this milestone is verified by. ## The defect `script/test` in full, the operative lines: ```sh go vet ./... go test ./... || go test -v ./... ``` Three separate policy violations in one line, one of which is a live correctness hole. **1. A flaky test yields a green build.** The rerun is the last command in the script, so its exit status becomes the script's exit status. A test that fails on the first run and passes on the retry produces exit 0. Policy calls this exact failure mode out by name: > The `exit 1` ensures the target always fails after a rerun — the first run already proved the tests are broken, so the build must not pass even if a flaky test happens to succeed on the second attempt. The rerun exists solely for diagnostic output. The blast radius is everything: the `Dockerfile` runs `make test`, `.gitea/workflows/check.yml` runs `script/cibuild` which runs `docker build`, so a flaky failure anywhere in the suite is invisible in CI. For a tool whose tests cover key derivation, encryption round-trips, and unlocker behavior, "the test failed once and we shipped it" is not an acceptable outcome. **2. No `-race`.** Policy's Go pattern specifies `-race`. Its absence matters concretely here: issue #34 concerns missing file locking and non-atomic writes in `internal/vault/secrets.go`. Race detection is precisely the instrument that would catch regressions in that area, and it is switched off. `CGO_ENABLED=1` is already exported by this script, so `-race` will work with no further change. **3. No `-timeout 30s` and no `-cover`.** Policy: > `make test` must complete in under 20 seconds. Add a 30-second timeout in the Makefile. Without it, a test that deadlocks on a lock or blocks on a terminal read hangs the CI runner until the job-level timeout kills it, with no indication which test hung. ## Definition of done - `script/test` runs `go test -timeout 30s -race -cover ./...` first. - On failure it reruns verbosely and then **exits non-zero**, exactly per the policy pattern: ```sh go test -timeout 30s -race -cover ./... || \ { echo "--- Rerunning with -v for details ---"; \ go test -timeout 30s -race -v ./...; exit 1; } ``` - `go vet ./...` continues to run first, unchanged. - The script stays POSIX sh, keeps `set -eu`, keeps the existing `CGO_ENABLED=1` export and the repo-root `cd` idiom. - `make check` is green with the new script, and the full suite still finishes in under 20 seconds **with `-race` enabled** — race instrumentation is a real slowdown, so this must be measured, not assumed. - If enabling `-race` or `-timeout 30s` makes any existing test fail or time out, that is a genuine bug this change has uncovered. Fix the underlying code or the test — do not weaken the flags, do not add skips, and report what was found on this issue. - `TODO.md` updated in the same commit. ## Implementation requirements - Do not touch any other script in the same commit. - The memlock ulimit requirement is real and pre-existing: the suite needs `--ulimit memlock=-1:-1` (see `script/cibuild`) for the memguard test. Verify locally under the same conditions CI uses rather than only on the host. - Report the measured wall-clock of the suite with `-race` on, in a comment here. If it exceeds 20 seconds, do not silently raise the timeout — say so and stop, because that is a policy conflict that needs a decision rather than a workaround.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:37:31 +02:00
clawbot commented 2026-08-09 06:57:13 +02:00
Author
Collaborator
Copy Link

Implementation plan

Branch fix-script-test-race off origin/main (6e5e0db), in a scratch worktree.

1. script/test — the only file changed besides TODO.md.

Keep the shebang, set -eu, the ROOT/cd idiom, and the CGO_ENABLED=1 export exactly as they are. Keep go vet ./... as the first command in main(). Replace only the go test ./... || go test -v ./... line with the policy pattern 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 whole point: the rerun is diagnostic only and can never make the script succeed. No other script is touched, .golangci.yml is not touched.

2. Verification, in this order.

  • make fmt (no Go changes expected, but run it so the tree is canonical).
  • make check on the host first, to get fast feedback and a first look at whether -race fires.
  • Then script/cibuild — docker build --ulimit memlock=-1:-1 ., which is what CI actually runs. The Dockerfile runs make check, so this is the CI-equivalent gate, and the memlock ulimit is what keeps the memguard test honest. A host-only pass does not count as verified.
  • Measure wall-clock of the test suite with -race on and report the number here. Policy budget is 20 seconds. If it comes in over, I will report the measurement and stop rather than raise the timeout — that is a decision, not a workaround.

3. If the race detector fires.

I will not remove -race, add t.Skip, weaken a test, or stretch the timeout. Per #34 this repo has no file locking and no atomic writes in non-test code, so a detector hit is a genuine finding. I will report the racing goroutines, the shared state, and the file paths here in detail. A small, obviously in-scope fix I will make; anything substantial stays with #34 and I will stop rather than half-fix it inside this commit.

4. Bookkeeping.

TODO.md gets one additive entry in the same commit, nothing else in the file rewritten — PR #29 is merge-ready and also edits TODO.md, so a broad edit here would only manufacture a conflict.

Commit subject ends with (closes #32). PR opened against main; labels and assignees left alone.

## Implementation plan Branch `fix-script-test-race` off `origin/main` (`6e5e0db`), in a scratch worktree. **1. `script/test` — the only file changed besides `TODO.md`.** Keep the shebang, `set -eu`, the `ROOT`/`cd` idiom, and the `CGO_ENABLED=1` export exactly as they are. Keep `go vet ./...` as the first command in `main()`. Replace only the `go test ./... || go test -v ./...` line with the policy pattern 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 whole point: the rerun is diagnostic only and can never make the script succeed. No other script is touched, `.golangci.yml` is not touched. **2. Verification, in this order.** - `make fmt` (no Go changes expected, but run it so the tree is canonical). - `make check` on the host first, to get fast feedback and a first look at whether `-race` fires. - Then `script/cibuild` — `docker build --ulimit memlock=-1:-1 .`, which is what CI actually runs. The `Dockerfile` runs `make check`, so this is the CI-equivalent gate, and the memlock ulimit is what keeps the memguard test honest. A host-only pass does not count as verified. - Measure wall-clock of the test suite with `-race` on and report the number here. Policy budget is 20 seconds. If it comes in over, I will report the measurement and stop rather than raise the timeout — that is a decision, not a workaround. **3. If the race detector fires.** I will not remove `-race`, add `t.Skip`, weaken a test, or stretch the timeout. Per #34 this repo has no file locking and no atomic writes in non-test code, so a detector hit is a genuine finding. I will report the racing goroutines, the shared state, and the file paths here in detail. A small, obviously in-scope fix I will make; anything substantial stays with #34 and I will stop rather than half-fix it inside this commit. **4. Bookkeeping.** `TODO.md` gets one additive entry in the same commit, nothing else in the file rewritten — PR #29 is merge-ready and also edits `TODO.md`, so a broad edit here would only manufacture a conflict. Commit subject ends with ` (closes #32)`. PR opened against `main`; labels and assignees left alone.
clawbot commented 2026-08-09 07:04:54 +02:00
Author
Collaborator
Copy Link

Result: implemented, measured, and blocked on a policy decision

PR #53 (branch fix-script-test-race, commit 3d615ef). script/test and one additive TODO.md entry, nothing else. The script keeps set -eu, the CGO_ENABLED=1 export, the repo-root cd, and go vet ./... first; only the test line changed, to the policy pattern verbatim with the trailing exit 1.

Confirmed end to end that the exit-status hole is closed: the run prints --- Rerunning with -v for details --- and then exits non-zero, so the rerun can no longer launder a first-run failure into a green build.

Measured wall-clock, as requested

Taken with script/cibuild (docker build --ulimit memlock=-1:-1 .), not on the host — the host's memlock hard limit is 8192 KB, so the memguard 10MB case panics with could not acquire lock ... limit reached before the suite gets far enough to time. The container is the only honest measurement here.

  • Without -race, main today: 18.7s for the make test layer, about 9s of which is go vet plus compilation. Slowest package internal/cli at 9.6s, everything else under 2s. Inside the 20s budget, but with almost no headroom.
  • With -race: the suite fails. internal/cli is killed at the 30s per-package timeout. Extrapolating from the completed subtests it needs roughly 55-60s, about 6x its non-race time.
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%

So the answer to the question this issue asked me to measure rather than assume: no, the suite does not fit in 20 seconds with -race on — it misses by roughly 3x. Per the instruction in the issue body, I am reporting that and stopping rather than raising the timeout.

The race detector did not fire

Worth being precise, because it is the more interesting half of the result. internal/secret, internal/vault, pkg/agehd and pkg/bip85 all ran to completion under -race and reported zero data races. internal/cli was killed at 30s, so its coverage under the detector is partial.

The internal/cli failure is not a race, not a deadlock, and not a hang. Nothing is blocked on a lock or a terminal read. The goroutine at the timeout is inside memguard/core.Wipe, reached from cli.(*Instance).ImportSecret at internal/cli/secrets.go:579 — ordinary forward progress, just slow. The cost is concentrated in the giant-size cases:

--- PASS: TestAddSecretVariousSizes (24.19s)
    --- PASS: TestAddSecretVariousSizes/10MB_secret (1.28s)
    --- PASS: TestAddSecretVariousSizes/99MB_secret (10.25s)
    --- PASS: TestAddSecretVariousSizes/100MB_secret_minus_1_byte (10.19s)
    --- PASS: TestAddSecretVariousSizes/101MB_secret_-_should_fail (1.89s)

24.19s of the 30s budget gone before TestImportSecretVariousSizes even starts, and that test is the same size ladder over the import path. Race instrumentation touches every memory access, and these cases push ~300MB through age encryption and memguard wipes, so the ~10x hit on the large cases is expected rather than anomalous.

This also means the clean result says nothing about #34. The suite contains no test that exercises the vault from two goroutines at once, so the detector had nothing to catch. #34's missing file locking and non-atomic writes remain entirely unaddressed and unmeasured by this change.

Why I did not "just fix the test"

The definition of done says to fix the underlying code or the test rather than weaken the flags. I could not find a fix that is both in scope and not a weakening:

  • Raising -timeout past 30s papers over it, and the 20s wall-clock budget would still be missed by 3x.
  • t.Skip or testing.Short() on the large cases is a skip, explicitly forbidden.
  • Shrinking the sizes deletes real coverage: those cases pin the 100MB cap at internal/cli/secrets.go:245 and :515 (const maxSize = 100 * 1024 * 1024) from both sides — 99MB and 100MB-minus-1 must pass, 101MB must fail.
  • Making the large-secret path actually faster is genuine performance work on the encrypt/import path, well outside this issue.

No small in-scope change closes a 6x gap, so forcing one into this commit would have been worse than reporting it.

Filed for the decision: #52

#52 carries the full numbers and four options — build-tag split for the large cases with a separate slower CI job, trimming to a single boundary pair, amending the policy budget for this repo, or optimizing the path. My read is the build-tag split, since it is the only one that gives up nothing, but it is a policy call and should be decided rather than picked by me.

PR #53 is therefore red on purpose and should land together with whatever #52 concludes. make lint and make fmt-check are both clean on the branch; make test fails only for the reason above.

## Result: implemented, measured, and blocked on a policy decision PR #53 (branch `fix-script-test-race`, commit `3d615ef`). `script/test` and one additive `TODO.md` entry, nothing else. The script keeps `set -eu`, the `CGO_ENABLED=1` export, the repo-root `cd`, and `go vet ./...` first; only the test line changed, to the policy pattern verbatim with the trailing `exit 1`. Confirmed end to end that the exit-status hole is closed: the run prints `--- Rerunning with -v for details ---` and then exits non-zero, so the rerun can no longer launder a first-run failure into a green build. ### Measured wall-clock, as requested Taken with `script/cibuild` (`docker build --ulimit memlock=-1:-1 .`), not on the host — the host's `memlock` hard limit is 8192 KB, so the memguard 10MB case panics with `could not acquire lock ... limit reached` before the suite gets far enough to time. The container is the only honest measurement here. - **Without `-race`, `main` today: 18.7s** for the `make test` layer, about 9s of which is `go vet` plus compilation. Slowest package `internal/cli` at 9.6s, everything else under 2s. Inside the 20s budget, but with almost no headroom. - **With `-race`: the suite fails.** `internal/cli` is killed at the 30s per-package timeout. Extrapolating from the completed subtests it needs roughly 55-60s, about 6x its non-race time. ``` 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% ``` So the answer to the question this issue asked me to measure rather than assume: **no, the suite does not fit in 20 seconds with `-race` on** — it misses by roughly 3x. Per the instruction in the issue body, I am reporting that and stopping rather than raising the timeout. ### The race detector did not fire Worth being precise, because it is the more interesting half of the result. `internal/secret`, `internal/vault`, `pkg/agehd` and `pkg/bip85` all ran to completion under `-race` and reported **zero** data races. `internal/cli` was killed at 30s, so its coverage under the detector is partial. The `internal/cli` failure is **not** a race, **not** a deadlock, and **not** a hang. Nothing is blocked on a lock or a terminal read. The goroutine at the timeout is inside `memguard/core.Wipe`, reached from `cli.(*Instance).ImportSecret` at `internal/cli/secrets.go:579` — ordinary forward progress, just slow. The cost is concentrated in the giant-size cases: ``` --- PASS: TestAddSecretVariousSizes (24.19s) --- PASS: TestAddSecretVariousSizes/10MB_secret (1.28s) --- PASS: TestAddSecretVariousSizes/99MB_secret (10.25s) --- PASS: TestAddSecretVariousSizes/100MB_secret_minus_1_byte (10.19s) --- PASS: TestAddSecretVariousSizes/101MB_secret_-_should_fail (1.89s) ``` 24.19s of the 30s budget gone before `TestImportSecretVariousSizes` even starts, and that test is the same size ladder over the import path. Race instrumentation touches every memory access, and these cases push ~300MB through age encryption and memguard wipes, so the ~10x hit on the large cases is expected rather than anomalous. This also means the clean result says nothing about #34. The suite contains no test that exercises the vault from two goroutines at once, so the detector had nothing to catch. #34's missing file locking and non-atomic writes remain entirely unaddressed and unmeasured by this change. ### Why I did not "just fix the test" The definition of done says to fix the underlying code or the test rather than weaken the flags. I could not find a fix that is both in scope and not a weakening: - Raising `-timeout` past 30s papers over it, and the 20s wall-clock budget would still be missed by 3x. - `t.Skip` or `testing.Short()` on the large cases is a skip, explicitly forbidden. - Shrinking the sizes deletes real coverage: those cases pin the 100MB cap at `internal/cli/secrets.go:245` and `:515` (`const maxSize = 100 * 1024 * 1024`) from both sides — 99MB and 100MB-minus-1 must pass, 101MB must fail. - Making the large-secret path actually faster is genuine performance work on the encrypt/import path, well outside this issue. No small in-scope change closes a 6x gap, so forcing one into this commit would have been worse than reporting it. ### Filed for the decision: #52 #52 carries the full numbers and four options — build-tag split for the large cases with a separate slower CI job, trimming to a single boundary pair, amending the policy budget for this repo, or optimizing the path. My read is the build-tag split, since it is the only one that gives up nothing, but it is a policy call and should be decided rather than picked by me. PR #53 is therefore **red on purpose** and should land together with whatever #52 concludes. `make lint` and `make fmt-check` are both clean on the branch; `make test` fails only for the reason above.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#32
Delete Branch "%!s()"

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?