Skip to content

7.0: CompareAndSwapInterface and race condition fixes in atomic operations - #24

Open
byjg wants to merge 6 commits into
masterfrom
7.0
Open

7.0: CompareAndSwapInterface and race condition fixes in atomic operations#24
byjg wants to merge 6 commits into
masterfrom
7.0

Conversation

@byjg

@byjg byjg commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Summary

Adds CompareAndSwapInterface — the conditional-write primitive needed to build a correct
distributed lock on top of a cache engine — and fixes three genuine race conditions in the existing
atomic operations.

The races are not theoretical. Forked-process tests reproduce each one against master:

  • Twenty concurrent increment() calls on Memcached ended at 8 instead of 20.
  • Twenty concurrent add() calls on Redis duplicated the seed value 16 times and dropped appends.
  • FileSystem values written with a TTL were already expired when written.

CompareAndSwapInterface

public function setIfAbsent(string $key, mixed $value, DateInterval|int|null $ttl = null): bool;
public function deleteIfEquals(string $key, mixed $value): bool;
public function expireIfEquals(string $key, mixed $value, DateInterval|int|null $ttl): bool;

Implemented by RedisCacheEngine (SET NX EX plus Lua for the guarded operations) and
MemcachedEngine (native add() plus cas()).

Deliberately a separate interface rather than an addition to AtomicOperationInterface, so
nothing that already implements the latter breaks. Callers probe with instanceof and degrade
explicitly.

FileSystemCacheEngine does not implement it. flock() attaches to an inode rather than a path,
and the engine unlinks the file on delete, so two processes can end up holding "the lock" on two
different inodes at the same path. The file system is not a reliable substrate for mutual exclusion
and the engine should not pretend otherwise.

Bug fixes

Memcached — non-atomic seeding in all three atomic operations. increment, decrement and
add initialised a missing key with get() === false then set(). Memcached's own increment()
is atomic, but those two preparatory calls are not:

P1: get() === false          P2: get() === false
P1: set(0)
P1: increment() -> 1
                             P2: set(0)           <- resets the counter
                             P2: increment() -> 1 <- same value issued twice

Now seeded with Memcached::add(), which the server resolves in a single step.

Redis — add() corrupted the list when converting from a plain value. The first add() to a
key written by set() did GETDEL → re-push → RPUSH unprotected, so concurrent callers each
deleted a list the others were rebuilding. Now a compare-guarded script that only rewrites the key
while it still holds the value that was read.

Redis — TTL applied as a separate command. increment/decrement set the expiry with a
follow-up EXPIRE; a crash in between left a counter with no expiry. Both now run as one script.

FileSystem — expiry dropped outside the lock, and a broken TTL conversion. The .ttl file was
deleted before the lock was taken, leaving the value briefly immortal. The atomic operations also
passed a relative TTL where an absolute timestamp was expected, so increment($key, 1, 60) wrote an
expiry in 1970.

Breaking changes

Area Before After
Redis atomic operations INCR/RPUSH + separate EXPIRE Lua scripts — requires EVAL to be available
Redis add() TTL accepted and silently ignored applied; lists built this way now expire
FileSystem atomic TTL written as an absolute timestamp converted with addToNow()

Full detail and migration steps in CHANGELOG-7.0.md.

Testing

  • tests/CompareAndSwapTest.php — 18 tests across both engines: expiry, non-owner rejection, and
    the stale-owner-versus-new-owner case.
  • tests/ConcurrencyTest.php — forks twenty real processes at a synchronised barrier. Sequential
    tests cannot prove atomicity. Each of these was verified to fail against master before being
    accepted.
  • TTL regression test added to tests/CachePSR16Test.php for every engine implementing
    AtomicOperationInterface.

181 tests, 1107 assertions passing. Psalm clean against a clean baseline.

Also included

CHANGELOG-6.0.md, which was sitting untracked in the working tree, committed separately.

byjg added 2 commits August 7, 2026 10:28
Introduce CompareAndSwapInterface with setIfAbsent, deleteIfEquals and
expireIfEquals: conditional writes the engine resolves in one indivisible
step, which is what a correct distributed lock needs. Implemented by
RedisCacheEngine (SET NX EX plus Lua) and MemcachedEngine (native add plus
cas). Kept separate from AtomicOperationInterface so existing implementors
are untouched.

Fix three real races in the existing atomic operations:

- Memcached seeded a missing key with get()===false then set() in
  increment, decrement and add. The increment itself is atomic but the
  seeding is not, so a slower caller's set(0) lands after a faster
  caller's increment and reissues the same value. Twenty concurrent
  increments ended at 8. Now seeded with Memcached::add().

- Redis add() converted a string key into a list with GET, DEL and
  re-push, unprotected, so concurrent callers deleted a list the others
  were rebuilding. Twenty concurrent appends duplicated the seed value 16
  times. The conversion is now a compare-guarded script.

- Redis increment and decrement applied the TTL as a separate EXPIRE,
  leaving a counter with no expiry if the process died in between. Both
  now run as one script, and add() finally honours the $ttl it had been
  accepting and discarding.

Also fix FileSystem, where the atomic operations passed a relative TTL
where an absolute timestamp was expected, so anything written with a TTL
expired in 1970; and where the expiry file was dropped before the lock was
taken. FileSystem does not implement CompareAndSwapInterface: flock binds
to an inode and the engine unlinks on delete, so two processes can hold
the lock on two different inodes at the same path.

Tests fork twenty real processes at a synchronised barrier; each fix has a
case that fails without it.

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

byjg added 4 commits August 7, 2026 10:38
The engines already declare the PSR-16 3.0 signatures - get(string $key,
mixed $default = null): mixed - so the constraint excluding ^3.0 kept the
package from installing alongside anything that requires it.

Drop ^1.0 at the same time. PSR-16 1.0 declares get($key, $default = null)
with untyped parameters, and adding a type to a parameter the interface
leaves untyped is a fatal declaration error, so that branch of the
constraint was never usable.
Both read $this->redis without calling lazyLoadRedisServer() first, so
either one used as the very first operation on a new instance died with
"Call to a member function exists() on null". Every other public method
already established the connection; these two were simply missed.

Surfaced by calling has() as the opening operation against a fresh engine.
Both landed after the changelog was written. Listed under Bug Fixes rather
than Breaking Changes: widening psr/simple-cache to allow 3.0 only unblocks
installs, and the dropped ^1.0 branch could never load in the first place.
The compare-and-swap and concurrency tests constructed their engines with
no arguments, which points them at 127.0.0.1. CI runs the job inside a
container, where the service containers are reachable by service name on
the Docker network rather than on loopback, so both classes skipped
themselves for the whole matrix and the build went green having never
exercised a real Redis or Memcached.

TestBase already read REDIS_SERVER and MEMCACHED_SERVER for its data
provider. That logic now lives in EngineFactory and every test goes through
it.

make() returns BaseCacheEngine&AtomicOperationInterface&CompareAndSwapInterface,
which is enough for the call sites to drop their local @var annotations and
carry one engine variable instead of two.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants