feat(distributed): expose the lease token so a write can be conditional on it - #2
feat(distributed): expose the lease token so a write can be conditional on it#2abnegate wants to merge 1 commit into
Conversation
…al on it refresh() proves ownership at the instant it returns, which is the wrong instant for a caller whose write commits later. Handing back the value on the key lets that caller store the token with whatever it recorded under the lease and refuse a stale write whose token no longer matches, and lets an operator compare a record against the live holder directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Greptile SummaryThe PR exposes each distributed-lock acquisition token so downstream writes can be fenced against the lease identity.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking API-documentation issue around stale tokens after lease loss. The accessor correctly exposes the stored fencing value, but its nullability documentation promises current lease state even though Redis expiry and failed refresh leave that value cached. Files Needing Attention: src/Distributed.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Distributed.php:143
**Clarify cached token semantics**
After the Redis TTL expires or `refresh()` fails, `token()` continues returning the cached acquisition value even though the instance no longer holds the lease. Describing `null` as meaning that the lock holds no lease conflates explicit release with current ownership and can lead callers to treat a stale token as proof of a live lease.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(distributed): expose the lease toke..." | Re-trigger Greptile |
| } | ||
|
|
||
| /** | ||
| * The value this lock wrote to the key, or null when it holds no lease. |
There was a problem hiding this comment.
Clarify cached token semantics
After the Redis TTL expires or refresh() fails, token() continues returning the cached acquisition value even though the instance no longer holds the lease. Describing null as meaning that the lock holds no lease conflates explicit release with current ownership and can lead callers to treat a stale token as proof of a live lease.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Distributed.php
Line: 143
Comment:
**Clarify cached token semantics**
After the Redis TTL expires or `refresh()` fails, `token()` continues returning the cached acquisition value even though the instance no longer holds the lease. Describing `null` as meaning that the lock holds no lease conflates explicit release with current ownership and can lead callers to treat a stale token as proof of a live lease.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
What
Distributed::token()returns the value this lock wrote to the key, ornullwhen it holds no lease.Why
refresh()proves ownership at the instant it returns. That is the wrong instant for a caller whose write commits later: between the successful refresh and the write, the lease can lapse and a successor can take the key, and nothing about the write itself says which lease it belongs to.Handing back the token lets the caller close that gap. It stores the token alongside whatever it recorded while holding the lease, and refuses a later write whose stored token no longer matches — the write becomes conditional on the lease rather than on a check that merely preceded it.
The caller here is Appwrite Cloud's dedicated-databases worker (DAT-2084), where a lease that lapsed during a ~5 minute PostgreSQL cutover let the finalization writes land on top of a successor's outcome.
Because the token is the literal value on the key, it doubles as a diagnostic: an operator can
GETthe lock key and compare it against the record to see whether the recorded owner is still the live holder.Tests
Three added to
DistributedTest, all against a real Redis:Verified red against the plausible wrong implementation (returning a freshly generated token rather than the stored one): 2 of the 3 fail.