Where it comes from. webhooker talks to SQLite through modernc.org/sqlite, which is pure Go: every database file is opened with it (internal/database/sqlite_open.go) and the open connection is handed to gorm. But gorm's SQLite adapter, gorm.io/driver/sqlite, also imports github.com/mattn/go-sqlite3, which wraps SQLite's C source and needs cgo. webhooker never uses that driver, yet its C SQLite is compiled into the binary.
What it costs. The image build needs a C compiler and a Debian Go image, builds the binary twice (the second time statically linked against glibc so it runs on Alpine), and carries three build tags whose only job is to make that static glibc binary safe. The shipped binary contains two SQLite engines, one of them unused.
Recommendation, being built. Build the binary with cgo off. mattn's package then compiles to a pure Go stub that is never called, the binary contains only the pure Go SQLite, and the second build and the three tags go. Your Go defaults and REPO_POLICIES.md already decide this, so it is filed and queued as #545. Tests keep cgo, because Go's race detector needs it on Linux.
Your choice: drop gorm as well? That would also take mattn out of go.mod, and your defaults say "not gorm". It touches 32 non-test files across internal/database, internal/delivery and internal/handlers: every query becomes plain database/sql with hand-written row scanning. The schema already comes from SQL migration files, not from gorm. Recommendation: not now. #545 removes cgo with a small change, and dropping gorm would collide with the six data-model changes you approved today. If you want it, I file it to build after those land.
Model: opus-5-5
**Where it comes from.** webhooker talks to SQLite through `modernc.org/sqlite`, which is pure Go: every database file is opened with it (`internal/database/sqlite_open.go`) and the open connection is handed to gorm. But gorm's SQLite adapter, `gorm.io/driver/sqlite`, also imports `github.com/mattn/go-sqlite3`, which wraps SQLite's C source and needs cgo. webhooker never uses that driver, yet its C SQLite is compiled into the binary.
**What it costs.** The image build needs a C compiler and a Debian Go image, builds the binary twice (the second time statically linked against glibc so it runs on Alpine), and carries three build tags whose only job is to make that static glibc binary safe. The shipped binary contains two SQLite engines, one of them unused.
**Recommendation, being built.** Build the binary with cgo off. mattn's package then compiles to a pure Go stub that is never called, the binary contains only the pure Go SQLite, and the second build and the three tags go. Your Go defaults and `REPO_POLICIES.md` already decide this, so it is filed and queued as https://git.eeqj.de/sneak/webhooker/issues/545. Tests keep cgo, because Go's race detector needs it on Linux.
**Your choice: drop gorm as well?** That would also take mattn out of `go.mod`, and your defaults say "not gorm". It touches 32 non-test files across `internal/database`, `internal/delivery` and `internal/handlers`: every query becomes plain `database/sql` with hand-written row scanning. The schema already comes from SQL migration files, not from gorm. Recommendation: not now. https://git.eeqj.de/sneak/webhooker/issues/545 removes cgo with a small change, and dropping gorm would collide with the six data-model changes you approved today. If you want it, I file it to build after those land.
Model: opus-5-5
clawbot
removed their assignment 2026-10-15 11:57:54 +02:00
sneak
was assigned by clawbot2026-10-15 11:57:54 +02:00
Ruling (sneak, 2026-10-16): keep gorm for now. The binary already builds with cgo off (PR 550); find a way to turn cgo off in tests as well.
Next: a webhooker session finds how the tests can run with cgo off. Tests kept cgo only because Go's race detector needed it on Linux; if the only way is to drop the race detector, that choice comes back here to sneak before anything is built. The fleet is paused at the primary account's weekly limit; this starts when it resumes on 18 October 13:00 UTC.
Done when:make test runs with cgo off on next, or the race-detector choice is put to sneak here.
Model: opus-5-5
**Ruling (sneak, 2026-10-16):** keep gorm for now. The binary already builds with cgo off (PR 550); find a way to turn cgo off in tests as well.
**Next:** a webhooker session finds how the tests can run with cgo off. Tests kept cgo only because Go's race detector needed it on Linux; if the only way is to drop the race detector, that choice comes back here to sneak before anything is built. The fleet is paused at the primary account's weekly limit; this starts when it resumes on 18 October 13:00 UTC.
**Done when:** `make test` runs with cgo off on `next`, or the race-detector choice is put to sneak here.
Model: opus-5-5
Finding. On Go 1.26.1, the version the repo pins, the race detector cannot run with cgo off on Linux. The go command refuses -race whenever cgo is off, on every system except macOS, and has no setting to override that: on Linux the race detector's runtime is built on the C library, so it needs cgo to link. The only way to run the tests with cgo off is to run them without the race detector. As planned above, nothing is built until you choose.
Option 1, recommended: keep the race detector, and with it cgo in the tests only. Nothing changes. The shipped binary is already built with cgo off (#550), so cgo never reaches what is deployed. The cost stays in the test phase: it needs the Debian Go image for its C compiler, and the test binaries compile mattn's C SQLite, which the tests never use. REPO_POLICIES.md asks for go test -race in the test phase.
Option 2: drop the race detector.make test runs with cgo off, no C compiler is needed, and the tests run faster and in less memory. The cost: a data race, two goroutines using the same memory without a lock, is no longer caught. webhooker runs its delivery workers, periodic sweeps and web handlers at the same time, which is where such bugs live. The test phase in REPO_POLICIES.md would change as well, since it requires -race.
Why option 1: the race detector is the only check in the gate that finds those bugs, and turning cgo off in tests changes nothing that ships.
Reply 1 or 2; option 2 is one small PR.
Model: opus-5-5
**Finding.** On Go 1.26.1, the version the repo pins, the race detector cannot run with cgo off on Linux. The `go` command refuses `-race` whenever cgo is off, on every system except macOS, and has no setting to override that: on Linux the race detector's runtime is built on the C library, so it needs cgo to link. The only way to run the tests with cgo off is to run them without the race detector. As planned above, nothing is built until you choose.
**Option 1, recommended: keep the race detector, and with it cgo in the tests only.** Nothing changes. The shipped binary is already built with cgo off (https://git.eeqj.de/sneak/webhooker/pulls/550), so cgo never reaches what is deployed. The cost stays in the test phase: it needs the Debian Go image for its C compiler, and the test binaries compile mattn's C SQLite, which the tests never use. `REPO_POLICIES.md` asks for `go test -race` in the test phase.
**Option 2: drop the race detector.** `make test` runs with cgo off, no C compiler is needed, and the tests run faster and in less memory. The cost: a data race, two goroutines using the same memory without a lock, is no longer caught. webhooker runs its delivery workers, periodic sweeps and web handlers at the same time, which is where such bugs live. The test phase in `REPO_POLICIES.md` would change as well, since it requires `-race`.
**Why option 1:** the race detector is the only check in the gate that finds those bugs, and turning cgo off in tests changes nothing that ships.
Reply 1 or 2; option 2 is one small PR.
Model: opus-5-5
clawbot
removed their assignment 2026-10-18 15:10:04 +02:00
sneak
was assigned by clawbot2026-10-18 15:10:04 +02:00
has the race detector actually caught any bugs since we have been using it? seems to me using locking correctly is something we are quite capable of doing.
has the race detector actually caught any bugs since we have been using it? seems to me using locking correctly is something we are quite capable of doing.
Short answer: almost nothing. In webhooker's history the race detector has caught one data race, and it was in test code, not in webhooker: #230 (a test logger writing after its test had finished). The real concurrency bugs were found by review and by ordinary tests, not by the race detector: a startup recovery that could deliver an event twice (#299), a reader that could wedge a webhook's database (#256) and SQLite set up for concurrent writers wrongly (#304). Its cost is real: under it, make test needed up to 6.7 GiB of memory (#344), and it is the only reason the tests still need cgo.
So option 2 (drop the race detector, run the tests with cgo off) costs little on this record. REPO_POLICIES.md asks for -race in the test phase, so option 2 also notes webhooker's exception there. Reply 2 to have it built, one small PR.
Model: opus-5-5
**Short answer: almost nothing.** In webhooker's history the race detector has caught one data race, and it was in test code, not in webhooker: https://git.eeqj.de/sneak/webhooker/issues/230 (a test logger writing after its test had finished). The real concurrency bugs were found by review and by ordinary tests, not by the race detector: a startup recovery that could deliver an event twice (https://git.eeqj.de/sneak/webhooker/issues/299), a reader that could wedge a webhook's database (https://git.eeqj.de/sneak/webhooker/issues/256) and SQLite set up for concurrent writers wrongly (https://git.eeqj.de/sneak/webhooker/issues/304). Its cost is real: under it, `make test` needed up to 6.7 GiB of memory (https://git.eeqj.de/sneak/webhooker/issues/344), and it is the only reason the tests still need cgo.
So option 2 (drop the race detector, run the tests with cgo off) costs little on this record. `REPO_POLICIES.md` asks for `-race` in the test phase, so option 2 also notes webhooker's exception there. Reply 2 to have it built, one small PR.
Model: opus-5-5
Recorded: drop the race detector, so make test runs with cgo off (option 2); the -race line in the test phase was never your ruling. It is built as one small reviewed change on next when an account has room; both worker accounts are at their week limit now (sneak/project-management#37).
Model: opus-5-5
Recorded: drop the race detector, so `make test` runs with cgo off (option 2); the `-race` line in the test phase was never your ruling. It is built as one small reviewed change on `next` when an account has room; both worker accounts are at their week limit now (https://git.eeqj.de/sneak/project-management/issues/37).
Model: opus-5-5
sneak
was unassigned by clawbot2026-10-21 06:09:09 +02:00
clawbot
self-assigned this 2026-10-21 06:09:09 +02:00
Plan (implementer's brief). Option 2, per #538 (comment). One PR on next, title ending (closes #538).
Dockerfile, test phase: both go test lines run with CGO_ENABLED=0 and without -race; keep -cover, -p 4 -parallel 8, the 90s timeout, the tmpfs and the rerun of failed tests. The stage stays on golang:1.26.1-bookworm only because it is the builder stage's image (the builder needs its git and make); its comment says that, not that -race needs a C compiler. Rewrite every comment in the stage that speaks of -race so it is true.
Dockerfile.browser: the browser test binary is built with CGO_ENABLED=0 too, so no test binary compiles mattn's C SQLite.
README.md: the make test line and the test phase description in the Dockerfile section no longer mention race detection or a C compiler. In the development section, one sentence: the tests run without Go's race detector and with cgo off, an exception to the vendored REPO_POLICIES.md, citing the ruling above. REPO_POLICIES.md itself is vendored and is not edited.
Code comments that say the suite runs under -race or the race detector (internal/server/early_shutdown_test.go, internal/archive/sweeper_test.go, internal/archive/evict_test.go, internal/logfield/logfield_test.go, internal/database/totals_test.go, internal/database/password.go, internal/middleware/loginguard.go): each one that is now false is reworded; a past measurement stated as such stays. No test is deleted or weakened.
Not in scope: go.mod (gorm stays), any new file or package.
Done when:make test builds and runs the tests with cgo off and no -race, no test binary links mattn's C SQLite, make test-browser passes, and script/cibuild is green on a fresh clone of the branch rebased on next.
Model: opus-5-5
**Plan (implementer's brief).** Option 2, per https://git.eeqj.de/sneak/webhooker/issues/538#issuecomment-153273. One PR on `next`, title ending `(closes #538)`.
- `Dockerfile`, test phase: both `go test` lines run with `CGO_ENABLED=0` and without `-race`; keep `-cover`, `-p 4 -parallel 8`, the 90s timeout, the tmpfs and the rerun of failed tests. The stage stays on `golang:1.26.1-bookworm` only because it is the builder stage's image (the builder needs its git and make); its comment says that, not that `-race` needs a C compiler. Rewrite every comment in the stage that speaks of `-race` so it is true.
- `Dockerfile.browser`: the browser test binary is built with `CGO_ENABLED=0` too, so no test binary compiles mattn's C SQLite.
- `README.md`: the `make test` line and the test phase description in the Dockerfile section no longer mention race detection or a C compiler. In the development section, one sentence: the tests run without Go's race detector and with cgo off, an exception to the vendored `REPO_POLICIES.md`, citing the ruling above. `REPO_POLICIES.md` itself is vendored and is not edited.
- Code comments that say the suite runs under `-race` or the race detector (`internal/server/early_shutdown_test.go`, `internal/archive/sweeper_test.go`, `internal/archive/evict_test.go`, `internal/logfield/logfield_test.go`, `internal/database/totals_test.go`, `internal/database/password.go`, `internal/middleware/loginguard.go`): each one that is now false is reworded; a past measurement stated as such stays. No test is deleted or weakened.
- Not in scope: `go.mod` (gorm stays), any new file or package.
**Done when:** `make test` builds and runs the tests with cgo off and no `-race`, no test binary links mattn's C SQLite, `make test-browser` passes, and `script/cibuild` is green on a fresh clone of the branch rebased on `next`.
Model: opus-5-5
Built as the plan above says, in #557: make test and the browser test now build and run with cgo off and without the race detector. It waits for review.
Model: opus-5-5
Built as the plan above says, in https://git.eeqj.de/sneak/webhooker/pulls/557: `make test` and the browser test now build and run with cgo off and without the race detector. It waits for review.
Model: opus-5-5
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.
Where it comes from. webhooker talks to SQLite through
modernc.org/sqlite, which is pure Go: every database file is opened with it (internal/database/sqlite_open.go) and the open connection is handed to gorm. But gorm's SQLite adapter,gorm.io/driver/sqlite, also importsgithub.com/mattn/go-sqlite3, which wraps SQLite's C source and needs cgo. webhooker never uses that driver, yet its C SQLite is compiled into the binary.What it costs. The image build needs a C compiler and a Debian Go image, builds the binary twice (the second time statically linked against glibc so it runs on Alpine), and carries three build tags whose only job is to make that static glibc binary safe. The shipped binary contains two SQLite engines, one of them unused.
Recommendation, being built. Build the binary with cgo off. mattn's package then compiles to a pure Go stub that is never called, the binary contains only the pure Go SQLite, and the second build and the three tags go. Your Go defaults and
REPO_POLICIES.mdalready decide this, so it is filed and queued as #545. Tests keep cgo, because Go's race detector needs it on Linux.Your choice: drop gorm as well? That would also take mattn out of
go.mod, and your defaults say "not gorm". It touches 32 non-test files acrossinternal/database,internal/deliveryandinternal/handlers: every query becomes plaindatabase/sqlwith hand-written row scanning. The schema already comes from SQL migration files, not from gorm. Recommendation: not now. #545 removes cgo with a small change, and dropping gorm would collide with the six data-model changes you approved today. If you want it, I file it to build after those land.Model: opus-5-5
no, just turn off cgo and keep gorm for now. figure out a way to turn off cgo in tests as well.
Ruling (sneak, 2026-10-16): keep gorm for now. The binary already builds with cgo off (PR 550); find a way to turn cgo off in tests as well.
Next: a webhooker session finds how the tests can run with cgo off. Tests kept cgo only because Go's race detector needed it on Linux; if the only way is to drop the race detector, that choice comes back here to sneak before anything is built. The fleet is paused at the primary account's weekly limit; this starts when it resumes on 18 October 13:00 UTC.
Done when:
make testruns with cgo off onnext, or the race-detector choice is put to sneak here.Model: opus-5-5
Finding. On Go 1.26.1, the version the repo pins, the race detector cannot run with cgo off on Linux. The
gocommand refuses-racewhenever cgo is off, on every system except macOS, and has no setting to override that: on Linux the race detector's runtime is built on the C library, so it needs cgo to link. The only way to run the tests with cgo off is to run them without the race detector. As planned above, nothing is built until you choose.Option 1, recommended: keep the race detector, and with it cgo in the tests only. Nothing changes. The shipped binary is already built with cgo off (#550), so cgo never reaches what is deployed. The cost stays in the test phase: it needs the Debian Go image for its C compiler, and the test binaries compile mattn's C SQLite, which the tests never use.
REPO_POLICIES.mdasks forgo test -racein the test phase.Option 2: drop the race detector.
make testruns with cgo off, no C compiler is needed, and the tests run faster and in less memory. The cost: a data race, two goroutines using the same memory without a lock, is no longer caught. webhooker runs its delivery workers, periodic sweeps and web handlers at the same time, which is where such bugs live. The test phase inREPO_POLICIES.mdwould change as well, since it requires-race.Why option 1: the race detector is the only check in the gate that finds those bugs, and turning cgo off in tests changes nothing that ships.
Reply 1 or 2; option 2 is one small PR.
Model: opus-5-5
has the race detector actually caught any bugs since we have been using it? seems to me using locking correctly is something we are quite capable of doing.
Short answer: almost nothing. In webhooker's history the race detector has caught one data race, and it was in test code, not in webhooker: #230 (a test logger writing after its test had finished). The real concurrency bugs were found by review and by ordinary tests, not by the race detector: a startup recovery that could deliver an event twice (#299), a reader that could wedge a webhook's database (#256) and SQLite set up for concurrent writers wrongly (#304). Its cost is real: under it,
make testneeded up to 6.7 GiB of memory (#344), and it is the only reason the tests still need cgo.So option 2 (drop the race detector, run the tests with cgo off) costs little on this record.
REPO_POLICIES.mdasks for-racein the test phase, so option 2 also notes webhooker's exception there. Reply 2 to have it built, one small PR.Model: opus-5-5
i never approved that in repo policies, so it’s ok to drop it.
Recorded: drop the race detector, so
make testruns with cgo off (option 2); the-raceline in the test phase was never your ruling. It is built as one small reviewed change onnextwhen an account has room; both worker accounts are at their week limit now (sneak/project-management#37).Model: opus-5-5
Plan (implementer's brief). Option 2, per #538 (comment). One PR on
next, title ending(closes #538).Dockerfile, test phase: bothgo testlines run withCGO_ENABLED=0and without-race; keep-cover,-p 4 -parallel 8, the 90s timeout, the tmpfs and the rerun of failed tests. The stage stays ongolang:1.26.1-bookwormonly because it is the builder stage's image (the builder needs its git and make); its comment says that, not that-raceneeds a C compiler. Rewrite every comment in the stage that speaks of-raceso it is true.Dockerfile.browser: the browser test binary is built withCGO_ENABLED=0too, so no test binary compiles mattn's C SQLite.README.md: themake testline and the test phase description in the Dockerfile section no longer mention race detection or a C compiler. In the development section, one sentence: the tests run without Go's race detector and with cgo off, an exception to the vendoredREPO_POLICIES.md, citing the ruling above.REPO_POLICIES.mditself is vendored and is not edited.-raceor the race detector (internal/server/early_shutdown_test.go,internal/archive/sweeper_test.go,internal/archive/evict_test.go,internal/logfield/logfield_test.go,internal/database/totals_test.go,internal/database/password.go,internal/middleware/loginguard.go): each one that is now false is reworded; a past measurement stated as such stays. No test is deleted or weakened.go.mod(gorm stays), any new file or package.Done when:
make testbuilds and runs the tests with cgo off and no-race, no test binary links mattn's C SQLite,make test-browserpasses, andscript/cibuildis green on a fresh clone of the branch rebased onnext.Model: opus-5-5
Built as the plan above says, in #557:
make testand the browser test now build and run with cgo off and without the race detector. It waits for review.Model: opus-5-5