A timing-sensitive login-guard test panics on a nil release and reds the whole internal/middleware package #186

New Issue

clawbot · 2026-08-18T03:14:50+02:00

clawbot commented

2026-08-18 03:14:50 +02:00

Found by the independent review of #180, as red CI on a commit whose own gates were green. Introduced by #171 and not touched by that PR.

internal/middleware/loginguard_test.go, TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing:

--- FAIL: TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing (0.13s)
    loginguard_test.go:308
    Messages: the slot must be reusable once released
panic: runtime error: invalid memory address or nil pointer dereference
  ...loginguard_test.go:312
FAIL sneak.berlin/go/webhooker/internal/middleware 0.242s

Mechanism: loginGuard.acquire returns nil, false on timer expiry, and the test uses a 10 ms wait. A third acquire that misses its window fails the assert.True at line 308 — which is non-fatal — and then line 312 calls the nil release, segfaults, and takes the whole package test binary down with it.

Milestoned 1.0.0. Not for the login guard, which is fine, but because it makes the gate unreliable: a timing miss reds the entire internal/middleware package, on any PR that happens to be running alongside other work on this shared host. This repo has spent the whole cycle on making the gate mean what it says — #119, #152, #109 — and a test that can nondeterministically red a package undoes that from the other direction. It also means next itself can go red without anything on next being wrong, which is the invariant the branch exists to hold.

It is load-sensitive, not deterministic: the reviewer could not reproduce it locally; both their Docker gate and make check were green. CI run 232 caught it while other work was running on the same host. Treat "I could not reproduce it" as expected, not as evidence it is fixed.

Definition of done

The nil dereference is impossible: use require rather than assert at line 308, so a failed acquire stops that test instead of panicking the binary. Sweep the file — and ideally the package — for the same shape, an assert on a value whose nil-ness the next line dereferences. This one was found by a segfault in CI; there may be siblings.
The timing sensitivity is addressed rather than papered over. A 10 ms wait on a shared, loaded host is a coin flip. Either give it a margin that is defensible on a busy machine, or restructure so the test does not depend on wall-clock at all — the latter is better, and the waitDone test from #134 is the model: it made a scheduler-dependent property deterministic rather than widening a timeout.
Say in the PR whether any other test in the repo depends on a wall-clock margin, and how tight.

Implementation requirements

Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md (see #112).
Run make bootstrap in a fresh clone before gating — browser assets are fetched at build time, and make lint needs Docker.
Gate on make check plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.
A green local run is NOT sufficient evidence here, since the defect is load-sensitive. Run the affected package repeatedly under load and say how many iterations you got clean.

Found by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/180, as red CI on a commit whose own gates were green. Introduced by https://git.eeqj.de/sneak/webhooker/pulls/171 and not touched by that PR. `internal/middleware/loginguard_test.go`, `TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing`: ``` --- FAIL: TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing (0.13s) loginguard_test.go:308 Messages: the slot must be reusable once released panic: runtime error: invalid memory address or nil pointer dereference ...loginguard_test.go:312 FAIL sneak.berlin/go/webhooker/internal/middleware 0.242s ``` Mechanism: `loginGuard.acquire` returns `nil, false` on timer expiry, and the test uses a 10 ms wait. A third acquire that misses its window fails the `assert.True` at line 308 — which is **non-fatal** — and then line 312 calls the nil `release`, segfaults, and **takes the whole package test binary down with it**. Milestoned `1.0.0`. Not for the login guard, which is fine, but because it makes the gate unreliable: a timing miss reds the entire `internal/middleware` package, on any PR that happens to be running alongside other work on this shared host. This repo has spent the whole cycle on making the gate mean what it says — https://git.eeqj.de/sneak/webhooker/issues/119, https://git.eeqj.de/sneak/webhooker/issues/152, https://git.eeqj.de/sneak/webhooker/issues/109 — and a test that can nondeterministically red a package undoes that from the other direction. It also means `next` itself can go red without anything on `next` being wrong, which is the invariant the branch exists to hold. It is load-sensitive, not deterministic: the reviewer could not reproduce it locally; both their Docker gate and `make check` were green. CI run 232 caught it while other work was running on the same host. Treat "I could not reproduce it" as expected, not as evidence it is fixed. ## Definition of done - The nil dereference is impossible: use `require` rather than `assert` at line 308, so a failed acquire stops that test instead of panicking the binary. **Sweep the file — and ideally the package — for the same shape**, an `assert` on a value whose nil-ness the next line dereferences. This one was found by a segfault in CI; there may be siblings. - The timing sensitivity is addressed rather than papered over. A 10 ms wait on a shared, loaded host is a coin flip. Either give it a margin that is defensible on a busy machine, or restructure so the test does not depend on wall-clock at all — the latter is better, and the `waitDone` test from https://git.eeqj.de/sneak/webhooker/issues/134 is the model: it made a scheduler-dependent property deterministic rather than widening a timeout. - Say in the PR whether any other test in the repo depends on a wall-clock margin, and how tight. ## Implementation requirements - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112). - Run `make bootstrap` in a fresh clone before gating — browser assets are fetched at build time, and `make lint` needs Docker. - Gate on `make check` plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host. - A green local run is NOT sufficient evidence here, since the defect is load-sensitive. Run the affected package repeatedly under load and say how many iterations you got clean.

clawbot added this to the 1.0.0 milestone 2026-08-18 03:14:50 +02:00

clawbot self-assigned this 2026-08-18 03:14:50 +02:00

clawbot referenced this issue

2026-08-18 03:38:11 +02:00

Route GORM's logger through slog and bound it (closes #178) #182

clawbot referenced this issue from a commit

2026-08-18 03:41:47 +02:00

Stop a slow host turning a login-guard test into a segfault (closes #186)

clawbot referenced this issue from a commit

2026-08-18 03:55:54 +02:00

Stop a slow host turning a login-guard test into a segfault (closes #186)

clawbot referenced a pull request that will close this issue