mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
xarmian
773222368c
Two deadlocks, one in the database and one in the application. THE DATABASE ONE, which is what BUG-2778 was filed about. A document rename takes row locks in two stages inside one transaction: updateLinksInTx writes every OTHER document whose content links the old title, then the final UPDATE writes THIS document. Two concurrent renames of documents that link to each other therefore take the same two locks in opposite orders, and Postgres aborts one with SQLSTATE 40P01 — a 500 on an ordinary rename. A throwaway probe against the unfixed code deadlocked on 12 of 12 rounds; this is deterministic, not theoretical. The fix serializes renames per workspace with a dedicated advisory key (`pad:document-rename:<ws>`), taken before any row lock and whenever a title is supplied. No-op on SQLite, whose single writer cannot produce the cycle. `SET LOCAL lock_timeout = '5s'` bounds the wait, because a transaction that waits with a pool connection already in hand converts contention into pool exhaustion; the handler maps 55P03 and 40P01 to a retryable 503 rather than a generic 500. WHY NOT `ORDER BY id`, which is what I proposed when I FILED this from reading rather than from a repro: each transaction's cascade set is a single row, and the cycle is cascade-then-self, so ordering the cascade leaves it exactly as reachable. Run as a mutation, that fix fails the regression test. Reproducing the bug is what refuted my own diagnosis. THE APPLICATION ONE, found in review and larger than the filed bug. Seven production paths issued a read through the connection POOL from inside an open transaction. Under a saturated pool the second connection never arrives, so the transaction cannot finish and never releases what it holds — no SQLSTATE names this and no lock timeout breaks it. All seven now take their executor: the document and item slug scans, both version checks, the done-field lookup on the item update and move paths, the open-children guard's collection read, and the OAuth startup backfill's workspace lookup. Three of those were found only after asking for the true population rather than for a sample; two were INDIRECT (through GetCollection), which a grep for `s.db` inside transaction bodies cannot see. The instrument is a one-connection pool, which makes the hazard deterministic instead of load-dependent. ALSO FIXED, adjacent and found by the same reviews: - The rename decided from a PRE-LOCK snapshot: the lock made the cascade safe against another rename and then handed it a stale OLD TITLE. It now re-reads under the lock and decides from that row. - A concurrent soft-delete could commit mid-rename, leaving the cascade's rewrites behind while the caller was told not-found. The final UPDATE now carries `deleted_at IS NULL` with a checked row count, so the rename and its cascade land together or not at all. - Class sweep (CONVE-18) of the same unordered-scan-then-per-row-update shape: the attachment remap (items and comments) and the outbox user-ref scrub are now ordered. Scoped claim — it orders those per-row updates, not every lock in those transactions — and NOT reproduced, unlike the rename. 18 mutations aimed, 15 die. The three survivors are written down where they live: one is an equivalent mutant (the cascade is the first row-locking stage, so "lock before the cascade" and "lock at the top of the transaction" are the same order), one is a guard covering a window no seam can currently schedule, and one is a class-sweep fix with no repro.
37 lines
1.5 KiB
Go
37 lines
1.5 KiB
Go
package server
|
|
|
|
import (
|
|
"errors"
|
|
"testing"
|
|
)
|
|
|
|
// BUG-2778. A rename serializes per workspace with a 5s lock timeout, so
|
|
// contention surfaces as SQLSTATE 55P03 — and a deadlock Postgres chooses to
|
|
// break surfaces as 40P01. Both mean "try again"; a generic 500 tells the
|
|
// caller the opposite.
|
|
//
|
|
// This drives the classifier directly rather than manufacturing contention
|
|
// through the HTTP stack: producing a real 55P03 needs two concurrent
|
|
// transactions and a Postgres backend, and the property under test is which
|
|
// errors are classified retryable — not whether Postgres emits them.
|
|
func TestIsRetryableLockError(t *testing.T) {
|
|
for _, tc := range []struct {
|
|
name string
|
|
err error
|
|
want bool
|
|
}{
|
|
{"lock timeout", errors.New(`ERROR: canceling statement due to lock timeout (SQLSTATE 55P03)`), true},
|
|
{"deadlock detected", errors.New(`ERROR: deadlock detected (SQLSTATE 40P01)`), true},
|
|
// Controls: a classifier that answered true for everything would pass
|
|
// the two legs above on its own.
|
|
{"unique violation", errors.New(`ERROR: duplicate key value violates unique constraint (SQLSTATE 23505)`), false},
|
|
{"invalid byte sequence", errors.New(`ERROR: invalid byte sequence for encoding "UTF8" (SQLSTATE 22021)`), false},
|
|
{"plain error", errors.New("update document: connection refused"), false},
|
|
{"nil", nil, false},
|
|
} {
|
|
if got := isRetryableLockError(tc.err); got != tc.want {
|
|
t.Errorf("%s: isRetryableLockError = %v, want %v", tc.name, got, tc.want)
|
|
}
|
|
}
|
|
}
|