Bound PRISM writer-lease acquisition and guard coordinator Postgres sessions - #137
Conversation
A coordinator client that vanishes without an RST (partition, SIGSTOP, VM pause) leaves its backend idle-in-transaction, still holding the qbit_ledger_writer_lease row lock taken by the landing CTE. The successor's startup lease upsert ran with no statement or lock timeout, so it blocked inside PsqlShareLedger.__init__ - before the watchdog arms - until TCP keepalive teardown, a multi-hour full-pool outage (issue #123). Settlement correctness was never at risk; the outbox row stays pending and replays. Two independent halves: - Every coordinator Postgres session (pooled native client, dedicated lease-guard session, psql subprocess backend) now carries session guards: idle_in_transaction_session_timeout reaps the orphaned transaction, and the server-side tcp_keepalives_* GUCs bound dead-socket detection to 30 + 3x10 = 60s. DSN/PGOPTIONS values the operator set are merged, with the coordinator fragments last so they win. - The startup lease upsert and adoption CAS run through _run_lease_acquisition_json: each attempt under the existing statement_timeout machinery (SET LOCAL statement_timeout/lock_timeout, or PGOPTIONS plus subprocess timeout), a diagnostic per timed-out attempt, and a RuntimeError naming the locked lease row after the retry budget. 5 attempts x 5s outlasts the 15s idle-in-transaction timeout, so the orphan case self-heals; only a genuinely stuck lock exits the process for the supervisor to restart. The outer _ensure_writer_lease TTL wait is unchanged - waiting out a live holder is intended failover. Six tunables (PRISM_POSTGRES_IDLE_IN_TRANSACTION_TIMEOUT_SECONDS, PRISM_POSTGRES_TCP_KEEPALIVES_{IDLE_SECONDS,INTERVAL_SECONDS,COUNT}, PRISM_LEDGER_LEASE_ACQUIRE_{LOCK_TIMEOUT_SECONDS,ATTEMPTS}) flow through LedgerConfig, the coordinator wiring, .env.example, and compose.yaml, with fail-closed validation in both load_coordinator_config and PsqlShareLedger.__init__ for the direct-construction daemons. Regression tests cover the guard options on all three connection paths, conninfo option merging, the bounded startup deadline, the bounded retry with visible failure, recovery on a later attempt, and config defaults/validation; each was observed failing against the unmodified tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The fatal path in _run_lease_acquisition_json diagnosed every LedgerOperationTimeout as a lock conflict on the qbit_ledger_writer_lease row. A connect timeout, an exhausted pool slot, a psql subprocess timeout, and a healthy-but-overloaded server all raise the same exception, so an unreachable Postgres was reported to the operator as a stuck transaction. The RuntimeError was also raised without `from`, discarding the one exception that said what actually failed. Both the per-attempt line and the fatal error now name the lock conflict as the most common cause without asserting it, quote the underlying exception text, and the fatal error chains the originating timeout. Two mutations of the acquisition bound survived the suite: - Reverting _try_adopt_writer_lease to a bare _run_json left all eight tests green. Two new tests drive the real _ensure_writer_lease adoption branch — fast-adoption capable, a heartbeat-prefixed predecessor silent past both the row and guard edges — and assert the CAS runs under a bounded deadline and that a persistently blocked CAS stops after DEFAULT_LEASE_ACQUIRE_ATTEMPTS with a chained RuntimeError. - Widening `except LedgerOperationTimeout` also left the suite green. A new test asserts a non-timeout failure (the RuntimeError run_json raises for a wrapped OperationalError, and an IdleInTransactionSessionTimeout — sqlstate 25P03, an InternalError rather than an OperationalError) propagates unchanged on the first attempt, with no retry and no diagnostic printed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Several comments from the bounded-acquisition change claimed more than the code provides. The PostgresSessionGuards docstring said no session the coordinator opens can become an unreapable lock holder; tcp_keepalives_* are silently ignored on Unix-socket connections and on platforms that do not implement them, and session settings reach only this coordinator's own sessions — a foreign holder is bounded solely by the acquisition deadline. The attempt-budget comments said the ~25s budget "exceeds" the 15s idle_in_transaction_session_timeout so the orphan is reaped mid-budget; that timer runs on the blocking backend's clock from when its transaction went idle, not from when the successor starts retrying, so the budget is sized to outlast a typical reap rather than guaranteed to contain one. Also documents the deployment caveats in the ops guide: native connections now always set the libpq `options` parameter, so a pooler that rejects startup options fails visibly at connect (the default compose topology connects directly and is unaffected); the psql backend carries the guards in PGOPTIONS, which a wrapper command that drops its environment silently discards; and Unix-socket DSNs ignore the keepalive GUCs, leaving the idle-in-transaction timeout as the guard that still applies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review round: five findings appliedAn independent adversarial review returned ship with changes. All five findings are addressed 1 (major) — the fatal path misdiagnosed the cause and discarded it. 2 (test gap) — the adoption CAS bound was unpinned. Reverting 3 (test gap) — retry exception classification was unpinned. Widening the 4 (minor) — overstated comments. Seven comments softened. The two that mattered: the 5 (minor) — deployment caveats now documented in Verification of this roundBoth mutations from findings 2 and 3 were re-applied locally and confirmed to fail the new tests
Unchanged from the original round: the six tunables, their defaults, and the design. Findings 1-5 |
…cquisition # Conflicts: # lab/prism/share_ledger.py
The harness suites encode #123 as an unfixed defect; this branch fixes it, so fourteen of their assertions had to be answered rather than muted. The orphan-lock bound is now proven. OrphanedLeaseLockFailoverBoundTests was authored against the fixed behaviour with an expectedFailure marker and a docstring demanding its removal by the fixing commit; it passes, so the marker is gone and the assertion is untouched. Its two siblings encoded "the bug is present": the successor now parks under the configured acquisition deadline instead of forever, so the wait is asserted as a positive bound tied to the shipped tunable, and the "does not end" test now pins how it ends -- the exact retry budget, the elapsed time that budget implies, and a RuntimeError chaining the underlying LedgerOperationTimeout. The orphan and the successor's park on it are kept as evidence throughout. FakeSqlBackend needed a fidelity fix first, and it is the substantive one. Both shipped LedgerSqlPort implementations translate a cancellation caused by the caller's own deadline into LedgerOperationTimeout, and only when that deadline was armed; the model did this for pool-slot exhaustion but not for the tuple-lock wait, because before this branch no deadline ever reached a pooled lease statement. Without the translation the model raised a bare LockTimeout straight through the retry loop, so the suites appeared to exercise a bounded retry that in fact ran once. Arming the acquisition deadline also makes the acquire transaction-scoped, which is a real widening: a coordinator that vanishes mid-acquire can now orphan the lease lock on a path that previously could not produce one. The model-fidelity test that used the acquire as its example of an autocommit statement drives an unscoped renewal instead, the new shape is pinned by a test of its own, and docs/prism-ledger-ops.md records the trade -- the new orphan is bounded by idle_in_transaction_session_timeout, which is why that guard is not optional. This harness models no reaper, so that test pins only what the model supports and says where the guard itself is pinned. Remaining changes are exact-trace updates: the acquire and adoption CAS now commit explicitly, so each offers a precommit stop. Traces were replaced with the observed ones rather than loosened to prefix or subset matches, since exact order is what they exist to prove. The lost-adoption-CAS driver needed one extra step to reach the same interleaving, the successor's observation no longer committing itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
|
…rdiff) (#141) * Land PRISM core fixups from the PRs-134-139 stability review F1: _acquire_operation_gate's slicing path now reads the injected self._monotonic clock at all three deadline sites, so a virtual test clock can drive admission-wait slicing deterministically; covered by a new virtual-clock test. F2: load_coordinator_config rejects PRISM_WATCHDOG_TIMEOUT_SECONDS below 1.0 at startup, keeping the derived landing-budget ceiling (half the tolerance) meaningfully below the watchdog tolerance. F3: the sliced-admission liveness test drops its per-gap 0.3s wall clock bound, which false-reds on loaded CI hosts; the determinism intent (stamp count, total elapsed, raised timeout) is unchanged and cadence is pinned by the new virtual-clock test instead. F4: the concurrency harness baton timeout is overridable via PRISM_HARNESS_BATON_TIMEOUT_SECONDS (default unchanged at 20.0); it remains a false-red failure detector and never influences ordering. F5: startup warns on stderr when the idle-in-transaction timeout exceeds the writer-lease acquire budget (attempts x per-attempt lock deadline), where an orphaned backend holding the lease row lock can no longer be reaped mid-budget and startup crash-loops instead of self-healing. F6: the superseded-envelope restore authority proof now runs after the payout balance lock and the publication order guard are acquired, so a writer deposed during the unbounded lock wait no longer restores on stale authority; the withheld-authority diagnostic is deferred until the locks are released. A new test deposes the writer inside the publication guard and fails against the old check-then-act shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Replace the racy socket-only prism-postgres healthcheck with a TCP-gated probe plus SELECT 1 confirm The postgres image's initdb temporary server accepts unix-socket connections and then shuts down, so a socket-only pg_isready can release dependents against the throwaway server (the race PR #134 fixed in the test scripts). Gate on the real TCP listener first, then confirm with a real query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Wire the #137 lease/session-guard env knobs into the ctv broadcaster and backfill scripts Both operator-run scripts construct PsqlShareLedger directly and got the class defaults for lease_acquire_lock_timeout_seconds, lease_acquire_attempts, and the four Postgres session-guard knobs, so operator tuning of PRISM_LEDGER_LEASE_ACQUIRE_* / PRISM_POSTGRES_* never reached them. Mirror the coordinator: read the same six env vars with the coordinator_config helpers and defaults and pass them through. The backfill script exposes them as CLI flags with env defaults, matching its existing --lease-ttl-seconds pattern, with construction extracted into ledger_from_args so the path is testable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document that vardiff retention is process-local and can strand a stuck-high resume The SessionDifficultyStore is in-memory: a coordinator restart, including the watchdog's terminal exit_process(1), empties it, so the post-restart reconnect wave is not smoothed by retention. And with the idle sweep off (the deployed default) a rig that converged high and reconnects within TTL on weaker hardware can resume too high with no share signal to bring it down, bounded by the resume TTL. Honest-disclosure comments and .env.example notes only; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Fixes #123.
Failure mode
Deadline-scoped ledger statements run inside an explicit
conn.transaction()in_NativePostgresClient.run_json, soCOMMITis a separate client message rather than animplicit autocommit. A client that vanishes without sending RST — network partition,
SIGSTOP,VM pause — leaves the Postgres backend idle in transaction, still holding the
qbit_ledger_writer_leaserow lock its landing CTE took.The successor coordinator's startup lease upsert (
_try_acquire_writer_lease, reached from_ensure_writer_lease) ran with no statement or lock timeout. It therefore blocked insidePsqlShareLedger.__init__— before the coordinator's own watchdog arms, so nothing broke thewait — until kernel TCP keepalive teardown. At OS defaults that is hours.
idle_in_transaction_session_timeoutwas set nowhere in the repository (confirmed by grep onthe unmodified tree; the only
lock_timeoutuses were statement-scoped).Result: a multi-hour full-pool availability outage. Settlement correctness was never at risk —
the outbox row stays pending and replays — so this is purely availability.
Fix
Two independent layers, either of which alone bounds the outage.
1. Coordinator sessions release orphaned transactions on their own. A new frozen
PostgresSessionGuards, built once inPsqlShareLedger.__init__, is carried by all threeconnection paths — the pooled native client, the dedicated lease-guard session, and the psql
subprocess backend:
idle_in_transaction_session_timeoutmakes the server abort a transaction whose clientvanished between statements and release its locks.
tcp_keepalives_idle/tcp_keepalives_interval/tcp_keepalives_countGUCs (all
PGC_USERSET, settable per session) bound how long the server keeps a dead socketalive — the backstop for backends the idle-in-transaction timer cannot cover. With the
defaults, worst-case detection is 30 + 3×10 = 60s instead of roughly two hours.
Options an operator embedded in the DSN or in
PGOPTIONSare merged, not clobbered, with thecoordinator's fragments last so a DSN-level default cannot silently disarm the guards. The psql
backend previously built
PGOPTIONSonly when a deadline was armed; it now always sets theguards, since an orphaned transaction is precisely what happens when no deadline-scoped work is
running.
2. Startup lease acquisition is bounded and fails visibly. The startup upsert and the
adoption CAS both go through a new
_run_lease_acquisition_json, which reuses the existingstatement_timeoutmachinery —SET LOCAL statement_timeoutplusSET LOCAL lock_timeoutonthe native backend, the
PGOPTIONSequivalents plus a subprocess timeout on psql — for abounded retry. Each timed-out attempt logs the lock-blocked lease row and the attempt number.
After the budget, construction raises a
RuntimeErrornaming the locked row;__init__'sexisting close-and-reraise turns that into a visible process exit the supervisor can restart,
instead of a silent hang.
The defaults are chosen so the ordinary case self-heals without operator action: 5 attempts ×
5s (~25s) deliberately outlasts the 15s idle-in-transaction timeout, so the server reaps the
orphan partway through the retry budget and a later attempt lands the lease. Only a lock still
held after every attempt reaches the fatal error.
The outer
_ensure_writer_leasewait is deliberately not bounded — waiting out a liveholder's lease TTL is intended failover behaviour. Only the per-statement lock wait is bounded.
New tunables
All six follow the existing
coordinator_config.pyconventions —DEFAULT_*constants sharedfrom
share_ledger.py,env_positive_float/env_positive_inthelpers whose fail-closedvalidation rejects empty, non-numeric, non-finite, zero, and negative values with
SystemExit—and are documented in
.env.example, passed through incompose.yaml, and described indocs/prism-ledger-ops.md.PRISM_POSTGRES_IDLE_IN_TRANSACTION_TIMEOUT_SECONDS15PRISM_POSTGRES_TCP_KEEPALIVES_IDLE_SECONDS30PRISM_POSTGRES_TCP_KEEPALIVES_INTERVAL_SECONDS10PRISM_POSTGRES_TCP_KEEPALIVES_COUNT3PRISM_LEDGER_LEASE_ACQUIRE_LOCK_TIMEOUT_SECONDS5PRISM_LEDGER_LEASE_ACQUIRE_ATTEMPTS5PsqlShareLedger.__init__validates the same six withValueError, becauserun_ctv_broadcaster_daemonandbackfill_ctv_fanoutsconstruct the ledger directly and bypassload_coordinator_config; neither may disarm a guard with a zero or negative value.Verification
Confirmed failing first. The new tests were applied to the unmodified
2.x.xtree(
4d3d262) withlab/left pristine, and run. The core regression test —test_startup_lease_upsert_runs_under_bounded_deadline— failed withAssertionError: unexpectedly None: the deadline in effect at the startup upsert wasNone,which is the defect observed directly.
test_lock_blocked_acquisition_fails_visibly_after_bounded_retrieserrored withLedgerOperationTimeoutescaping construction on the first attempt (no retry, no visibleRuntimeError). The config tests failed withSystemExit not raisedand missingLedgerConfigattributes. The guard-option tests failed on the absent
PostgresSessionGuards.Ran and passing:
tests.test_prism_share_ledger,tests.test_prism_coordinator_config,tests.test_prism_coordinator_config_loading,tests.test_prism_compose_profile,tests.test_check_env_production_gate— 356 tests, OK.share_ledger/coordinator_config/prism_coordinator(45 modules) — 1438 tests, OK.
tests.test_prism_share_ledgeragain under a realpsycopg3.3.4 install — 177 tests, OK.This is the CI job that exercises the real
psycopg.conninfooption-merge path.tests.test_mainnet_compose_contract— 23 tests, OK (compose.yamlis touched).test-prism-postgres-ledgeragainst real PostgreSQL 16 in Docker — PASS, including the A1gate. This exercises the psql-subprocess path with the always-on guards live against a real
server.
test-prism-postgres-native-ledgeragainst real PostgreSQL 16 — PASS. Validates the mergedoptionsstring and theconninfo_to_dictmerge against real psycopg and a real server.test-prism-postgres-scaleagainst real PostgreSQL 16 — PASS.python -m compileall -q docker lab tests examples scripts— OK.bash -nover all tracked shell scripts, and ShellCheck at--severity=warningin Docker —clean (no shell files are touched).
Not verified.
scripts/prism-self-check.pyreports FAIL forqbit.rpc,postgres.ready,stratum.tcp,coordinator.healthz, andaudit.writable— every one because no qbitd,Postgres service, or coordinator container is running here, not because of this change. It was
not run against a live stack. The live regtest/stratum suites and the Rust workspace tests were
not run.
Unrelated and pre-existing: five
os.splice/ spool tests intests/test_prism_job_builder.pyfail on macOS and pass on Linux.
Deployment note
The libpq
optionsconnect parameter is now always set on native connections. A deployment thatroutes
PRISM_DATABASE_URLthrough an option-rejecting connection pooler (older PgBouncerbuilds reject startup options) would fail at connect — visibly at startup rather than silently,
but it is a behaviour change for such setups. The default compose topology connects directly to
prism-postgresand is unaffected.The guards only cover sessions this coordinator opens. A foreign session holding the lease row
is bounded only by the second layer's visible
RuntimeError, which is the intended design.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.