fix(selector): expire token locks by the consuming transaction - #2244
fix(selector): expire token locks by the consuming transaction#2244HayimShaul wants to merge 6 commits into
Conversation
The SQLite lease cleanup correlated token_locks to token_requests on tl.tx_id = tr.tx_id, the transaction that created the locked token, while Postgres correlated on tl.consumer_tx_id, the transaction that is trying to spend it. Only the latter says anything about whether a lock is still live, so on SQLite a lock held by a Deleted or Orphan consumer leaked until the lease aged out, and a lock held by an in-flight consumer was dropped as soon as the producing transaction died. The SQLite delete was also scoped as tx_id IN (SELECT tl.tx_id ...) while the primary key is (tx_id, idx), so one expired lock evicted the live locks on every other output of the same transaction. Move the cleanup statement into the shared store as IsStaleLock, correlated on consumer_tx_id and scoped per row, and let both backends use it: SQLite inherits it, Postgres keeps only the logging of the rows about to go. The generated Postgres SQL is unchanged. Replace the SQLite tests that asserted the generated SQL string - and so encoded the wrong join as the expected output - with behavioural cases in the shared dbtest suite, which runs on SQLite, the in-memory backend and Postgres: a lock of a Deleted or Orphan consumer is released, a lock whose producing transaction died is kept, sibling indices survive, an aged lease is reclaimed, and a fresh pending lock is left alone. Four of them fail against the previous SQLite implementation. Fixes #2018 Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
3918ec1 to
00c2023
Compare
AkramBitar
left a comment
There was a problem hiding this comment.
On SQLite, lock cleanup checked the status of the transaction that created the token instead of the one spending it — so when a spend died, its lock was never released and the token stayed unspendable.
In general the fix is OK. I verified it independently: the generated SQLite DELETE now correlates on consumer_tx_id per row and is valid SQL; the generated Postgres SQL is genuinely unchanged; and reverting only sqlite/tokenlock.go to main makes exactly four of the new shared cases fail (TestReleaseOnDeletedConsumer, TestReleaseOnOrphanConsumer, TestKeepOnDeletedProducer, TestKeepSiblingIndices), so the regression guard really guards. go vet and go test ./token/services/storage/... are clean, no references to the deleted sqlite.IsStale remain, and only sqlite and postgres embed common.TokenLockStore, so the promoted Cleanup resolves unambiguously.
Some minor issues, inline below. One does not sit on a line in this diff, so it goes here:
token/services/storage/db/driver/token.go:342 — should fix here. The authoritative interface godoc still reads "Cleanup removes the locks such that either: 1. The transaction that locked that token is valid or invalid; 2. The lock is too old." That is the exact ambiguity behind #2018 — "the transaction that locked that token" is what the old SQLite query read as the producer, and "valid or invalid" covers every status while the implementation only collects Deleted/Orphan. This PR corrects docs/ and adds a good explanatory comment on IsStaleLock, but leaves the contract a new driver author would actually implement against unchanged.
| // and leaves the other indices of the same transaction locked. See #2018. | ||
| func IsStaleLock(tokenLocks, tokenRequests common3.Table, leaseExpiry time.Duration) cond.Condition { | ||
| return cond.Or( | ||
| cond.OlderThan(tokenLocks.Field("created_at"), leaseExpiry), |
There was a problem hiding this comment.
Should fix here. Timezone mismatch between the two halves of this comparison on Postgres.
created_at is declared TIMESTAMP (line 98) — no time zone — but cond.OlderThan renders NOW() - INTERVAL '…', which is a timestamptz. Postgres resolves timestamp < timestamptz by interpreting the bare value in the session TimeZone, not UTC, even though Lock writes time.Now().UTC().
For a lock created at 12:00 UTC with a 5s lease:
session TimeZone |
stored 12:00 read as |
effect |
|---|---|---|
UTC |
12:00 UTC | correct |
Asia/Tokyo |
03:00 UTC | already 9h "old" — deleted on the first cleanup tick, freeing a token whose spend is still in flight, so two selections can race for it |
America/New_York |
16:00 UTC | 4h in the future — the lease does not expire for ~4 hours |
SQLite is unaffected because datetime('now') is UTC on both sides. That is also why the new claim at docs/services/selector.md:142 that lock lifetime is backend-independent does not hold as written.
Pre-existing rather than introduced here, but this PR consolidates the query into one place and documents the behaviour as uniform, so it is the natural moment. created_at TIMESTAMPTZ, or comparing against NOW() AT TIME ZONE 'UTC', closes it. Note postgres/tokenlock.go:109 already selects NOW() next to created_at, so the same skew shows up in the debug output too.
The SQLite lease cleanup correlated
token_lockstotoken_requestsontl.tx_id = tr.tx_id— the transaction that created the locked token — while Postgres correlated ontl.consumer_tx_id, the transaction that is trying to spend it. Only the consumer says anything about whether a lock is still live, so on SQLite a lock held by aDeleted/Orphanconsumer leaked until the lease aged out, and a lock held by an in-flight consumer was dropped as soon as the producing transaction died. The SQLite delete was also scoped astx_id IN (SELECT tl.tx_id ...)while the primary key is(tx_id, idx), so one expired lock evicted the live locks on every other output of the same transaction.The cleanup statement now lives once in the shared store as
IsStaleLock, correlated onconsumer_tx_idand scoped per row: SQLite inherits it, Postgres keeps only its logging of the rows about to go. The generated Postgres SQL is unchanged. The SQLite tests that asserted the generated SQL string — and so encoded the wrong join as the expected output — are replaced by behavioural cases in the shareddbtestsuite, which runs on SQLite, the in-memory backend and Postgres; four of them fail against the previous SQLite implementation.docs/services/selector.mdnow documents the lease-expiry semantics.Note this changes SQLite/in-memory behaviour visibly: locks of dead consumers are released on the next tick instead of up to
leaseExpirylater, and locks of live consumers are no longer dropped when the producing transaction dies.make checkspasses.make lintreports 17 pre-existingrevive: unhandled-errorfindings that reproduce identically on a pristineorigin/main; none are in the files touched here, and the changed packages lint clean.Fixes #2018