Repository navigation
Keep writer lease renewal safe after lock waits, silent networks and shutdown - #502
Merged
Merged
Conversation
…shutdown Renewal was a single UPDATE whose WHERE clause checked that the lease had not expired. When that UPDATE waits for the row, PostgreSQL rechecks the clause afterwards only if the blocking transaction changed the row. A takeover that finds the lease still live takes the row FOR UPDATE and leaves it unchanged, so a renewal queued behind it kept its pre-wait check and could extend a lease that expired while it waited. Renewal now runs a transaction on its dedicated connection: it takes the row FOR NO KEY UPDATE, which still does not conflict with fenced writes' FOR KEY SHARE, and only then runs the conditional UPDATE, which checks owner, fencing and expiry against clock_timestamp() after the lock is held. The five-second statement timeout and the stall timer still bound it. The lease keeping in main.ts moves unchanged into WriterLease (apps/server/src/storage/writer-lease.ts) so its timers can be tested. Two defects are fixed there. A renewal that succeeded after shutdown had cleared the safety watchdog armed a new one, which could fire signal() after drain. It no longer arms once renewal has stopped. The startup failure path cleared only the renewal interval and watchdog, so a pending retry, or a renewal that failed during cleanup, kept retrying. That path and drain() now both call WriterLease.stop(), which clears the interval, the retry and the watchdog and waits for any renewal in flight. docs/storage.md describes both behaviors. Assistant-model: Claude Opus 5.5 (fast) Assistant-workflow: inline (fast subagents) Assistant-verification: red before fix: the new PostgreSQL test (a session holds the lease row FOR UPDATE, renewal waits, the lease expires, the lock is released) received an extended lease from the old single UPDATE; it returns undefined after the fix Assistant-verification: red before fix: on the behavior-identical WriterLease extraction, the watchdog re-armed by a renewal finishing after stop() called the lost callback (received 1, expected 0) Assistant-verification: red before fix: with stop() reduced to the old startup-failure cleanup (interval and watchdog only), the two retry tests made 4 and 10 renewal attempts instead of 1 Assistant-verification: lock mode check: taking the lease row FOR UPDATE instead of FOR NO KEY UPDATE makes the existing "renews the writer lease while a fenced commit waits" test fail Assistant-verification: PostgreSQL suite passed: 93 pass, 0 fail (apps/server/src/storage/postgres against a disposable PostgreSQL 17) Assistant-verification: bun test passed: 4869 pass, 4 skip, 0 fail (GIT_CONFIG_GLOBAL=/dev/null) Assistant-verification: bun run types passed Assistant-verification: bun run ci passed: dprint, oxlint, tokens, design contract, design record, Impeccable (no new findings) Co-authored-by: Alex Lavaee <alexlavaee@microsoft.com>
Renewal became a transaction in the previous commit: it locks the lease
row FOR NO KEY UPDATE, then runs the conditional UPDATE. If the network
dropped the server's replies just after that UPDATE ran, the renewal
rejected at its six-second stall timer, but its backend stayed idle in
transaction and kept the row lock. The stall path closed the connection
with close({ timeout: 0 }), and Bun 1.4.2 treats a zero timeout as "no
timeout": it waits for pending queries, so the socket was never closed.
The replacement connection's renewal then waited for the row until its
statement timeout, and the lease was lost. The old single autocommit
UPDATE could not leave a lock behind.
The renewal connection now asks PostgreSQL to end a renewal transaction
that is idle for two seconds (idle_in_transaction_session_timeout), and
sets a four-second lock_timeout below the five-second statement timeout.
A stalled connection is closed with a timeout of 0.001 seconds, which
really drops the socket. As a backstop, the shared pool also terminates
the abandoned session, found by an application_name unique to each
renewal connection. A recorded backend pid could already belong to an
unrelated session after a reconnect, so the name is used instead.
Bun registers a transaction.unsafe() fragment as a pending query that
nothing awaits. Force-closing the connection rejected it as an unhandled
rejection, so the renewal UPDATE now spells out its RETURNING columns.
docs/storage.md describes the abandoned-renewal handling.
Assistant-model: Claude Opus 5.5 (fast)
Assistant-workflow: inline (fast subagents)
Assistant-verification: red before fix: the new PostgreSQL test (a TCP proxy partitions the renewal connection once the server replies "UPDATE 1") could not lock the lease row FOR UPDATE NOWAIT within 4 s (freedAfter Infinity, expected < 3000)
Assistant-verification: after the fix the row is free about 2.0 s after the partition, the stalled renewal fails as unavailable, and the next renewal succeeds within 2 s
Assistant-verification: Bun close semantics: with a pending query, close({ timeout: 0 }) was still pending after 1 s; close({ timeout: 0.001 }) returned in 2 ms and rejected the query with ERR_POSTGRES_CONNECTION_CLOSED
Assistant-verification: backstop check: with idle_in_transaction_session_timeout disabled, pg_terminate_backend freed the row at the 6 s stall (6020 ms) and the next renewal succeeded
Assistant-verification: unhandled rejection reproduced in isolation: force-closing a transaction whose query uses transaction.unsafe() fails a bun test; the same query without the fragment passes
Assistant-verification: PostgreSQL suite passed: 94 pass, 0 fail (apps/server/src/storage/postgres against a disposable PostgreSQL 17)
Assistant-verification: bun test passed: 4869 pass, 4 skip, 0 fail (GIT_CONFIG_GLOBAL=/dev/null)
Assistant-verification: bun run types passed
Assistant-verification: bun run ci passed: dprint, oxlint, tokens, design contract, design record, Impeccable (no new findings)
Co-authored-by: Alex Lavaee <alexlavaee@microsoft.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
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 #490.
Follow-ups from the writer-lease fix (#486, #487):
FOR NO KEY UPDATE) and only then checks owner, fencing and expiry. A renewal that waited behind a takeover'sFOR UPDATEcan no longer extend a lease that expired during the wait. The lock mode doesn't conflict with fenced writes'FOR KEY SHARE, so renewal still never waits behind a write.idle_in_transaction_session_timeout, alock_timeoutbelow the statement timeout, and a uniqueapplication_name. A stalled connection is force-closed (close({ timeout: 0.001 });timeout: 0waits in Bun 1.4.2), and its backend is terminated by that name as a backstop, so a silent network can't leave the lease row locked.storage/writer-lease.ts. A renewal finishing after drain no longer re-arms the watchdog, and the startup-failure path clears the retry timer.Assistant-model: Claude Opus 5.5 (fast)
Assistant-workflow: inline (fast subagents)
Assistant-verification: Postgres adapter suite passed (94 pass), including a TCP-blackhole-after-UPDATE test that failed before the fix
Assistant-verification: bun test, bun run types and bun run ci passed
Assistant-verification: Sol and Luna fast review passed after repairs (no P0/P1)
Co-authored-by: Alex Lavaee alexlavaee@microsoft.com