Repository navigation
migrate: let only the caller's cancellation end the unknown-outcome test's lock wait - #145
Conversation
…est's lock wait TestRunAcceptBlockingReportsAnUnknownOutcomeHonestly widens lock_timeout so that cancelling the context is the only thing that can end the DROP INDEX's wait behind the held lock. statement_timeout counts the lock wait too, and it was left at the fixture's 5s default. When the pg_stat_activity poll took longer than that to see the waiting statement, the server cancelled it first and the test observed a statement-budget failure instead of the unknown outcome it asserts. Widen statement_timeout with lock_timeout. Register the pool close as a cleanup before the lock holder's cleanup rather than as a defer. The holder keeps a connection until its cleanup rolls it back; a deferred Close ran first and waited on that connection, so a failed assertion parked the test until the package deadline instead of failing it.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 1/2: adversarial correctness review of 0 blocking, 2 non-blocking. The fix is right, and it fixes the root cause rather than buying time. The test is about the caller's cancellation. A 5s statement budget gave the server a second way to end the wait, and the test never meant to allow one. Widening it states the test's intent; nothing in the system under test gets more time to be slow. I reproduced both halves of the description with a 6s sleep injected before
The test still pins the safety property. The verdict is The cleanup reorder fixes a second thing as well. On base, Non-blocking1. The flake was a real product edge, and nothing pins it now: a lock that is never granted, cut off by a shorter statement budget, is reported as a statement failure, not a refusal. This PR is right to take the test off that path: the test is about cancellation. The path itself is still reachable, though. The design says otherwise: "failure before the statement starts … remains a typed refusal with exit code 2 because nothing ran". The comment at L104-106 says "the lock was granted", and in this case it was not. This is fail-closed, since nothing executed and nothing is reported as committed, so it is not blocking. The cost is that the operator is told the work needs a bigger statement budget when the table was just busy. The smallest fix is in AB-1's validation: refuse a statement budget that is not longer than the lock budget, before a session is acquired. Then Test that fails on 99e9549// An accepted statement whose lock is never granted executes nothing,
// whichever bound ends the wait: when the statement budget is the shorter
// of the two it is still a lock that was never granted (AB-2, AB-5).
func TestRunAcceptBlockingReportsAnUngrantedLockAsARefusalUnderEitherBound(t *testing.T) {
url := testutil.StartPostgres(t)
pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
require.NoError(t, err)
t.Cleanup(pool.Close)
schema := testutil.NewSchema(t, pool)
_, err = pool.Exec(t.Context(), fmt.Sprintf(`
CREATE TABLE %[1]s.orders (id int PRIMARY KEY);
CREATE INDEX orders_id_idx ON %[1]s.orders (id)`, schema))
require.NoError(t, err)
holder, err := pool.Begin(t.Context())
require.NoError(t, err)
t.Cleanup(func() { _ = holder.Rollback(context.WithoutCancel(t.Context())) })
_, err = holder.Exec(t.Context(), fmt.Sprintf("LOCK TABLE %s.orders IN ACCESS EXCLUSIVE MODE", schema))
require.NoError(t, err)
opts := acceptBlockingOptions(schema + ".orders")
opts.Budget.Brief.LockTimeout = 10 * time.Second
opts.Budget.Brief.StatementTimeout = time.Second
v, err := migrate.Run(t.Context(), pool,
parseOne(t, fmt.Sprintf("DROP INDEX %s.orders_id_idx", schema)), opts)
require.NoError(t, err)
assert.Equal(t, verdict.OutcomeRefused, v.Outcome)
assert.Equal(t, verdict.CauseLockBudget, v.Cause)
require.NoError(t, holder.Rollback(t.Context()))
assert.True(t, indexExists(t, pool, schema, "orders_id_idx"))
}
2. The cleanup-order fix applies to many other tests, which still leak their schemas on a shared server. 71 tests call Verified
Each mutant was run against the head test. All edits were restored with
This review was generated by Claude Code (claude-opus-5-5). |
|
🤖 2/2: OSS adoption and integration ease, at For adoption, this is the kind of test fix contributors should copy. The comment explains why both budgets have to move and why the pool close has to be registered before the holder, so the next person who adds a lock-holding test can see the trap. A suite that hangs for 10 minutes and takes every other test in the package down with it is one of the fastest ways to lose an outside contributor's trust in CI. This PR turns that hang back into a red assertion. 1/2 non-blocking 2 is the same lesson applied to the rest of the suite. 1. An importer would get more from "lost before COMMIT" than from "unknown". This test pins For an orchestrator such as SchemaBot the difference matters. This review was generated by Claude Code (claude-opus-5-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Stamping with comments: 0 blocking, 3 non-blocking (see the two review comments above).
This stamp was left by Claude Code (claude-opus-5-5).
… the lock wait statement_timeout counts the lock wait, so a statement budget that is not longer than the lock budget could cancel an accepted statement whose lock was never granted. That cancellation arrives as SQLSTATE 57014 and the accepted path classifies it as a statement failure (exit 1, budget-statement-exceeded), although nothing ran and the design promises a lock-budget refusal (exit 2) for an ungranted lock. BlockingBudget.validate now requires the statement bound to be longer than the lock bound, so lock_timeout is the only bound that can end a lock wait and a statement-budget cancellation on this path always means the work had started. The pair is refused before a session is acquired, as ErrInvalidBlockingBudget, like every other invalid bound. The CLI help, AB-1, the passthrough design doc and the outcome-code table state the ordering rule. The tests that set budgets keep the statement bound the longer one, and a new integration test holds the table's lock and proves the inverted pair is refused with no verdict and no DROP.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/145, follow-up commit One of the three non-blocking findings is fixed in this PR with the reviewer's smallest fix; the two that the review itself placed outside this PR are tracked as internal follow-ups. The reviewer's test was taken in the validation form the review proposed.
Verified — no action: AB-5 upheld as the review found; the mutant table's results are unchanged by this commit, and the cleanup-order fix the review confirmed is kept. Fixes refer to the two review comments at |
morgo
left a comment
morgo
left a comment
There was a problem hiding this comment.
🤖 Automated adversarial review, posted on Morgan Tocker's behalf.
Approving. Turning a flake investigation into a product fix is the right instinct, and the round-1 finding is a real one: a 57014 on this path genuinely could mean a lock that was never granted, and that misreports a refusal as work that ran. The fix is in the right place (executor validation, before a session is acquired), the docs and AB-1 were updated together rather than left to drift, and the test for the new rule asserts the whole refusal contract — zero verdict, nothing submitted, index still standing with the table locked by someone else.
One note below is load-bearing: the ordering rule does not actually establish the invariant AB-1 now claims for it. The rest is minor.
Verified rather than assumed:
- The cleanup reordering is correct, and for the stated reason. Deferred calls in a test body run before the testing framework invokes
t.Cleanupfunctions, so the olddefer pool.Close()really did run ahead of the holder's rollback and really would park on a connection that could only come back afterwards. Registeringpool.Closeas the first cleanup puts it last in LIFO order, behind both the holder's rollback and the schema drop thattestutil.NewSchemaregisters after it — and the schema drop still has a live pool when it runs. - The widened budgets are self-consistent. 60s statement against 30s lock satisfies the new rule, and the statement-cutoff subtest's move to 200ms clears the fixture's 100ms lock budget while the ~500ms rebuild still overruns it.
- The default flag pair is not broken by the new validation.
--lock-timeoutdefaults to 3s and--statement-timeoutto 30s, so an operator who overrides neither still validates comfortably. This was my first concern — a validation added below a pair of defaults that happened to be equal would have failed every invocation — and it does not apply.
The ordering rule does not make lock_timeout the only bound that can end a lock wait
This is the claim the invariant, the doc section and the new acceptBlocking comment all now rest on:
Requiring
statement_timeout > lock_timeoutmakeslock_timeoutthe only bound that can end a lock wait, so an ungranted lock is always the lock-budget refusal
lock_timeout applies separately to each lock acquisition attempt, not to the statement's total time spent waiting. statement_timeout is cumulative from when the backend receives the statement. So the implication only holds for a statement that acquires exactly one lock.
All three admitted forms acquire more than one. DROP INDEX takes ACCESS EXCLUSIVE on the index's owning table and on the index itself; REINDEX INDEX the same; REINDEX TABLE takes the table and then each of its indexes.
Concrete failure, using the pair the new test explicitly blesses as valid (lock + 1ms):
lock_timeout = 10s,statement_timeout = 10.001s, operator runsDROP INDEX orders_id_idx.- The backend waits 9s for
ACCESS EXCLUSIVEonorders— under the 10s lock bound, so no55P03— and is granted it. - It then begins waiting for the lock on the index.
- At t = 10.001s
statement_timeoutfires. The index lock wait has been running ~1s, nowhere near its own bound. 57014arrives with a lock still ungranted.acceptBlockingclassifies itCauseStatement, and the operator getsbudget-statement-exceededand exit 1 for a statement that never started its work — the exact outcome round 1 set out to eliminate, and a violation of the exit-2 refusal contract inlock-budgeted-passthrough.md.
The requirement the invariant actually needs is statement_timeout > N × lock_timeout, where N is the number of lock acquisitions the statement makes — not knowable from the parsed statement, since REINDEX TABLE's N depends on how many indexes the table has.
To be clear about blame: this gap predates the PR, and requiring the statement bound to be the longer one strictly reduces the window. What is new is the assertion that the window is closed. AB-1, the failure-semantics section and the comment in acceptBlocking all now tell the next reader that a 57014 here is proof the lock was granted, which means nobody re-derives it.
Two ways out, ranked:
- Take the lock explicitly, first, in the engine-owned transaction.
LOCK TABLE <table> IN ACCESS EXCLUSIVE MODEas its own statement, then the DDL. A55P03on theLOCKis unambiguously a lock refusal; a57014on the DDL is unambiguously work that ran. No inference from budget ordering at all, and the acknowledgement already names exactly the table to lock. It also collapses N: once the transaction holdsACCESS EXCLUSIVEon the owning table, nothing else can hold a conflicting lock on its indexes, so the remaining acquisitions are uncontended. This makes the invariant true rather than approximately true. - Keep the ordering rule but require a real margin — some multiple of the lock bound rather than one millisecond — and state in AB-1 that it is a heuristic that narrows the window rather than a proof. Cheaper, but it leaves
REINDEX TABLEon a many-index table unbounded in N, so the margin is a guess.
If neither is worth doing now, the honest minimum is softening the three places that claim the window is closed, so the classification stays visibly approximate.
Minor
The new test rolls the holder back twice. require.NoError(t, holder.Rollback(t.Context())) runs inline, and the t.Cleanup registered above rolls back again. The cleanup discards its error so nothing fails, but the inline call is what the assertion depends on and the cleanup is what makes it safe on an early failure — a reader meeting both wonders which is load-bearing. A one-line comment, or dropping the inline rollback and asserting indexExists through a separate connection, removes the question.
ErrInvalidBlockingBudget now covers three distinct operator mistakes — a bound that is absent or sub-millisecond, one that is unrepresentable, and a pair in the wrong order. The first two are "you did not give me a usable number"; the third is "both numbers are fine but their relationship is wrong", and it is the only one whose remedy is to change the other flag. The message text distinguishes them well, and invalid-blocking-budget is documented as covering all three, so this is only worth noting if the exit-code table ever grows a consumer that keys on the code to suggest a remedy.
lock_timeout bounds each lock acquisition on its own; statement_timeout counts them all together. The admitted statements acquire more than one lock — DROP INDEX the table then the index, REINDEX INDEX the table then the index, REINDEX TABLE the table then each index — so a statement bound longer than the lock bound did not keep statement_timeout from ending a lock wait: a first wait granted late followed by a second one was cut off as 57014 and reported as budget-statement-exceeded, work that never ran. ExecuteAcceptedBlocking now takes the schema-qualified table the caller resolved from the catalog and requests its ACCESS EXCLUSIVE lock as a statement of its own before the DDL. Every way that wait can end arrives while nothing has been submitted: 55P03 and a 57014 of the request itself are the lock-budget refusal, and the caller's context ending is cancelled-by-caller rather than an unknown outcome. Holding the table's ACCESS EXCLUSIVE lock means no other session holds a conflicting lock on any of its indexes, so the DDL's own requests are granted at once and its statement budget measures work. The request recurses to partitions, which the DDL would lock in turn. A materialized view, which LOCK TABLE cannot name, is the one target the statement still locks itself; migrate passes nil for it. The statement-longer-than-lock rule stays as a consistency check — a pair in the other order could never let the lock budget apply — and the invariants, passthrough design, execution-model row, and comments stop claiming it is what keeps a lock wait from being reported as work. Tests: a REINDEX INDEX behind a writer that releases the table after a second and a reader that keeps the index locked, with budgets that fit each wait but not both, is a lock refusal at the lock bound (without the pre-lock it is a statement failure at the statement bound); a context cancelled during the table lock wait is cancelled-by-caller with the index standing, at both the executor and migrate layers; the unknown-outcome test cancels a running slow REINDEX instead of a lock wait; REINDEX TABLE on a materialized view commits without a pre-lock. The inline holder rollbacks in the remaining tests say why the cleanup's second rollback is harmless.
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/145, follow-up commit Round 2: 3 findings — 2 fixed, 1 no action. Round 1's three findings were answered in the earlier response (1 fixed, 2 tracked as internal follow-ups) and are repeated here for one consolidated view.
Source: comment 5987651487 · comment 5987652135 · review 5409771888 · review 5420614804 — reviewed at head |
…bm/cs7-pgoutput * origin/kiran01bm/cs7-slot: decode: prove the server, verify the publication's shape, type the refusals schemachange: re-derive the swapped-table proof from the catalog for the post-swap resume (#146) migrate: let only the caller's cancellation end the unknown-outcome test's lock wait (#145) applier: flush drained batches column-wise with the unique-move fallback (#149) # Conflicts: # SAFETY.md # docs/copy-and-swap-design.md # docs/invariants.md # pkg/decode/doc.go
3 participants
TestRunAcceptBlockingReportsAnUnknownOutcomeHonestlycould observe a statement-budget failure instead of the unknown outcome it asserts, and when it did, the test hung on pool close until the package deadline instead of failing. Both budgets the test relies on are now widened, and the pool is closed from a cleanup registered before the lock holder's. Review round 1 then closed the product edge the flake exposed:BlockingBudgetrequires the statement bound to be longer than the lock bound. Review round 2 closed what that rule could not: the executor now takes the acknowledged table's lock as a statement of its own before the DDL, so a statement-budget cancellation on the accepted path is never an ungranted lock wait.Why
The test holds
ACCESS EXCLUSIVEonorders, runsDROP INDEXthroughRunAcceptBlocking, waits forpg_stat_activityto show the statement waiting on the lock, then cancels the context and asserts*BlockingOutcomeUnknownError. It widenslock_timeoutto 30s so that only the caller's cancellation can end the wait.statement_timeoutalso counts the lock wait, and it was left at the fixture's 5s default. Whenever the poll took more than 5s to see the waiting statement (a loaded runner is enough), the server cancelled the statement first; the context was not yet cancelled, soacceptedBlockingStatementErrorclassified57014asBudgetError{CauseStatement}andrequire.ErrorAsfailed.That failure then became a 10-minute timeout rather than a red assertion:
defer pool.Close()ran before the holder'st.Cleanuprolled back the lock transaction, soClosewaited for a connection that would only be returned afterClosefinished. Every test inpkg/migratefor that PostgreSQL version was lost with it.What
opts.Budget.Brief.StatementTimeoutwidened alongside the existingLockTimeoutline (and kept the longer of the two); the comment states why both budgets have to be widened.defer pool.Close()→t.Cleanup(pool.Close)registered before the holder's cleanup, so cleanup LIFO order rolls back the holder first and the pool closes without waiting; a failed assertion now fails the test.Review round 1
The flake was reachable in production too: an operator running
--accept-blockingwith an explicit--statement-timeoutshorter than--lock-timeout(3s default) against a contended table getsbudget-statement-exceeded/ exit 1 for a lock that was never granted, where the design promises a lock-budget refusal / exit 2.BlockingBudget.validaterefuses a statement bound that is not longer than the lock bound (ErrInvalidBlockingBudget, before a session is acquired). Round 2 reframes this as a consistency check — a pair in the other order could never let the lock budget apply — rather than the guarantee it was first described as.--accept-blockinghelp, AB-1 indocs/invariants.md, the budgets and failure-semantics sections ofdocs/lock-budgeted-passthrough.md, and theinvalid-blocking-budgetrow indocs/execution-model.mdstate the ordering rule.TestRunAcceptBlockingRefusesAStatementBudgetThatCouldEndTheLockWait: lock 10s, statement 1s, table locked by another session →ErrInvalidBlockingBudget, zero verdict, index stands.TestBlockingBudgetValidationgains equal / shorter / one-millisecond-longer cases. The statement-cutoff subtest moves its bound from 100ms to 200ms so it stays above the fixture's 100ms lock budget while the 500ms rebuild still overruns it.Review round 2
lock_timeoutbounds each lock acquisition on its own;statement_timeoutcounts them all together. Every admitted statement acquires more than one lock in sequence —DROP INDEXthe table then the index,REINDEX INDEXthe table then the index,REINDEX TABLEthe table then each index — so the ordering rule did not close the window: a first wait granted late followed by a second one is cut off by the statement bound before the second wait's own lock bound, and the57014reads as work that ran.ExecuteAcceptedBlockingtakes the schema-qualified table the caller resolved from the catalog (pgx.Identifier) and runsLOCK TABLE <table> IN ACCESS EXCLUSIVE MODEas its own statement afterSET LOCALand before the DDL. Every way that wait can end arrives while nothing has been submitted:55P03, and a57014of the request itself (reachable only when it recurses to partitions), are the lock-budget refusal; the caller's context ending iscancelled-by-callerwith a verdict that can say nothing was submitted, not the unknown outcome. Holding the table'sACCESS EXCLUSIVElock means no other session holds a conflicting lock on any of its indexes, so the DDL's own requests are granted at once and its statement budget measures work.migrate.lockedTablereturns the table's relkind alongside its name; a materialized view, whichLOCK TABLEcannot name, is the one target passed asnil, and the statement acquires its own locks there. TheacceptBlockingcomment, AB-1, AB-2, the session / budgets / failure-semantics sections of the passthrough design, and thecancelled-by-callerrow ofdocs/execution-model.mddescribe the lock request and stop claiming the ordering rule is what keeps a lock wait from being reported as work.TestExecuteAcceptedBlockingLocksTheTableBeforeTheStatementis the reviewer's scenario:REINDEX INDEXbehind a writer that releases the table after 1s and a reader whose open transaction keeps the index locked, budgets lock 1.5s / statement 1.6s — with the pre-lock a55P03lock refusal at 1.5s, without it (run once withnil, not committed) aCauseStatementfailure at 1.6s.…ReportsACancelledLockWaitAsNothingSubmittedat both layers: cancel during theLOCK TABLEwait →ErrCancelledByCaller, codecancelled-by-caller, index stands.TestRunAcceptBlockingReportsAnUnknownOutcomeHonestlynow cancels a running slowREINDEX INDEX(the only place the unknown outcome can still arise) instead of a lock wait.REINDEX TABLEon a materialized view commits through both layers without a pre-lock. The remaining inline holder rollbacks carry a one-line comment on why the cleanup's second rollback is harmless.Decisions to veto
ACCESS EXCLUSIVEfor every admitted form, includingREINDEX, whose own table lock isSHARE. In practiceREINDEXalready blocks every new query on the table — planning opens every index of a table under a lock the index'sACCESS EXCLUSIVEconflicts with — so the stronger table lock adds nothing an operator who acknowledged the table has not accepted, and it is what makes the index requests free. Taking the statement's own mode instead would leaveREINDEX TABLEwith one lock wait per index inside the statement budget.ONLY: on a partitioned table the lock request recurses to the partitions, whichDROP INDEXof a partitioned index locks in turn anyway, and a57014of that request is classified as the lock refusal with the statement budget named as the one that cut it off.failedverdict with codecancelled-by-caller(the existing code for the caller's own cancellation), not a refusal; the detail says nothing was committed.testutil.NewPoolthat registerst.Cleanup(pool.Close); and splittingblocking-outcome-unknowninto "lost beforeCOMMIT" and a genuinely ambiguous commit failure, which changes AB-5 and is a design decision of its own.Before / after
Round 2,
REINDEX INDEXwith a writer on the table (releases at 1s) and a reader on the index, lock 1.5s / statement 1.6s:Round 1 (unchanged), slow
pg_stat_activitypoll beforecancel():Tests
PG_VERSION=16 scripts/test-flaky.sh TestExecuteAcceptedBlockingLocksTheTableBeforeTheStatement 5 ./pkg/executor/— 5/5; the same test with the pre-lock disabled fails withCauseStatementat 1.6s.PG_VERSION=16 scripts/test-flaky.sh TestExecuteAcceptedBlockingReportsACancelledLockWaitAsNothingSubmitted 5 ./pkg/executor/andPG_VERSION=16 scripts/test-flaky.sh 'TestRunAcceptBlockingReportsAnUnknownOutcomeHonestly|TestRunAcceptBlockingReportsACancelledLockWaitAsNothingSubmitted' 5 ./pkg/migrate/— 5/5 each.PG_VERSION=16 go test ./pkg/executor/ ./pkg/migrate/ ./internal/cli/ -race -count=1,SKIP_INTEGRATION=1 go test ./...,make lint— clean.