From 0289571f81e10ee7e58293945434d23c6e0d1a00 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 30 Sep 2026 17:22:11 +0200 Subject: [PATCH 1/3] fix(snowflake): pass sequence ID arguments in the declared order ISequence::nextId() takes the server id, the seconds and the milliseconds, but the generator passed the seconds, the milliseconds and the server id, so the sequences were keyed on the wrong values. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Carl Schwan --- lib/private/Snowflake/SnowflakeGenerator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Snowflake/SnowflakeGenerator.php b/lib/private/Snowflake/SnowflakeGenerator.php index a247700ebdee5..23b8febc2a399 100644 --- a/lib/private/Snowflake/SnowflakeGenerator.php +++ b/lib/private/Snowflake/SnowflakeGenerator.php @@ -36,7 +36,7 @@ public function nextId(): string { $serverId = $this->serverInfo->getServerId(); $isCli = (int)$this->isCli(); // 1 bit - $sequenceId = $this->sequenceGenerator->nextId($seconds, $milliseconds, $serverId); // 12 bits + $sequenceId = $this->sequenceGenerator->nextId($serverId, $seconds, $milliseconds); // 12 bits if ($sequenceId > 0xFFF || $sequenceId === false) { // Throttle a bit, wait for next millisecond usleep(1000); From 0d9d1f018fcfdfe19c300d4cd2292a7718576572 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 30 Sep 2026 17:22:12 +0200 Subject: [PATCH 2/3] perf(snowflake): use fixed slots and drop fsync in FileSequence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store one slot per second of the TTL window and per millisecond in each lock file, so generating an id only reads and writes 8 bytes instead of decoding, filtering and rewriting a JSON map of the last 30 seconds. The fsync is not needed as the files only coordinate processes running at the same time. Generating an id went from 660µs to 22µs with the temporary directory on disk, and from 24µs to 11µs on tmpfs. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Carl Schwan --- lib/private/Snowflake/FileSequence.php | 50 +++++++++--------------- tests/lib/Snowflake/FileSequenceTest.php | 37 ++++++++++++++++-- 2 files changed, 53 insertions(+), 34 deletions(-) diff --git a/lib/private/Snowflake/FileSequence.php b/lib/private/Snowflake/FileSequence.php index 55319932994d1..3c31236f1e98a 100644 --- a/lib/private/Snowflake/FileSequence.php +++ b/lib/private/Snowflake/FileSequence.php @@ -19,9 +19,11 @@ class FileSequence implements ISequence { /** Lock file directory **/ public const LOCK_FILE_DIRECTORY = 'sfi_file_sequence'; /** Lock filename format **/ - private const LOCK_FILE_FORMAT = 'seq-%03d.lock'; - /** Delete sequences after SEQUENCE_TTL seconds **/ + private const LOCK_FILE_FORMAT = 'seq-%03d.slots'; + /** Reuse the slot of a sequence after SEQUENCE_TTL seconds **/ private const SEQUENCE_TTL = 30; + /** Size of a slot: the seconds and the last sequence ID, both as unsigned 32-bit integers **/ + private const SLOT_SIZE = 8; private string $workDir; @@ -69,42 +71,28 @@ public function nextId(int $serverId, int $seconds, int $milliseconds): int { throw new \Exception('Unable to acquire lock on sequence ID file: ' . $filePath); } - // Read content - $content = (string)fgets($fp); - $locks = $content === '' - ? [] - : json_decode($content, true, 3, JSON_THROW_ON_ERROR); - - // Generate new ID - if (isset($locks[$seconds])) { - if (isset($locks[$seconds][$milliseconds])) { - ++$locks[$seconds][$milliseconds]; - } else { - $locks[$seconds][$milliseconds] = 0; + // Each file holds one slot per second of the TTL window and per millisecond mapped to it + $slot = (($seconds % self::SEQUENCE_TTL + self::SEQUENCE_TTL) % self::SEQUENCE_TTL) * intdiv(1000, self::NB_FILES) + + intdiv($milliseconds, self::NB_FILES); + $slotSecondsKey = $seconds & 0xFFFFFFFF; + fseek($fp, $slot * self::SLOT_SIZE); + $data = fread($fp, self::SLOT_SIZE); + $sequenceId = 0; + if (is_string($data) && strlen($data) === self::SLOT_SIZE) { + ['seconds' => $slotSeconds, 'sequence' => $slotSequenceId] = unpack('Nseconds/Nsequence', $data); + if ($slotSeconds === $slotSecondsKey) { + $sequenceId = $slotSequenceId + 1; } - } else { - $locks[$seconds] = [ - $milliseconds => 0 - ]; } - // Clean old sequence IDs - $cleanBefore = $seconds - self::SEQUENCE_TTL; - $locks = array_filter($locks, static function ($key) use ($cleanBefore) { - return $key >= $cleanBefore; - }, ARRAY_FILTER_USE_KEY); - - // Write data - ftruncate($fp, 0); - $content = json_encode($locks, JSON_THROW_ON_ERROR); - rewind($fp); - fwrite($fp, $content); - fsync($fp); + // No fsync needed, the file only coordinates processes running at the same time + fseek($fp, $slot * self::SLOT_SIZE); + fwrite($fp, pack('NN', $slotSecondsKey, $sequenceId)); // Release lock fclose($fp); - return $locks[$seconds][$milliseconds]; + return $sequenceId; } private function getFilePath(int $fileId): string { diff --git a/tests/lib/Snowflake/FileSequenceTest.php b/tests/lib/Snowflake/FileSequenceTest.php index ed3f4e79a9f9d..c59f256a911d8 100644 --- a/tests/lib/Snowflake/FileSequenceTest.php +++ b/tests/lib/Snowflake/FileSequenceTest.php @@ -21,16 +21,47 @@ class FileSequenceTest extends ISequenceBase { #[\Override] public function setUp():void { $tempManager = $this->createMock(ITempManager::class); - $this->path = sys_get_temp_dir(); + $this->path = sys_get_temp_dir() . '/' . uniqid('file_sequence_test_'); + mkdir($this->path); $tempManager->method('getTempBaseDir')->willReturn($this->path); $this->sequence = new FileSequence($tempManager); } #[\Override] public function tearDown():void { - $lockDirectory = $this->path . '/' . FileSequence::LOCK_FILE_DIRECTORY; - foreach (glob($lockDirectory . '/*') as $file) { + foreach (glob($this->path . '/' . FileSequence::LOCK_FILE_DIRECTORY . '*/*') as $file) { unlink($file); } + foreach (glob($this->path . '/' . FileSequence::LOCK_FILE_DIRECTORY . '*') as $directory) { + rmdir($directory); + } + rmdir($this->path); + } + + public function testSequenceIncrementsWithinTheSameMillisecond(): void { + $this->assertSame(0, $this->sequence->nextId(42, 1000, 500)); + $this->assertSame(1, $this->sequence->nextId(42, 1000, 500)); + $this->assertSame(2, $this->sequence->nextId(42, 1000, 500)); + } + + public function testSequenceStartsAtZeroForEachMillisecond(): void { + $this->assertSame(0, $this->sequence->nextId(42, 1000, 500)); + // Same lock file, different slot + $this->assertSame(0, $this->sequence->nextId(42, 1000, 520)); + $this->assertSame(0, $this->sequence->nextId(42, 1001, 500)); + // Same slot, reused after the TTL window + $this->assertSame(0, $this->sequence->nextId(42, 1030, 500)); + } + + public function testSequenceKeepsEarlierMillisecondsWithinTheWindow(): void { + $this->assertSame(0, $this->sequence->nextId(42, 1000, 500)); + $this->assertSame(0, $this->sequence->nextId(42, 1001, 500)); + $this->assertSame(1, $this->sequence->nextId(42, 1000, 500)); + } + + public function testSequenceWithSecondsBeforeTheEpoch(): void { + $this->assertSame(0, $this->sequence->nextId(42, -5, 10)); + $this->assertSame(1, $this->sequence->nextId(42, -5, 10)); + $this->assertSame(0, $this->sequence->nextId(42, 25, 10)); } } From 8e03c9ba5673fee92673fc2e4d98c5872506df21 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 1 Oct 2026 10:12:44 +0200 Subject: [PATCH 3/3] fix(snowflake): Reject time before epoch This allow to simplify a bit the code, also add a small description of the algorithm. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Carl Schwan --- lib/private/Snowflake/FileSequence.php | 21 +++++++++++++++----- lib/private/Snowflake/ISequence.php | 2 ++ lib/private/Snowflake/SnowflakeGenerator.php | 3 +++ lib/public/Snowflake/ISnowflakeGenerator.php | 1 + tests/lib/Snowflake/FileSequenceTest.php | 6 ------ tests/lib/Snowflake/GeneratorTest.php | 9 +++++++++ 6 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/private/Snowflake/FileSequence.php b/lib/private/Snowflake/FileSequence.php index 3c31236f1e98a..4b444a1d73426 100644 --- a/lib/private/Snowflake/FileSequence.php +++ b/lib/private/Snowflake/FileSequence.php @@ -13,6 +13,19 @@ use OCP\ITempManager; use Override; +/** + * Sequence IDs shared between processes through lock files in the temporary directory. + * + * Each lock file is a table of counters. To get a sequence ID, the file chosen by the + * millisecond (ms % NB_FILES) is locked and the slot of that second and millisecond is + * read: ((seconds % SEQUENCE_TTL) * (1000 / NB_FILES) + ms / NB_FILES). A slot holds the + * seconds it was last used for and the last sequence ID handed out. If the seconds match, + * the next sequence ID is that one plus one, otherwise it starts at 0. The result is + * written back and the file unlocked. + * + * A slot is reused SEQUENCE_TTL seconds later, the seconds mismatch then resets its counter. + * No fsync is needed, as the files only coordinate processes running at the same time. + */ class FileSequence implements ISequence { /** Number of files to use */ private const NB_FILES = 20; @@ -72,22 +85,20 @@ public function nextId(int $serverId, int $seconds, int $milliseconds): int { } // Each file holds one slot per second of the TTL window and per millisecond mapped to it - $slot = (($seconds % self::SEQUENCE_TTL + self::SEQUENCE_TTL) % self::SEQUENCE_TTL) * intdiv(1000, self::NB_FILES) - + intdiv($milliseconds, self::NB_FILES); - $slotSecondsKey = $seconds & 0xFFFFFFFF; + $slot = ($seconds % self::SEQUENCE_TTL) * intdiv(1000, self::NB_FILES) + intdiv($milliseconds, self::NB_FILES); fseek($fp, $slot * self::SLOT_SIZE); $data = fread($fp, self::SLOT_SIZE); $sequenceId = 0; if (is_string($data) && strlen($data) === self::SLOT_SIZE) { ['seconds' => $slotSeconds, 'sequence' => $slotSequenceId] = unpack('Nseconds/Nsequence', $data); - if ($slotSeconds === $slotSecondsKey) { + if ($slotSeconds === $seconds) { $sequenceId = $slotSequenceId + 1; } } // No fsync needed, the file only coordinates processes running at the same time fseek($fp, $slot * self::SLOT_SIZE); - fwrite($fp, pack('NN', $slotSecondsKey, $sequenceId)); + fwrite($fp, pack('NN', $seconds, $sequenceId)); // Release lock fclose($fp); diff --git a/lib/private/Snowflake/ISequence.php b/lib/private/Snowflake/ISequence.php index e5e1f6e414d32..22c5f52a840dc 100644 --- a/lib/private/Snowflake/ISequence.php +++ b/lib/private/Snowflake/ISequence.php @@ -20,6 +20,8 @@ public function isAvailable(): bool; /** * Returns next sequence ID for current time and server + * + * @param non-negative-int $seconds seconds since the Snowflake epoch */ public function nextId(int $serverId, int $seconds, int $milliseconds): int|false; } diff --git a/lib/private/Snowflake/SnowflakeGenerator.php b/lib/private/Snowflake/SnowflakeGenerator.php index 23b8febc2a399..280a1c9c1e376 100644 --- a/lib/private/Snowflake/SnowflakeGenerator.php +++ b/lib/private/Snowflake/SnowflakeGenerator.php @@ -33,6 +33,9 @@ public function __construct( public function nextId(): string { // Relative time [$seconds, $milliseconds] = $this->getCurrentTime(); + if ($seconds < 0) { + throw new \InvalidArgumentException('Snowflake IDs cannot be generated for a time before ' . date(DATE_ATOM, self::TS_OFFSET)); + } $serverId = $this->serverInfo->getServerId(); $isCli = (int)$this->isCli(); // 1 bit diff --git a/lib/public/Snowflake/ISnowflakeGenerator.php b/lib/public/Snowflake/ISnowflakeGenerator.php index f3a8a4cabd85f..ad3a18a253cee 100644 --- a/lib/public/Snowflake/ISnowflakeGenerator.php +++ b/lib/public/Snowflake/ISnowflakeGenerator.php @@ -39,6 +39,7 @@ interface ISnowflakeGenerator { * * Each call to this method is guaranteed to return a different ID. * + * @throws \InvalidArgumentException if the current time is before 2025-10-01, as such a time cannot be represented * @since 33.0 */ public function nextId(): string; diff --git a/tests/lib/Snowflake/FileSequenceTest.php b/tests/lib/Snowflake/FileSequenceTest.php index c59f256a911d8..0cf49cea0179c 100644 --- a/tests/lib/Snowflake/FileSequenceTest.php +++ b/tests/lib/Snowflake/FileSequenceTest.php @@ -58,10 +58,4 @@ public function testSequenceKeepsEarlierMillisecondsWithinTheWindow(): void { $this->assertSame(0, $this->sequence->nextId(42, 1001, 500)); $this->assertSame(1, $this->sequence->nextId(42, 1000, 500)); } - - public function testSequenceWithSecondsBeforeTheEpoch(): void { - $this->assertSame(0, $this->sequence->nextId(42, -5, 10)); - $this->assertSame(1, $this->sequence->nextId(42, -5, 10)); - $this->assertSame(0, $this->sequence->nextId(42, 25, 10)); - } } diff --git a/tests/lib/Snowflake/GeneratorTest.php b/tests/lib/Snowflake/GeneratorTest.php index 7e43639e61406..487d0771913af 100644 --- a/tests/lib/Snowflake/GeneratorTest.php +++ b/tests/lib/Snowflake/GeneratorTest.php @@ -83,6 +83,15 @@ public function testMinForTime(): void { $this->assertEquals(0, $data->getServerId()); } + public function testGeneratorRejectsTimestampBeforeEpoch(): void { + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('now')->willReturn(new \DateTimeImmutable('2025-09-30 23:59:59 UTC')); + $generator = new SnowflakeGenerator($timeFactory, $this->sequence, $this->serverInfo); + + $this->expectException(\InvalidArgumentException::class); + $generator->nextId(); + } + #[DataProvider('provideSnowflakeData')] public function testGeneratorWithFixedTime(string $date, int $expectedSeconds, int $expectedMilliseconds): void { $dt = new \DateTimeImmutable($date);