Never hold transactions or contended locks across external I/O - #20
Merged
Conversation
Fixes two v0.3.0 production bugs and codifies the underlying rule as constraint C-C6 in SYNC_ENGINE.md: database transactions and shared locks must not overlap HTTP/GraphQL/REST calls or client network writes; external fetches finish first and results are materialized before client writes. The sole documented exception is C-C1's dedicated per-entity observation session (never an open transaction), now re-justified where the locks are defined, with the advisory-key acquisition-ordering rule documented beside the key definitions. Issue #18: the coordinator held the shared repository observation lock across the entire GitHub batch -- fetch, hydration, and all writes -- so concurrent batches for one repository serialized on pg_advisory_lock and died on statement_timeout, with retries amplifying contention. The repository lock is now a post-hydration, apply-only window around ApplyRepositoryObserved. Deterministic regressions with per-instance fake response gates prove a delayed batch cannot starve a concurrent one past statement_timeout, and C-C2 resolves older-response-applies-last interleavings. Lock ordering across every two-lock site was enumerated and is consistent. Issue #19: CloseContext released the observation's dedicated connection back to pgxpool even when the session-lock release failed, so a later borrower could inherit a locked session. Cleanup now runs under a bounded context.WithoutCancel window, checks pg_advisory_unlock's boolean result (false means the lock was not held -- also a destroy-and-log signal), releases to the pool only after confirmed unlock, and otherwise hijacks and closes the physical connection so backend termination frees the lock server-side -- verified by polling pg_stat_activity and pg_locks. Defer-swallowed cleanup failures now surface as bounded slog warnings. The C-C6 audit covered every transaction and held lock in the codebase; one additional violation was found and fixed: the example API streamed SSE snapshot rows while holding its repeatable-read transaction. Snapshots now preflight count/size bounds (100k entities / 64MiB, at most two concurrent materializations), commit, then write; overflow emits an explicit resync advisory instead of a partial snapshot. Also fixed en route: a graceful-shutdown race in the example API's tailer startup, and a streamclient test that measured backoff with a fixed sleep instead of polling the actual lock wait (production behavior unchanged). Fixes #18 Fixes #19 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes #18, fixes #19 — and codifies the maintainer's rule as constraint C-C6 in
SYNC_ENGINE.md, with the per-entity observation session as the sole documented exception and the advisory-lock ordering rule written down beside the key definitions.statement_timeout, with C-C2 resolving out-of-order repo metadata. Two-lock ordering enumerated across coordinator, discovery, membership resolution, drift, and sweeps.pg_advisory_unlock's result; release-to-pool only after confirmed unlock; failed/false/ambiguous cleanup hijacks and closes the physical connection (backend termination frees the lock server-side — poll-verified); boundedWithoutCancelcleanup window so routine cancellation doesn't churn connections; swallowed defer errors surfaced via bounded slog warnings.SYNC_ENGINE.md); one extra violation found and fixed — example-api streamed SSE while holding its snapshot transaction; now bounded materialize → commit → write with explicit overflow resync semantics.Built and adversarially reviewed by paired Codex sol-xhigh agents (7 findings fixed, one critical). Full gate green locally: build, vet, DB-backed suite,
-race -count=3on changed packages, lint, gen, tidy.🤖 Generated with Claude Code