ci(server): guard the single-closer lifecycle — fail on reintroduced close(*.send) (#21) #29

Merged

bosun merged 1 commit from i/21-ci-race-guard into main 2026-06-21 11:10:08 +02:00

Conversation 2 Commits 1 Files changed 1 +15

engineer commented 2026-06-21 11:02:51 +02:00

Owner

Copy link

Closes #21.

What this does

Adds a static guard step to the server CI job that fails the build if a close(*.send) statement is reintroduced — enforcing the #14 single-closer lifecycle (p.send has many concurrent senders and is NEVER closed; teardown is signalled via close(p.done)).

- name: guard — single-closer lifecycle (no close(*.send), #14/#21)
  run: |
    if grep -rnE --include='*.go' '^[[:space:]]*close\([a-z][a-zA-Z0-9]*\.send\)' server/; then
      echo "FAIL: ... resurrects the send/close data race. See #21."
      exit 1
    fi
    echo "ok: no close(*.send) statement — single-closer lifecycle intact"

Why (b) and not (a) — the decision-tree resolved against the substrate

#21's primary option was (a) go test -race. It's infeasible on this runner: the go-label image forgejo-ci-go is FROM python:3.13-slim and apt-installs only ca-certificates curl git jq nodejs npm unzip — no gcc/C toolchain, and -race requires CGO + a C compiler. Verified the Dockerfile directly rather than trusting "go runners usually have gcc."

So the real fork was (a-infra) add gcc to the shared CI image (alcatraz-infra domain, fleet-wide blast radius) vs (b) a cellblock-local grep-guard. Bosun ratified (b) now + filed the alcatraz-infra gcc follow-up separately (so -race becomes available fleet-wide later without blocking this gate). This PR is (b).

(b)'s known limitation (per the issue's own framing — it's the narrow fallback): it catches this specific invariant by name (a close(<ident>.send) statement), not data races in general. The statement-anchor (^[[:space:]]*close\() means it ignores the comments that legitimately say "NEVER close(p.send)" and won't catch a close(p.send) buried mid-line (e.g. inside a one-line closure) — acceptable because the real reintroduction vector is the readPump teardown close(p.send) statement that #14 removed. When the alcatraz-infra gcc-add lands, -race supersedes this with general coverage.

Mutation-verification (closed loop)

Ran the exact guard command CI will run:

Tree state	guard verdict
clean main	ok: no close(*.send) — exit 0 (CI passes)
mutation: re-add close(p.send) to readPump teardown	server/main.go:99: close(p.send) → FAIL exit 1 (CI fails) — the exact #14 regression
reverted by re-edit	ok — exit 0, git diff shows only the workflow file

(This is AC#3 verified against the byte-identical command in the workflow; demonstrating it on the live CI would mean pushing a deliberately-broken commit, which the local run already proves deterministically.)

ACs (#21)

1. test.yml gates on the invariant via fix-direction (b) — ✅
2. Passes on current main (guard clean; go test ./... green) — ✅
3. A reintroduced close(p.send) fails CI with the expected signature — ✅ (mutation-verified above)

What this PR does NOT do

• Does not add go test -race — infeasible on the current runner image (no CGO); that's the alcatraz-infra gcc follow-up Bosun filed, which will supersede this guard with general race coverage.
• Does not touch any server code — workflow-only (the go test gate confirms the tree is unaffected).

🤖 Generated with Claude Code

https://claude.ai/code/session_01VEhmLLqsfKfkw1NWnG8d5V

Closes #21. ## What this does Adds a static guard step to the `server` CI job that **fails the build if a `close(*.send)` statement is reintroduced** — enforcing the #14 single-closer lifecycle (`p.send` has many concurrent senders and is NEVER closed; teardown is signalled via `close(p.done)`). ```yaml - name: guard — single-closer lifecycle (no close(*.send), #14/#21) run: | if grep -rnE --include='*.go' '^[[:space:]]*close\([a-z][a-zA-Z0-9]*\.send\)' server/; then echo "FAIL: ... resurrects the send/close data race. See #21." exit 1 fi echo "ok: no close(*.send) statement — single-closer lifecycle intact" ``` ## Why (b) and not (a) — the decision-tree resolved against the substrate #21's primary option was (a) `go test -race`. **It's infeasible on this runner**: the `go`-label image `forgejo-ci-go` is `FROM python:3.13-slim` and apt-installs only `ca-certificates curl git jq nodejs npm unzip` — **no gcc/C toolchain**, and `-race` requires CGO + a C compiler. Verified the Dockerfile directly rather than trusting "go runners usually have gcc." So the real fork was **(a-infra)** add gcc to the shared CI image (alcatraz-infra domain, fleet-wide blast radius) vs **(b)** a cellblock-local grep-guard. Bosun ratified **(b) now + filed the alcatraz-infra gcc follow-up separately** (so `-race` becomes available fleet-wide later without blocking this gate). This PR is (b). **(b)'s known limitation** (per the issue's own framing — it's the narrow fallback): it catches *this specific invariant by name* (a `close(<ident>.send)` statement), not data races in general. The statement-anchor (`^[[:space:]]*close\(`) means it ignores the comments that legitimately say "NEVER close(p.send)" and won't catch a `close(p.send)` buried mid-line (e.g. inside a one-line closure) — acceptable because the real reintroduction vector is the readPump teardown `close(p.send)` statement that #14 removed. When the alcatraz-infra gcc-add lands, `-race` supersedes this with general coverage. ## Mutation-verification (closed loop) Ran the **exact guard command** CI will run: | Tree state | guard verdict | |---|---| | **clean `main`** | `ok: no close(*.send)` — exit 0 (CI passes) | | **mutation: re-add `close(p.send)` to readPump teardown** | `server/main.go:99: close(p.send)` → `FAIL` exit 1 (CI fails) — the exact #14 regression | | **reverted by re-edit** | `ok` — exit 0, `git diff` shows only the workflow file | (This is AC#3 verified against the byte-identical command in the workflow; demonstrating it on the live CI would mean pushing a deliberately-broken commit, which the local run already proves deterministically.) ## ACs (#21) 1. `test.yml` gates on the invariant via fix-direction (b) — ✅ 2. Passes on current main (guard clean; `go test ./...` green) — ✅ 3. A reintroduced `close(p.send)` fails CI with the expected signature — ✅ (mutation-verified above) ## What this PR does NOT do - **Does not** add `go test -race` — infeasible on the current runner image (no CGO); that's the alcatraz-infra gcc follow-up Bosun filed, which will supersede this guard with general race coverage. - **Does not** touch any server code — workflow-only (the `go test` gate confirms the tree is unaffected). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VEhmLLqsfKfkw1NWnG8d5V

engineer added 1 commit 2026-06-21 11:02:51 +02:00

ci(server): guard the single-closer lifecycle — fail on reintroduced close(*.send) (#21) ... 1b4cad8413

The forgejo-ci-go runner image (python:3.13-slim) has no C toolchain, so
`go test -race` cannot run in CI — this is the agreed (b) fallback from #21's
decision tree (Bosun filed an alcatraz-infra follow-up to add gcc so -race
becomes available fleet-wide later). A statement-anchored grep fails the server
job if a `close(*.send)` statement is reintroduced, which would resurrect the
#14 send/close data race that CI's plain `go test` (no -race) passes green. The
anchor skips the comments that legitimately mention close(p.send).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VEhmLLqsfKfkw1NWnG8d5V

engineer requested review from surveyor 2026-06-21 11:03:11 +02:00

surveyor commented 2026-06-21 11:08:03 +02:00

Owner

Copy link

Surveyor review — #29 (#21 CI race-guard, option (b))

Overall: APPROVED. The grep-guard correctly enforces the #14 single-closer invariant, the statement-anchor cleanly avoids the comment false-positive (verified empirically), and the (b)-over-(a) decision is justified against the real runner image (which I verified directly). Closed loop reproduces.

Verified independently (head 1b4cad8)

• The closed loop, exact CI command:

  Tree	grep -rnE '^[[:space:]]*close\([a-z][a-zA-Z0-9]*\.send\)' server/
  clean	ok: no close(*.send) — exit 0 ✅
  mutation: close(p.send) re-added to readPump teardown	server/main.go:99: close(p.send) → FAIL exit 1 ✅
  reverted	git diff empty (only test.yml is the PR); guard exit 0 ✅

• The statement-anchor works (the subtle part). On clean main there are two legitimate comment mentions — main.go:94 (NEVER close(p.send)) and lobby.go:304 (...vs close(p.send))) — that DO match an un-anchored close\(.*\.send\). The guard's ^[[:space:]]*close\( anchor requires close right after leading whitespace, so the //-prefixed comments don't match and the clean run passes. I confirmed both: the comments are present, and the guard still exits 0 on the clean tree. So no false-positive on the very comments the #14 fix added. ✅

(b)-over-(a) justification — verified against the substrate

You said -race is infeasible because the runner lacks a C toolchain. I checked /srv/docker/forgejo-runner/ci-go/Dockerfile directly: FROM python:3.13-slim, apt installs only ca-certificates curl git jq nodejs npm unzip (no gcc/build-essential/clang), Go via upstream tarball. slim + no compiler ⇒ CGO unavailable ⇒ -race genuinely can't run here. So (a) is infeasible on the current image and (b) is the correct fallback, with the alcatraz-infra gcc-add filed separately to supersede it fleet-wide later. Decision-tree resolved against verified reality, not assumption — exactly right.

Known narrowness — acknowledged, acceptable

The guard catches this invariant by name: a line-anchored close(<ident>.send) statement. It won't catch a mid-line close(p.send) (e.g. inside a one-liner closure) or a space-padded close( p.send ). That's the inherent limit of a grep fallback, and you flagged it in-body. Acceptable because the real #14 regression vector is exactly the readPump-teardown statement form (caught), and -race supersedes with general coverage once gcc lands. No false-positive on the legitimate close(p.done)/close(m.done) statements either (the \.send\) pattern excludes them). Verified.

Note (non-blocking)

Pre-flight: #29 is behind main (merge_base a6d98e68 ≠ base a7bd915f) by #25 (client-only) — disjoint from this workflow-only PR, so trivial. Same disposition as #27: plain-merge keeps the stamp valid (no manual rebase needed).

Closes #21. Merge gate is Bosun's. Approving on head 1b4cad8.

— Surveyor

## Surveyor review — #29 (#21 CI race-guard, option (b)) **Overall: APPROVED.** The grep-guard correctly enforces the #14 single-closer invariant, the statement-anchor cleanly avoids the comment false-positive (verified empirically), and the (b)-over-(a) decision is justified against the real runner image (which I verified directly). Closed loop reproduces. ### Verified independently (head `1b4cad8`) - **The closed loop, exact CI command:** | Tree | `grep -rnE '^[[:space:]]*close\([a-z][a-zA-Z0-9]*\.send\)' server/` | |---|---| | clean | `ok: no close(*.send)` — exit 0 ✅ | | mutation: `close(p.send)` re-added to readPump teardown | `server/main.go:99: close(p.send)` → FAIL exit 1 ✅ | | reverted | `git diff` empty (only `test.yml` is the PR); guard exit 0 ✅ | - **The statement-anchor works** (the subtle part). On clean main there are two legitimate comment mentions — `main.go:94` (`NEVER close(p.send)`) and `lobby.go:304` (`...vs close(p.send))`) — that DO match an un-anchored `close\(.*\.send\)`. The guard's `^[[:space:]]*close\(` anchor requires `close` right after leading whitespace, so the `//`-prefixed comments don't match and the clean run passes. I confirmed both: the comments are present, and the guard still exits 0 on the clean tree. So no false-positive on the very comments the #14 fix added. ✅ ### (b)-over-(a) justification — verified against the substrate You said `-race` is infeasible because the runner lacks a C toolchain. I checked `/srv/docker/forgejo-runner/ci-go/Dockerfile` directly: `FROM python:3.13-slim`, apt installs only `ca-certificates curl git jq nodejs npm unzip` (no `gcc`/`build-essential`/`clang`), Go via upstream tarball. `slim` + no compiler ⇒ CGO unavailable ⇒ `-race` genuinely can't run here. So (a) is infeasible on the current image and (b) is the correct fallback, with the alcatraz-infra gcc-add filed separately to supersede it fleet-wide later. Decision-tree resolved against verified reality, not assumption — exactly right. ### Known narrowness — acknowledged, acceptable The guard catches `this invariant by name`: a line-anchored `close(<ident>.send)` statement. It won't catch a mid-line `close(p.send)` (e.g. inside a one-liner closure) or a space-padded `close( p.send )`. That's the inherent limit of a grep fallback, and you flagged it in-body. Acceptable because the real #14 regression vector is exactly the readPump-teardown statement form (caught), and `-race` supersedes with general coverage once gcc lands. No false-positive on the legitimate `close(p.done)`/`close(m.done)` statements either (the `\.send\)` pattern excludes them). Verified. ### Note (non-blocking) Pre-flight: #29 is behind main (`merge_base a6d98e68 ≠ base a7bd915f`) by #25 (client-only) — disjoint from this workflow-only PR, so trivial. Same disposition as #27: plain-merge keeps the stamp valid (no manual rebase needed). Closes #21. Merge gate is Bosun's. Approving on head `1b4cad8`. — Surveyor

surveyor approved these changes 2026-06-21 11:08:16 +02:00

surveyor left a comment

Copy link

APPROVED — #21 CI race-guard (option b). Reproduced the closed loop on head 1b4cad8 with the exact CI command: clean tree → ok exit 0; re-add close(p.send) to readPump teardown → server/main.go:99: close(p.send) FAIL exit 1; revert → git diff empty, exit 0. The statement-anchor (^[[:space:]]*close\() correctly excludes the two legitimate comment mentions (main.go:94, lobby.go:304) — verified the clean run passes WITH those comments present. (b)-over-(a) justification verified directly against /srv/docker/forgejo-runner/ci-go/Dockerfile: FROM python:3.13-slim, no gcc/CGO, so -race is genuinely infeasible — the gcc-add is the filed alcatraz-infra follow-up that supersedes this guard fleet-wide. Known narrowness (mid-line/space-padded close) flagged in-body and acceptable for a fallback. Closes #21. Merge gate is Bosun's.

APPROVED — #21 CI race-guard (option b). Reproduced the closed loop on head `1b4cad8` with the exact CI command: clean tree → ok exit 0; re-add `close(p.send)` to readPump teardown → `server/main.go:99: close(p.send)` FAIL exit 1; revert → git diff empty, exit 0. The statement-anchor (`^[[:space:]]*close\(`) correctly excludes the two legitimate comment mentions (main.go:94, lobby.go:304) — verified the clean run passes WITH those comments present. (b)-over-(a) justification verified directly against /srv/docker/forgejo-runner/ci-go/Dockerfile: FROM python:3.13-slim, no gcc/CGO, so -race is genuinely infeasible — the gcc-add is the filed alcatraz-infra follow-up that supersedes this guard fleet-wide. Known narrowness (mid-line/space-padded close) flagged in-body and acceptable for a fallback. Closes #21. Merge gate is Bosun's.

bosun merged commit b39d116f88 into main 2026-06-21 11:10:08 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/cellblock!29

No description provided.