Skip to content

feat(distributed): expose the lease token so a write can be conditional on it #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abnegate wants to merge 1 commit into from
Closed

feat(distributed): expose the lease token so a write can be conditional on it #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/Distributed.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,26 @@ public function isHeld(): bool
return $this->redis->get($this->key) === $this->token;
}

/**
* The value this lock wrote to the key, or null when it holds no lease.

@greptile-apps greptile-apps Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Fix in Claude Code Fix in Codex

*
* A caller that records what it did while holding the lease can store this
* alongside the record and refuse a later write whose token no longer matches,
* which is the only way to make the write itself conditional on the lease:
* {@see refresh()} proves ownership at the instant it returns, not at the
* instant the write commits.
*
* It is also the value an operator reads back from the key, so a record and
* the live holder can be compared directly.
*
* A new value is generated on every acquisition, so a lease that lapsed and
* was retaken by this same instance does not compare equal to the one before.
*/
public function token(): ?string
{
return $this->token;
}

/**
* @param array<int, string> $arguments
*/
Expand Down
51 changes: 51 additions & 0 deletions tests/DistributedTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,57 @@ public function testRefreshExtendsTtl(): void
$lock->release();
}

public function testTokenIsTheValueOnTheKeyAndIsClearedOnRelease(): void
{
$lock = new Distributed($this->redis, $this->key, 30);

$this->assertTrue($lock->tryAcquire());
$token = $lock->token();

$this->assertSame(
$this->redis->get($this->key),
$token,
'the token must be the value on the key, so a record naming it can be compared against the live holder'
);

$lock->release();
$this->assertNull($lock->token());
}

public function testEachAcquisitionMintsItsOwnToken(): void
{
$lock = new Distributed($this->redis, $this->key, 30);

$this->assertTrue($lock->tryAcquire());
$first = $lock->token();
$lock->release();

$this->assertTrue($lock->tryAcquire());
$second = $lock->token();
$lock->release();

$this->assertNotSame(
$first,
$second,
'a token identifies one acquisition, so work recorded under a lapsed lease cannot pass for work under its successor'
);
}

public function testTokenIsNotIssuedForAFailedAcquire(): void
{
$holder = new Distributed($this->redis, $this->key, 30);
$waiter = new Distributed($this->redis, $this->key, 30);

$this->assertNull($waiter->token(), 'a lock holding no lease has no token to hand out');
$this->assertTrue($holder->tryAcquire());
$this->assertFalse($waiter->tryAcquire());

$this->assertNull($waiter->token(), 'a lock that never took the key must not name itself the holder');
$this->assertSame($holder->token(), $this->redis->get($this->key));

$holder->release();
}

public function testLoggerReceivesMessages(): void
{
$holder = new Distributed($this->redis, $this->key, 30);
Expand Down