From dee75a0bff07e814d586973434b2a412d65af273 Mon Sep 17 00:00:00 2001 From: Paul Golmann Date: Thu, 1 Oct 2026 12:29:36 +0200 Subject: [PATCH 1/2] Stop srcHttp temp sinks from accumulating on disk. sink => true left pulp-http-* files behind after each response, including failed requests. The path-backed File now removes the temp file it owns, and a caller-supplied sink path stays in place. --- packages/pulp/CHANGELOG.md | 4 + packages/pulp/Pulp.php | 3 +- packages/pulp/README.md | 2 +- packages/pulp/src/File.php | 19 ++++- packages/pulp/src/SrcHttpHandler.php | 26 ++++++- packages/pulp/test/FileTest.php | 15 ++++ packages/pulp/test/SrcHttpHandlerTest.php | 92 +++++++++++++++++++++++ 7 files changed, 157 insertions(+), 4 deletions(-) diff --git a/packages/pulp/CHANGELOG.md b/packages/pulp/CHANGELOG.md index 1a9090c..822a645 100644 --- a/packages/pulp/CHANGELOG.md +++ b/packages/pulp/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to `mapsight/pulp` are documented here. ## Unreleased +### Fixed + +- Remove the temp file created by `srcHttp` `sink => true` when the path-backed `File` is released. A failed request removes it immediately. A caller-supplied sink path is left in place. + ## 1.2.0 - 2026-09-02 ### Added diff --git a/packages/pulp/Pulp.php b/packages/pulp/Pulp.php index 0c2dbe1..b872490 100644 --- a/packages/pulp/Pulp.php +++ b/packages/pulp/Pulp.php @@ -60,7 +60,8 @@ public static function srcFile( /** * ## Options * * `sink`: `true` writes the body to a temp file and emits a path-backed - * `File` (use this for large responses). A string path writes there. + * `File` (use this for large responses). The temp file is removed when + * that `File` is released. A string path writes there and is left in place. * (default: `null` — load the body as a string) * * `client`: Guzzle `Client` instance. Tests inject a mock handler here. * (default: `new Client()`) diff --git a/packages/pulp/README.md b/packages/pulp/README.md index a54aba1..499add4 100644 --- a/packages/pulp/README.md +++ b/packages/pulp/README.md @@ -112,7 +112,7 @@ Pulp::start() ->run(); ``` -- `sink => true` writes the body to a temp file and emits a path-backed `File`, so a ~100 MB response does not have to live as a PHP string. Pass a path string to choose the file. +- `sink => true` writes the body to a temp file and emits a path-backed `File`, so a ~100 MB response does not have to live as a PHP string. The temp file is removed when that `File` is released. Pass a path string to keep the body at a path you manage. - `client` injects a Guzzle `Client` (tests use a mock handler). - `successStatuses` throws unless the status is in the list. Pair with Guzzle `http_errors => false` when 304/204 are first-class. - The file also carries `httpStatus`, `httpLastModified`, `httpEtag`, and `httpType`. diff --git a/packages/pulp/src/File.php b/packages/pulp/src/File.php index 378ca55..cd8820e 100644 --- a/packages/pulp/src/File.php +++ b/packages/pulp/src/File.php @@ -17,6 +17,7 @@ class File private array $___data = []; private ?string $___contentPath = null; + private bool $___unlinkOnDestruct = false; /** * File constructor. @@ -26,7 +27,11 @@ public function __construct(public $fileName, ?string $srcFileName = null) $this->srcFileName = empty($srcFileName) ? $this->fileName : $srcFileName; } - public static function fromPath(string $path, ?string $fileName = null): File + /** + * @param bool $unlinkOnDestruct Remove `$path` when this object is released. + * Only for temp files this `File` owns. Caller-supplied paths stay. + */ + public static function fromPath(string $path, ?string $fileName = null, bool $unlinkOnDestruct = false): File { $file = new File( $fileName ?: $path, @@ -38,6 +43,7 @@ public static function fromPath(string $path, ?string $fileName = null): File } $file->___contentPath = $path; + $file->___unlinkOnDestruct = $unlinkOnDestruct; $stats = @stat($path); if ($stats !== false) { @@ -47,6 +53,17 @@ public static function fromPath(string $path, ?string $fileName = null): File return $file; } + public function __destruct() + { + if (!$this->___unlinkOnDestruct || $this->___contentPath === null) { + return; + } + + $path = $this->___contentPath; + $this->___unlinkOnDestruct = false; + @unlink($path); + } + /** * @return resource */ diff --git a/packages/pulp/src/SrcHttpHandler.php b/packages/pulp/src/SrcHttpHandler.php index 3f9012a..07ab810 100644 --- a/packages/pulp/src/SrcHttpHandler.php +++ b/packages/pulp/src/SrcHttpHandler.php @@ -10,6 +10,9 @@ class SrcHttpHandler extends AbstractHandler { + /** Temp file created for `sink => true`. Null once a File owns it, or after it is removed. */ + private ?string $ephemeralSinkPath = null; + protected function getConstructorParamDefs(): array { return ['method', 'uri', 'guzzleOptions', 'aliasFileName', 'options']; @@ -39,7 +42,13 @@ public function onEnd(): void $this->assertSuccessStatus($statusCode); if ($sinkPath !== null) { - $tmpFile = File::fromPath($sinkPath, $this->cp->aliasFileName); + $tmpFile = File::fromPath( + $sinkPath, + $this->cp->aliasFileName, + $this->ephemeralSinkPath !== null, + ); + // File removes this on release. A caller-supplied path is not ephemeral. + $this->ephemeralSinkPath = null; } else { $tmpFile = new File($this->cp->aliasFileName); $tmpFile->content = (string) $res->getBody(); @@ -64,6 +73,8 @@ public function onEnd(): void } else { throw $err; } + } finally { + $this->discardEphemeralSink(); } if ($file instanceof File) { @@ -91,6 +102,8 @@ private function resolveSinkPath(): ?string throw new RuntimeException('Unable to create HTTP sink temp file'); } + $this->ephemeralSinkPath = $path; + return $path; } if (is_string($sink) && $sink !== '') { @@ -102,6 +115,17 @@ private function resolveSinkPath(): ?string return is_string($guzzleSink) && $guzzleSink !== '' ? $guzzleSink : null; } + private function discardEphemeralSink(): void + { + if ($this->ephemeralSinkPath === null) { + return; + } + + $path = $this->ephemeralSinkPath; + $this->ephemeralSinkPath = null; + @unlink($path); + } + private function assertSuccessStatus(int $statusCode): void { $allowed = $this->cp->options['successStatuses'] ?? null; diff --git a/packages/pulp/test/FileTest.php b/packages/pulp/test/FileTest.php index 6f2b55c..ffb60cd 100644 --- a/packages/pulp/test/FileTest.php +++ b/packages/pulp/test/FileTest.php @@ -77,6 +77,21 @@ public function testStreamReadsFromPathWithoutTouchingContent(): void } } + public function testFromPathCanUnlinkWhenReleased(): void + { + $tmpFile = tempnam(sys_get_temp_dir(), 'pulp-file-test-'); + $this->assertIsString($tmpFile); + file_put_contents($tmpFile, 'owned'); + + $f = File::fromPath($tmpFile, 'file.txt', true); + $this->assertSame('owned', $f->content); + $this->assertFileExists($tmpFile); + + unset($f); + + $this->assertFileDoesNotExist($tmpFile); + } + public function testStreamFromGeneratedContent(): void { $f = new File('file.txt'); diff --git a/packages/pulp/test/SrcHttpHandlerTest.php b/packages/pulp/test/SrcHttpHandlerTest.php index 7bfaef7..f999edd 100644 --- a/packages/pulp/test/SrcHttpHandlerTest.php +++ b/packages/pulp/test/SrcHttpHandlerTest.php @@ -60,6 +60,88 @@ public function testSinkWritesPathBackedFile(): void } } + public function testEphemeralSinkStaysUntilTheFileIsReleased(): void + { + $result = Pulp::start() + ->pipe(Pulp::srcHttp( + 'GET', + 'https://example.test/large.json', + [], + 'large.json', + [ + 'client' => $this->client(new Response(200, [], '{"sites":true}')), + 'sink' => true, + ] + )) + ->run(); + + $path = $result[0]->srcFileName; + $this->assertIsString($path); + $this->assertMatchesRegularExpression('#/pulp-http-[^/]+$#', $path); + $this->assertFileExists($path); + $this->assertSame('{"sites":true}', $result[0]->content); + $this->assertFileExists($path); + + unset($result); + gc_collect_cycles(); + + $this->assertFileDoesNotExist($path); + } + + public function testEphemeralSinkIsRemovedWhenTheRequestFails(): void + { + $before = $this->ephemeralSinks(); + + try { + Pulp::start() + ->pipe(Pulp::srcHttp( + 'GET', + 'https://example.test/status', + ['http_errors' => false], + 'status.json', + [ + 'client' => $this->client(new Response(500, [], 'nope')), + 'sink' => true, + 'successStatuses' => [200], + ] + )) + ->run(); + $this->fail('Expected the failed response to throw'); + } catch (RuntimeException $err) { + $this->assertStringContainsString('returned 500', $err->getMessage()); + } + + $this->assertSame($before, $this->ephemeralSinks()); + } + + public function testCallerSinkPathIsKeptWhenTheRequestFails(): void + { + $sink = tempnam(sys_get_temp_dir(), 'pulp-http-sink-'); + $this->assertIsString($sink); + + try { + Pulp::start() + ->pipe(Pulp::srcHttp( + 'GET', + 'https://example.test/status', + ['http_errors' => false], + 'status.json', + [ + 'client' => $this->client(new Response(500, [], 'nope')), + 'sink' => $sink, + 'successStatuses' => [200], + ] + )) + ->run(); + $this->fail('Expected the failed response to throw'); + } catch (RuntimeException $err) { + $this->assertStringContainsString('returned 500', $err->getMessage()); + $this->assertFileExists($sink); + } finally { + @unlink($sink); + } + } + public function testSuccessStatusesRejectUnexpectedCodes(): void { $this->expectException(RuntimeException::class); @@ -101,6 +183,16 @@ public function testNotModifiedStillEmitsFileWithStatus(): void $this->assertSame('', $result[0]->content); } + /** + * @return list + */ + private function ephemeralSinks(): array + { + $paths = glob(sys_get_temp_dir() . '/pulp-http-*'); + + return is_array($paths) ? $paths : []; + } + private function client(Response $response): Client { return new Client([ From a5c273185476ff2c2a1063ebdfc862c852380486 Mon Sep 17 00:00:00 2001 From: Paul Golmann Date: Thu, 1 Oct 2026 13:49:31 +0200 Subject: [PATCH 2/2] Share srcHttp temp sink ownership across File clones. split, shadow, and results clone path-backed files. A per-object unlink flag deleted the sink when the first clone was released. --- packages/pulp/CHANGELOG.md | 2 +- packages/pulp/Pulp.php | 3 +- packages/pulp/README.md | 2 +- packages/pulp/src/File.php | 25 +++++++--------- packages/pulp/src/SharedTempPath.php | 20 +++++++++++++ packages/pulp/test/FileTest.php | 18 ++++++++++++ packages/pulp/test/SrcHttpHandlerTest.php | 36 +++++++++++++++++++++++ 7 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 packages/pulp/src/SharedTempPath.php diff --git a/packages/pulp/CHANGELOG.md b/packages/pulp/CHANGELOG.md index 822a645..fbb9eaf 100644 --- a/packages/pulp/CHANGELOG.md +++ b/packages/pulp/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to `mapsight/pulp` are documented here. ### Fixed -- Remove the temp file created by `srcHttp` `sink => true` when the path-backed `File` is released. A failed request removes it immediately. A caller-supplied sink path is left in place. +- Remove the temp file created by `srcHttp` `sink => true` when the last path-backed `File` is released, including clones. A failed request removes it immediately. A caller-supplied sink path is left in place. ## 1.2.0 - 2026-09-02 diff --git a/packages/pulp/Pulp.php b/packages/pulp/Pulp.php index b872490..cf34264 100644 --- a/packages/pulp/Pulp.php +++ b/packages/pulp/Pulp.php @@ -61,7 +61,8 @@ public static function srcFile( * ## Options * * `sink`: `true` writes the body to a temp file and emits a path-backed * `File` (use this for large responses). The temp file is removed when - * that `File` is released. A string path writes there and is left in place. + * the last `File` that shares it is released, including clones. A string + * path writes there and is left in place. * (default: `null` — load the body as a string) * * `client`: Guzzle `Client` instance. Tests inject a mock handler here. * (default: `new Client()`) diff --git a/packages/pulp/README.md b/packages/pulp/README.md index 499add4..a78fec2 100644 --- a/packages/pulp/README.md +++ b/packages/pulp/README.md @@ -112,7 +112,7 @@ Pulp::start() ->run(); ``` -- `sink => true` writes the body to a temp file and emits a path-backed `File`, so a ~100 MB response does not have to live as a PHP string. The temp file is removed when that `File` is released. Pass a path string to keep the body at a path you manage. +- `sink => true` writes the body to a temp file and emits a path-backed `File`, so a ~100 MB response does not have to live as a PHP string. The temp file is removed when the last `File` that shares it is released, including clones from `split`, `shadow`, and `results`. Pass a path string to keep the body at a path you manage. - `client` injects a Guzzle `Client` (tests use a mock handler). - `successStatuses` throws unless the status is in the list. Pair with Guzzle `http_errors => false` when 304/204 are first-class. - The file also carries `httpStatus`, `httpLastModified`, `httpEtag`, and `httpType`. diff --git a/packages/pulp/src/File.php b/packages/pulp/src/File.php index cd8820e..6e3b03b 100644 --- a/packages/pulp/src/File.php +++ b/packages/pulp/src/File.php @@ -17,7 +17,11 @@ class File private array $___data = []; private ?string $___contentPath = null; - private bool $___unlinkOnDestruct = false; + + /** + * Shared by clones. The path is removed when the last File releases it. + */ + private ?SharedTempPath $___tempPath = null; /** * File constructor. @@ -28,8 +32,8 @@ public function __construct(public $fileName, ?string $srcFileName = null) } /** - * @param bool $unlinkOnDestruct Remove `$path` when this object is released. - * Only for temp files this `File` owns. Caller-supplied paths stay. + * @param bool $unlinkOnDestruct Remove `$path` when the last File that shares + * it is released, including clones. Caller-supplied paths stay. */ public static function fromPath(string $path, ?string $fileName = null, bool $unlinkOnDestruct = false): File { @@ -43,7 +47,9 @@ public static function fromPath(string $path, ?string $fileName = null, bool $un } $file->___contentPath = $path; - $file->___unlinkOnDestruct = $unlinkOnDestruct; + if ($unlinkOnDestruct) { + $file->___tempPath = new SharedTempPath($path); + } $stats = @stat($path); if ($stats !== false) { @@ -53,17 +59,6 @@ public static function fromPath(string $path, ?string $fileName = null, bool $un return $file; } - public function __destruct() - { - if (!$this->___unlinkOnDestruct || $this->___contentPath === null) { - return; - } - - $path = $this->___contentPath; - $this->___unlinkOnDestruct = false; - @unlink($path); - } - /** * @return resource */ diff --git a/packages/pulp/src/SharedTempPath.php b/packages/pulp/src/SharedTempPath.php new file mode 100644 index 0000000..63507f3 --- /dev/null +++ b/packages/pulp/src/SharedTempPath.php @@ -0,0 +1,20 @@ +path); + } +} diff --git a/packages/pulp/test/FileTest.php b/packages/pulp/test/FileTest.php index ffb60cd..525eb67 100644 --- a/packages/pulp/test/FileTest.php +++ b/packages/pulp/test/FileTest.php @@ -92,6 +92,24 @@ public function testFromPathCanUnlinkWhenReleased(): void $this->assertFileDoesNotExist($tmpFile); } + public function testOwnedPathSurvivesUntilTheLastCloneIsReleased(): void + { + $tmpFile = tempnam(sys_get_temp_dir(), 'pulp-file-test-'); + $this->assertIsString($tmpFile); + file_put_contents($tmpFile, 'owned'); + + $original = File::fromPath($tmpFile, 'file.txt', true); + $clone = clone $original; + + unset($original); + $this->assertFileExists($tmpFile); + $this->assertSame('owned', $clone->content); + + unset($clone); + + $this->assertFileDoesNotExist($tmpFile); + } + public function testStreamFromGeneratedContent(): void { $f = new File('file.txt'); diff --git a/packages/pulp/test/SrcHttpHandlerTest.php b/packages/pulp/test/SrcHttpHandlerTest.php index f999edd..c4d3eb4 100644 --- a/packages/pulp/test/SrcHttpHandlerTest.php +++ b/packages/pulp/test/SrcHttpHandlerTest.php @@ -88,6 +88,42 @@ public function testEphemeralSinkStaysUntilTheFileIsReleased(): void $this->assertFileDoesNotExist($path); } + public function testEphemeralSinkSurvivesClonesUntilTheLastFileIsReleased(): void + { + $kept = []; + $result = Pulp::start() + ->pipe(Pulp::srcHttp( + 'GET', + 'https://example.test/large.json', + [], + 'large.json', + [ + 'client' => $this->client(new Response(200, [], '{"sites":true}')), + 'sink' => true, + ] + )) + ->pipe(Pulp::results(static function (array $files) use (&$kept): void { + $kept = $files; + })) + ->run(); + + $path = $result[0]->srcFileName; + $this->assertIsString($path); + $this->assertFileExists($path); + + unset($result); + gc_collect_cycles(); + + $this->assertFileExists($path); + $this->assertSame('{"sites":true}', $kept[0]->content); + $this->assertFileExists($path); + + unset($kept); + gc_collect_cycles(); + + $this->assertFileDoesNotExist($path); + } + public function testEphemeralSinkIsRemovedWhenTheRequestFails(): void { $before = $this->ephemeralSinks();