mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
xarmian
7e56b20d0c
* fix(store): eliminate spurious SQLITE_BUSY on concurrent writes
`pad item update --comment ...` (and any concurrent write workload)
intermittently failed with `internal error` and a server log line of
`update item: database is locked (5) (SQLITE_BUSY)`. The skill's CLI
reference even documented a workaround — "use a separate `pad item
comment` call rather than --comment on update" — but that just lowered
the contention probability; both call paths hit the same root cause.
Root cause
Go's default `db.Begin()` issues `BEGIN DEFERRED` on SQLite, which
takes only a SHARED lock at BEGIN time. The first INSERT/UPDATE in
the transaction tries to upgrade to a write lock — and SQLite refuses
that upgrade with SQLITE_BUSY *immediately* if any other connection
already holds the write lock. busy_timeout's wait-and-retry behavior
does NOT apply on lock-upgrade because waiting would risk deadlock
between two connections both holding SHARED locks. Net effect: under
even modest write concurrency, transactions fail in milliseconds
instead of waiting out the 5-second busy_timeout we configured.
Repro before the fix: 20 concurrent CreateItem calls produced ~4
SQLITE_BUSY errors. Under the running server, two PATCHes within a
few ms of each other (e.g. status update + activity-log write) hit
this regularly during workflow tooling like /ship-tasks.
Fix
Set `_txlock=immediate` in the DSN. Every `db.Begin()` now issues
`BEGIN IMMEDIATE`, acquiring the write lock up-front. Lock-acquisition
DOES honor busy_timeout, so concurrent writers wait up to 5 seconds
to serialize cleanly instead of failing fast. Reads are unaffected:
single-statement SELECTs don't open a transaction at the SQL layer.
Also fold `foreign_keys=on` into the DSN's `_pragma` list. FK
enforcement is per-connection in SQLite, so the previous
`db.Exec("PRAGMA foreign_keys=ON")` only configured the one
connection that received the call — every OTHER pool member ran
without FK enforcement. The DSN form applies it to every connection
the driver opens.
`journal_mode=WAL` stays as a `db.Exec` call because WAL is a
database-level setting recorded in the file header; it persists
across connections after the first one applies it.
Validation
- Reproduced the failure under the live binary: 20 concurrent PATCHes
in a tight loop produced 4 SQLITE_BUSY errors. After this change,
same workload: 0 errors.
- New regression test `TestSQLiteConcurrentWritersNoBusy` does 20
concurrent CreateItem calls and asserts zero errors. Skipped under
PAD_TEST_POSTGRES_URL (postgres has different concurrency model).
- Existing `TestConcurrentWritePerformance` benchmark now reports 0
errors at every concurrency level it tests (1, 5, 10, 25, 50
workers). Previously this benchmark was acknowledging non-zero
errors at high concurrency as expected.
- Full test suite green: go test ./... — all 14 packages pass.
* fix(store): document IMMEDIATE tradeoff + tighten regression test (Codex round 1)
Address all three findings from Codex review of #239:
MEDIUM — IMMEDIATE widens the writer critical section because update
flows now hold the write lock during diff/version-throttle reads and
slug-collision checks, not just the final UPDATE. Document this
tradeoff explicitly in the DSN comment block: the pre-fix behaviour
was "fail fast with BUSY" and the post-fix behaviour is "wait briefly
for cleanly serialized work" — strictly better. If a future hot path
produces pathologically long write transactions (>100ms holding the
lock), the right move is to narrow that specific transaction, not to
revert this fix.
LOW — Foreign-key enforcement was previously per-connection, applied
to only one pool member. Latent integrity violations in databases
written through other pool members (with FKs disabled) may now
surface as errors on the next write. Document the rollout note and
mention `PRAGMA foreign_key_check` as the diagnostic.
LOW — Tighten TestSQLiteConcurrentWritersNoBusy: the original 20×1
form gave goroutines no synchronization, so a slow CI runner could
sequentialize the work and let a regression slip through. New form
uses an explicit start gate (sync.WaitGroup acting as a barrier) so
all goroutines try to write at the same moment, plus 25 workers ×
5 ops each (125 total) so each goroutine produces several BEGIN/
COMMIT cycles. Still passes; significantly less prone to false
negatives on CI.
No code-path change beyond test tightening; the comment additions
are docstring-only.
* fix(store): use true barrier in concurrency test (Codex round 2)
Codex round 2 caught that the previous start-gate pattern wasn't a
real barrier:
var startGate sync.WaitGroup
startGate.Add(1)
for ... { go func() { startGate.Wait(); ... }() }
startGate.Done() // <-- fires before all goroutines reach Wait()
`startGate.Done()` runs as soon as the launch loop finishes, with no
guarantee the scheduler has actually run the goroutines yet. Late-
scheduled goroutines reach `startGate.Wait()` after Done() has already
fired and proceed without ever parking — so on a slow CI runner with
goroutine startup spread across tens of milliseconds, the contention
window we wanted to create simply doesn't exist, and a regressed
deferred-transaction build could quietly pass.
Switch to the standard two-WaitGroup barrier: every worker signals
"ready" via `ready.Done()` and parks on `release.Wait()`, the main
thread `ready.Wait()`s for all workers to confirm they're parked,
then `release.Done()`s to fire them all simultaneously. This
guarantees every goroutine reaches BEGIN IMMEDIATE inside the same
narrow contention window regardless of scheduler latency.
Confirmed: `go test -count=20 -run TestSQLiteConcurrentWritersNoBusy
./internal/store` — all 20 invocations green.
* docs(store): be honest about barrier imprecision + add empirical proof (Codex round 2)
Codex round 2 noted the two-WaitGroup pattern still has a small
unobservable gap between ready.Done() and release.Wait() in each
worker. That's technically correct — the barrier isn't mathematically
exact, and a worker descheduled in that gap could miss the simultaneous
release. The previous comment overstated the guarantee by calling it
a "TRUE barrier".
Soften the comment to acknowledge the gap honestly, AND back the test
with empirical proof: with `_txlock=immediate` removed from the DSN
this test reliably FAILS (22/125 errors per run, all SQLITE_BUSY).
With the fix in place, 20 consecutive `go test -count=20` invocations
all pass. So the small theoretical imprecision in the barrier doesn't
impair the test's regression-catching ability — the multiple-ops-per-
worker structure means even slightly-late workers still produce enough
concurrent BEGIN IMMEDIATE attempts to exercise the race.
Documentation-only commit. No code change.
* docs(store): comment-consistency cleanups (Codex round 3)
Two LOW findings, both pure doc:
1. Inline comment on `ready.Wait()` was still asserting "every worker
is parked on release.Wait()", contradicting the softened block
comment above. Change to "every worker has called ready.Done()
(best-effort gate)".
2. Block comment hardcoded "22 errors out of 125 ops per run" as
though it were a standing expectation. The exact rate is host-
and scheduler-dependent; reword as a representative observation
("a representative run on a developer laptop produced ~22 errors
...; the exact rate is host- and scheduler-dependent but
consistently >0").
No code change.