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.
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
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.
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.
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 = 2highest 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 13ok lines with real durations, zero(cached), zeroFAIL, 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 next2026-08-18 05:01:14 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #186.
The two defects
1. A failed non-fatal assertion became a segfault.
acquirereturns(nil, false)on every refusal path,TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueingcheckedokwithassert.True, and the next line called the nilrelease. Onetiming miss therefore killed the whole
internal/middlewaretest binaryrather than one test. That assertion, and every other one in the repo
whose value is dereferenced afterwards, is now
require.2.
acquireitself could shed a request with slots standing free.This is the part that is not confined to the test.
acquireselectedover 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
acquirenow takes a free slot in a non-blocking preamble, before anytimer is armed — the same shape
lifecycle.waitDoneuses to settle itsown 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
passwordVerifyMaxWaiterscomment is unchanged: 16 parkedwaiters 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 alreadycancelled is now granted a free slot every time, where the old select
refused it about half the time.
acquire's doc still claimed acancelled 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
ctxand the slot comesstraight back, and refusing would mean shedding a request with capacity
standing free — the very thing defect 2 is about.
ctxis stillhonoured once a request has to wait, which is what
TestLoginGuard_AcquireHonoursCancellationpins.TestLoginGuard_FreeSlotBeatsAnExpiredWaitpins the preamble, modelledon
TestWaitDone_DrainedBeforeExpiredContext: 1000 passes with a waitthat 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 claimsdetection probability
1 - 2^-N: a pass is only a coin flip once thezero-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_SemaphoreBoundsConcurrentVerificationsused a 10 mssleep 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 fixesthe 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 wouldarrive after the first holders had already decremented, and
highestwould report
concurrencyhowever many were really let in. A firstrevision 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.WaitGroupofworkers, withDone()called on the refusal path immediately, and on the success pathdirectly after the holder has recorded itself in
highest. Recordingbefore 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
highestis the true maximum. No sleep, and no wall-clock margin wasreintroduced: the only duration left in the test is the 5 s
time.AfterFuncdeadlock guard, which no assertion depends on and whichis reachable only by a worker that never returns from
acquireat all.Mutation evidence,
newLoginGuard'sslots: make(chan struct{}, concurrency)changed toconcurrency * 6, gated throughmake test(
GOFLAGS=-count=1, so no run served a cached result):GOMAXPROCS1 / 2 / 4 / 812 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 truncatedobservation. The unmutated tree passes at
GOMAXPROCS1, 2 and default,in 2.01 s each, which is the refused workers' wait and not a margin.
The sibling sweep
Every
*_test.goin the repo was swept for the same shape — a non-fatalassertion on a value's validity that a later line dereferences, indexes
or calls. One sibling, in another package:
internal/database/webhook_db_manager_test.gochecked a slice lengthwith
assert.Lenand indexed it on the next line, so the veryregression it guards (deleting one webhook's DB destroying another's
rows) would have surfaced as an index-out-of-range panic through
internal/databaseinstead of a failing test. Nowrequire.Len.Also checked, and clean: no testify call anywhere in the repo is made
from a goroutine, an
Eventuallycondition, or a spawned cleanup, whereFailNowwould 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 hererather than left for the next red night:
assert.Less(got.elapsed, 100ms)— bounded the latency of a goroutinehand-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 = 200msfeeding arequire.NotNil— the probe had to becreated, 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'srequire.Eventuallybudget went 1 s to 5 s for the samereason — 1 s is the same order as the stalls this suite must survive.
probeQueueCapno longer returns*bool; it returns two bools, whichis the surrounding idiom. (Unnamed, because
nonamedreturnsrejectsnamed bool results.)
Remaining (A)-class risk in the repo — one, not touched here:
internal/delivery/target_database_test.go:203-211— two back-to-backwrites 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-stopguards);
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:65andinternal/server/shutdown_test.go:35are pure-function table inputs, noclock read;
lifecycle_test.go:18,102,retention_lifecycle_test.go:35,245andengine_lifecycle_test.go:36,59are deadlines that are guaranteed toexpire (the thing they wait on never completes), so slowness cannot flip
the assertion;
circuit_breaker_test.go:104,135,161,281sleep 60 ms pasta 50 ms cooldown, one-directional — oversleeping only makes the
assertion more true;
internal/delivery/engine_integration_test.go:1094is 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,912are 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 hasexited before Notify"),
engine_test.go:777(100 ms handler sleep toforce worker overlap, asserting only an upper bound),
engine_test.go:1023(50 ms, soExportScheduleRetry's goroutineattempts the overflow send before the channel is drained),
engine_lifecycle_test.go:230andretention_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/testserves(cached)results, so I compiled thepackage test binary with the same flags
script/testuses(
go test -c -race, run with-test.timeout 30s) and looped that.Disclosure: that
go test -cis the one raw toolchain invocation inthis work; every gate below is a make target or
script/entrypoint.The mutation runs above used
make testwithGOFLAGS=-count=1, whichis 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:
GOMAXPROCS=1GOMAXPROCS=2, 8 roundsHost 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=1under 16 spinners, 128 16-wayparallel) 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 checkexits 0. Disclosure: its hostgo testlineswere served
(cached), because the mutation loops above had already runthe same tree — so the uncached test evidence is the Docker gate below,
not
make check. The authoritative run is the Docker gatewith the cache defeated,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., exit 0:golangci-lint:v2.12.2container ran 47.5 s andreported
0 issues.;make fmt-checkran.zero
(cached)lines and zeroFAILlines in the whole build log.TestLoginGuard_FreeSlotBeatsAnExpiredWait,TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing,TestLoginGuard_SemaphoreBoundsConcurrentVerifications,TestLoginGuard_ShedsPastTheQueueCapandTestWebhookDBManager_MultipleWebhooksall appear as--- PASS.CACHEDlayers are the two pinned base-image resolves and sixdeterministic 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 -ais empty, and no prune ofany kind was run.
TODO.mdis untouched by this commit.clawbot referenced this pull request2026-08-18 04:10:16 +02:00
Review: FAIL —
needs-reworkIndependent review of
b8940c0againstnext(b573959). One blocking finding: the de-flake ofTestLoginGuard_SemaphoreBoundsConcurrentVerificationstraded 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
concurrencyholders overlap: correct, and animprovement). It destroys the upper bound, which is what the test actually asserts. Holders leave the
instant the
concurrency-th arrives, sohighestis sampled over a window that collapses to nothing:under a guard that admits more than
concurrency, the extra goroutines reachinside++after thebarrier has opened and after the first two have already decremented, and
highestcomes back as exactlyconcurrency. The old 10 ms hold kept every admitted worker inside simultaneously, sohighestreportedthe true maximum.
Measured, one production mutation only —
newLoginGuard'sslots: make(chan struct{}, concurrency)changed to
concurrency*6, i.e. the bound simply does not hold — gated throughmake test:b8940c0GOMAXPROCSdefault x9, and 1, 2, 4, 8)b573959GOMAXPROCS=1)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_SaturatedSemaphoreRefusesRatherThanQueueingandTestLoginGuard_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'sacquire attempt has returned, success or refusal — e.g. a
sync.WaitGroupofworkers,Done()called immediately after
AcquireForTestreturns in every path,Wait()ed by one goroutine that thencloses
overlapped. Correct guard: the 10 refused workers return withinguardWait(2 s) and the barrieropens; broken guard: every admitted worker is inside at once and
highestreports the truth. Still nosleep, still deterministic, detection back to 100%. The existing 5 s
time.AfterFuncstays as thedeadlock guard.
MINOR 2 —
acquire's doc comment is no longer true of the codeinternal/middleware/loginguard.go:172-178: "It reports false ... or when the request was cancelledfirst." The preamble does not consult
ctx, so a request whose context is already cancelled is nowgranted 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 soExportScheduleRetry's goroutine attempts theoverflow 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 clienttimeout. (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), andtarget_database_test.go:203-211— genuinely (A): astall over 2 s between the two writes flips
Reopens()from 1 to 2 and reds correct code. Agree it shouldbe filed. "Not yet observed" is exactly what was true of this issue's defect until run 232.
Nits
probeQueueCapreturning*bool(loginguard_test.go:585) is not the surrounding idiom; two namedbools, or the retained struct, read better.
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 probability1 - 2^-N: a pass isonly 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
chansend/chanrecvrather than from the harness: asender parks only when
qcount == dataqsizandrecvqis empty; a receive with a non-emptysendqgoes through
recv(), which copies the parked sender's value straight back into the buffer slot it justvacated, so
qcountstays atdataqsizfor as long as any sender is parked. The preamble'snon-blocking send therefore fails — on the lock-free
full(c)fast path, or under the lock — whenever awaiter 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.
the blocking select, so the 16-plus-2 arithmetic in the
passwordVerifyMaxWaiterscomment still boundsretained 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.
alone fails
TestLoginGuard_FreeSlotBeatsAnExpiredWait. Reverting queue admission to a blocking sendfails
TestLoginGuard_ShedsPastTheQueueCapin 5.03 s, so the 200 ms to 5 s change and the removal ofassert.Less(elapsed, 100ms)did not cost that test its regression.fillQueue's 1 s to 5 s change ispatience-only.
asserttorequiresweep re-run independently across every*_test.go: one sibling, the onefound (
internal/database/webhook_db_manager_test.go:342). Theassert.True(t, ok)sites insession_test.goare followed by value comparisons, not dereferences. No testify call is made from agoroutine, an
Eventuallycondition, or a spawned cleanup.next, one commit, title ends(closes #186), merges clean against currentnext,TODO.mduntouched, no attribution trailers or vendor references anywhere, inclusive terminology clean.
go test -c -racewas used for iteration only; every gate result cited is a maketarget 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)b8940c0:success, run 235, "Successful in 2m46s".make check: exit 0. Lint in the pinned container,0 issues.after 47.6 s;go test13oklines,zero
(cached).docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .: exit 0.golangci-lint run47.6 s andmake fmt-checkboth ran uncached;[builder 9/11] RUN make testran53.8 s; zero
(cached)lines in the whole log; 11--- PASS: TestLoginGuard*lines. The 8CACHEDlayers are base-image resolves, the
go.mod/go.sumcopy andgo mod download.docker ps -aempty, no prune of any kind run.b8940c0424tof932a86e8dReworked, head
f932a86. One commit still; body updated.BLOCKING 1 — upper bound restored. The barrier no longer opens at the
concurrency-th holder. A secondsync.WaitGroupofworkersgates it:Done()on the refusal path immediately, and on the success path directly after the holder has recorded itself inhighest. 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*6mutation throughmake test,GOFLAGS=-count=1so nothing was served cached:f932a86f932a86,GOMAXPROCS1 / 2 / 4 / 812/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 atGOMAXPROCS1, 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,ctxis honoured only once the request has to wait. Argued in the body rather than changed silently.MINOR 3.
engine_test.go:1023added to the fail-green list (now five sites);engine_integration_test.go:1094added as (C).target_database_test.go:203-211now points at #190.Nits.
probeQueueCapreturns two bools — unnamed, sincenonamedreturnsrejects named bool results. "pass 1" corrected to pass 2 in both body and commit message.racePasses's1 - 2^-Nclaim dropped; the comment now claims only independence.Untouched, as reviewed: barge argument, waiter accounting, both mutation checks, the
assert-to-requiresweep, and the honest 228-iteration control (which stays disclosed, and was not re-run after the rework — disclosed as such).Gates on
f932a86.make checkexit 0 — its hostgo testlines 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 s0 issues.,make fmt-checkran, tests 53.7 s over 13 packages, zero(cached)and zeroFAILlines, 8CACHEDlayers all base-image resolves and stage-2 packaging. CI run 239 success in 2m50s. Image removed,docker ps -aempty, no prune of any kind. Rebased onnextat9313b0f;TODO.mduntouched.Review: PASS
Independent re-review of
f932a86againstnext(9313b0f). Round-1 BLOCKING 1 is genuinelyfixed. 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*6mutation onnewLoginGuard'sslots, throughmake testwithGOFLAGS=-count=1:f932a86, defaultf932a86,GOMAXPROCS1 / 2 / 4 / 812/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 truemaximum, not a window-truncated observation.
Stronger probe, not run by the author:
concurrency+1, one extra slot with no buffer-sizetell — 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 afterAcquireForTestreturns) the barrier can openwhile an admitted worker sits between its return and its
inside++; another holder is thenreleased from
<-overlapped, decrements, andhighestunder-reports. Recording beforesignalling closes that: every admitted worker's
inside++happens-before itsDone(), nodecrement precedes the barrier, so
highestequals 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 atGOMAXPROCS1/2/4/8, 4 atGOMAXPROCS=2under 24 spinners — 2.00-2.01 s each, which is
guardWait, not a margin. Only duration left inthe test is the 5 s
time.AfterFunc; I checked the failure mode rather than the comment: if itfired early under a correct guard the holders release and late workers acquire, but with
concurrency = 2higheststill cannot exceed 2, so the assertion holds either way. It cannotred correct code and cannot green a broken guard.
Round-1 mutations re-verified against the changed test file
Preamble revert still fails
TestLoginGuard_FreeSlotBeatsAnExpiredWait3/3. Queue admissionback to a blocking send still fails
TestLoginGuard_ShedsPastTheQueueCapin 5.02 s (authorreported 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 everytime.Sleep/time.After/Eventually/WithTimeoutin the suite independently and every one is classified; spot-checksof
engine_test.go:1023(fail-green: a short sleep makes the overflow send un-attempted, so itunder-observes),
engine_integration_test.go:1094(C, one-directional) andtarget_database_test.go:203-211(A, correctly filed as#190) are right.
nonamedreturnsis enabled(
default: all, not disabled), so the unnamed two-boolprobeQueueCapsignature is forced, asclaimed.
assert-to-requiresweep re-run: one sibling, the one fixed. CIsuccessonf932a86(run 239, 2m50s), basenext, one commit, title ends(closes #186), fast-forwardsonto
next,TODO.mduntouched, no attribution trailers or vendor references, inclusiveterminology clean. Every other figure in the body checks out against
f932a86.Non-blocking observations
the first failing pass at 0, 0, 1 across three runs (
passis 0-indexed); round 1 measured2. 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'sguardWait(2 s); itscomment justifies it only against scheduling delay. Benign per the analysis above, but the
coupling is undocumented.
(this PR correctly says five, after adding
engine_test.go:1023) and listsinternal/delivery/retention_lifecycle_test.go:209, which lives ininternal/database/.Disclosures
b8940c0is unreachable (force-pushed; absent from git and from the API), so "behaviourunchanged from the previous head" rests on comparing the current preamble against round 1's own
quotation of it, not on a direct diff.
go test, used solely to isolate the mutation failure text thatmake test's-vinterleaving obscured. Every gate and every count above is a make target or the Docker build.
GOFLAGS=-count=1onmake testis inside the make-targets-only rule in my judgement: thetarget still runs and
script/test's-raceand-timeout 30sare preserved; only the testcache is defeated.
unfixed binary is still stated in the body.
Gate evidence (my own fresh
/tmpclone, aftermake bootstrap)docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0.[lint 9/9] golangci-lint runran 50.9 s,0 issues.;[lint 7/9] make fmt-checkran;tests produced 13
oklines with real durations, zero(cached), zeroFAIL, and--- PASSfor all fourTestLoginGuard_*under review plusTestWebhookDBManager_MultipleWebhooks. The 8CACHEDlayers are the two pinned base-imageresolves (#7, #8) and six stage-2 packaging steps (#28-#33) — no lint or test layer among them.
Image removed,
docker ps -aempty, all load spinners confirmed killed, no prune of any kind.