Cap the escalated landing budget below the watchdog tolerance - #135
Open
kiwidream wants to merge 1 commit into
Open
Cap the escalated landing budget below the watchdog tolerance#135kiwidream wants to merge 1 commit into
kiwidream wants to merge 1 commit into
Conversation
A landing-class ledger step cost two independent budgets of heartbeat silence, and the escalated budget could exceed what the watchdog would wait for. Both halves are fixed here (issue #125). Half A -- the ledger admission wait is no longer heartbeat-silent. PsqlShareLedger._operation_gate waited for the writer lock or read semaphore in a single blocking acquire sized by _remaining_operation_timeout, which under statement_timeout is the full constant budget. That wait is local admission, not database work: no statement has been sent, so no server-side deadline bounds it and the ledger reports nothing until the gate opens. A new operation_progress thread-local scope -- shaped exactly like its operation_timeout and statement_timeout siblings -- installs a callback plus a slice; when one is installed the acquire is served in slices and the callback fires between them. Without a hook the path is unchanged: one blocking acquire and the same LedgerOperationTimeout message, so non-block-work callers pay nothing. The caller's deadline stays authoritative, the loop always attempts the acquire at least once so an expired budget still fails where it always did, the callback never runs while this thread holds the gate, and an exception from it propagates rather than being swallowed inside a lock wait. Both statement-timeout scopes in BlockCandidateService run on the watchdog-monitored block-work owner thread, so they now install that hook (duck-typed, like the statement_timeout/operation_timeout probes beside them) with _record_block_submitter_wait as the callback and the already-watchdog-clamped _block_work_wait_slice as the slice. Ledgers predating the hook keep working unchanged. Half B -- the landing cap is derived from the configured watchdog. DEFAULT_BLOCK_LANDING_DB_TIMEOUT_MAX_SECONDS (120s) and the watchdog default (120s) were independent literals, so an escalated landing budget could legally outrun the tolerance. _block_landing_db_timeout now clamps both the base and the escalation cap to watchdog_timeout_seconds * BLOCK_LANDING_DB_TIMEOUT_WATCHDOG_FRACTION (0.5), read from the configured coordinator rather than a second hardcoded number, and DEFAULT_PRISM_WATCHDOG_TIMEOUT_SECONDS replaces the literal in the env default so the two cannot drift apart. Arithmetic. Before: admission wait (up to 120s) plus a fresh server-side statement (120s) gave 240s of silence against a 120s watchdog; the watchdog hard-exited mid-landing, the in-memory escalation counts died with the process, and the restart replayed the same doomed attempt from the base budget. After: the silent span of one landing step is a single statement, and at the 120s default the ceiling is 60s with escalation running 30s -> 60s -> 60s. At the 300s production tolerance the ceiling is 150s, so the reviewed 120s cap is granted in full and deployments using that override see no behavior change. Tests. New coverage pins the escalated budget inside half the configured watchdog (and pins the 300s override leaving the 120s cap intact), the sliced admission wait stamping progress while blocked and still raising at the caller's deadline, the unhooked gate path staying a single acquire, hook validation/nesting/propagation, and both scopes installing the hook while a ledger without operation_progress still works. Two existing tests that assert budgets above half the default watchdog now set watchdog_timeout_seconds explicitly rather than weakening their assertions. Refs #125 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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 #125.
A landing-class ledger step could stay heartbeat-silent for roughly twice the escalated landing budget, and that escalated budget was allowed to reach the watchdog tolerance itself. At the default tolerance the watchdog hard-exited the coordinator mid-landing; because the escalation counter lives only in memory, the restart dropped back to the base budget and replayed the same doomed attempt, deferring found-block settlement while the pool restart-looped. Accounting was never at risk — the outbox row stays pending and replays idempotently — but settlement was.
The arithmetic
One landing step costs a ledger admission wait (winning the writer lock or read semaphore) plus the SQL statement. Under
statement_timeout,_remaining_operation_timeoutreturns the constant statement budget, so the admission wait got the full landing budget — and then the statement got a fresh one.watchdog x 0.5)Escalation now runs 30s -> 60s -> 60s at the default tolerance instead of 30s -> 60s -> 120s.
Production override. Production runs
PRISM_WATCHDOG_TIMEOUT_SECONDS=300, which widened the window but did not close it: the admission wait was unbounded by any heartbeat regardless of tolerance. At 300s the derived ceiling is 150s, so the reviewed 120s cap is still granted in full — deployments using that override see no change to the escalation ladder, only the newly non-silent admission wait.The change
Both halves called for in the issue:
Half A — the admission wait is no longer heartbeat-silent.
PsqlShareLedger._operation_gatewaited in a single blockingacquire(). That wait is local admission, not database work: no statement has been sent, so no server-side deadline bounds it and the ledger reports nothing until the gate opens. A newoperation_progressthread-local scope — shaped exactly like itsoperation_timeout/statement_timeoutsiblings — installs a callback plus a slice; when one is installed the acquire is served in slices and the callback fires between them.With no hook installed the path is byte-for-byte what it was: one blocking acquire, same
LedgerOperationTimeoutmessage. So non-block-work callers pay nothing. The caller's deadline stays authoritative, the loop always attempts the acquire at least once (an expired budget still fails exactly where it did), the callback never runs while the thread holds the gate, and an exception from it propagates rather than being swallowed inside a lock wait.Both statement-timeout scopes in
BlockCandidateServicerun on the watchdog-monitored block-work owner thread, so they install that hook — duck-typed, like thestatement_timeout/operation_timeoutprobes beside them — using_record_block_submitter_waitas the callback and the already-watchdog-clamped_block_work_wait_sliceas the slice. Ledgers predating the hook keep working unchanged.Half B — the cap is derived from the configured watchdog, not hardcoded.
DEFAULT_BLOCK_LANDING_DB_TIMEOUT_MAX_SECONDS(120s) and the watchdog default (120s) were independent literals that could drift._block_landing_db_timeoutnow clamps both the base and the escalation cap towatchdog_timeout_seconds * BLOCK_LANDING_DB_TIMEOUT_WATCHDOG_FRACTION(0.5), read from the configured coordinator. The reviewed cap becomes an upper bound that the derived ceiling can only lower.DEFAULT_PRISM_WATCHDOG_TIMEOUT_SECONDSalso replaces the bare literal in the env default so there is one source for the tolerance.The issue offered persisting per-hash escalation state in the outbox row as an alternative to capping. Capping is taken here: once the budget cannot outrun the tolerance, the restart-loses-escalation path stops being reachable, so no new persisted state is needed.
Tests
New coverage, all confirmed failing on
2.x.xbefore the change:test_escalated_landing_budget_stays_inside_watchdog_tolerance— fails on base withAssertionError: 120.0 not less than or equal to 60.0, the exact arithmetic above. Also pins the 300s override still granting the full 120s cap.test_landing_scope_stamps_progress_during_ledger_admission— fails on base withAssertionError: 0 != 2(no hook installed).share_ledgertests covering the sliced wait stamping progress while blocked and still raising at the caller's deadline, the unhooked path staying a single acquire, and hook validation / nesting / exception propagation — all fail on base withAttributeError: 'PsqlShareLedger' object has no attribute 'operation_progress'.test_landing_scope_accepts_ledgers_without_a_progress_hook— pins the duck-typed fallback.Two existing tests assert budgets above half the default tolerance; both now set
watchdog_timeout_secondsexplicitly rather than weakening their assertions, so the escalation ladder and the per-hash reset are still asserted.Verification
tests.test_prism_block_candidatestests.test_prism_share_ledgertests.test_prism_background_servicesunittest discover -s testsos.splicefailures intest_prism_job_builder(pass on Linux)python -m compileall docker lab tests examples scriptsNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.