`go fix` carries the modernize analyzers now, and the tree had drifted behind
them. This is the mechanical result, reviewed rather than trusted: the tool is
capable of rewriting code into something that no longer tests or does what it
did, so every non-test change was read individually and the concurrency-bearing
packages were re-run under -race.
Production code, four changes, all semantics-preserving:
- leases/manager.go: wg.Add(1) + go + defer wg.Done() becomes wg.Go. The
comment above that function turns on Add happening before the goroutine
starts, so that a Wait cannot return before the worker has run. wg.Go does
the Add synchronously on the calling goroutine, so the invariant it
describes still holds.
- auth/token/handler.go: strings.Fields -> strings.FieldsSeq, same splitting,
iterated rather than allocated.
- hold/gc/gc.go: a hand-written map copy -> maps.Copy.
- hold/pds/scan_broadcaster.go: three-clause loop -> range over int.
The rest are tests. The one worth naming is carstore_contention_test.go, where a
careless rewrite could have quietly stopped exercising contention: go fix
converted the reader and side-table goroutines to loopWG.Go but correctly
declined to touch the writer loop, which passes its index as a parameter. The
writer/reader/side-table shape and the stop channel are unchanged, so the test
still contends over the same carstore transactions.
Verified: go build for hold and appview, `make lint` 0 issues, the deploy and
credential-helper modules 0 issues, `make test` green across all 43 packages,
and -race green on leases, hold/pds, hold/gc and auth/token. The scanner module's
two lint findings are unchanged from HEAD and are in files go fix never touched.
Kept separate from the HTTP/2 commit so that one stays readable, and so this can
be reverted on its own if a modernization turns out to matter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TA9D4DjaLZTvzQ7dJbu4eg
A PRAGMA is per-connection, and database/sql opens one connection per
concurrent caller. hold_db.go and scan_broadcaster.go each set
busy_timeout = 5000 with a one-shot query against the pool, which configures
whichever connection happened to serve it and leaves every other writer at
zero. Probing the live pools showed exactly that: conn 0 at 5000, conns 1 and 2
at 0. That is the database is locked on COMMIT, and the carstore had no
busy_timeout and no WAL on any connection at all.
Opening a database now goes through a connector that runs the PRAGMA on every
connection the pool creates, ported from the appview, which had already solved
this. Applied to the carstore, the shared hold DB, the records index, the event
broadcaster and the scan broadcaster. In-memory databases are left unwrapped,
since libsql gives each connection its own, and the embedded replica path still
skips PRAGMAs because a remote rejects them.
WAL is not a new risk: production was already WAL, because journal mode is a
persistent property of the file and OpenHoldDB has been setting it. This makes
the carstore's own opener agree rather than leaving standalone and test
databases in rollback-journal mode. A database that refuses WAL logs a warning
instead of failing boot, so a network mount cannot stop the hold starting.
NewHoldPDS's file mode opened the same file a second time for the records
index; it now shares the carstore's pool. Two pools on one file buy nothing,
since SQLite's write lock is per database, so the second only adds contenders.
Production already shared a pool and was unaffected by that half.
Note for anyone reading OpenHoldDB: its foreign_keys pragma has the same
one-connection scope, but libsql reports foreign_keys on for every fresh
connection anyway, so it is decorative rather than broken.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
Evan JarrettandClaude Opus 5