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

Test suite cannot meet the 20s policy budget once -race is enabled: internal/cli 100MB boundary tests #52

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

Split out of #32, which adds -timeout 30s -race -cover to script/test per REPO_POLICIES.md. Enabling -race is correct and must stay; the measurements below show it collides with the policy's timing budget, and that collision needs a decision rather than a workaround.

Measured, in the CI-equivalent container

Both numbers come from script/cibuild (docker build --ulimit memlock=-1:-1 .), which runs make test inside the builder stage. The host cannot be used for this: its memlock hard limit is 8192 KB, so the memguard 10MB case aborts before the suite gets far enough to be timed.

Without -race (current main): the make test layer takes 18.7s wall, of which roughly 9s is go vet plus compilation. Slowest package internal/cli at 9.6s. Everything else is under 2s. That is inside the 20-second budget, but with very little headroom.

With -race (the #32 change): internal/cli blows through the 30-second per-package timeout and the suite fails.

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%

Where the time goes

TestAddSecretVariousSizes alone accounts for 24.19s of the 30s budget under race instrumentation, and its two largest cases are essentially the whole cost:

--- 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)

TestImportSecretVariousSizes is the same size ladder over the import path and costs about the same again, which is why the package dies partway through it. So internal/cli needs roughly 55-60s under -race, versus 9.6s without: about a 6x package-level slowdown, and close to 10x on the large cases specifically. That is expected — the race detector instruments every memory access, and these cases push ~300MB through age encryption and memguard wipes.

This is not a hang and not a deadlock. Nothing is blocked on a lock or a terminal read; the work is simply slow. The stack at the timeout is inside memguard/core.Wipe called from cli.(*Instance).ImportSecret (internal/cli/secrets.go:579), i.e. ordinary forward progress.

Why it was not "just fixed" in #32

The obvious levers are all excluded by #32's definition of done, and rightly so:

  • Raising -timeout past 30s: papering over it, and the 20s wall-clock budget would still be violated by 3x.
  • t.Skip / testing.Short() on the large cases: a skip, forbidden.
  • Shrinking the sizes: these are boundary tests for the real 100MB cap at internal/cli/secrets.go:245 and :515 (const maxSize = 100 * 1024 * 1024). The 99MB, 100MB-minus-1, and 101MB cases exist precisely to pin that boundary from both sides. Shrinking them deletes the coverage that justifies them.
  • Making the code faster: real work on the encrypt/import path, far outside #32's scope.

No small in-scope change closes a 3x-to-6x gap, so #32 reports and stops here instead of forcing one.

Options, for a decision

  1. Move the large-size cases behind a build tag (e.g. //go:build sizetests) and run them in a separate, slower CI job that keeps -race. Keeps the boundary coverage, keeps -race everywhere, gets make test back under budget. Costs a second CI entrypoint.
  2. Keep one boundary pair only — drop 99MB and keep 100MB minus 1 plus 101MB on both the add and import paths. Halves the cost while still pinning the cap from both sides. Cheapest, still a coverage reduction.
  3. Accept a larger budget for this repo and amend REPO_POLICIES.md for it, raising -timeout accordingly. Honest, but it relaxes a policy that exists for good reason.
  4. Speed up the large-secret path so the tests fit. Best outcome, most work, and would want its own issue.

My read is option 1: it is the only one that gives up nothing. But this is a policy call, not an implementation detail, so it should be decided rather than picked by whoever is holding the keyboard.

Also worth recording

The race detector found no data races in any package that ran to completion: internal/secret, internal/vault, pkg/agehd, pkg/bip85 are all clean under -race. internal/cli was killed at 30s, so its coverage under the detector is partial. This is not evidence that #34's missing file locking and non-atomic writes are harmless — the suite contains no test that exercises the vault from two goroutines at once, so there is nothing there for the detector to catch. #34 stands on its own.

Split out of #32, which adds `-timeout 30s -race -cover` to `script/test` per `REPO_POLICIES.md`. Enabling `-race` is correct and must stay; the measurements below show it collides with the policy's timing budget, and that collision needs a decision rather than a workaround. ## Measured, in the CI-equivalent container Both numbers come from `script/cibuild` (`docker build --ulimit memlock=-1:-1 .`), which runs `make test` inside the builder stage. The host cannot be used for this: its `memlock` hard limit is 8192 KB, so the memguard 10MB case aborts before the suite gets far enough to be timed. **Without `-race` (current `main`):** the `make test` layer takes **18.7s** wall, of which roughly 9s is `go vet` plus compilation. Slowest package `internal/cli` at 9.6s. Everything else is under 2s. That is inside the 20-second budget, but with very little headroom. **With `-race` (the #32 change):** `internal/cli` blows through the 30-second per-package timeout and the suite fails. ``` 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% ``` ## Where the time goes `TestAddSecretVariousSizes` alone accounts for 24.19s of the 30s budget under race instrumentation, and its two largest cases are essentially the whole cost: ``` --- 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) ``` `TestImportSecretVariousSizes` is the same size ladder over the import path and costs about the same again, which is why the package dies partway through it. So `internal/cli` needs roughly 55-60s under `-race`, versus 9.6s without: about a 6x package-level slowdown, and close to 10x on the large cases specifically. That is expected — the race detector instruments every memory access, and these cases push ~300MB through age encryption and memguard wipes. This is **not** a hang and **not** a deadlock. Nothing is blocked on a lock or a terminal read; the work is simply slow. The stack at the timeout is inside `memguard/core.Wipe` called from `cli.(*Instance).ImportSecret` (`internal/cli/secrets.go:579`), i.e. ordinary forward progress. ## Why it was not "just fixed" in #32 The obvious levers are all excluded by #32's definition of done, and rightly so: - Raising `-timeout` past 30s: papering over it, and the 20s wall-clock budget would still be violated by 3x. - `t.Skip` / `testing.Short()` on the large cases: a skip, forbidden. - Shrinking the sizes: these are boundary tests for the real 100MB cap at `internal/cli/secrets.go:245` and `:515` (`const maxSize = 100 * 1024 * 1024`). The 99MB, 100MB-minus-1, and 101MB cases exist precisely to pin that boundary from both sides. Shrinking them deletes the coverage that justifies them. - Making the code faster: real work on the encrypt/import path, far outside #32's scope. No small in-scope change closes a 3x-to-6x gap, so #32 reports and stops here instead of forcing one. ## Options, for a decision 1. **Move the large-size cases behind a build tag** (e.g. `//go:build sizetests`) and run them in a separate, slower CI job that keeps `-race`. Keeps the boundary coverage, keeps `-race` everywhere, gets `make test` back under budget. Costs a second CI entrypoint. 2. **Keep one boundary pair only** — drop `99MB` and keep `100MB minus 1` plus `101MB` on both the add and import paths. Halves the cost while still pinning the cap from both sides. Cheapest, still a coverage reduction. 3. **Accept a larger budget for this repo** and amend `REPO_POLICIES.md` for it, raising `-timeout` accordingly. Honest, but it relaxes a policy that exists for good reason. 4. **Speed up the large-secret path** so the tests fit. Best outcome, most work, and would want its own issue. My read is option 1: it is the only one that gives up nothing. But this is a policy call, not an implementation detail, so it should be decided rather than picked by whoever is holding the keyboard. ## Also worth recording The race detector found **no** data races in any package that ran to completion: `internal/secret`, `internal/vault`, `pkg/agehd`, `pkg/bip85` are all clean under `-race`. `internal/cli` was killed at 30s, so its coverage under the detector is partial. This is not evidence that #34's missing file locking and non-atomic writes are harmless — the suite contains no test that exercises the vault from two goroutines at once, so there is nothing there for the detector to catch. #34 stands on its own.
sneak was assigned by clawbot 2026-08-09 07:06:01 +02:00
clawbot added this to the 1.0.0 milestone 2026-08-09 07:06:01 +02:00
clawbot commented 2026-08-09 07:06:22 +02:00
Author
Collaborator
Copy Link

Assigning to @sneak. Option 3 amends REPO_POLICIES.md, and options 1 and 2 change what make check green actually means — none of those is mine to pick. Added to the 1.0.0 milestone. PR #53 is parked on needs-checks until this is decided; nothing else is blocked by it.

Option 2 does not work, and should be struck from the list. I checked the arithmetic rather than taking it at face value. internal/cli needs roughly 55-60s under -race. Dropping the 99MB case from both ladders saves about 20.5s (10.25s x 2), leaving ~34.5-39.5s. That still exceeds the 30s per-package timeout, and still exceeds the 20s wall-clock budget by nearly 2x. So option 2 pays real boundary coverage and does not buy a passing build. It is the worst of the four.

A finding that reframes the whole question, and which I think is more important than the -race conflict itself: the suite already takes 18.7s without -race. The 20-second budget is not something -race breaks — it is something the repo is already within 1.3 seconds of violating on a good day, on one machine, with warm caches. The next test anyone adds breaks it. internal/cli at 9.6s is over half the total, and the large-size cases dominate that.

That means the large-size tests do not belong in the fast inner-loop suite regardless of what is decided about -race. They are 100MB boundary tests pushing ~300MB through age encryption and memguard wipes; they are valuable and they should keep running, but they are not inner-loop tests, and treating them as such is what put the budget on a knife edge.

Recommendation: option 1, the build-tag split — and I would take it even if -race were off the table entirely, which is why I think it is the right answer rather than merely the convenient one:

  • Move the 99MB, 100MB minus 1, and 101MB cases on both the add and import paths behind //go:build sizetests.
  • Add a script/test-sizes entrypoint running exactly those with -race and a generous timeout, plus a make test-sizes shim.
  • Wire it into .gitea/workflows/ as a second job so the boundary coverage still gates every push. Not nightly, not manual — if it does not run on every push it will rot.
  • make test returns to roughly 8-9s with -race on, which restores real headroom instead of consuming it.

Nothing is given up: every assertion still runs in CI on every push, -race stays on everywhere, and the boundary at internal/cli/secrets.go:245 and :515 stays pinned from both sides.

The cost is honest and worth stating: make check alone no longer runs the size tests, so a developer can be green locally while the size job fails. That is a real regression in the local signal. It is mitigated by the CI job being mandatory and by make test-sizes existing for anyone who wants it, and I judge it a better trade than a 20-second budget that is already effectively breached — but it is exactly the kind of trade you should be the one to accept.

If you would rather not split the suite, option 4 (speed up the large-secret path) is the only remaining answer that gives up nothing, and it wants its own issue and a real look at why 99MB takes 10.25s under instrumentation. Option 3 is defensible too, but it relaxes a rule that exists precisely to stop suites drifting slow — and this one already has.

One thing worth not losing: the race detector found zero data races in every package that ran to completion — internal/secret, internal/vault, pkg/agehd, pkg/bip85. That is a genuinely good result. It is emphatically not evidence that #34's missing file locking is harmless: the suite has no test that touches a vault from two goroutines at once, so the detector had nothing to find. #34 stands entirely on its own, and closing it will need tests written specifically to provoke the races, not just -race switched on.

Assigning to @sneak. Option 3 amends `REPO_POLICIES.md`, and options 1 and 2 change what `make check` green actually means — none of those is mine to pick. Added to the `1.0.0` milestone. PR #53 is parked on `needs-checks` until this is decided; nothing else is blocked by it. **Option 2 does not work, and should be struck from the list.** I checked the arithmetic rather than taking it at face value. `internal/cli` needs roughly 55-60s under `-race`. Dropping the `99MB` case from both ladders saves about 20.5s (10.25s x 2), leaving **~34.5-39.5s**. That still exceeds the 30s per-package timeout, and still exceeds the 20s wall-clock budget by nearly 2x. So option 2 pays real boundary coverage and does not buy a passing build. It is the worst of the four. **A finding that reframes the whole question, and which I think is more important than the `-race` conflict itself:** the suite already takes **18.7s without `-race`**. The 20-second budget is not something `-race` breaks — it is something the repo is already within 1.3 seconds of violating on a good day, on one machine, with warm caches. The next test anyone adds breaks it. `internal/cli` at 9.6s is over half the total, and the large-size cases dominate that. That means the large-size tests do not belong in the fast inner-loop suite **regardless of what is decided about `-race`.** They are 100MB boundary tests pushing ~300MB through age encryption and memguard wipes; they are valuable and they should keep running, but they are not inner-loop tests, and treating them as such is what put the budget on a knife edge. **Recommendation: option 1, the build-tag split** — and I would take it even if `-race` were off the table entirely, which is why I think it is the right answer rather than merely the convenient one: - Move the `99MB`, `100MB minus 1`, and `101MB` cases on both the add and import paths behind `//go:build sizetests`. - Add a `script/test-sizes` entrypoint running exactly those with `-race` and a generous timeout, plus a `make test-sizes` shim. - Wire it into `.gitea/workflows/` as a second job so the boundary coverage still gates every push. Not nightly, not manual — if it does not run on every push it will rot. - `make test` returns to roughly 8-9s with `-race` on, which restores real headroom instead of consuming it. Nothing is given up: every assertion still runs in CI on every push, `-race` stays on everywhere, and the boundary at `internal/cli/secrets.go:245` and `:515` stays pinned from both sides. The cost is honest and worth stating: `make check` alone no longer runs the size tests, so a developer can be green locally while the size job fails. That is a real regression in the local signal. It is mitigated by the CI job being mandatory and by `make test-sizes` existing for anyone who wants it, and I judge it a better trade than a 20-second budget that is already effectively breached — but it is exactly the kind of trade you should be the one to accept. If you would rather not split the suite, option 4 (speed up the large-secret path) is the only remaining answer that gives up nothing, and it wants its own issue and a real look at why 99MB takes 10.25s under instrumentation. Option 3 is defensible too, but it relaxes a rule that exists precisely to stop suites drifting slow — and this one already has. **One thing worth not losing:** the race detector found **zero data races** in every package that ran to completion — `internal/secret`, `internal/vault`, `pkg/agehd`, `pkg/bip85`. That is a genuinely good result. It is emphatically **not** evidence that #34's missing file locking is harmless: the suite has no test that touches a vault from two goroutines at once, so the detector had nothing to find. #34 stands entirely on its own, and closing it will need tests written specifically to provoke the races, not just `-race` switched on.
clawbot commented 2026-08-10 16:13:48 +02:00
Author
Collaborator
Copy Link

Cap ruling landed org-wide and changes the arithmetic here: 60s hard for CI/green, 20s target, anything between filed as an improvement bug (sneak/prompts#41 (comment); 90s per-package backstop proposed in sneak/prompts#42).

It does not unblock this on its own. internal/cli needs ~55-60s under -race; packages run in parallel, so wall clock is that plus ~9s of vet/compile, i.e. ~65-70s — still over the 60s hard cap. The 90s backstop would stop the per-package kill, so the failure mode changes from "timeout panic" to "over the cap", but green still needs one of the four options.

Option 1 (build-tag split) remains my recommendation and now looks better, not worse: it lands make test at ~15-18s with -race, inside the 20s target rather than merely inside the hard cap. Option 3 is now narrower than when filed — it would ask for a repo-local exemption from a cap that was just set org-wide, rather than a change to a single number.

Cap ruling landed org-wide and changes the arithmetic here: 60s hard for CI/green, 20s target, anything between filed as an improvement bug (https://git.eeqj.de/sneak/prompts/issues/41#issuecomment-53166; 90s per-package backstop proposed in https://git.eeqj.de/sneak/prompts/pulls/42). It does not unblock this on its own. `internal/cli` needs ~55-60s under `-race`; packages run in parallel, so wall clock is that plus ~9s of vet/compile, i.e. **~65-70s** — still over the 60s hard cap. The 90s backstop would stop the per-package kill, so the failure mode changes from "timeout panic" to "over the cap", but green still needs one of the four options. Option 1 (build-tag split) remains my recommendation and now looks better, not worse: it lands `make test` at ~15-18s with `-race`, inside the 20s target rather than merely inside the hard cap. Option 3 is now narrower than when filed — it would ask for a repo-local exemption from a cap that was just set org-wide, rather than a change to a single number.
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 sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#52
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?