Replace three tight overall deadlines for must-happen async events with the existing 30-second eventualWait: attempt expiry, collection retry, and remote web-listener failure.
Document why must-happen waits use a load-tolerant bound while must-not-happen assertions retain short observation windows.
Follow up on the remaining cases found after #467–#469; see #471.
Verification
make cover — every statement covered (100%).
make lint — go vet and staticcheck pass.
actionlint and git diff --check pass.
Focused expiry test repeated 50 times; collection retry test repeated 10 times; remote web-access test repeated 20 times.
## Summary
- Replace three tight overall deadlines for must-happen async events with the existing 30-second `eventualWait`: attempt expiry, collection retry, and remote web-listener failure.
- Document why must-happen waits use a load-tolerant bound while must-not-happen assertions retain short observation windows.
- Follow up on the remaining cases found after #467–#469; see #471.
## Verification
- `make cover` — every statement covered (100%).
- `make lint` — `go vet` and `staticcheck` pass.
- `actionlint` and `git diff --check` pass.
- Focused expiry test repeated 50 times; collection retry test repeated 10 times; remote web-access test repeated 20 times.
Fixes #471
Thanks, good catch. My sweep for #466 only searched for time.After(time.Second), so it missed these two deadline loops. Both changes look right to me, and so does the docs/development.md rule.
I searched your branch for every short bound left in tests, to see what the new rule still leaves out:
One more must-happen wait with a tight bound:
apps/sisyphusd/web_access_runtime_test.go:148: deadline := time.Now().Add(2 * time.Second) waits for a listener failure to show in the live state. That has to happen, so by your rule it should use eventualWait. That package has no eventualWait yet. The root package has one in pins_test.go, so it's the same package (main) and can use it directly.
Moderate bounds (3–5 s), consistent with the rule but not urgent:pins_test.go 358/372/438/490/762, reciprocal_test.go 343/456, replication_test.go:206, replication/follower_test.go 892/1028/1073, worker/worker_test.go 177/290 and worker/offers_test.go:252. I'd leave these unless one flakes.
Two tests where the short bound is the assertion, so a longer bound would defeat them:
pins_test.go:510: the first release must be collected within the 250 ms debounce window despite later releases.
worker/peers_internal_test.go:140: the third relay stream must start within 400 ms because a healthy snapshot reset the backoff.
Both measure wall-clock gaps, so heavy load could still make them flake. The real fix there would be an injected clock rather than a longer wait. Worth a separate issue if either shows up red.
If you'd like, I can push the web_access_runtime_test.go change onto this branch as one commit, so it all lands together under #471. Say yes and I'll do it; otherwise it's fine to merge as it is.
Thanks, good catch. My sweep for #466 only searched for `time.After(time.Second)`, so it missed these two deadline loops. Both changes look right to me, and so does the `docs/development.md` rule.
I searched your branch for every short bound left in tests, to see what the new rule still leaves out:
**One more must-happen wait with a tight bound:**
- `apps/sisyphusd/web_access_runtime_test.go:148`: `deadline := time.Now().Add(2 * time.Second)` waits for a listener failure to show in the live state. That has to happen, so by your rule it should use `eventualWait`. That package has no `eventualWait` yet. The root package has one in `pins_test.go`, so it's the same package (`main`) and can use it directly.
**Moderate bounds (3–5 s), consistent with the rule but not urgent:** `pins_test.go` 358/372/438/490/762, `reciprocal_test.go` 343/456, `replication_test.go:206`, `replication/follower_test.go` 892/1028/1073, `worker/worker_test.go` 177/290 and `worker/offers_test.go:252`. I'd leave these unless one flakes.
**Two tests where the short bound *is* the assertion**, so a longer bound would defeat them:
- `pins_test.go:510`: the first release must be collected within the 250 ms debounce window despite later releases.
- `worker/peers_internal_test.go:140`: the third relay stream must start within 400 ms because a healthy snapshot reset the backoff.
Both measure wall-clock gaps, so heavy load could still make them flake. The real fix there would be an injected clock rather than a longer wait. Worth a separate issue if either shows up red.
If you'd like, I can push the `web_access_runtime_test.go` change onto this branch as one commit, so it all lands together under #471. Say yes and I'll do it; otherwise it's fine to merge as it is.
Good catch — yes, please add the web_access_runtime_test.go wait to this branch using eventualWait, and include the focused test. I will hold merge until that update has full green CI. Thanks for the careful sweep.
Good catch — yes, please add the `web_access_runtime_test.go` wait to this branch using `eventualWait`, and include the focused test. I will hold merge until that update has full green CI. Thanks for the careful sweep.
/cc @rvnminers-A-and-N
Pushed test: give the web listener failure the must-happen wait to this branch. It changes web_access_runtime_test.go:148 from 2 s to eventualWait (the root package already declares it in pins_test.go). Focused test: go test -race -count=20 -run ^TestRemoteWebAccessIsOptInTLSAndCanBeDisabled$ ./apps/sisyphusd passes, and gofmt and vet are clean. Over to you to squash-merge once CI is green.
Separately, a fourth full-suite -race -count=4 stress run on dev at 7cf1ada finished clean overnight: no failures in any package.
Pushed `test: give the web listener failure the must-happen wait` to this branch. It changes `web_access_runtime_test.go:148` from 2 s to `eventualWait` (the root package already declares it in `pins_test.go`). Focused test: `go test -race -count=20 -run ^TestRemoteWebAccessIsOptInTLSAndCanBeDisabled$ ./apps/sisyphusd` passes, and gofmt and vet are clean. Over to you to squash-merge once CI is green.
Separately, a fourth full-suite `-race -count=4` stress run on `dev` at `7cf1ada` finished clean overnight: no failures in any package.
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.
rvnminers-A-and-N
Summary
eventualWait: attempt expiry, collection retry, and remote web-listener failure.Verification
make cover— every statement covered (100%).make lint—go vetandstaticcheckpass.actionlintandgit diff --checkpass.Fixes #471
Thanks, good catch. My sweep for #466 only searched for
time.After(time.Second), so it missed these two deadline loops. Both changes look right to me, and so does thedocs/development.mdrule.I searched your branch for every short bound left in tests, to see what the new rule still leaves out:
One more must-happen wait with a tight bound:
apps/sisyphusd/web_access_runtime_test.go:148:deadline := time.Now().Add(2 * time.Second)waits for a listener failure to show in the live state. That has to happen, so by your rule it should useeventualWait. That package has noeventualWaityet. The root package has one inpins_test.go, so it's the same package (main) and can use it directly.Moderate bounds (3–5 s), consistent with the rule but not urgent:
pins_test.go358/372/438/490/762,reciprocal_test.go343/456,replication_test.go:206,replication/follower_test.go892/1028/1073,worker/worker_test.go177/290 andworker/offers_test.go:252. I'd leave these unless one flakes.Two tests where the short bound is the assertion, so a longer bound would defeat them:
pins_test.go:510: the first release must be collected within the 250 ms debounce window despite later releases.worker/peers_internal_test.go:140: the third relay stream must start within 400 ms because a healthy snapshot reset the backoff.Both measure wall-clock gaps, so heavy load could still make them flake. The real fix there would be an injected clock rather than a longer wait. Worth a separate issue if either shows up red.
If you'd like, I can push the
web_access_runtime_test.gochange onto this branch as one commit, so it all lands together under #471. Say yes and I'll do it; otherwise it's fine to merge as it is.Good catch — yes, please add the
web_access_runtime_test.gowait to this branch usingeventualWait, and include the focused test. I will hold merge until that update has full green CI. Thanks for the careful sweep./cc @rvnminers-A-and-N
Pushed
test: give the web listener failure the must-happen waitto this branch. It changesweb_access_runtime_test.go:148from 2 s toeventualWait(the root package already declares it inpins_test.go). Focused test:go test -race -count=20 -run ^TestRemoteWebAccessIsOptInTLSAndCanBeDisabled$ ./apps/sisyphusdpasses, and gofmt and vet are clean. Over to you to squash-merge once CI is green.Separately, a fourth full-suite
-race -count=4stress run ondevat7cf1adafinished clean overnight: no failures in any package.