fix: released binaries could not start — swap SQLite driver to pure Go - #128
fix: released binaries could not start — swap SQLite driver to pure Go#128skyoo2003 wants to merge 5 commits into
Conversation
github-actions
Bot
added
documentation
.goreleaser.yaml builds with CGO_ENABLED=0 while the store imported mattn/go-sqlite3, which is cgo-only. That combination compiles: the driver ships a stub for the no-cgo build, and the stub fails at run time. Every tagged binary from v0.2.0 onward therefore exited at startup with init s3: enable WAL: Binary was compiled with 'CGO_ENABLED=0', go-sqlite3 requires cgo to work. This is a stub so the tar.gz/zip artifacts and the Homebrew formula were dead on arrival. Only the container image worked, because docker/Dockerfile builds with CGO_ENABLED=1 against sqlite-dev. modernc.org/sqlite is a pure Go translation of SQLite and needs no toolchain per target. Its RegisterCollationUtf8 takes func(left, right string) int, the exact signature of compareNumericText, so NUMTEXT ports as-is — and because registration is process-wide, the per-connection ConnectHook goes away. The public surface of this package is unchanged, so none of its 108 importers move. IsUniqueConstraintError already matched on the message text rather than a driver error type, and the on-disk file format is SQLite's, so existing data_dir contents still open. Verified: full suite green under both CGO_ENABLED=1 and CGO_ENABLED=0; the boto3 suite green (775) against a CGO_ENABLED=0 binary, which previously could not start; all six release targets cross-compile. The compatibility suite runs about 15% slower (63s against 55s), the expected cost of transpiled C.
With the driver in pure Go, nothing in the tree needs cgo or SQLite headers. Six workflows installed libsqlite3-dev, two Dockerfiles installed gcc/musl-dev/sqlite-dev, the runtime image installed sqlite-libs, and troubleshooting told users to install headers to fix a build error they can no longer hit. All of it goes. The substantive part is CGO_ENABLED: everything now builds and tests at 0, the mode .goreleaser.yaml publishes in. Testing CGO_ENABLED=1 while shipping CGO_ENABLED=0 is how a binary that cannot open its own database passed every gate for two releases. The boto3 suite in particular now runs against a binary built exactly as the released one is, so those 775 tests are the smoke test for what users download. Verified: full suite green at CGO_ENABLED=0 (109 packages); golangci-lint clean; the linux/amd64 binary is static with no reference to libsqlite3, which is why the runtime image no longer needs sqlite-libs. The container build itself is unverified locally — no Docker daemon here — and cd.yml does not run on pull requests, so it is first exercised on merge.
27f7a60 to
e230fb5
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e230fb5311
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mattn/go-sqlite3 set PRAGMA busy_timeout to 5000ms on every connection whether or not the DSN asked for it (sqlite3.go:1201, :1623). modernc.org/sqlite applies the pragma only when the DSN carries _busy_timeout or _timeout (sqlite.go:364-365), and Open passed a bare path, so the swap dropped that 5-second retry window and left SQLite's default of no waiting at all. The window matters here. The gateway serves on net/http (gateway.go:68,97), so handlers run concurrently, and Open sets no connection limit — database/sql opens as many connections as callers ask for. Two handlers writing one service database therefore hold two SQLite connections contending for a single write lock. DynamoDB's RWMutex does not prevent this: PutItem and DeleteItem take RLock (store.go:360, :470), which guards the in-memory table map, not the write transaction beneath it. s3 metadata tags, secretsmanager, sns and cloudwatchlogs take no lock at all. Nothing in the tree retries on SQLITE_BUSY, so the loser's error surfaces to the SDK caller as a spurious "database is locked" — measured at 0.96ms to failure, against the 5s it used to wait. The setting travels in the DSN rather than as an Exec beside the journal_mode pragma because busy_timeout is per-connection state. One Exec configures whichever pooled connection answers and leaves the rest at zero: measured at 1.06ms to failure, indistinguishable from no fix. journal_mode gets away with a single Exec only because the database file records it. 5000ms restores mattn's prior behaviour exactly. The regression test holds the write lock for 200ms and asserts the second writer waits it out; it fails with SQLITE_BUSY in 0.10s against the unfixed Open. Reported in review on #128.
The go-vet and go-build hooks forced CGO_ENABLED=1. That was never a response to the cgo-only SQLite driver — both lines date from the bootstrap commit 7d3b17d, before internal/storage/sqlite existed — but with the driver now pure Go it is the last place in the tree that asks a contributor for a C toolchain. Someone without one had every commit rejected by hooks testing a build configuration the project no longer produces. CGO_ENABLED=0 matches .goreleaser.yaml, ci.yml, compat.yml, smithy-sync.yml and the Makefile, so the hooks now vet and build what the release actually contains rather than a second, untested mode. Verified: CGO_ENABLED=0 go vet ./... and go build ./... both exit 0, and no CGO_ENABLED=1 remains anywhere in the tree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b3d2c588
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ = tx.Rollback() | ||
| }() | ||
|
|
||
| start := time.Now() |
There was a problem hiding this comment.
If the new goroutine is scheduled immediately, its 200 ms sleep begins before start is recorded, so the lock can be released legitimately less than hold after this timestamp and the exact elapsed >= hold assertion fails. This makes TestOpen_SecondWriterWaitsForTheLock scheduling-dependent; record the start before launching the goroutine or allow timing slack while still proving that the second writer waited.
Useful? React with 👍 / 👎.
| // mattn/go-sqlite3 applied 5s unconditionally; modernc.org/sqlite installs | ||
| // no busy handler unless asked, so without this a second concurrent writer | ||
| // fails instantly with SQLITE_BUSY instead of waiting its turn. | ||
| db, err := sql.Open(driverName, dbPath+"?_pragma=busy_timeout(5000)") |
There was a problem hiding this comment.
When a service has an existing database at dbPath, appending ?_pragma=... to this ordinary filename changes the filename passed to SQLite rather than adding URI parameters, because SQLite only interprets the query portion for names beginning with file:. On Unix this opens a new database literally named like service.db?_pragma=busy_timeout(5000), making persisted service state appear lost after upgrading; on Windows the ? also makes the filename invalid and can prevent startup. Construct an escaped file: URI before adding the pragma, or configure the pragma without altering the filename.
Useful? React with 👍 / 👎.
1 participant
Summary
Every downloadable artifact DevCloud has published since v0.2.0 exits at startup.
.goreleaser.yaml:5-7builds withCGO_ENABLED=0whileinternal/storage/sqlite/store.goimportedmattn/go-sqlite3, which is cgo-only. That combination compiles — the driver ships a no-cgo stub — and fails at run time.Related Issue
Refs #127
Changes
Reproduction, before the fix:
cmd/devcloud/main.gotreats a failed default-service init as fatal, so it never gets pasts3.Scope. Broader than it first looks — the versioned container images are affected too, because
.goreleaser.yaml:83builds them fromDockerfile.goreleaser, which copies the GoReleaser binary:tar.gz/zipbinaries (6 targets)CGO_ENABLED=0*-alpineimagesDockerfile.goreleaser← GoReleaser binarylatestimagecd.yml→docker/Dockerfile,CGO_ENABLED=1Present since v0.2.0:
git show v0.2.0:.goreleaser.yamlhasCGO_ENABLED=0and the same tag'sgo.modhasgo-sqlite3. Introduced in #26.The fix.
modernc.org/sqliteis a pure Go translation of SQLite. ItsRegisterCollationUtf8(zName string, impl func(left, right string) int) errortakes exactly the signature of the existingcompareNumericText, so the NUMTEXT collation ports unchanged — and because registration is process-wide, the per-connectionConnectHookdisappears. The diff is one file,init()going from five lines to one.The package's public surface does not change, so none of its 108 importers move.
IsUniqueConstraintErroralready matched on message text rather than a driver error type, and the on-disk format is SQLite's, so existingdata_dircontents still open.The gate that was missing. Nothing exercised the released build configuration, which is why this survived two releases and every review round on #127. Everything now builds and tests at
CGO_ENABLED=0, so the boto3 suite runs against a binary built exactly as the released one is — those 775 tests are the smoke test.With cgo gone, six workflows no longer install
libsqlite3-dev, two Dockerfiles dropgcc/musl-dev/sqlite-dev, the runtime image dropssqlite-libs, anddocs/troubleshooting.mdloses a section telling users to install headers for a build error they can no longer hit.Test Plan
CGO_ENABLED=0binary starts, initialises every service, and answers the admin API with HTTP 200.CGO_ENABLED=0 go test ./...— 109 packages, exit 0. Same atCGO_ENABLED=1.internal/storage/sqlite/store_test.go(38-digit precision ordering) and the DynamoDB suite that consumes it viastore.go:627both pass.CGO_ENABLED=0.libsqlite3— which is why the runtime image no longer needssqlite-libs.golangci-lint run ./...— 0 issues.Not verified locally: the container build. No Docker daemon in this environment, and
cd.ymldoes not run on pull requests, sodocker/Dockerfileis first exercised on merge.Cost: the compatibility suite runs about 15% slower (63s against 55s), the expected price of transpiled C.
Checklist
golangci-lint run)