From 74deca8f1494465d33c978a40a453ef8b40c5a4e 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 fa07f39858381..0d61471cd6e59 100644 --- a/lib/private/Snowflake/SnowflakeGenerator.php +++ b/lib/private/Snowflake/SnowflakeGenerator.php @@ -36,7 +36,7 @@ public function nextId(): string { $serverId = $this->getServerId(); // Already 9 bits $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 9bffa692574f69ddbdca08448df70c5ca132ce66 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 08e6357f363c1..fbf375c1a262f 100644 --- a/tests/lib/Snowflake/FileSequenceTest.php +++ b/tests/lib/Snowflake/FileSequenceTest.php @@ -20,15 +20,46 @@ class FileSequenceTest extends ISequenceBase { 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); } 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 91dc5bff2ad1cf8ea3b7609d240676be53067c98 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 0d61471cd6e59..cde13e85f75a0 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->getServerId(); // Already 9 bits $isCli = (int)$this->isCli(); // 1 bit diff --git a/lib/public/Snowflake/ISnowflakeGenerator.php b/lib/public/Snowflake/ISnowflakeGenerator.php index 276e5dc52fa22..9391c13d6080f 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 fbf375c1a262f..e51e0b0b49852 100644 --- a/tests/lib/Snowflake/FileSequenceTest.php +++ b/tests/lib/Snowflake/FileSequenceTest.php @@ -56,10 +56,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 a4ea7e3520387..b57c02372c293 100644 --- a/tests/lib/Snowflake/GeneratorTest.php +++ b/tests/lib/Snowflake/GeneratorTest.php @@ -63,6 +63,15 @@ public function testGenerator(): void { $this->assertEquals(42, $data->getServerId()); } + public function testGeneratorRejectsTimeBeforeEpoch(): void { + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('now')->willReturn(new \DateTimeImmutable('2025-09-30 23:59:59 UTC')); + $generator = new SnowflakeGenerator($timeFactory, $this->config, $this->sequence); + + $this->expectException(\InvalidArgumentException::class); + $generator->nextId(); + } + #[DataProvider('provideSnowflakeData')] public function testGeneratorWithFixedTime(string $date, int $expectedSeconds, int $expectedMilliseconds): void { $dt = new \DateTimeImmutable($date);