sneak/vaultik
1
0
Fork 0
Code Issues 19 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

make test-integration is a silent no-op — no file carries the integration build tag #69

New Issue
Closed
opened 2026-08-09 03:42:34 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 03:42:34 +02:00
Collaborator
Copy Link

Makefile:70-71 defines:

test-integration:
	go test -v -tags=integration ./...

But grep -rn '//go:build' --include=*.go . returns nothing. No file
in the repo carries a build tag of any kind, so -tags=integration
selects no additional files and make test-integration is an exact
duplicate of make test.

The practical effect is worse than a useless target: internal/vaultik/ integration_test.go (1,033 lines of full chunk → pack → encrypt → upload
→ restore round-trips) runs unconditionally on every make test, and
anyone reading the Makefile reasonably concludes it does not.

Related risk in the same area: script/test runs
go test -race -timeout 30s ./.... That is a per-package 30-second
timeout applied to internal/vaultik, which does full round-trips under
-race. Measured locally it takes ~6.7s, and internal/database ~6.9s,
but on slower CI this is the first thing that will flake.

Definition of done

  1. Decide and implement one of:

    • (a) Tag the genuinely slow/integration tests with
      //go:build integration so make test is fast and
      make test-integration is meaningfully different; or
    • (b) Delete the test-integration target and its script/
      counterpart if one exists, and document that make test runs
      everything.

    State which was chosen and why in the commit message. Option (a) is
    preferred if the round-trip tests are slow enough to be worth gating;
    otherwise (b) — a target that lies is worse than no target.

  2. If (a): make test and make test-integration demonstrably run
    different sets of tests, and CI runs both. The integration set must
    still run somewhere in CI, not silently drop out of coverage.

  3. The -timeout 30s in script/test is reviewed against actual
    measured runtimes with margin for slow CI, and either raised with a
    comment explaining the chosen value, or explicitly confirmed as
    adequate. Do not leave it unexamined.

  4. Per repo policy, make and script/ entrypoints only — no raw
    go test invocations introduced.

  5. make check green.

`Makefile:70-71` defines: ``` test-integration: go test -v -tags=integration ./... ``` But `grep -rn '//go:build' --include=*.go .` returns **nothing**. No file in the repo carries a build tag of any kind, so `-tags=integration` selects no additional files and `make test-integration` is an exact duplicate of `make test`. The practical effect is worse than a useless target: `internal/vaultik/ integration_test.go` (1,033 lines of full chunk → pack → encrypt → upload → restore round-trips) runs unconditionally on every `make test`, and anyone reading the Makefile reasonably concludes it does not. Related risk in the same area: `script/test` runs `go test -race -timeout 30s ./...`. That is a **per-package** 30-second timeout applied to `internal/vaultik`, which does full round-trips under `-race`. Measured locally it takes ~6.7s, and `internal/database` ~6.9s, but on slower CI this is the first thing that will flake. ## Definition of done 1. Decide and implement one of: - **(a)** Tag the genuinely slow/integration tests with `//go:build integration` so `make test` is fast and `make test-integration` is meaningfully different; or - **(b)** Delete the `test-integration` target and its `script/` counterpart if one exists, and document that `make test` runs everything. State which was chosen and why in the commit message. Option (a) is preferred if the round-trip tests are slow enough to be worth gating; otherwise (b) — a target that lies is worse than no target. 2. If (a): `make test` and `make test-integration` demonstrably run different sets of tests, and CI runs both. The integration set must still run somewhere in CI, not silently drop out of coverage. 3. The `-timeout 30s` in `script/test` is reviewed against actual measured runtimes with margin for slow CI, and either raised with a comment explaining the chosen value, or explicitly confirmed as adequate. Do not leave it unexamined. 4. Per repo policy, `make` and `script/` entrypoints only — no raw `go test` invocations introduced. 5. `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:34 +02:00
clawbot commented 2026-08-09 09:29:47 +02:00
Author
Collaborator
Copy Link

New evidence raising the priority of this issue's timeout item, plus one
thing that checks out clean.

The timeout is a live flake risk as of today

The shared BuildKit cache on this host was destroyed — a subagent on
another repo ran docker builder prune -af, wiping ~41 GB. Every session
lost its Docker layer cache, so builds and containerised test runs are
cold.

script/test runs:

go test -race -timeout 30s ./...

That is a per-package 30-second timeout, under -race, and it now
runs against an empty Go build cache inside a fresh container. A sibling
repo hit exactly this failure within minutes: timeout 30 go test ./...
died on an empty build cache in a fresh container, then passed in 11s on
retry.

vaultik's measured warm timings leave less headroom than they appear to:
internal/database ~5.9s and internal/vaultik ~6.2s warm. Compilation
of a large -race build on a cold cache is charged against that same
30-second budget, and the margin is thinner than the numbers suggest.

Practical consequence right now: a test timeout observed today is
more likely cold-cache compilation than a real defect. Retry before
treating it as one, and say in any report whether the run was cold.

This strengthens the case for definition-of-done item 3 — the timeout
should be reviewed against a genuinely cold cache with margin for slow
CI, not just against warm local runs, and the chosen value should carry a
comment explaining it.

What checks out clean

script/test's retry does not swallow failures. The sibling-repo bug
where a verbose rerun could convert a failure into a pass does not exist
here:

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

The unconditional exit 1 after the rerun guarantees a non-zero exit
regardless of what the second run reports, and the header comment states
that intent outright. Verified by reading, not assumed. Recording it here
because it is a plausible place to look for a fourth false-green
mechanism in this repo, and it is genuinely sound.

Worth noting the retry does double the work on failure, so a cold-cache
timeout costs two full -race runs before reporting. Not a correctness
problem, but relevant when judging what timeout value is appropriate.

New evidence raising the priority of this issue's timeout item, plus one thing that checks out clean. ## The timeout is a live flake risk as of today The shared BuildKit cache on this host was destroyed — a subagent on another repo ran `docker builder prune -af`, wiping ~41 GB. Every session lost its Docker layer cache, so builds and containerised test runs are cold. `script/test` runs: ```sh go test -race -timeout 30s ./... ``` That is a **per-package** 30-second timeout, under `-race`, and it now runs against an empty Go build cache inside a fresh container. A sibling repo hit exactly this failure within minutes: `timeout 30 go test ./...` died on an empty build cache in a fresh container, then passed in 11s on retry. vaultik's measured warm timings leave less headroom than they appear to: `internal/database` ~5.9s and `internal/vaultik` ~6.2s warm. Compilation of a large `-race` build on a cold cache is charged against that same 30-second budget, and the margin is thinner than the numbers suggest. **Practical consequence right now:** a test timeout observed today is more likely cold-cache compilation than a real defect. Retry before treating it as one, and say in any report whether the run was cold. This strengthens the case for definition-of-done item 3 — the timeout should be reviewed against a genuinely cold cache with margin for slow CI, not just against warm local runs, and the chosen value should carry a comment explaining it. ## What checks out clean `script/test`'s retry does **not** swallow failures. The sibling-repo bug where a verbose rerun could convert a failure into a pass does not exist here: ```sh go test -race -timeout 30s ./... || { echo "--- Rerunning with -v for details ---" go test -race -timeout 30s -v ./... exit 1 } ``` The unconditional `exit 1` after the rerun guarantees a non-zero exit regardless of what the second run reports, and the header comment states that intent outright. Verified by reading, not assumed. Recording it here because it is a plausible place to look for a fourth false-green mechanism in this repo, and it is genuinely sound. Worth noting the retry does double the work on failure, so a cold-cache timeout costs two full `-race` runs before reporting. Not a correctness problem, but relevant when judging what timeout value is appropriate.
clawbot commented 2026-08-09 11:59:04 +02:00
Author
Collaborator
Copy Link

Plan

Implementing this together with #93 as a single PR — both change
script/test, and DoD item 3 here (the -timeout 30s budget) has to be
decided against the uncached behaviour #93 introduces, or it would be
tuned against runs that did no work.

Item 1: choosing (b), delete the target

Confirmed the premise: grep -rn '//go:build' --include=*.go . returns
nothing, so -tags=integration selects no extra files and
make test-integration is byte-for-byte make test plus -v.

I intend to take (b) — delete test-integration and document that
make test runs everything, including the round-trips in
internal/vaultik/integration_test.go. The DoD says (a) is preferred
"if the round-trip tests are slow enough to be worth gating", and
measured on main at 3f9c2e5 they are not: a full cold make test
across all 18 packages under -race is 15.9s, with the two heaviest
packages at 6.6s (internal/vaultik) and 6.4s (internal/database).
Splitting the round-trips out would save a few seconds off a
sixteen-second suite, in exchange for a build-tag scheme and a second CI
path that must be kept wired up. Given that this repo has now found five
separate ways for a gate to report a green it did not earn (#78, #80,
#85, #88, #93), adding a mechanism whose failure mode is "some tests
silently stopped running" is a bad trade for those seconds. (b) also
discharges DoD item 2 for free: nothing can drop out of CI coverage if
nothing is conditional.

Item 3: the timeout, and a claim I want to check first

The comment above states that cold-cache compilation is charged against
the 30s budget. I do not believe it is: -timeout is handed to the
compiled test binary as -test.timeout, and that clock starts inside
testing.M.Run, after compilation and linking have finished. The
sibling-repo failure cited was timeout 30 go test ./... — a shell
timeout around the whole invocation, which does include compilation.
Different mechanism.

I am measuring this rather than asserting it: a cold containerised run
via script/cibuild has an empty GOCACHE inside the image, so if
compilation counted, its per-package durations would be far larger than
the warm host ones. If they come back close to warm, the claim is
disproved and the real risk is only slow/contended CI executing the
tests themselves more slowly.

Either way the value gets reviewed and carries a comment. My prior is
that 30s is too thin — not because compilation counts, but because a
-timeout is a hang backstop rather than a performance budget, and
4.5x over the slowest measured package is not enough margin for a
loaded CI runner. Final number and its supporting measurement will be in
the PR.

## Plan Implementing this together with #93 as a single PR — both change `script/test`, and DoD item 3 here (the `-timeout 30s` budget) has to be decided against the uncached behaviour #93 introduces, or it would be tuned against runs that did no work. ### Item 1: choosing (b), delete the target Confirmed the premise: `grep -rn '//go:build' --include=*.go .` returns nothing, so `-tags=integration` selects no extra files and `make test-integration` is byte-for-byte `make test` plus `-v`. I intend to take **(b)** — delete `test-integration` and document that `make test` runs everything, including the round-trips in `internal/vaultik/integration_test.go`. The DoD says (a) is preferred "if the round-trip tests are slow enough to be worth gating", and measured on `main` at `3f9c2e5` they are not: a full cold `make test` across all 18 packages under `-race` is **15.9s**, with the two heaviest packages at 6.6s (`internal/vaultik`) and 6.4s (`internal/database`). Splitting the round-trips out would save a few seconds off a sixteen-second suite, in exchange for a build-tag scheme and a second CI path that must be kept wired up. Given that this repo has now found five separate ways for a gate to report a green it did not earn (#78, #80, #85, #88, #93), adding a mechanism whose failure mode is "some tests silently stopped running" is a bad trade for those seconds. (b) also discharges DoD item 2 for free: nothing can drop out of CI coverage if nothing is conditional. ### Item 3: the timeout, and a claim I want to check first The comment above states that cold-cache compilation is charged against the 30s budget. I do not believe it is: `-timeout` is handed to the compiled test binary as `-test.timeout`, and that clock starts inside `testing.M.Run`, after compilation and linking have finished. The sibling-repo failure cited was `timeout 30 go test ./...` — a shell `timeout` around the whole invocation, which does include compilation. Different mechanism. I am measuring this rather than asserting it: a cold containerised run via `script/cibuild` has an empty `GOCACHE` inside the image, so if compilation counted, its per-package durations would be far larger than the warm host ones. If they come back close to warm, the claim is disproved and the real risk is only slow/contended CI executing the tests themselves more slowly. Either way the value gets reviewed and carries a comment. My prior is that 30s is too thin — not because compilation counts, but because a `-timeout` is a hang backstop rather than a performance budget, and 4.5x over the slowest measured package is not enough margin for a loaded CI runner. Final number and its supporting measurement will be in the PR.
clawbot commented 2026-08-09 12:13:53 +02:00
Author
Collaborator
Copy Link

Correcting a claim I made on this issue. It was wrong, and it was the
premise for treating the timeout as urgent.

I wrote:

compilation of a large -race build on a cold cache is charged against
that same 30-second budget

That is false. -timeout is passed to the compiled test binary as
-test.timeout, and that clock starts inside testing.M.Run — after
compilation and linking have completed. Build time is not charged against
it.

Measured rather than argued, in PR #98. A containerised run with an empty
GOCACHE spent 46.3 seconds compiling before the first result
appeared, then reported per-package durations essentially identical to a
warm host run:

package warm host cold container (46s compile first)
internal/database 6.368s 7.081s
internal/vaultik 6.641s 7.064s
internal/snapshot 2.101s 2.293s

If compilation were charged, the cold column would be tens of seconds
larger. It is within ~11%.

Where my error came from: I generalised from a sibling repo's failure
that was reported as timeout 30 go test ./... — a shell timeout
wrapping the entire invocation, which does include compilation. That is
a different mechanism from the -timeout flag, and the analogy does not
carry. I should have checked which of the two vaultik actually used
before propagating the concern, especially since I quoted vaultik's
script/test line in the same comment.

The conclusion survives, for a different reason. The real exposure was
margin, not compilation. The slowest package measured anywhere — warm or
cold, host or container — is internal/database at 8.113s on a cold
contended containerised run. Against that, 30s left only 3.7x headroom,
which is thin for a throttled CI runner, and script/test doubles the
work on failure because it reruns verbosely. Raised to 120s, about
15x, which still bounds a hung package plus its rerun to a few minutes.

The framing in the PR is the right one and worth keeping: a -timeout is
a hang backstop, not a performance budget. Its job is to turn a
deadlocked test into a stack dump rather than a wedged CI job, so it
should sit far above the slowest legitimate runtime rather than snugly
above it. Nothing is gained by failing at 30s that is not gained by
failing at 120s.

Recording this prominently because "cold cache eats your timeout" is a
plausible-sounding claim that would have propagated to other repos
unchallenged, and because an implementer disproving their instructions
with measurement is exactly the behaviour I want — not an argument to be
smoothed over.

Correcting a claim I made on this issue. It was wrong, and it was the premise for treating the timeout as urgent. I wrote: > compilation of a large `-race` build on a cold cache is charged against > that same 30-second budget **That is false.** `-timeout` is passed to the compiled test binary as `-test.timeout`, and that clock starts inside `testing.M.Run` — after compilation and linking have completed. Build time is not charged against it. Measured rather than argued, in PR #98. A containerised run with an empty `GOCACHE` spent **46.3 seconds compiling** before the first result appeared, then reported per-package durations essentially identical to a warm host run: | package | warm host | cold container (46s compile first) | | --- | --- | --- | | `internal/database` | 6.368s | 7.081s | | `internal/vaultik` | 6.641s | 7.064s | | `internal/snapshot` | 2.101s | 2.293s | If compilation were charged, the cold column would be tens of seconds larger. It is within ~11%. **Where my error came from:** I generalised from a sibling repo's failure that was reported as `timeout 30 go test ./...` — a shell `timeout` wrapping the entire invocation, which *does* include compilation. That is a different mechanism from the `-timeout` flag, and the analogy does not carry. I should have checked which of the two vaultik actually used before propagating the concern, especially since I quoted vaultik's `script/test` line in the same comment. **The conclusion survives, for a different reason.** The real exposure was margin, not compilation. The slowest package measured anywhere — warm or cold, host or container — is `internal/database` at **8.113s** on a cold contended containerised run. Against that, 30s left only 3.7x headroom, which is thin for a throttled CI runner, and `script/test` doubles the work on failure because it reruns verbosely. Raised to **120s**, about 15x, which still bounds a hung package plus its rerun to a few minutes. The framing in the PR is the right one and worth keeping: a `-timeout` is a **hang backstop, not a performance budget**. Its job is to turn a deadlocked test into a stack dump rather than a wedged CI job, so it should sit far above the slowest legitimate runtime rather than snugly above it. Nothing is gained by failing at 30s that is not gained by failing at 120s. Recording this prominently because "cold cache eats your timeout" is a plausible-sounding claim that would have propagated to other repos unchallenged, and because an implementer disproving their instructions with measurement is exactly the behaviour I want — not an argument to be smoothed over.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
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/vaultik#69
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?