Repository navigation
Participant join: never return a connection holding a lock to the pool - #3040
Merged
Merged
Conversation
The pid_auto trigger takes a session advisory lock per conversation and gives it back only after an INSERT that succeeds. A participant INSERT that failed after taking it (two joiners handed the same pid, an existing (zid, uid), a lock timeout) kept the lock through ROLLBACK, and addParticipant returned that connection to the pool. Every later joiner of the conversation on another connection, on any server, then waited with no timeout while holding a read-write connection. - addParticipant closes a connection that saw any error instead of pooling it. - The participant transaction sets lock_timeout to 5 s, so no join waits indefinitely on a lock held elsewhere. - A joiner that loses the pid race retries once on a new connection, so it still gets its participant. - The CSV import worker closes its batch connection: its ON CONFLICT DO NOTHING participant insert leaves the lock held for every skipped row, even when the batch commits. - Guard test server/__tests__/participation-pg/join-lock.pgtest.ts (run by scripts/test-delphi-result-reads.sh): fails on the previous code, passes now. No schema change; the trigger is untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018NVGBuYCk4EZmiz4csUv9s
joinConversation (POST /api/v3/participants, /api/v3/joinWithInvite) retried any failed join up to ten times, so a participant lock held elsewhere cost those routes ten lock timeouts, about 50 s, holding a read-write connection throughout. A lock timeout (SQLSTATE 55P03) now ends the chain: another attempt would only wait out the same timeout. Every other error is retried as before. The held-lock guard now covers both join paths, addParticipant and joinConversation; the joinConversation case fails without this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018NVGBuYCk4EZmiz4csUv9s
colinmegill
force-pushed
the
join-lock-leak
branch
from
October 12, 2026 20:25
996f725 to
8f2e3dd
Compare
Delphi Coverage Report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Observable changes
None for participants beyond no longer stalling. In detail:
lock_timeout = '5s'. Past that, Postgres raises 55P03 and the route returns its usual error. That applies to every join path: votes and comments (throughensureParticipantand the comment route), andPOST /api/v3/participantsandPOST /api/v3/joinWithInvite(throughjoinConversation).joinConversationretries a failed join up to ten times; it now stops on a lock timeout. Otherwise those two routes would have answered after about 50 s. Before this change the join waited until the lock holder's connection closed. That was 10 s of pool idle time, or never if the connection stayed busy, and the join held a read-write connection the whole time. The participant's browser gives up at 10 s either way.ensureParticipantand then either succeeded or stalled (see below).participant pid collision; retrying once. This makes the collision rate countable.No schema change. The
pid_autotrigger is untouched.The defect
pid_auto(BEFORE INSERT onparticipants) takespg_advisory_lock(873791983, zid), a session lock.pid_auto_unlock(AFTER INSERT) gives it back at the end of the INSERT statement, before COMMIT. Two joiners racing in one conversation can therefore both computeMAX(pid) + 1with the same value: the second gets the lock as soon as the first's INSERT ends, while the first's row is not yet committed. The second INSERT then fails on(zid, pid), its AFTER trigger never runs, and the session keeps the lock through ROLLBACK.addParticipantthen calledclient.release(), which returned that connection to the pool still holding the lock. After that, every later joiner of the conversation that landed on a different connection, on this server or any other, blocked insidepg_advisory_lock:Each blocked joiner holds one of the server's 12 read-write connections. A join burst at the start of a conversation can take all of them, and then every vote on that server waits too.
The fix
addParticipantcloses (release(true)) any connection that saw an error, instead of pooling it. The lock ends with the session. This covers a lost pid race, an existing(zid, uid)(the BEFORE trigger has already taken the lock then too) and a lock timeout.lock_timeout = '5s'(SET LOCAL, so pooled connections keep their defaults). This bounds every lock wait on the path: the advisory lock, theconversationsrow, and a concurrent same-user insert.joinConversation's ten-attempt retry chain stops on SQLSTATE 55P03 (lock timeout): another attempt would only wait out the same timeout again. Every other error is retried as before.flushBatchToDb) always closes its batch connection. ItsINSERT … ON CONFLICT (zid, uid) DO NOTHINGfires the BEFORE trigger for rows it then skips, and the AFTER trigger never runs for them. So the lock stays held even when the batch commits, whenever a batch re-proposes an existing participant. This was checked on Postgres 17: lock count 0 after a first batch, and still held after a second batch with two skipped rows.How the participation path stays clear
Evidence
Branch rebased onto edge
25e9ea52d(includes PR #3039). Everything below was run locally (arm64, Docker) on that tree.Guard test:
server/__tests__/participation-pg/join-lock.pgtest.ts, run byscripts/test-delphi-result-reads.sh, in the style of the result-read concurrency guard. It is generated data against a real, migrated Postgres 17 through the real join code:addParticipantand loses the race.addParticipant(votes, comments) andjoinConversation(POST /api/v3/participants,/api/v3/joinWithInvite).On the previous code (
fd33de4ef), the race and held-lock tests fail:C waited until B's leaked connection hit the pool's 10 s idle timeout. The held-lock join never answered.
With the first commit only, the
joinConversationcase still fails, because its ten-attempt retry keeps waiting:With both commits, all three pass:
Reads job.
scripts/test-delphi-result-reads.sh --ci: 6 suites and 42 tests pass, the new guard and the result-read concurrency guard included.Server jest job. Run with the "Server Integration Tests" job's steps: migrations applied and checked,
npm run contract:check,node src/db/migrations.cjs, and the job's exactnpm test -- --ci --coverage --maxWorkers=2with its environment block. The test stack's host ports were moved through a compose override, with the simulator issuer and JWKS following the moved port. Result: 123 suites passed (1 skipped), 1768 tests passed (4 skipped), includingconcurrent-same-participant,importProcessorBoundariesand the vote-path recordings.npm run lintandtsc --noEmitare clean.Other session-level advisory locks (
git grep pg_advisory_lock,pg_try_advisory_lock)pid_autoviaaddParticipant(server/src/participant.ts)pid_autovia the CSV import worker (server/src/workers/import-processor.ts)ON CONFLICTrowspid_autovia treevite acceptance (server/src/invites/treevites.ts)pg.queryPautocommit; a failed query's connection is already closed (release(err)inserver/src/db/pg-query.ts)tid_autooncomments(000000_initial.sql), same session-lock shapeserver/srcgoes throughpg.queryP, which closes a failed connectiondelphi/scripts/math_poller.pydelphi/polismath/tools/engine_rebuild.pyqueue-rs/polis-migratedelphi/polismath/poller/coordinator_bridge.pyci/probe_box/provision_login.pypg_advisory_xact_lock(transaction-scoped)delphi/tests/…includingdelphi/tests/dynamo_removal/math_proof.py,queue-rs/polis-migrate/tests/prove.py,server/postgres/migrations/down/test_000021_down.py)server/shadow/snapshot-pool.cjs(and its test)scripts/after_install.shNot in this PR
The root cause is the trigger itself: a session lock released before COMMIT. Making
pid_autotakepg_advisory_xact_lock, with no AFTER unlock, would stop duplicate pids altogether. That is a schema change and needs its own discussion.tid_autohas the same shape.The treevite participant insert (
server/src/invites/treevites.ts, autocommit throughpg.queryP) cannot leak the lock, but it has nolock_timeout. It can still wait with no limit on a lock held elsewhere, for example by a server on the previous release during a rolling deploy.🤖 Generated with Claude Code
https://claude.ai/code/session_018NVGBuYCk4EZmiz4csUv9s