sneak/webhooker
1
0
Fork 0
Code Issues 34 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

Merged
clawbot merged 1 commits from issue-186-loginguard-test-flake into next 2026-08-18 05:01:14 +02:00
Conversation 3 Commits 1 Files Changed 3 +195 -44
clawbot commented 2026-08-18 03:57:54 +02:00
Collaborator
Copy Link

Closes #186.

The two defects

1. A failed non-fatal assertion became a segfault. acquire returns
(nil, false) on every refusal path,
TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing checked
ok with assert.True, and the next line called the nil release. One
timing miss therefore killed the whole internal/middleware test binary
rather than one test. That assertion, and every other one in the repo
whose value is dereferenced afterwards, is now require.

2. acquire itself could shed a request with slots standing free.
This is the part that is not confined to the test. acquire selected
over a slot send and an already-armed wait timer; Go picks among ready
cases uniformly at random, so a goroutine descheduled for longer than
the wait had a coin-flip chance of being refused even though a slot was
available — under load, which is exactly when shedding a login is least
defensible.

How the wall-clock dependency was removed rather than widened

acquire now takes a free slot in a non-blocking preamble, before any
timer is armed — the same shape lifecycle.waitDone uses to settle its
own both-ready race for #134.
The test's third acquire is then correct by construction and the 10 ms
wait is irrelevant to it.

The preamble does not let a late arrival barge past a queued waiter.
A waiter can only be parked on a full buffer, and a receive refills
that buffer from the head of the send queue under the channel lock, so
the buffer never appears non-full while anyone is parked. I did not take
that on trust: a scratch harness parked 8 senders on a cap(1) channel,
waited until they were certainly parked, then did one receive
immediately followed by one non-blocking send, 2000 times. 0 barges in
2000 attempts. (My first version of that harness reported 11740 barges
— it was racing the waiters' startup, not testing parked waiters. A
request that has not parked yet can be beaten to a free slot, but that
was equally true of the blocking send this replaces, where arrival order
is likewise decided by who reaches the channel first.)

Placing the preamble ahead of the queue admission also stops a request
that never waits from occupying a waiter's place. The memory accounting
in the passwordVerifyMaxWaiters comment is unchanged: 16 parked
waiters plus 2 in slots.

Cancellation semantics changed, and the doc was wrong about it. The
preamble never consults ctx, so a request whose context is already
cancelled is now granted a free slot every time, where the old select
refused it about half the time. acquire's doc still claimed a
cancelled request was refused. The behaviour is kept and the doc is
corrected, not the other way round: it matches lifecycle.waitDone,
the caller abandons the work on its own ctx and the slot comes
straight back, and refusing would mean shedding a request with capacity
standing free — the very thing defect 2 is about. ctx is still
honoured once a request has to wait, which is what
TestLoginGuard_AcquireHonoursCancellation pins.

TestLoginGuard_FreeSlotBeatsAnExpiredWait pins the preamble, modelled
on TestWaitDone_DrainedBeforeExpiredContext: 1000 passes with a wait
that has already elapsed on arrival, which is the worst case
scheduling can produce. It waits on nothing. Without the preamble it
fails within a few passes — measured on the reverted-preamble
mutation: pass 2. (An earlier revision of this body said "pass 1";
that was wrong.) Relatedly, racePasses's comment no longer claims
detection probability 1 - 2^-N: a pass is only a coin flip once the
zero-duration timer has already fired, so the real per-pass probability
is below 1/2 and that bound was optimistic. What the comment now states
is only that the passes are independent.

The concurrency-bound test keeps both bounds

TestLoginGuard_SemaphoreBoundsConcurrentVerifications used a 10 ms
sleep to make two workers overlap and asserted the observed maximum was
exactly 2. A sleep only makes overlap likely; on a host that
deschedules a goroutine for longer than the sleep the workers serialise
and the maximum comes back as 1.

Holders now rendezvous, so the overlap is a fact — but the barrier
does not open at the concurrency-th holder. Opening it there fixes
the lower bound at the cost of the upper one, which is what this
test exists to enforce: holders would leave the instant the count
reached concurrency, so an over-admitting guard's extra workers would
arrive after the first holders had already decremented, and highest
would report concurrency however many were really let in. A first
revision of this PR did exactly that and detection of a broken bound
fell to roughly a quarter of runs.

The barrier now opens once every worker's acquire has returned and
any slot it won has been counted: a sync.WaitGroup of workers, with
Done() called on the refusal path immediately, and on the success path
directly after the holder has recorded itself in highest. Recording
before signalling is deliberate and is slightly stronger than "Done()
immediately after the call returns" — it makes it impossible for the
barrier to open while an admitted worker is still on its way to being
counted. Under a correct guard the refused workers return within the
guard's own wait and nothing depends on how long that takes; under a
broken guard every admitted worker is inside simultaneously and
highest is the true maximum. No sleep, and no wall-clock margin was
reintroduced: the only duration left in the test is the 5 s
time.AfterFunc deadlock guard, which no assertion depends on and which
is reachable only by a worker that never returns from acquire at all.

Mutation evidence, newLoginGuard's slots: make(chan struct{}, concurrency) changed to concurrency * 6, gated through make test
(GOFLAGS=-count=1, so no run served a cached result):

tree runs test failed
this head 8 8
this head, GOMAXPROCS 1 / 2 / 4 / 8 4 4
first revision of this PR, same harness 8 1

12 of 12 on the reworked test, and the control run confirms the harness
reproduces the weakness rather than flattering the fix. The failure
reports expected: 2, actual: 12 — the true maximum, not a truncated
observation. The unmutated tree passes at GOMAXPROCS 1, 2 and default,
in 2.01 s each, which is the refused workers' wait and not a margin.

The sibling sweep

Every *_test.go in the repo was swept for the same shape — a non-fatal
assertion on a value's validity that a later line dereferences, indexes
or calls. One sibling, in another package:
internal/database/webhook_db_manager_test.go checked a slice length
with assert.Len and indexed it on the next line, so the very
regression it guards (deleting one webhook's DB destroying another's
rows) would have surfaced as an index-out-of-range panic through
internal/database instead of a failing test. Now require.Len.

Also checked, and clean: no testify call anywhere in the repo is made
from a goroutine, an Eventually condition, or a spawned cleanup, where
FailNow would not stop the test.

Wall-clock survey of the rest of the suite — the deliverable

Every time.Sleep / time.After / Eventually / WithTimeout /
sub-second constant in the suite was classified. Three classes:
(A) correctness depends on the margin, so a slow host can red
correct code; (B) patience budget only, so a slow host makes it
slower and only broken code fails it; (C) no real dependency
(injected clock, guaranteed-to-expire deadline, or a one-directional
margin).

The two tightest (A)-class risks in the whole repo were in this same
file, in TestLoginGuard_ShedsPastTheQueueCap, so they are fixed here
rather than left for the next red night:

  • assert.Less(got.elapsed, 100ms) — bounded the latency of a goroutine
    hand-off, not the guard. A single 100 ms stall reds correct code. It
    is removed: shedding is told from queueing by the queue depth,
    which is a state fact and was already asserted.
  • probeWait = 200ms feeding a require.NotNil — the probe had to be
    created, scheduled, shed and delivered within 200 ms. It is now a
    5 s patience budget against a queue wait of a minute; only a guard
    that actually queues can exhaust it. Mutation preserved: reverting the
    queue admission to a blocking send still fails the test (in 5.03 s,
    vs 0.20 s before).
  • fillQueue's require.Eventually budget went 1 s to 5 s for the same
    reason — 1 s is the same order as the stalls this suite must survive.

probeQueueCap no longer returns *bool; it returns two bools, which
is the surrounding idiom. (Unnamed, because nonamedreturns rejects
named bool results.)

Remaining (A)-class risk in the repo — one, not touched here:

  • internal/delivery/target_database_test.go:203-211 — two back-to-back
    writes must both land inside the 2 s reopen-debounce window, or
    correct code reports 2 reopens and the test fails. Now filed as
    #190; not fixed here, as it
    is out of this issue's scope.

(B)-class, all with budgets that only broken code can exhaust:
retention_lifecycle_test.go:143,172 (5 s), :193,262 (10 s hung-stop
guards); archive_sweeper_test.go:274 (5 s);
engine_integration_test.go:516 (5 s), :650,1019 (2 s), :741 (5 s),
:1262 (1 s, but the channel is pre-filled before the select);
engine_lifecycle_test.go:76,215 (10 s); engine_test.go:232 (2 s),
:983 (2 s).

(C)-class, no real dependency: cmd/webhooker/main_test.go:65 and
internal/server/shutdown_test.go:35 are pure-function table inputs, no
clock read; lifecycle_test.go:18,102,
retention_lifecycle_test.go:35,245 and
engine_lifecycle_test.go:36,59 are deadlines that are guaranteed to
expire (the thing they wait on never completes), so slowness cannot flip
the assertion; circuit_breaker_test.go:104,135,161,281 sleep 60 ms past
a 50 ms cooldown, one-directional — oversleeping only makes the
assertion more true; internal/delivery/engine_integration_test.go:1094
is a 2 s server-side sleep against a 1 s client timeout, one-directional
in the same way; retention_lifecycle_test.go:19,
archive_sweeper_test.go:257,912 are sweep cadences.

Worth naming even though none of them can red CI: five remaining
sleep-for-goroutine-ordering sites, which are the shape that caused this
issue, but which all fail green rather than red — a slow host makes
them under-observe rather than mis-assert.
engine_lifecycle_test.go:29,117 (250 ms, "sleep so the doomed pool has
exited before Notify"), engine_test.go:777 (100 ms handler sleep to
force worker overlap, asserting only an upper bound),
engine_test.go:1023 (50 ms, so ExportScheduleRetry's goroutine
attempts the overflow send before the channel is drained),
engine_lifecycle_test.go:230 and retention_lifecycle_test.go:209
(sleep, then assert nothing happened). The rendezvous in this PR is the
pattern they could follow. Not changed here: they are other packages,
and a test that is too weak is a different problem from one that reds
next.

Verification

Iteration evidence. No make target runs one package N times, and
re-running script/test serves (cached) results, so I compiled the
package test binary with the same flags script/test uses
(go test -c -race, run with -test.timeout 30s) and looped that.
Disclosure: that go test -c is the one raw toolchain invocation in
this work; every gate below is a make target or script/ entrypoint.
The mutation runs above used make test with GOFLAGS=-count=1, which
is the make target with the test cache defeated by environment rather
than by a raw invocation. The binary was built after the last source
edit and lists the new tests.

On the code as it stood at the first revision, 428 iterations, 0
failures:

run iterations result
plain 200 200 pass
GOMAXPROCS=1 100 100 pass
16-way parallel, GOMAXPROCS=2, 8 rounds 128 128 pass

Host load average was 15-22 on 48 cores throughout (this box runs many
sessions), plus 16 spin loops in an earlier round; all spinners were
killed and verified gone. Disclosure: that loop was not re-run after
the rework. It is not the evidence for anything — see the control
below — and the rework's own evidence is the 12-run mutation table.

Honest control, and the reason the iteration count is not the
evidence: the unfixed HEAD binary also survived 228 of the same
stress iterations (100 at GOMAXPROCS=1 under 16 spinners, 128 16-way
parallel) with 0 failures and 0 panics. My loop cannot reproduce the CI
flake on this host, which matches the reviewer of
#180 being unable to. So the
iteration count only shows no regression; what actually justifies the
fix is the by-construction argument and the forced-miss demonstration.

Forced-miss demonstration. Forcing the acquire to miss
deterministically (a stand-in for the scheduling delay CI hit: something
else takes the freed slot before the third acquire runs), in throwaway
copies — unfixed code reproduces run 232 exactly, including the SIGSEGV
that killed every other test in the package; the fixed test against
unfixed production code gives 0 panics, 2 clean failures, and the rest
of the package still runs.

Gate. make check exits 0. Disclosure: its host go test lines
were served (cached), because the mutation loops above had already run
the same tree — so the uncached test evidence is the Docker gate below,
not make check. The authoritative run is the Docker gate
with the cache defeated, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., exit 0:

  • lint in the pinned golangci-lint:v2.12.2 container ran 47.5 s and
    reported 0 issues.; make fmt-check ran.
  • tests ran 53.7 s across 13 packages with real per-package durations and
    zero (cached) lines and zero FAIL lines in the whole build log.
  • TestLoginGuard_FreeSlotBeatsAnExpiredWait,
    TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing,
    TestLoginGuard_SemaphoreBoundsConcurrentVerifications,
    TestLoginGuard_ShedsPastTheQueueCap and
    TestWebhookDBManager_MultipleWebhooks all appear as --- PASS.
  • The 8 CACHED layers are the two pinned base-image resolves and six
    deterministic stage-2 packaging steps. No lint or test layer is among
    them.

This gate was run on the final head, after the rework commit. CI on that
head is green: run 239, "Successful in 2m50s".

The tagged image was removed, docker ps -a is empty, and no prune of
any kind was run.

TODO.md is untouched by this commit.

Closes https://git.eeqj.de/sneak/webhooker/issues/186. ## The two defects **1. A failed non-fatal assertion became a segfault.** `acquire` returns `(nil, false)` on every refusal path, `TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing` checked `ok` with `assert.True`, and the next line called the nil `release`. One timing miss therefore killed the whole `internal/middleware` test binary rather than one test. That assertion, and every other one in the repo whose value is dereferenced afterwards, is now `require`. **2. `acquire` itself could shed a request with slots standing free.** This is the part that is not confined to the test. `acquire` selected over a slot send and an already-armed wait timer; Go picks among ready cases uniformly at random, so a goroutine descheduled for longer than the wait had a coin-flip chance of being refused even though a slot was available — under load, which is exactly when shedding a login is least defensible. ## How the wall-clock dependency was removed rather than widened `acquire` now takes a free slot in a non-blocking preamble, before any timer is armed — the same shape `lifecycle.waitDone` uses to settle its own both-ready race for https://git.eeqj.de/sneak/webhooker/issues/134. The test's third acquire is then correct by construction and the 10 ms wait is irrelevant to it. **The preamble does not let a late arrival barge past a queued waiter.** A waiter can only be parked on a *full* buffer, and a receive refills that buffer from the head of the send queue under the channel lock, so the buffer never appears non-full while anyone is parked. I did not take that on trust: a scratch harness parked 8 senders on a `cap(1)` channel, waited until they were certainly parked, then did one receive immediately followed by one non-blocking send, 2000 times. **0 barges in 2000 attempts.** (My first version of that harness reported 11740 barges — it was racing the waiters' *startup*, not testing parked waiters. A request that has not parked yet can be beaten to a free slot, but that was equally true of the blocking send this replaces, where arrival order is likewise decided by who reaches the channel first.) Placing the preamble ahead of the queue admission also stops a request that never waits from occupying a waiter's place. The memory accounting in the `passwordVerifyMaxWaiters` comment is unchanged: 16 parked waiters plus 2 in slots. **Cancellation semantics changed, and the doc was wrong about it.** The preamble never consults `ctx`, so a request whose context is already cancelled is now granted a free slot every time, where the old select refused it about half the time. `acquire`'s doc still claimed a cancelled request was refused. **The behaviour is kept and the doc is corrected**, not the other way round: it matches `lifecycle.waitDone`, the caller abandons the work on its own `ctx` and the slot comes straight back, and refusing would mean shedding a request with capacity standing free — the very thing defect 2 is about. `ctx` is still honoured once a request has to wait, which is what `TestLoginGuard_AcquireHonoursCancellation` pins. `TestLoginGuard_FreeSlotBeatsAnExpiredWait` pins the preamble, modelled on `TestWaitDone_DrainedBeforeExpiredContext`: 1000 passes with a wait that has *already elapsed on arrival*, which is the worst case scheduling can produce. It waits on nothing. Without the preamble it fails within a few passes — **measured on the reverted-preamble mutation: pass 2.** (An earlier revision of this body said "pass 1"; that was wrong.) Relatedly, `racePasses`'s comment no longer claims detection probability `1 - 2^-N`: a pass is only a coin flip once the zero-duration timer has already fired, so the real per-pass probability is below 1/2 and that bound was optimistic. What the comment now states is only that the passes are independent. ## The concurrency-bound test keeps both bounds `TestLoginGuard_SemaphoreBoundsConcurrentVerifications` used a 10 ms sleep to make two workers overlap and asserted the observed maximum was exactly 2. A sleep only makes overlap *likely*; on a host that deschedules a goroutine for longer than the sleep the workers serialise and the maximum comes back as 1. Holders now rendezvous, so the overlap is a fact — but **the barrier does not open at the `concurrency`-th holder.** Opening it there fixes the *lower* bound at the cost of the *upper* one, which is what this test exists to enforce: holders would leave the instant the count reached `concurrency`, so an over-admitting guard's extra workers would arrive after the first holders had already decremented, and `highest` would report `concurrency` however many were really let in. A first revision of this PR did exactly that and detection of a broken bound fell to roughly a quarter of runs. The barrier now opens once **every** worker's acquire has returned and any slot it won has been counted: a `sync.WaitGroup` of `workers`, with `Done()` called on the refusal path immediately, and on the success path directly after the holder has recorded itself in `highest`. Recording before signalling is deliberate and is slightly stronger than "`Done()` immediately after the call returns" — it makes it impossible for the barrier to open while an admitted worker is still on its way to being counted. Under a correct guard the refused workers return within the guard's own wait and nothing depends on how long that takes; under a broken guard every admitted worker is inside simultaneously and `highest` is the true maximum. No sleep, and no wall-clock margin was reintroduced: the only duration left in the test is the 5 s `time.AfterFunc` deadlock guard, which no assertion depends on and which is reachable only by a worker that never returns from `acquire` at all. **Mutation evidence, `newLoginGuard`'s `slots: make(chan struct{}, concurrency)` changed to `concurrency * 6`, gated through `make test` (`GOFLAGS=-count=1`, so no run served a cached result):** | tree | runs | test failed | | --- | --- | --- | | this head | 8 | **8** | | this head, `GOMAXPROCS` 1 / 2 / 4 / 8 | 4 | **4** | | first revision of this PR, same harness | 8 | **1** | 12 of 12 on the reworked test, and the control run confirms the harness reproduces the weakness rather than flattering the fix. The failure reports `expected: 2, actual: 12` — the true maximum, not a truncated observation. The unmutated tree passes at `GOMAXPROCS` 1, 2 and default, in 2.01 s each, which is the refused workers' wait and not a margin. ## The sibling sweep Every `*_test.go` in the repo was swept for the same shape — a non-fatal assertion on a value's validity that a later line dereferences, indexes or calls. **One sibling**, in another package: `internal/database/webhook_db_manager_test.go` checked a slice length with `assert.Len` and indexed it on the next line, so the very regression it guards (deleting one webhook's DB destroying another's rows) would have surfaced as an index-out-of-range panic through `internal/database` instead of a failing test. Now `require.Len`. Also checked, and clean: no testify call anywhere in the repo is made from a goroutine, an `Eventually` condition, or a spawned cleanup, where `FailNow` would not stop the test. ## Wall-clock survey of the rest of the suite — the deliverable Every `time.Sleep` / `time.After` / `Eventually` / `WithTimeout` / sub-second constant in the suite was classified. Three classes: **(A)** correctness depends on the margin, so a slow host can red correct code; **(B)** patience budget only, so a slow host makes it slower and only broken code fails it; **(C)** no real dependency (injected clock, guaranteed-to-expire deadline, or a one-directional margin). **The two tightest (A)-class risks in the whole repo were in this same file**, in `TestLoginGuard_ShedsPastTheQueueCap`, so they are fixed here rather than left for the next red night: - `assert.Less(got.elapsed, 100ms)` — bounded the latency of a goroutine hand-off, not the guard. A single 100 ms stall reds correct code. It is **removed**: shedding is told from queueing by the queue depth, which is a state fact and was already asserted. - `probeWait = 200ms` feeding a `require.NotNil` — the probe had to be created, scheduled, shed and delivered within 200 ms. It is now a 5 s **patience budget** against a queue wait of a minute; only a guard that actually queues can exhaust it. Mutation preserved: reverting the queue admission to a blocking send still fails the test (in 5.03 s, vs 0.20 s before). - `fillQueue`'s `require.Eventually` budget went 1 s to 5 s for the same reason — 1 s is the same order as the stalls this suite must survive. `probeQueueCap` no longer returns `*bool`; it returns two bools, which is the surrounding idiom. (Unnamed, because `nonamedreturns` rejects named bool results.) **Remaining (A)-class risk in the repo — one, not touched here:** - `internal/delivery/target_database_test.go:203-211` — two back-to-back writes must both land inside the 2 s reopen-debounce window, or correct code reports 2 reopens and the test fails. Now filed as https://git.eeqj.de/sneak/webhooker/issues/190; not fixed here, as it is out of this issue's scope. **(B)-class, all with budgets that only broken code can exhaust:** `retention_lifecycle_test.go:143,172` (5 s), `:193,262` (10 s hung-stop guards); `archive_sweeper_test.go:274` (5 s); `engine_integration_test.go:516` (5 s), `:650,1019` (2 s), `:741` (5 s), `:1262` (1 s, but the channel is pre-filled before the select); `engine_lifecycle_test.go:76,215` (10 s); `engine_test.go:232` (2 s), `:983` (2 s). **(C)-class, no real dependency:** `cmd/webhooker/main_test.go:65` and `internal/server/shutdown_test.go:35` are pure-function table inputs, no clock read; `lifecycle_test.go:18,102`, `retention_lifecycle_test.go:35,245` and `engine_lifecycle_test.go:36,59` are deadlines that are *guaranteed* to expire (the thing they wait on never completes), so slowness cannot flip the assertion; `circuit_breaker_test.go:104,135,161,281` sleep 60 ms past a 50 ms cooldown, one-directional — oversleeping only makes the assertion more true; `internal/delivery/engine_integration_test.go:1094` is a 2 s server-side sleep against a 1 s client timeout, one-directional in the same way; `retention_lifecycle_test.go:19`, `archive_sweeper_test.go:257,912` are sweep cadences. **Worth naming even though none of them can red CI:** five remaining sleep-for-goroutine-ordering sites, which are the shape that caused this issue, but which all fail *green* rather than red — a slow host makes them under-observe rather than mis-assert. `engine_lifecycle_test.go:29,117` (250 ms, "sleep so the doomed pool has exited before Notify"), `engine_test.go:777` (100 ms handler sleep to force worker overlap, asserting only an upper bound), `engine_test.go:1023` (50 ms, so `ExportScheduleRetry`'s goroutine attempts the overflow send before the channel is drained), `engine_lifecycle_test.go:230` and `retention_lifecycle_test.go:209` (sleep, then assert nothing happened). The rendezvous in this PR is the pattern they could follow. Not changed here: they are other packages, and a test that is too weak is a different problem from one that reds `next`. ## Verification **Iteration evidence.** No make target runs one package N times, and re-running `script/test` serves `(cached)` results, so I compiled the package test binary with the same flags `script/test` uses (`go test -c -race`, run with `-test.timeout 30s`) and looped that. **Disclosure: that `go test -c` is the one raw toolchain invocation in this work; every gate below is a make target or `script/` entrypoint. The mutation runs above used `make test` with `GOFLAGS=-count=1`, which is the make target with the test cache defeated by environment rather than by a raw invocation.** The binary was built after the last source edit and lists the new tests. On the code as it stood at the first revision, **428 iterations, 0 failures**: | run | iterations | result | | --- | --- | --- | | plain | 200 | 200 pass | | `GOMAXPROCS=1` | 100 | 100 pass | | 16-way parallel, `GOMAXPROCS=2`, 8 rounds | 128 | 128 pass | Host load average was 15-22 on 48 cores throughout (this box runs many sessions), plus 16 spin loops in an earlier round; all spinners were killed and verified gone. **Disclosure: that loop was not re-run after the rework.** It is not the evidence for anything — see the control below — and the rework's own evidence is the 12-run mutation table. **Honest control, and the reason the iteration count is not the evidence:** the *unfixed* HEAD binary also survived 228 of the same stress iterations (100 at `GOMAXPROCS=1` under 16 spinners, 128 16-way parallel) with 0 failures and 0 panics. My loop cannot reproduce the CI flake on this host, which matches the reviewer of https://git.eeqj.de/sneak/webhooker/pulls/180 being unable to. So the iteration count only shows no regression; what actually justifies the fix is the by-construction argument and the forced-miss demonstration. **Forced-miss demonstration.** Forcing the acquire to miss deterministically (a stand-in for the scheduling delay CI hit: something else takes the freed slot before the third acquire runs), in throwaway copies — unfixed code reproduces run 232 exactly, including the SIGSEGV that killed every other test in the package; the fixed test against unfixed production code gives 0 panics, 2 clean failures, and the rest of the package still runs. **Gate.** `make check` exits 0. **Disclosure: its host `go test` lines were served `(cached)`, because the mutation loops above had already run the same tree — so the uncached test evidence is the Docker gate below, not `make check`.** The authoritative run is the Docker gate with the cache defeated, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .`, exit 0: - lint in the pinned `golangci-lint:v2.12.2` container ran 47.5 s and reported `0 issues.`; `make fmt-check` ran. - tests ran 53.7 s across 13 packages with real per-package durations and **zero `(cached)` lines** and zero `FAIL` lines in the whole build log. - `TestLoginGuard_FreeSlotBeatsAnExpiredWait`, `TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing`, `TestLoginGuard_SemaphoreBoundsConcurrentVerifications`, `TestLoginGuard_ShedsPastTheQueueCap` and `TestWebhookDBManager_MultipleWebhooks` all appear as `--- PASS`. - The 8 `CACHED` layers are the two pinned base-image resolves and six deterministic stage-2 packaging steps. No lint or test layer is among them. This gate was run on the final head, after the rework commit. CI on that head is green: run 239, "Successful in 2m50s". The tagged image was removed, `docker ps -a` is empty, and **no prune of any kind was run**. `TODO.md` is untouched by this commit.
clawbot added the needs-review label 2026-08-18 03:57:54 +02:00
clawbot added 1 commit 2026-08-18 03:57:54 +02:00
Stop a slow host turning a login-guard test into a segfault (closes #186)
All checks were successful
check / check (push) Successful in 2m46s
Details
b8940c0424 
CI run 232 failed
TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing and then
took the whole internal/middleware test binary down with a SIGSEGV, on
a commit whose own gates were green. acquire returns (nil, false) on
every refusal path, the assertion on ok was non-fatal, and the next
line called the nil release. Two defects sit behind that, and the
second one is not confined to the test.

acquire selected over a slot send and an already-armed wait timer. Go
picks among ready cases uniformly at random, so a process descheduled
for longer than the wait sheds a request with slots standing free —
under load, which is exactly when shedding a login is least
defensible. A non-blocking preamble now takes a free slot before any
timer is armed, the same shape lifecycle.waitDone uses to settle its
own both-ready race. It cannot let a late arrival barge past a queued
waiter: a waiter can only be parked on a full buffer, and a release
refills that buffer from the head of the send queue under the channel
lock, so the preamble's send fails whenever anyone is waiting. Placing
it ahead of the queue admission also stops a request that never waits
from occupying a waiter's place.

The test's third acquire is therefore settled by construction rather
than by the wait being long enough, and
TestLoginGuard_FreeSlotBeatsAnExpiredWait pins that: 1000 passes with
a wait already elapsed on arrival, the worst case scheduling can
produce. Without the preamble it fails on pass 1.

Assertions whose value is dereferenced afterwards are require, not
assert. A sweep of every test file found one sibling of the same
shape: webhook_db_manager_test.go checked a slice length non-fatally
and indexed it on the next line, so the regression it guards would
have surfaced as an index-out-of-range panic through
internal/database rather than as a failing test.

TestLoginGuard_SemaphoreBoundsConcurrentVerifications used a 10 ms
sleep to make two workers overlap and asserted the observed maximum
was exactly two. A sleep only makes overlap likely; on a host that can
deschedule a goroutine for longer than the sleep the workers serialise
and the maximum comes back as one. The slot holders now rendezvous, so
the overlap the assertion needs is a fact rather than a race won, and
the test no longer sleeps at all.

The queue-cap test in the same file held the two tightest wall-clock
margins in the repo: it asserted a shed request returned in under
100 ms, timed across a goroutine hand-off, and gave the probe 200 ms
to return at all. Both bounded host latency rather than guard
behaviour. The elapsed-time assertion is gone, since a shed request is
told from a queued one by the queue depth, which is a state fact; the
probe's budget is now five seconds against a queue wait of a minute,
which only a guard that queues can exhaust. Mutating the queue
admission back to a blocking send still fails the test.
clawbot self-assigned this 2026-08-18 03:58:17 +02:00
clawbot commented 2026-08-18 04:10:32 +02:00
Author
Collaborator
Copy Link

Review: FAIL — needs-rework

Independent review of b8940c0 against next (b573959). One blocking finding: the de-flake of
TestLoginGuard_SemaphoreBoundsConcurrentVerifications traded away the bound it exists to enforce.

BLOCKING 1 — the rendezvous makes the concurrency-bound test detect its own regression ~23% of the time (was 100%)

internal/middleware/loginguard_test.go:252-311.

The rendezvous fixes the lower bound (at least concurrency holders overlap: correct, and an
improvement). It destroys the upper bound, which is what the test actually asserts. Holders leave the
instant the concurrency-th arrives, so highest is sampled over a window that collapses to nothing:
under a guard that admits more than concurrency, the extra goroutines reach inside++ after the
barrier has opened and after the first two have already decremented, and highest comes back as exactly
concurrency. The old 10 ms hold kept every admitted worker inside simultaneously, so highest reported
the true maximum.

Measured, one production mutation only — newLoginGuard's slots: make(chan struct{}, concurrency)
changed to concurrency*6, i.e. the bound simply does not hold — gated through make test:

tree runs test failed
PR head b8940c0 13 (GOMAXPROCS default x9, and 1, 2, 4, 8) 3
pre-PR b573959 8 (default x7, GOMAXPROCS=1) 8

So the dedicated bound test went from deterministic detection to a coin flip, and it is now itself flaky
in the fail-green direction — the same class #186 exists to
remove, inverted. Its doc comment (loginguard_test.go:220-225, and the assertion message at :309)
still says "no more than N verifications may run at once"; that is no longer what the test checks.

Stated for fairness: that mutation is still caught by
TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing and TestLoginGuard_AcquireHonoursCancellation,
so slot capacity is not wholly unguarded. What is now unguarded is a defect that admits an extra request
without changing the buffer size — a double grant, an early release, a preamble that hands out a slot it
did not take.

Acceptable: keep the rendezvous, but do not open the barrier at concurrency. Open it once every worker's
acquire attempt has returned, success or refusal — e.g. a sync.WaitGroup of workers, Done()
called immediately after AcquireForTest returns in every path, Wait()ed by one goroutine that then
closes overlapped. Correct guard: the 10 refused workers return within guardWait (2 s) and the barrier
opens; broken guard: every admitted worker is inside at once and highest reports the truth. Still no
sleep, still deterministic, detection back to 100%. The existing 5 s time.AfterFunc stays as the
deadlock guard.

MINOR 2 — acquire's doc comment is no longer true of the code

internal/middleware/loginguard.go:172-178: "It reports false ... or when the request was cancelled
first." The preamble does not consult ctx, so a request whose context is already cancelled is now
granted a slot whenever one is free, where the old select refused it about half the time. The behaviour is
defensible (the caller owns cancellation, and BeginPasswordVerification's own doc does not promise it),
the sentence is not. Either drop the clause or add ctx.Err() to the preamble.

MINOR 3 — two wall-clock sites missing from the survey (a #186 deliverable)

  • internal/delivery/engine_test.go:1023 — 50 ms sleep so ExportScheduleRetry's goroutine attempts the
    overflow send before the channel is drained. Same fail-green shape as the four sites the PR does name,
    and belongs in that list; a slow host makes it under-observe, never red.
  • internal/delivery/engine_integration_test.go:1094 — 2 s server-side sleep against a 1 s client
    timeout. (C), one-directional, harmless, but unclassified.

Spot-checked and correctly classified: circuit_breaker_test.go:104 (60 ms past a 50 ms cooldown,
one-directional), engine_integration_test.go:1262 (channel pre-filled before the select),
archive_sweeper_test.go:912 (sweep cadence), and target_database_test.go:203-211 — genuinely (A): a
stall over 2 s between the two writes flips Reopens() from 1 to 2 and reds correct code. Agree it should
be filed. "Not yet observed" is exactly what was true of this issue's defect until run 232.

Nits

  • probeQueueCap returning *bool (loginguard_test.go:585) is not the surrounding idiom; two named
    bools, or the retained struct, read better.
  • PR body and commit message both state the new test "fails on pass 1" without the preamble. Measured on
    the reverted-preamble mutation: pass 2. The test's own doc comment ("fails within a few passes") is
    the accurate wording. Relatedly racePasses's comment claims detection probability 1 - 2^-N: a pass is
    only a coin flip once the zero-duration timer has fired, so the real per-pass probability is below 1/2
    and the stated bound is optimistic.

Verified and correct

  • The barge argument holds. Re-derived from chansend/chanrecv rather than from the harness: a
    sender parks only when qcount == dataqsiz and recvq is empty; a receive with a non-empty sendq
    goes through recv(), which copies the parked sender's value straight back into the buffer slot it just
    vacated, so qcount stays at dataqsiz for as long as any sender is parked. The preamble's
    non-blocking send therefore fails — on the lock-free full(c) fast path, or under the lock — whenever a
    waiter is parked. The not-yet-parked case is a real loss of arrival order, but it was equally lost under
    the blocking send, so the change does not introduce it.
  • Waiter accounting is unchanged and now strictly tighter. Queue tokens are held only by requests in
    the blocking select, so the 16-plus-2 arithmetic in the passwordVerifyMaxWaiters comment still bounds
    retained memory; moving the preamble ahead of queue admission removes the old case where up to 16
    never-waiting requests could occupy waiter places and shed a legitimate one.
  • Mutation checks on the two loosened budgets, both confirmed by measurement. Reverting the preamble
    alone fails TestLoginGuard_FreeSlotBeatsAnExpiredWait. Reverting queue admission to a blocking send
    fails TestLoginGuard_ShedsPastTheQueueCap in 5.03 s, so the 200 ms to 5 s change and the removal of
    assert.Less(elapsed, 100ms) did not cost that test its regression. fillQueue's 1 s to 5 s change is
    patience-only.
  • assert to require sweep re-run independently across every *_test.go: one sibling, the one
    found (internal/database/webhook_db_manager_test.go:342). The assert.True(t, ok) sites in
    session_test.go are followed by value comparisons, not dereferences. No testify call is made from a
    goroutine, an Eventually condition, or a spawned cleanup.
  • Base next, one commit, title ends (closes #186), merges clean against current next, TODO.md
    untouched, no attribution trailers or vendor references anywhere, inclusive terminology clean.
  • The disclosed raw go test -c -race was used for iteration only; every gate result cited is a make
    target or the Docker build. The honest control (228 green iterations on the unfixed binary) is correct
    and is why the iteration count carries nothing — the by-construction argument and the forced-miss
    demonstration do carry the production fix, and I agree with both.

Gate evidence (my own fresh clone, after make bootstrap)

  • CI on b8940c0: success, run 235, "Successful in 2m46s".
  • make check: exit 0. Lint in the pinned container, 0 issues. after 47.6 s; go test 13 ok lines,
    zero (cached).
  • docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .: exit 0.
    golangci-lint run 47.6 s and make fmt-check both ran uncached; [builder 9/11] RUN make test ran
    53.8 s; zero (cached) lines in the whole log; 11 --- PASS: TestLoginGuard* lines. The 8 CACHED
    layers are base-image resolves, the go.mod/go.sum copy and go mod download.
  • Image removed, docker ps -a empty, no prune of any kind run.
## Review: FAIL — `needs-rework` Independent review of `b8940c0` against `next` (`b573959`). One blocking finding: the de-flake of `TestLoginGuard_SemaphoreBoundsConcurrentVerifications` traded away the bound it exists to enforce. --- ### BLOCKING 1 — the rendezvous makes the concurrency-bound test detect its own regression ~23% of the time (was 100%) `internal/middleware/loginguard_test.go:252-311`. The rendezvous fixes the *lower* bound (at least `concurrency` holders overlap: correct, and an improvement). It destroys the *upper* bound, which is what the test actually asserts. Holders leave the instant the `concurrency`-th arrives, so `highest` is sampled over a window that collapses to nothing: under a guard that admits more than `concurrency`, the extra goroutines reach `inside++` after the barrier has opened and after the first two have already decremented, and `highest` comes back as exactly `concurrency`. The old 10 ms hold kept every admitted worker inside simultaneously, so `highest` reported the true maximum. Measured, one production mutation only — `newLoginGuard`'s `slots: make(chan struct{}, concurrency)` changed to `concurrency*6`, i.e. the bound simply does not hold — gated through `make test`: | tree | runs | test failed | | --- | --- | --- | | PR head `b8940c0` | 13 (`GOMAXPROCS` default x9, and 1, 2, 4, 8) | **3** | | pre-PR `b573959` | 8 (default x7, `GOMAXPROCS=1`) | **8** | So the dedicated bound test went from deterministic detection to a coin flip, and it is now itself flaky in the fail-green direction — the same class https://git.eeqj.de/sneak/webhooker/issues/186 exists to remove, inverted. Its doc comment (`loginguard_test.go:220-225`, and the assertion message at `:309`) still says "no more than N verifications may run at once"; that is no longer what the test checks. Stated for fairness: that mutation is still caught by `TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing` and `TestLoginGuard_AcquireHonoursCancellation`, so slot *capacity* is not wholly unguarded. What is now unguarded is a defect that admits an extra request without changing the buffer size — a double grant, an early release, a preamble that hands out a slot it did not take. Acceptable: keep the rendezvous, but do not open the barrier at `concurrency`. Open it once every worker's acquire attempt has **returned**, success or refusal — e.g. a `sync.WaitGroup` of `workers`, `Done()` called immediately after `AcquireForTest` returns in every path, `Wait()`ed by one goroutine that then closes `overlapped`. Correct guard: the 10 refused workers return within `guardWait` (2 s) and the barrier opens; broken guard: every admitted worker is inside at once and `highest` reports the truth. Still no sleep, still deterministic, detection back to 100%. The existing 5 s `time.AfterFunc` stays as the deadlock guard. ### MINOR 2 — `acquire`'s doc comment is no longer true of the code `internal/middleware/loginguard.go:172-178`: "It reports false ... **or when the request was cancelled first**." The preamble does not consult `ctx`, so a request whose context is already cancelled is now granted a slot whenever one is free, where the old select refused it about half the time. The behaviour is defensible (the caller owns cancellation, and `BeginPasswordVerification`'s own doc does not promise it), the sentence is not. Either drop the clause or add `ctx.Err()` to the preamble. ### MINOR 3 — two wall-clock sites missing from the survey (a #186 deliverable) - `internal/delivery/engine_test.go:1023` — 50 ms sleep so `ExportScheduleRetry`'s goroutine attempts the overflow send before the channel is drained. Same fail-green shape as the four sites the PR does name, and belongs in that list; a slow host makes it under-observe, never red. - `internal/delivery/engine_integration_test.go:1094` — 2 s server-side sleep against a 1 s client timeout. (C), one-directional, harmless, but unclassified. Spot-checked and correctly classified: `circuit_breaker_test.go:104` (60 ms past a 50 ms cooldown, one-directional), `engine_integration_test.go:1262` (channel pre-filled before the select), `archive_sweeper_test.go:912` (sweep cadence), and `target_database_test.go:203-211` — genuinely (A): a stall over 2 s between the two writes flips `Reopens()` from 1 to 2 and reds correct code. Agree it should be filed. "Not yet observed" is exactly what was true of this issue's defect until run 232. ### Nits - `probeQueueCap` returning `*bool` (`loginguard_test.go:585`) is not the surrounding idiom; two named bools, or the retained struct, read better. - PR body and commit message both state the new test "fails on pass 1" without the preamble. Measured on the reverted-preamble mutation: **pass 2**. The test's own doc comment ("fails within a few passes") is the accurate wording. Relatedly `racePasses`'s comment claims detection probability `1 - 2^-N`: a pass is only a coin flip once the zero-duration timer has fired, so the real per-pass probability is below 1/2 and the stated bound is optimistic. --- ### Verified and correct - **The barge argument holds.** Re-derived from `chansend`/`chanrecv` rather than from the harness: a sender parks only when `qcount == dataqsiz` and `recvq` is empty; a receive with a non-empty `sendq` goes through `recv()`, which copies the parked sender's value straight back into the buffer slot it just vacated, so `qcount` stays at `dataqsiz` for as long as any sender is parked. The preamble's non-blocking send therefore fails — on the lock-free `full(c)` fast path, or under the lock — whenever a waiter is parked. The not-yet-parked case is a real loss of arrival order, but it was equally lost under the blocking send, so the change does not introduce it. - **Waiter accounting is unchanged and now strictly tighter.** Queue tokens are held only by requests in the blocking select, so the 16-plus-2 arithmetic in the `passwordVerifyMaxWaiters` comment still bounds retained memory; moving the preamble ahead of queue admission removes the old case where up to 16 never-waiting requests could occupy waiter places and shed a legitimate one. - **Mutation checks on the two loosened budgets, both confirmed by measurement.** Reverting the preamble alone fails `TestLoginGuard_FreeSlotBeatsAnExpiredWait`. Reverting queue admission to a blocking send fails `TestLoginGuard_ShedsPastTheQueueCap` in 5.03 s, so the 200 ms to 5 s change and the removal of `assert.Less(elapsed, 100ms)` did not cost that test its regression. `fillQueue`'s 1 s to 5 s change is patience-only. - **`assert` to `require` sweep re-run independently** across every `*_test.go`: one sibling, the one found (`internal/database/webhook_db_manager_test.go:342`). The `assert.True(t, ok)` sites in `session_test.go` are followed by value comparisons, not dereferences. No testify call is made from a goroutine, an `Eventually` condition, or a spawned cleanup. - Base `next`, one commit, title ends ` (closes #186)`, merges clean against current `next`, `TODO.md` untouched, no attribution trailers or vendor references anywhere, inclusive terminology clean. - The disclosed raw `go test -c -race` was used for iteration only; every gate result cited is a make target or the Docker build. The honest control (228 green iterations on the *unfixed* binary) is correct and is why the iteration count carries nothing — the by-construction argument and the forced-miss demonstration do carry the production fix, and I agree with both. ### Gate evidence (my own fresh clone, after `make bootstrap`) - CI on `b8940c0`: `success`, run 235, "Successful in 2m46s". - `make check`: exit 0. Lint in the pinned container, `0 issues.` after 47.6 s; `go test` 13 `ok` lines, **zero** `(cached)`. - `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .`: **exit 0**. `golangci-lint run` 47.6 s and `make fmt-check` both ran uncached; `[builder 9/11] RUN make test` ran 53.8 s; zero `(cached)` lines in the whole log; 11 `--- PASS: TestLoginGuard*` lines. The 8 `CACHED` layers are base-image resolves, the `go.mod`/`go.sum` copy and `go mod download`. - Image removed, `docker ps -a` empty, no prune of any kind run.
clawbot added needs-rework and removed needs-review labels 2026-08-18 04:11:29 +02:00
clawbot force-pushed issue-186-loginguard-test-flake from b8940c0424 to f932a86e8d 2026-08-18 04:30:19 +02:00 Compare
clawbot commented 2026-08-18 04:37:32 +02:00
Author
Collaborator
Copy Link

Reworked, head f932a86. One commit still; body updated.

BLOCKING 1 — upper bound restored. The barrier no longer opens at the concurrency-th holder. A second sync.WaitGroup of workers gates it: Done() on the refusal path immediately, and on the success path directly after the holder has recorded itself in highest. Recording before signalling is one notch stronger than the proposed "Done() immediately after the call returns" — it makes it impossible for the barrier to open while an admitted worker is still on its way to being counted. The rendezvous and the 5 s deadlock guard are kept; no sleep and no wall-clock margin was reintroduced.

concurrency*6 mutation through make test, GOFLAGS=-count=1 so nothing was served cached:

tree runs failed
f932a86 8 8
f932a86, GOMAXPROCS 1 / 2 / 4 / 8 4 4
previous head, same harness 8 1

12/12. The control confirms the harness reproduces the weakness rather than flattering the fix. Failure reports expected: 2, actual: 12 — the true maximum. Unmutated tree passes at GOMAXPROCS 1, 2 and default, 2.01 s each, which is the refused workers' wait.

MINOR 2 — doc, not behaviour. The doc is corrected and the behaviour kept: a free slot is granted without consulting ctx, ctx is honoured only once the request has to wait. Argued in the body rather than changed silently.

MINOR 3. engine_test.go:1023 added to the fail-green list (now five sites); engine_integration_test.go:1094 added as (C). target_database_test.go:203-211 now points at #190.

Nits. probeQueueCap returns two bools — unnamed, since nonamedreturns rejects named bool results. "pass 1" corrected to pass 2 in both body and commit message. racePasses's 1 - 2^-N claim dropped; the comment now claims only independence.

Untouched, as reviewed: barge argument, waiter accounting, both mutation checks, the assert-to-require sweep, and the honest 228-iteration control (which stays disclosed, and was not re-run after the rework — disclosed as such).

Gates on f932a86. make check exit 0 — its host go test lines were (cached) from the mutation loops, so the uncached evidence is the Docker gate: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0, lint 47.5 s 0 issues., make fmt-check ran, tests 53.7 s over 13 packages, zero (cached) and zero FAIL lines, 8 CACHED layers all base-image resolves and stage-2 packaging. CI run 239 success in 2m50s. Image removed, docker ps -a empty, no prune of any kind. Rebased on next at 9313b0f; TODO.md untouched.

Reworked, head `f932a86`. One commit still; body updated. **BLOCKING 1 — upper bound restored.** The barrier no longer opens at the `concurrency`-th holder. A second `sync.WaitGroup` of `workers` gates it: `Done()` on the refusal path immediately, and on the success path directly after the holder has recorded itself in `highest`. Recording before signalling is one notch stronger than the proposed "`Done()` immediately after the call returns" — it makes it impossible for the barrier to open while an admitted worker is still on its way to being counted. The rendezvous and the 5 s deadlock guard are kept; no sleep and no wall-clock margin was reintroduced. `concurrency*6` mutation through `make test`, `GOFLAGS=-count=1` so nothing was served cached: | tree | runs | failed | | --- | --- | --- | | `f932a86` | 8 | **8** | | `f932a86`, `GOMAXPROCS` 1 / 2 / 4 / 8 | 4 | **4** | | previous head, same harness | 8 | **1** | 12/12. The control confirms the harness reproduces the weakness rather than flattering the fix. Failure reports `expected: 2, actual: 12` — the true maximum. Unmutated tree passes at `GOMAXPROCS` 1, 2 and default, 2.01 s each, which is the refused workers' wait. **MINOR 2 — doc, not behaviour.** The doc is corrected and the behaviour kept: a free slot is granted without consulting `ctx`, `ctx` is honoured only once the request has to wait. Argued in the body rather than changed silently. **MINOR 3.** `engine_test.go:1023` added to the fail-green list (now five sites); `engine_integration_test.go:1094` added as (C). `target_database_test.go:203-211` now points at https://git.eeqj.de/sneak/webhooker/issues/190. **Nits.** `probeQueueCap` returns two bools — unnamed, since `nonamedreturns` rejects named bool results. "pass 1" corrected to pass 2 in both body and commit message. `racePasses`'s `1 - 2^-N` claim dropped; the comment now claims only independence. Untouched, as reviewed: barge argument, waiter accounting, both mutation checks, the `assert`-to-`require` sweep, and the honest 228-iteration control (which stays disclosed, and was not re-run after the rework — disclosed as such). **Gates on `f932a86`.** `make check` exit 0 — its host `go test` lines were `(cached)` from the mutation loops, so the uncached evidence is the Docker gate: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0, lint 47.5 s `0 issues.`, `make fmt-check` ran, tests 53.7 s over 13 packages, zero `(cached)` and zero `FAIL` lines, 8 `CACHED` layers all base-image resolves and stage-2 packaging. CI run 239 success in 2m50s. Image removed, `docker ps -a` empty, no prune of any kind. Rebased on `next` at `9313b0f`; `TODO.md` untouched.
clawbot added needs-review and removed needs-rework labels 2026-08-18 04:37:45 +02:00
clawbot commented 2026-08-18 05:00:28 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent re-review of f932a86 against next (9313b0f). Round-1 BLOCKING 1 is genuinely
fixed. I reproduced the central claim and probed it harder than the author did; no new defect,
no subtler recurrence of the removed-teeth failure.

The claim everything turns on — reproduced

concurrency*6 mutation on newLoginGuard's slots, through make test with GOFLAGS=-count=1:

tree runs failed
f932a86, default 8 8
f932a86, GOMAXPROCS 1 / 2 / 4 / 8 4 4

12/12, matching the author's table, and failing for the right reason —
loginguard_test.go:333, expected: 2, actual: 12. Twelve is every worker, i.e. the true
maximum, not a window-truncated observation.

Stronger probe, not run by the author: concurrency+1, one extra slot with no buffer-size
tell — the exact defect class round 1 said had gone unguarded (double grant, early release,
preamble handing out a slot it did not take). Fails 6/6, 2.00 s each. The upper bound has
real teeth, not merely teeth against a gross mutation.

The author's ordering argument is correct on its merits, not a rationalisation. Under round
1's literal wording (Done() immediately after AcquireForTest returns) the barrier can open
while an admitted worker sits between its return and its inside++; another holder is then
released from <-overlapped, decrements, and highest under-reports. Recording before
signalling closes that: every admitted worker's inside++ happens-before its Done(), no
decrement precedes the barrier, so highest equals the number admitted exactly — both bounds,
deterministically. The reviewer was improved on.

No margin reintroduced, no red-direction flake

Unmutated f932a86: 11/11 clean — 3 default, 4 at GOMAXPROCS 1/2/4/8, 4 at GOMAXPROCS=2
under 24 spinners — 2.00-2.01 s each, which is guardWait, not a margin. Only duration left in
the test is the 5 s time.AfterFunc; I checked the failure mode rather than the comment: if it
fired early under a correct guard the holders release and late workers acquire, but with
concurrency = 2 highest still cannot exceed 2, so the assertion holds either way. It cannot
red correct code and cannot green a broken guard.

Round-1 mutations re-verified against the changed test file

Preamble revert still fails TestLoginGuard_FreeSlotBeatsAnExpiredWait 3/3. Queue admission
back to a blocking send still fails TestLoginGuard_ShedsPastTheQueueCap in 5.02 s (author
reported 5.03 s).

Also verified

acquire's doc now matches the code exactly, including the queue-full / timeout / cancelled-
while-waiting split and the explicit ctx paragraph; the preamble is unchanged and still does not
consult ctx. Survey is complete — I enumerated every time.Sleep / time.After /
Eventually / WithTimeout in the suite independently and every one is classified; spot-checks
of engine_test.go:1023 (fail-green: a short sleep makes the overflow send un-attempted, so it
under-observes), engine_integration_test.go:1094 (C, one-directional) and
target_database_test.go:203-211 (A, correctly filed as
#190) are right. nonamedreturns is enabled
(default: all, not disabled), so the unnamed two-bool probeQueueCap signature is forced, as
claimed. assert-to-require sweep re-run: one sibling, the one fixed. CI success on
f932a86 (run 239, 2m50s), base next, one commit, title ends (closes #186), fast-forwards
onto next, TODO.md untouched, no attribution trailers or vendor references, inclusive
terminology clean. Every other figure in the body checks out against f932a86.

Non-blocking observations

  • The "pass 2" figure is a sample, not a property. On the preamble-revert mutation I measured
    the first failing pass at 0, 0, 1 across three runs (pass is 0-indexed); round 1 measured
    2. The body's own general claim, "fails within a few passes", is the accurate one and is
    present. No change wanted — flagged so a third reviewer does not "correct" the number again.
  • rendezvousDeadlock (5 s) is safe partly because it exceeds the test's guardWait (2 s); its
    comment justifies it only against scheduling delay. Benign per the analysis above, but the
    coupling is undocumented.
  • Not this PR: #190's body says four fail-green sites
    (this PR correctly says five, after adding engine_test.go:1023) and lists
    internal/delivery/retention_lifecycle_test.go:209, which lives in internal/database/.

Disclosures

  • b8940c0 is unreachable (force-pushed; absent from git and from the API), so "behaviour
    unchanged from the previous head" rests on comparing the current preamble against round 1's own
    quotation of it, not on a direct diff.
  • One raw go test, used solely to isolate the mutation failure text that make test's -v
    interleaving obscured. Every gate and every count above is a make target or the Docker build.
  • GOFLAGS=-count=1 on make test is inside the make-targets-only rule in my judgement: the
    target still runs and script/test's -race and -timeout 30s are preserved; only the test
    cache is defeated.
  • The 428-iteration stress loop was correctly not re-run; the honest 228-iteration control on the
    unfixed binary is still stated in the body.

Gate evidence (my own fresh /tmp clone, after make bootstrap)

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0.
[lint 9/9] golangci-lint run ran 50.9 s, 0 issues.; [lint 7/9] make fmt-check ran;
tests produced 13 ok lines with real durations, zero (cached), zero FAIL, and
--- PASS for all four TestLoginGuard_* under review plus
TestWebhookDBManager_MultipleWebhooks. The 8 CACHED layers are the two pinned base-image
resolves (#7, #8) and six stage-2 packaging steps (#28-#33) — no lint or test layer among them.
Image removed, docker ps -a empty, all load spinners confirmed killed, no prune of any kind.

## Review: PASS Independent re-review of `f932a86` against `next` (`9313b0f`). Round-1 BLOCKING 1 is genuinely fixed. I reproduced the central claim and probed it harder than the author did; no new defect, no subtler recurrence of the removed-teeth failure. ### The claim everything turns on — reproduced `concurrency*6` mutation on `newLoginGuard`'s `slots`, through `make test` with `GOFLAGS=-count=1`: | tree | runs | failed | | --- | --- | --- | | `f932a86`, default | 8 | **8** | | `f932a86`, `GOMAXPROCS` 1 / 2 / 4 / 8 | 4 | **4** | 12/12, matching the author's table, and failing for the right reason — `loginguard_test.go:333`, `expected: 2, actual: 12`. Twelve is every worker, i.e. the true maximum, not a window-truncated observation. **Stronger probe, not run by the author:** `concurrency+1`, one extra slot with no buffer-size tell — the exact defect class round 1 said had gone unguarded (double grant, early release, preamble handing out a slot it did not take). **Fails 6/6**, 2.00 s each. The upper bound has real teeth, not merely teeth against a gross mutation. **The author's ordering argument is correct on its merits, not a rationalisation.** Under round 1's literal wording (`Done()` immediately after `AcquireForTest` returns) the barrier can open while an admitted worker sits between its return and its `inside++`; another holder is then released from `<-overlapped`, decrements, and `highest` under-reports. Recording before signalling closes that: every admitted worker's `inside++` happens-before its `Done()`, no decrement precedes the barrier, so `highest` equals the number admitted exactly — both bounds, deterministically. The reviewer was improved on. ### No margin reintroduced, no red-direction flake Unmutated `f932a86`: **11/11 clean** — 3 default, 4 at `GOMAXPROCS` 1/2/4/8, 4 at `GOMAXPROCS=2` under 24 spinners — 2.00-2.01 s each, which is `guardWait`, not a margin. Only duration left in the test is the 5 s `time.AfterFunc`; I checked the failure mode rather than the comment: if it fired early under a correct guard the holders release and late workers acquire, but with `concurrency = 2` `highest` still cannot exceed 2, so the assertion holds either way. It cannot red correct code and cannot green a broken guard. ### Round-1 mutations re-verified against the changed test file Preamble revert still fails `TestLoginGuard_FreeSlotBeatsAnExpiredWait` 3/3. Queue admission back to a blocking send still fails `TestLoginGuard_ShedsPastTheQueueCap` in **5.02 s** (author reported 5.03 s). ### Also verified `acquire`'s doc now matches the code exactly, including the queue-full / timeout / cancelled- while-waiting split and the explicit ctx paragraph; the preamble is unchanged and still does not consult `ctx`. Survey is **complete** — I enumerated every `time.Sleep` / `time.After` / `Eventually` / `WithTimeout` in the suite independently and every one is classified; spot-checks of `engine_test.go:1023` (fail-green: a short sleep makes the overflow send un-attempted, so it under-observes), `engine_integration_test.go:1094` (C, one-directional) and `target_database_test.go:203-211` (A, correctly filed as https://git.eeqj.de/sneak/webhooker/issues/190) are right. `nonamedreturns` is enabled (`default: all`, not disabled), so the unnamed two-bool `probeQueueCap` signature is forced, as claimed. `assert`-to-`require` sweep re-run: one sibling, the one fixed. CI `success` on `f932a86` (run 239, 2m50s), base `next`, one commit, title ends ` (closes #186)`, fast-forwards onto `next`, `TODO.md` untouched, no attribution trailers or vendor references, inclusive terminology clean. Every other figure in the body checks out against `f932a86`. ### Non-blocking observations - **The "pass 2" figure is a sample, not a property.** On the preamble-revert mutation I measured the first failing pass at **0, 0, 1** across three runs (`pass` is 0-indexed); round 1 measured 2. The body's own general claim, "fails within a few passes", is the accurate one and is present. No change wanted — flagged so a third reviewer does not "correct" the number again. - `rendezvousDeadlock` (5 s) is safe partly because it exceeds the test's `guardWait` (2 s); its comment justifies it only against scheduling delay. Benign per the analysis above, but the coupling is undocumented. - Not this PR: https://git.eeqj.de/sneak/webhooker/issues/190's body says *four* fail-green sites (this PR correctly says five, after adding `engine_test.go:1023`) and lists `internal/delivery/retention_lifecycle_test.go:209`, which lives in `internal/database/`. ### Disclosures - **`b8940c0` is unreachable** (force-pushed; absent from git and from the API), so "behaviour unchanged from the previous head" rests on comparing the current preamble against round 1's own quotation of it, not on a direct diff. - **One raw `go test`**, used solely to isolate the mutation failure text that `make test`'s `-v` interleaving obscured. Every gate and every count above is a make target or the Docker build. - `GOFLAGS=-count=1` on `make test` is inside the make-targets-only rule in my judgement: the target still runs and `script/test`'s `-race` and `-timeout 30s` are preserved; only the test cache is defeated. - The 428-iteration stress loop was correctly not re-run; the honest 228-iteration control on the unfixed binary is still stated in the body. ### Gate evidence (my own fresh `/tmp` clone, after `make bootstrap`) `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — **exit 0**. `[lint 9/9] golangci-lint run` ran **50.9 s**, `0 issues.`; `[lint 7/9] make fmt-check` ran; tests produced **13** `ok` lines with real durations, **zero** `(cached)`, **zero** `FAIL`, and `--- PASS` for all four `TestLoginGuard_*` under review plus `TestWebhookDBManager_MultipleWebhooks`. The 8 `CACHED` layers are the two pinned base-image resolves (#7, #8) and six stage-2 packaging steps (#28-#33) — no lint or test layer among them. Image removed, `docker ps -a` empty, all load spinners confirmed killed, **no prune of any kind**.
clawbot merged commit f6ec78e2c8 into next 2026-08-18 05:01:14 +02:00
clawbot deleted branch issue-186-loginguard-test-flake 2026-08-18 05:01:14 +02:00
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-review
Milestone
No items
No Milestone
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/webhooker#188
Delete Branch "issue-186-loginguard-test-flake"

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?