Skip to content

Commit 8f34270

Browse files
SanderMullerclaude
andcommitted
Sort the result cache's package dependencies before writing them
Every other section of the file is ordered before it is written; this one followed the order the workers happened to finish in, so two identical runs of a 4524-file project produced caches of the same size differing in 9848 lines. A run reading the file back does not care, but it means a result cache cannot be hashed, deduplicated or compared between machines. The packages of a single file need the same treatment: they are collected in the order that file's dependencies are reflected, so a file naming Mailer before Logger records their packages in that order. The e2e fixture makes both observable without depending on a race. Four files are split into two parallel jobs, so the section reaches the main process interleaved and no worker completion order can put it in ascending order by accident; and each file declares the two packages in reverse alphabetical order. Dropping either sort fails the assertion on every run, naming what is out of order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a9ef13d commit 8f34270

15 files changed

Lines changed: 291 additions & 0 deletions

File tree

‎.github/workflows/e2e-tests.yml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,6 +930,17 @@ jobs:
930930
echo "$OUTPUT"
931931
../bashunit -a contains 'Composer packages changed (test/logger); re-analysing the files depending on them and the files with errors.' "$OUTPUT"
932932
../bashunit -a contains 'Result cache restored. 1 file will be reanalysed.' "$OUTPUT"
933+
- script: |
934+
cd e2e/result-cache-deterministic-order
935+
composer install
936+
# Nothing in the written file may depend on the order the parallel workers happened to
937+
# finish in: a result cache that is not reproducible cannot be hashed, compared or
938+
# deduplicated between two machines. The fixture splits four files into two jobs, so no
939+
# completion order puts them in ascending order by accident, and assert-sorted.php fails
940+
# if any entry section of the cache is out of key order.
941+
../../bin/phpstan analyse
942+
OUTPUT=$(php assert-sorted.php tmp/resultCache.php 2>&1) || { echo "$OUTPUT"; exit 1; }
943+
echo "$OUTPUT"
933944
- script: |
934945
cd e2e/result-cache-path-repository
935946
composer install
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
/vendor
2+
/composer.lock
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
<?php declare(strict_types = 1);
2+
3+
// Reads the framed result cache and fails if any of its entry sections is not in key order.
4+
// The order the workers happen to finish in must not reach the file: a cache that is not
5+
// reproducible cannot be hashed, compared or deduplicated between two machines.
6+
7+
$path = $argv[1] ?? null;
8+
if ($path === null || !is_file($path)) {
9+
fwrite(STDERR, sprintf("Result cache %s does not exist.\n", $path ?? '<missing argument>'));
10+
exit(1);
11+
}
12+
13+
$handle = fopen($path, 'r');
14+
if ($handle === false) {
15+
fwrite(STDERR, sprintf("Cannot open %s.\n", $path));
16+
exit(1);
17+
}
18+
19+
fgets($handle); // the PHP prefix line that keeps the file inert when included
20+
21+
$unsorted = [];
22+
$sections = 0;
23+
$packageLists = [];
24+
while (($header = fgets($handle)) !== false) {
25+
$header = rtrim($header, "\n");
26+
if ($header === '') {
27+
continue;
28+
}
29+
30+
$parts = explode(' ', $header, 2);
31+
if (count($parts) !== 2) {
32+
fwrite(STDERR, sprintf("Malformed frame header \"%s\".\n", $header));
33+
exit(1);
34+
}
35+
36+
[$name, $size] = $parts;
37+
if (!str_ends_with($name, '*')) {
38+
fread($handle, (int) $size);
39+
40+
continue;
41+
}
42+
43+
$name = substr($name, 0, -1);
44+
$keys = [];
45+
for ($i = 0; $i < (int) $size; $i++) {
46+
$length = (int) rtrim((string) fgets($handle), "\n");
47+
$entry = unserialize((string) fread($handle, $length));
48+
$key = (string) array_key_first($entry);
49+
$keys[] = $key;
50+
if ($name !== 'packageDependencies') {
51+
continue;
52+
}
53+
54+
// The packages of one file are collected in the order its dependencies are reflected,
55+
// so their order has to be fixed too, not just the order of the files. Unlike the key
56+
// order, this does not depend on how the scheduler composes jobs, so it still fails
57+
// without the sort even when the whole project runs as a single job.
58+
$packageLists[$key] = $entry[array_key_first($entry)];
59+
}
60+
61+
$sections++;
62+
$sorted = $keys;
63+
sort($sorted, SORT_STRING);
64+
if ($keys === $sorted) {
65+
continue;
66+
}
67+
68+
$unsorted[$name] = $keys;
69+
}
70+
71+
fclose($handle);
72+
73+
if ($sections === 0) {
74+
fwrite(STDERR, "The result cache has no entry sections, so nothing was checked.\n");
75+
exit(1);
76+
}
77+
78+
if ($packageLists === []) {
79+
fwrite(STDERR, "The result cache recorded no package dependencies, so nothing was checked.\n");
80+
exit(1);
81+
}
82+
83+
$problems = [];
84+
foreach ($unsorted as $name => $keys) {
85+
$problems[] = sprintf('Section "%s" is not in key order: %s', $name, implode(', ', array_map('basename', $keys)));
86+
}
87+
88+
foreach ($packageLists as $file => $packages) {
89+
$sortedPackages = $packages;
90+
sort($sortedPackages, SORT_STRING);
91+
if ($packages === $sortedPackages) {
92+
continue;
93+
}
94+
95+
$problems[] = sprintf('The packages of %s are not in order: %s', basename($file), implode(', ', $packages));
96+
}
97+
98+
if ($problems !== []) {
99+
fwrite(STDERR, implode("\n", $problems) . "\n");
100+
exit(1);
101+
}
102+
103+
echo sprintf(
104+
"All %d entry sections of the result cache are in key order, and so are the packages of all %d files that have package dependencies.\n",
105+
$sections,
106+
count($packageLists),
107+
);
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
{
2+
"name": "phpstan/result-cache-deterministic-order-e2e",
3+
"repositories": [
4+
{
5+
"type": "path",
6+
"url": "./logger",
7+
"options": {
8+
"symlink": false
9+
}
10+
},
11+
{
12+
"type": "path",
13+
"url": "./mailer",
14+
"options": {
15+
"symlink": false
16+
}
17+
}
18+
],
19+
"require": {
20+
"test/logger": "*",
21+
"test/mailer": "*"
22+
},
23+
"autoload": {
24+
"classmap": ["src"]
25+
}
26+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{ "name": "test/logger", "version": "1.0.0", "autoload": { "classmap": ["src"] } }
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?php
2+
3+
namespace Test\Logger;
4+
5+
class Logger
6+
{
7+
8+
public function info(string $message): void
9+
{
10+
}
11+
12+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{ "name": "test/mailer", "version": "1.0.0", "autoload": { "classmap": ["src"] } }
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?php
2+
3+
namespace Test\Mailer;
4+
5+
class Mailer
6+
{
7+
8+
public function send(string $message): void
9+
{
10+
}
11+
12+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
parameters:
2+
level: 5
3+
tmpDir: tmp
4+
paths:
5+
- src
6+
parallel:
7+
# Two jobs of two files each, so the four files reach the main process interleaved
8+
# (F1, F3 in one job, F2, F4 in the other) and no worker completion order can put
9+
# them in ascending order by accident.
10+
jobSize: 2
11+
maximumNumberOfProcesses: 4
12+
minimumNumberOfJobsPerProcess: 1
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
namespace ResultCacheDeterministicOrderE2E;
4+
5+
use Test\Logger\Logger;
6+
use Test\Mailer\Mailer;
7+
8+
// Mailer is declared before Logger on purpose: the packages of one file are collected in the
9+
// order its dependencies are reflected, so without a sort this file records test/mailer first.
10+
class F1
11+
{
12+
13+
public function __construct(private Mailer $mailer, private Logger $logger)
14+
{
15+
}
16+
17+
public function doFoo(): void
18+
{
19+
$this->mailer->send('hello');
20+
$this->logger->info('hello');
21+
}
22+
23+
}

0 commit comments

Comments
 (0)