From bbc8600431f1c4870600e6221a2e8c7e1654de91 Mon Sep 17 00:00:00 2001 From: Tobias Harnickell Date: Wed, 29 Jul 2026 16:45:21 +0200 Subject: [PATCH 1/3] perf(files_external): opt-in same-endpoint S3 MultipartCopy Fixes #62645. Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Tobias Harnickell --- apps/files_external/lib/ConfigLexicon.php | 2 + .../lib/Lib/Storage/AmazonS3.php | 324 ++++++++++++++++++ .../tests/Storage/Amazons3MultiPartTest.php | 35 ++ .../tests/Storage/Amazons3Test.php | 181 ++++++++++ .../tests/Storage/CrossMountS3ConfigTrait.php | 62 ++++ .../tests/env/start-amazons3-ceph.sh | 4 + 6 files changed, 608 insertions(+) create mode 100644 apps/files_external/tests/Storage/CrossMountS3ConfigTrait.php diff --git a/apps/files_external/lib/ConfigLexicon.php b/apps/files_external/lib/ConfigLexicon.php index 427bcdfcc6f07..31f5a24c76767 100644 --- a/apps/files_external/lib/ConfigLexicon.php +++ b/apps/files_external/lib/ConfigLexicon.php @@ -23,6 +23,7 @@ class ConfigLexicon implements ILexicon { public const ALLOW_USER_MOUNTING = 'allow_user_mounting'; public const USER_MOUNTING_BACKENDS = 'user_mounting_backends'; + public const AMAZONS3_SERVER_SIDE_COPY = 'amazons3_server_side_copy'; #[\Override] public function getStrictness(): Strictness { @@ -34,6 +35,7 @@ public function getAppConfigs(): array { return [ new Entry(self::ALLOW_USER_MOUNTING, ValueType::BOOL, false, 'allow users to mount their own external filesystems', true), new Entry(self::USER_MOUNTING_BACKENDS, ValueType::STRING, '', 'list of mounting backends available for users', true), + new Entry(self::AMAZONS3_SERVER_SIDE_COPY, ValueType::BOOL, false, 'dispatch server-side S3 copy for cross-mount MOVE/COPY between AmazonS3 mounts sharing an endpoint. The destination-mount credential MUST hold s3:GetObject on the source bucket.', true), ]; } diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 7de496e8fba1a..3aa37f672792e 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -9,6 +9,7 @@ namespace OCA\Files_External\Lib\Storage; use Aws\S3\Exception\S3Exception; +use Aws\S3\MultipartCopy; use Icewind\Streams\CallbackWrapper; use Icewind\Streams\CountWrapper; use Icewind\Streams\IteratorDirectory; @@ -16,11 +17,17 @@ use OC\Files\ObjectStore\S3ConnectionTrait; use OC\Files\ObjectStore\S3ObjectTrait; use OC\Files\Storage\Common; +use OC\Files\Storage\Wrapper\Encryption; +use OC\Files\Storage\Wrapper\Jail; +use OC\Files\Storage\Wrapper\Wrapper; +use OCA\Files_External\ConfigLexicon; use OCP\Cache\CappedMemoryCache; use OCP\Constants; use OCP\Files\FileInfo; use OCP\Files\IMimeTypeDetector; use OCP\Files\NotPermittedException; +use OCP\Files\Storage\IStorage; +use OCP\IAppConfig; use OCP\ICache; use OCP\ICacheFactory; use OCP\ITempManager; @@ -45,6 +52,21 @@ class AmazonS3 extends Common { private ICache $memCache; private ?bool $versioningEnabled = null; + private bool $serverSideCopyEnabled; + + /** Failure counter keyed by endpoint fingerprint. Static so sibling mounts share state within a request. */ + private static array $serverSideCopyFailureCounter = []; + + /** Per-endpoint cap. Fast path stays disabled for the rest of the request after this many consecutive S3Exceptions. */ + private const SERVER_SIDE_COPY_FAILURE_LIMIT = 4; + + /** + * @internal Reset the per-endpoint failure counter. Test-only. + */ + public static function resetServerSideCopyFailureCounter(): void { + self::$serverSideCopyFailureCounter = []; + } + public function __construct(array $parameters) { parent::__construct($parameters); $this->parseParams($parameters); @@ -56,6 +78,12 @@ public function __construct(array $parameters) { $cacheFactory = Server::get(ICacheFactory::class); $this->memCache = $cacheFactory->createLocal('s3-external'); $this->logger = Server::get(LoggerInterface::class); + // Fallback covers installs that predate the lexicon entry registration. + $this->serverSideCopyEnabled = Server::get(IAppConfig::class)->getValueBool( + 'files_external', + ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, + false, + ); } private function normalizePath(string $path): string { @@ -826,4 +854,300 @@ public function getDirectDownloadById(string $fileId): array|false { $entry = $this->getCache()->get((int)$fileId); return $this->getDirectDownload($entry->getPath()); } + + #[Override] + public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, bool $preserveMtime = false): bool { + if ($preserveMtime === true) { + return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime); + } + $eligibility = $this->evaluateFastPathEligibility($sourceStorage, $sourceInternalPath); + if ($eligibility === null) { + return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime); + } + $unwrappedSource = $eligibility['source']; + $unjailedSourcePath = $eligibility['path']; + $identityKey = $eligibility['identityKey']; + $sourceKey = $unwrappedSource->normalizePath($unjailedSourcePath); + $targetKey = $this->normalizePath($targetInternalPath); + + if ($this->isSameObjectAsPeer($unwrappedSource, $sourceKey, $targetKey)) { + return true; + } + + try { + $this->performServerSideCopy($unwrappedSource, $unjailedSourcePath, $targetInternalPath); + self::$serverSideCopyFailureCounter[$identityKey] = 0; + return true; + } catch (S3Exception $e) { + $this->handleFastPathFailure($e, $targetKey, $identityKey, 'copy'); + return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime); + } + } + + #[Override] + public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { + $eligibility = $this->evaluateFastPathEligibility($sourceStorage, $sourceInternalPath); + if ($eligibility === null) { + return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); + } + $unwrappedSource = $eligibility['source']; + $unjailedSourcePath = $eligibility['path']; + $identityKey = $eligibility['identityKey']; + $sourceKey = $unwrappedSource->normalizePath($unjailedSourcePath); + $targetKey = $this->normalizePath($targetInternalPath); + + if ($this->isSameObjectAsPeer($unwrappedSource, $sourceKey, $targetKey)) { + // Move where source and target resolve to the same S3 object is malformed. + // Delegating to the parent path would copy-to-self then unlink the source, destroying the object. + $this->logger->warning('S3 cross-mount move: source and target resolve to the same object. Rejected as malformed.', [ + 'app' => 'files_external', + 'bucket' => $this->getBucket(), + 'key' => $sourceKey, + ]); + return false; + } + + try { + $this->performServerSideCopy($unwrappedSource, $unjailedSourcePath, $targetInternalPath); + } catch (S3Exception $e) { + $this->handleFastPathFailure($e, $targetKey, $identityKey, 'move'); + return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); + } + + if (!$this->deleteSourceAfterMove($unwrappedSource, $unjailedSourcePath, $targetInternalPath)) { + return false; + } + self::$serverSideCopyFailureCounter[$identityKey] = 0; + return true; + } + + /** + * Evaluate fast-path guards. Feature flag, encryption wrapper, wrapper unwrap, endpoint + * identity, failure limit. + * + * @return array{source: self, path: string, identityKey: string}|null Payload for the + * fast-path caller when eligible. null when the streamed parent path MUST be taken. + */ + private function evaluateFastPathEligibility(IStorage $sourceStorage, string $sourceInternalPath): ?array { + if (!$this->serverSideCopyEnabled) { + return null; + } + if ($sourceStorage->instanceOfStorage(Encryption::class)) { + return null; + } + + $unwrapped = $this->unwrapSource($sourceStorage, $sourceInternalPath); + if ($unwrapped === null) { + return null; + } + [$unwrappedSource, $unjailedSourcePath] = $unwrapped; + + if (!$this->isSameS3Endpoint($unwrappedSource)) { + return null; + } + + $identityKey = $this->endpointIdentityKey(); + if ((self::$serverSideCopyFailureCounter[$identityKey] ?? 0) >= self::SERVER_SIDE_COPY_FAILURE_LIMIT) { + return null; + } + + return [ + 'source' => $unwrappedSource, + 'path' => $unjailedSourcePath, + 'identityKey' => $identityKey, + ]; + } + + private function handleFastPathFailure(S3Exception $exception, string $targetKey, string $identityKey, string $operation): void { + $this->abortMultipartUploadForKey($targetKey); + $this->logger->warning('S3 server-side ' . $operation . ' failed, falling back to stream copy', [ + 'app' => 'files_external', + 'exception' => $exception, + ]); + self::$serverSideCopyFailureCounter[$identityKey] = (self::$serverSideCopyFailureCounter[$identityKey] ?? 0) + 1; + } + + /** + * @return array{0: self, 1: string}|null [unwrapped storage, unjailed path] when the terminal + * storage is an AmazonS3, else null. + */ + private function unwrapSource(IStorage $sourceStorage, string $sourceInternalPath): ?array { + $current = $sourceStorage; + $path = $sourceInternalPath; + while ($current instanceof Wrapper) { + if ($current instanceof Jail) { + $path = $current->getUnjailedPath($path); + $current = $current->getUnjailedStorage(); + continue; + } + $current = $current->getWrapperStorage(); + } + if (!$current instanceof self) { + return null; + } + return [$current, $path]; + } + + private function isSameS3Endpoint(self $other): bool { + return $this->endpointFingerprint() === $other->endpointFingerprint(); + } + + /** + * @return list Normalised endpoint identity tuple. Two AmazonS3 instances with + * identical fingerprints target the same S3 endpoint under the same access-key ID. + */ + private function endpointFingerprint(): array { + return [ + $this->params['hostname'], + (string)(int)$this->params['port'], + ($this->params['use_ssl'] ?? true) ? '1' : '0', + $this->params['region'], + $this->params['key'], + ]; + } + + /** + * Opaque map key derived from `endpointFingerprint`. Hashed so the raw access key does + * not linger in a request-scoped static array. + */ + private function endpointIdentityKey(): string { + return hash('xxh128', implode("\0", $this->endpointFingerprint())); + } + + private function isSameObjectAsPeer(self $peer, string $sourceKey, string $targetKey): bool { + return $peer->getBucket() === $this->getBucket() && $sourceKey === $targetKey; + } + + /** + * Copy $sourceInternalPath from a peer AmazonS3 into $this at $targetInternalPath via S3 + * server-side operations. Recurse into directories. Forward SSE-C source parameters so + * HEAD against an encrypted source succeeds. + */ + private function performServerSideCopy(self $source, string $sourceInternalPath, string $targetInternalPath): void { + if (!$source->is_dir($sourceInternalPath)) { + $this->copySingleObjectFromPeer($source, $sourceInternalPath, $targetInternalPath); + return; + } + + // Pre-existing target directory MUST be tolerated. Per-file failures propagate. + $this->mkdir($targetInternalPath); + foreach ($source->getDirectoryContent($sourceInternalPath) as $item) { + $childSource = $sourceInternalPath . '/' . $item['name']; + $childTarget = $targetInternalPath . '/' . $item['name']; + if ($item['mimetype'] === FileInfo::MIMETYPE_FOLDER) { + $this->performServerSideCopy($source, $childSource, $childTarget); + } else { + $this->copySingleObjectFromPeer($source, $childSource, $childTarget); + } + } + } + + private function copySingleObjectFromPeer(self $source, string $sourceInternalPath, string $targetInternalPath): void { + $this->copyObjectFromForeignBucket( + $source->getBucket(), + $source->normalizePath($sourceInternalPath), + $this->normalizePath($targetInternalPath), + $source->getServerSideEncryptionParameters(), + $source->getSSECParameters(true), + ); + } + + /** + * Copy one object from a foreign bucket on the same endpoint into $this bucket. Uses the + * trait's MultipartCopy dispatch for large payloads and single-op copy for small ones. + * + * @param array $sourceHeadParams SSE parameters used against the source HEAD. SSE-C sources MUST include SSECustomerKey. + * @param array $sourceCopyParams CopySource* SSE-C parameters. KMS re-encryption uses destination-side params only. + * @throws S3Exception on transport, permission, or provider-compat failure. + */ + private function copyObjectFromForeignBucket( + string $sourceBucket, + string $from, + string $to, + array $sourceHeadParams, + array $sourceCopyParams, + ): void { + $sourceMetadata = $this->getConnection()->headObject([ + 'Bucket' => $sourceBucket, + 'Key' => $from, + ] + $sourceHeadParams); + + $size = (int)$sourceMetadata->get('ContentLength'); + $sseParams = $this->getServerSideEncryptionParameters() + $sourceCopyParams; + + if ($this->useMultipartCopy && $size > $this->copySizeLimit) { + $copy = new MultipartCopy($this->getConnection(), [ + 'source_bucket' => $sourceBucket, + 'source_key' => $from, + ], [ + 'bucket' => $this->getBucket(), + 'key' => $to, + 'acl' => 'private', + 'params' => $sseParams, + 'source_metadata' => $sourceMetadata, + ]); + $copy->copy(); + return; + } + + // Threshold decided above. Disable the SDK-internal MPU switch. + $this->getConnection()->copy($sourceBucket, $from, $this->getBucket(), $to, 'private', [ + 'params' => $sseParams, + 'mup_threshold' => PHP_INT_MAX, + ]); + } + + /** + * Delete the source after a successful server-side copy. Roll back the destination on + * cleanup failure to mirror AmazonS3::rename() semantics and to let the caller retry. + */ + private function deleteSourceAfterMove(self $source, string $sourcePath, string $targetPath): bool { + $deleted = $source->is_dir($sourcePath) ? $source->rmdir($sourcePath) : $source->unlink($sourcePath); + if ($deleted) { + return true; + } + if ($this->is_dir($targetPath)) { + $this->rmdir($targetPath); + } else { + $this->unlink($targetPath); + } + return false; + } + + /** + * Abort any in-flight MultipartUpload for $targetKey in this bucket. Errors are logged, not + * thrown. Match on exact key: prefix match would cancel unrelated concurrent uploads. + * Paginate to cover buckets with more than 1000 in-flight uploads. + */ + private function abortMultipartUploadForKey(string $targetKey): void { + try { + $keyMarker = ''; + $uploadIdMarker = ''; + do { + $response = $this->getConnection()->listMultipartUploads([ + 'Bucket' => $this->getBucket(), + 'Prefix' => $targetKey, + 'KeyMarker' => $keyMarker, + 'UploadIdMarker' => $uploadIdMarker, + ]); + foreach ($response['Uploads'] ?? [] as $upload) { + if (($upload['Key'] ?? null) !== $targetKey) { + continue; + } + $this->getConnection()->abortMultipartUpload([ + 'Bucket' => $this->getBucket(), + 'Key' => $targetKey, + 'UploadId' => $upload['UploadId'], + ]); + } + $keyMarker = $response['NextKeyMarker'] ?? ''; + $uploadIdMarker = $response['NextUploadIdMarker'] ?? ''; + } while (($response['IsTruncated'] ?? false) === true); + } catch (S3Exception $e) { + $this->logger->warning('Failed to clean up multipart upload after server-side copy error', [ + 'app' => 'files_external', + 'exception' => $e, + ]); + } + } } diff --git a/apps/files_external/tests/Storage/Amazons3MultiPartTest.php b/apps/files_external/tests/Storage/Amazons3MultiPartTest.php index 8a694e2095646..25713991ba92f 100644 --- a/apps/files_external/tests/Storage/Amazons3MultiPartTest.php +++ b/apps/files_external/tests/Storage/Amazons3MultiPartTest.php @@ -8,6 +8,7 @@ namespace OCA\Files_External\Tests\Storage; +use OC\Files\Cache\Scanner; use OCA\Files_External\Lib\Storage\AmazonS3; /** @@ -20,6 +21,11 @@ #[\PHPUnit\Framework\Attributes\Group(name: 'S3')] class Amazons3MultiPartTest extends \Test\Files\Storage\Storage { use ConfigurableStorageTrait; + use CrossMountS3ConfigTrait; + + // Above the peer's 1-byte copy threshold: forces the MultipartCopy branch on every object. + private const MPU_TRIGGER_BYTES = 6 * 1024 * 1024; + /** @var AmazonS3 */ protected $instance; @@ -27,7 +33,12 @@ protected function setUp(): void { parent::setUp(); $this->loadConfig(__DIR__ . '/../config.amazons3.php'); + if ($this->config['run_cross_mount'] ?? false) { + $this->enableServerSideCopyFlagBeforeInstances(); + } + // putSizeLimit / copySizeLimit = 1 forces every single-object PUT and every copy to take + // the MultipartUploader / MultipartCopy branch. $this->instance = new AmazonS3($this->config + [ 'putSizeLimit' => 1, 'copySizeLimit' => 1, @@ -38,6 +49,8 @@ protected function tearDown(): void { if ($this->instance) { $this->instance->rmdir(''); } + AmazonS3::resetServerSideCopyFailureCounter(); + $this->restoreServerSideCopyFlag(); parent::tearDown(); } @@ -45,4 +58,26 @@ protected function tearDown(): void { public function testStat(): void { $this->markTestSkipped('S3 doesn\'t update the parents folder mtime'); } + + public function testCrossMountLargeFileTriggersMultipartCopy(): void { + $peer = $this->newPeerStorage([ + 'putSizeLimit' => 1, + 'copySizeLimit' => 1, + ]); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + + $payload = str_repeat('x', self::MPU_TRIGGER_BYTES); + $this->instance->file_put_contents('mpu-cross.bin', $payload); + $this->instance->getScanner()->scanFile('mpu-cross.bin'); + + $result = $peer->moveFromStorage($this->instance, 'mpu-cross.bin', 'mpu-cross.bin'); + $this->assertTrue($result); + $this->assertFalse($this->instance->file_exists('mpu-cross.bin'), 'source must be deleted after MPU-forcing move'); + $this->assertSame($payload, $peer->file_get_contents('mpu-cross.bin')); + } finally { + $peer->unlink('mpu-cross.bin'); + $this->instance->unlink('mpu-cross.bin'); + } + } } diff --git a/apps/files_external/tests/Storage/Amazons3Test.php b/apps/files_external/tests/Storage/Amazons3Test.php index 7bee84a32ff7f..f6ab334ca92eb 100644 --- a/apps/files_external/tests/Storage/Amazons3Test.php +++ b/apps/files_external/tests/Storage/Amazons3Test.php @@ -10,7 +10,14 @@ namespace OCA\Files_External\Tests\Storage; use OC\Files\Cache\Scanner; +use OCA\Files_External\ConfigLexicon; use OCA\Files_External\Lib\Storage\AmazonS3; +use OCP\Config\Lexicon\Preset; +use OCP\IAppConfig; +use OCP\Server; +use ReflectionClassConstant; +use ReflectionMethod; +use ReflectionProperty; /** * Class Amazons3Test @@ -22,6 +29,7 @@ #[\PHPUnit\Framework\Attributes\Group('S3')] class Amazons3Test extends \Test\Files\Storage\Storage { use ConfigurableStorageTrait; + use CrossMountS3ConfigTrait; /** @var AmazonS3 */ protected $instance; @@ -29,6 +37,9 @@ protected function setUp(): void { parent::setUp(); $this->loadConfig(__DIR__ . '/../config.amazons3.php'); + if ($this->config['run_cross_mount'] ?? false) { + $this->enableServerSideCopyFlagBeforeInstances(); + } $this->instance = new AmazonS3($this->config); } @@ -36,10 +47,18 @@ protected function tearDown(): void { if ($this->instance) { $this->instance->rmdir(''); } + AmazonS3::resetServerSideCopyFailureCounter(); + $this->restoreServerSideCopyFlag(); parent::tearDown(); } + private function arrangeSourceFile(string $path, string $payload, ?AmazonS3 $storage = null): void { + $storage = $storage ?? $this->instance; + $storage->file_put_contents($path, $payload); + $storage->getScanner()->scanFile($path); + } + /** * Regression test for the '.' vs '' root path mismatch in getDirectoryMetaData. * @@ -91,4 +110,166 @@ public function testGetMetaDataDirectoryPreservesStorageMtimeSeparateFromMtime() 'getMetaData must return storage_mtime from cache, not the propagated mtime' ); } + + public function testLexiconDefaultIsFalse(): void { + $lexicon = new ConfigLexicon(); + foreach ($lexicon->getAppConfigs() as $entry) { + if ($entry->getKey() === ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY) { + // Entry::convertFromBool serialises the bool default via '1'/'0' strings. + $this->assertSame('0', $entry->getDefault(Preset::PRIVATE)); + return; + } + } + $this->fail('AMAZONS3_SERVER_SIDE_COPY entry missing from ConfigLexicon'); + } + + public function testAppConfigReturnsFalseWhenFlagUnset(): void { + $this->restoreServerSideCopyFlag(); + $appConfig = Server::get(IAppConfig::class); + $appConfig->deleteKey('files_external', ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY); + // Passing true as the PHP fallback proves the lexicon default supplied the answer. + $this->assertFalse($appConfig->getValueBool('files_external', ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, true)); + } + + public function testCrossMountFileMoveEndToEnd(): void { + $peer = $this->newPeerStorage(); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + $this->arrangeSourceFile('cross-move-src.txt', 'payload-move'); + + $result = $peer->moveFromStorage($this->instance, 'cross-move-src.txt', 'cross-move-dst.txt'); + $this->assertTrue($result); + $this->assertFalse($this->instance->file_exists('cross-move-src.txt'), 'source must be deleted after move'); + $this->assertTrue($peer->file_exists('cross-move-dst.txt')); + $this->assertSame('payload-move', $peer->file_get_contents('cross-move-dst.txt')); + } finally { + $peer->unlink('cross-move-dst.txt'); + $this->instance->unlink('cross-move-src.txt'); + } + } + + public function testCrossMountFileCopyEndToEnd(): void { + $peer = $this->newPeerStorage(); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + $this->arrangeSourceFile('cross-copy-src.txt', 'payload-copy'); + + $result = $peer->copyFromStorage($this->instance, 'cross-copy-src.txt', 'cross-copy-dst.txt'); + $this->assertTrue($result); + $this->assertTrue($this->instance->file_exists('cross-copy-src.txt'), 'source must remain after copy'); + $this->assertTrue($peer->file_exists('cross-copy-dst.txt')); + $this->assertSame('payload-copy', $peer->file_get_contents('cross-copy-dst.txt')); + } finally { + $peer->unlink('cross-copy-dst.txt'); + $this->instance->unlink('cross-copy-src.txt'); + } + } + + public function testCrossMountDirectoryMoveRecurses(): void { + $peer = $this->newPeerStorage(); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + + $this->instance->mkdir('cross-tree'); + $this->instance->mkdir('cross-tree/sub'); + $this->instance->file_put_contents('cross-tree/root.txt', 'root-payload'); + $this->instance->file_put_contents('cross-tree/sub/leaf.txt', 'leaf-payload'); + $this->instance->getScanner()->scan('cross-tree'); + + $result = $peer->moveFromStorage($this->instance, 'cross-tree', 'cross-tree-dst'); + $this->assertTrue($result); + $this->assertFalse($this->instance->file_exists('cross-tree')); + $this->assertTrue($peer->is_dir('cross-tree-dst')); + $this->assertSame('root-payload', $peer->file_get_contents('cross-tree-dst/root.txt')); + $this->assertSame('leaf-payload', $peer->file_get_contents('cross-tree-dst/sub/leaf.txt')); + } finally { + $peer->rmdir('cross-tree-dst'); + $this->instance->rmdir('cross-tree'); + } + } + + public function testSameObjectCopyReturnsTrue(): void { + $this->requireCrossMountConfig(); + $peer = new AmazonS3($this->config); + try { + $this->arrangeSourceFile('same-object.txt', 'unchanged'); + + $result = $peer->copyFromStorage($this->instance, 'same-object.txt', 'same-object.txt'); + $this->assertTrue($result); + $this->assertSame('unchanged', $this->instance->file_get_contents('same-object.txt')); + } finally { + $this->instance->unlink('same-object.txt'); + } + } + + public function testSameObjectMoveReturnsFalseAndLogsWarning(): void { + $this->requireCrossMountConfig(); + $peer = new AmazonS3($this->config); + try { + $this->arrangeSourceFile('same-object-move.txt', 'preserved'); + + $result = $peer->moveFromStorage($this->instance, 'same-object-move.txt', 'same-object-move.txt'); + $this->assertFalse($result); + $this->assertSame('preserved', $this->instance->file_get_contents('same-object-move.txt'), 'source object must not be destroyed'); + } finally { + $this->instance->unlink('same-object-move.txt'); + } + } + + public function testFeatureFlagOffFallsBackToStreamCopy(): void { + $this->requireCrossMountConfig(); + // AmazonS3 snapshots the flag in its constructor. Rebuild both storages after flipping. + Server::get(IAppConfig::class)->setValueBool('files_external', ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, false); + $sourceOff = new AmazonS3($this->config); + $peer = $this->newPeerStorage(); + + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + $this->arrangeSourceFile('flag-off.txt', 'streamed', $sourceOff); + + $result = $peer->copyFromStorage($sourceOff, 'flag-off.txt', 'flag-off.txt'); + $this->assertTrue($result, 'streamed fallback must still copy the object'); + $this->assertSame('streamed', $peer->file_get_contents('flag-off.txt')); + } finally { + $peer->unlink('flag-off.txt'); + $sourceOff->unlink('flag-off.txt'); + } + } + + public function testPreserveMtimeTrueFallsBackToStreamCopy(): void { + $peer = $this->newPeerStorage(); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + $this->arrangeSourceFile('preserve-mtime.txt', 'mtime-preserved'); + + $result = $peer->copyFromStorage($this->instance, 'preserve-mtime.txt', 'preserve-mtime.txt', true); + $this->assertTrue($result); + $this->assertSame('mtime-preserved', $peer->file_get_contents('preserve-mtime.txt')); + } finally { + $peer->unlink('preserve-mtime.txt'); + $this->instance->unlink('preserve-mtime.txt'); + } + } + + public function testFailureLimitStopsFastPathAfterFourExceptions(): void { + $this->requireCrossMountConfig(); + $peer = $this->newPeerStorage(); + try { + $peer->getScanner()->scan('', Scanner::SCAN_SHALLOW); + $this->arrangeSourceFile('breaker.txt', 'via-fallback'); + + $counter = new ReflectionProperty(AmazonS3::class, 'serverSideCopyFailureCounter'); + $limit = (new ReflectionClassConstant(AmazonS3::class, 'SERVER_SIDE_COPY_FAILURE_LIMIT'))->getValue(); + $identityKey = (new ReflectionMethod(AmazonS3::class, 'endpointIdentityKey'))->invoke($peer); + $counter->setValue(null, [$identityKey => $limit]); + + $result = $peer->copyFromStorage($this->instance, 'breaker.txt', 'breaker.txt'); + $this->assertTrue($result, 'fallback stream copy must still succeed while breaker is tripped'); + $this->assertSame('via-fallback', $peer->file_get_contents('breaker.txt')); + $this->assertSame($limit, $counter->getValue()[$identityKey] ?? 0, 'counter must not reset while tripped'); + } finally { + $peer->unlink('breaker.txt'); + $this->instance->unlink('breaker.txt'); + } + } } diff --git a/apps/files_external/tests/Storage/CrossMountS3ConfigTrait.php b/apps/files_external/tests/Storage/CrossMountS3ConfigTrait.php new file mode 100644 index 0000000000000..df528ac6aae34 --- /dev/null +++ b/apps/files_external/tests/Storage/CrossMountS3ConfigTrait.php @@ -0,0 +1,62 @@ +config['run_cross_mount'] ?? false) || empty($this->config['bucket2'])) { + $this->markTestSkipped('run_cross_mount + bucket2 not set in config.amazons3.php'); + } + } + + /** + * Instantiate a peer AmazonS3 pointing at bucket2 on the same endpoint. Optional overrides + * merge onto the base config, letting the multipart test force `putSizeLimit`/`copySizeLimit`. + */ + protected function newPeerStorage(array $overrides = []): AmazonS3 { + $this->requireCrossMountConfig(); + $peerParams = array_merge($this->config, ['bucket' => $this->config['bucket2']], $overrides); + unset($peerParams['bucket2']); + return new AmazonS3($peerParams); + } + + /** + * MUST be called before any AmazonS3 instance is constructed because the constructor + * snapshots the flag once into a private property. + */ + protected function enableServerSideCopyFlagBeforeInstances(): void { + $appConfig = Server::get(IAppConfig::class); + $this->serverSideCopyPreviousValue = $appConfig->getValueBool( + 'files_external', + ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, + false, + ); + $appConfig->setValueBool('files_external', ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, true); + } + + protected function restoreServerSideCopyFlag(): void { + if ($this->serverSideCopyPreviousValue === null) { + return; + } + Server::get(IAppConfig::class)->setValueBool( + 'files_external', + ConfigLexicon::AMAZONS3_SERVER_SIDE_COPY, + $this->serverSideCopyPreviousValue, + ); + $this->serverSideCopyPreviousValue = null; + } +} diff --git a/apps/files_external/tests/env/start-amazons3-ceph.sh b/apps/files_external/tests/env/start-amazons3-ceph.sh index ecaa6e0ff46aa..f57c6aaab52c0 100755 --- a/apps/files_external/tests/env/start-amazons3-ceph.sh +++ b/apps/files_external/tests/env/start-amazons3-ceph.sh @@ -38,6 +38,8 @@ user=test accesskey=aaabbbccc secretkey=cccbbbaaa bucket=testbucket +# bucket2 is created lazily on first S3 call by S3ConnectionTrait::getConnection(). +bucket2=testbucket2 port=80 container=`docker run -d \ @@ -72,7 +74,9 @@ cat > $thisFolder/config.amazons3.php <true, + 'run_cross_mount'=>true, 'bucket'=>'$bucket', + 'bucket2'=>'$bucket2', 'hostname'=>'$host', 'port'=>'$port', 'key'=>'$accesskey', From 3931a82e314b4efc6c5d792296ab64551c9c4c63 Mon Sep 17 00:00:00 2001 From: Tobias Harnickell Date: Thu, 30 Jul 2026 10:20:22 +0200 Subject: [PATCH 2/3] style(files_external): trim AmazonS3 docblocks and inline comments Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Tobias Harnickell --- .../lib/Lib/Storage/AmazonS3.php | 42 +++++++------------ .../tests/env/start-amazons3-ceph.sh | 2 +- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 3aa37f672792e..68c3018518379 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -57,12 +57,10 @@ class AmazonS3 extends Common { /** Failure counter keyed by endpoint fingerprint. Static so sibling mounts share state within a request. */ private static array $serverSideCopyFailureCounter = []; - /** Per-endpoint cap. Fast path stays disabled for the rest of the request after this many consecutive S3Exceptions. */ + /** Fast path MUST stay disabled for the remainder of the request after this many consecutive S3Exceptions per endpoint. */ private const SERVER_SIDE_COPY_FAILURE_LIMIT = 4; - /** - * @internal Reset the per-endpoint failure counter. Test-only. - */ + /** @internal Test-only. */ public static function resetServerSideCopyFailureCounter(): void { self::$serverSideCopyFailureCounter = []; } @@ -897,7 +895,6 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP $targetKey = $this->normalizePath($targetInternalPath); if ($this->isSameObjectAsPeer($unwrappedSource, $sourceKey, $targetKey)) { - // Move where source and target resolve to the same S3 object is malformed. // Delegating to the parent path would copy-to-self then unlink the source, destroying the object. $this->logger->warning('S3 cross-mount move: source and target resolve to the same object. Rejected as malformed.', [ 'app' => 'files_external', @@ -921,13 +918,7 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP return true; } - /** - * Evaluate fast-path guards. Feature flag, encryption wrapper, wrapper unwrap, endpoint - * identity, failure limit. - * - * @return array{source: self, path: string, identityKey: string}|null Payload for the - * fast-path caller when eligible. null when the streamed parent path MUST be taken. - */ + /** @return array{source: self, path: string, identityKey: string}|null */ private function evaluateFastPathEligibility(IStorage $sourceStorage, string $sourceInternalPath): ?array { if (!$this->serverSideCopyEnabled) { return null; @@ -968,8 +959,10 @@ private function handleFastPathFailure(S3Exception $exception, string $targetKey } /** - * @return array{0: self, 1: string}|null [unwrapped storage, unjailed path] when the terminal - * storage is an AmazonS3, else null. + * Returns null when the terminal storage is not an AmazonS3. Otherwise returns the + * unwrapped storage plus the Jail-translated path. + * + * @return array{0: self, 1: string}|null */ private function unwrapSource(IStorage $sourceStorage, string $sourceInternalPath): ?array { $current = $sourceStorage; @@ -993,8 +986,10 @@ private function isSameS3Endpoint(self $other): bool { } /** - * @return list Normalised endpoint identity tuple. Two AmazonS3 instances with - * identical fingerprints target the same S3 endpoint under the same access-key ID. + * Normalised endpoint identity tuple. Two AmazonS3 instances with identical fingerprints + * target the same S3 endpoint under the same access-key ID. + * + * @return list */ private function endpointFingerprint(): array { return [ @@ -1018,11 +1013,7 @@ private function isSameObjectAsPeer(self $peer, string $sourceKey, string $targe return $peer->getBucket() === $this->getBucket() && $sourceKey === $targetKey; } - /** - * Copy $sourceInternalPath from a peer AmazonS3 into $this at $targetInternalPath via S3 - * server-side operations. Recurse into directories. Forward SSE-C source parameters so - * HEAD against an encrypted source succeeds. - */ + /** SSE-C source parameters MUST be forwarded so HEAD against an encrypted source succeeds. */ private function performServerSideCopy(self $source, string $sourceInternalPath, string $targetInternalPath): void { if (!$source->is_dir($sourceInternalPath)) { $this->copySingleObjectFromPeer($source, $sourceInternalPath, $targetInternalPath); @@ -1053,9 +1044,6 @@ private function copySingleObjectFromPeer(self $source, string $sourceInternalPa } /** - * Copy one object from a foreign bucket on the same endpoint into $this bucket. Uses the - * trait's MultipartCopy dispatch for large payloads and single-op copy for small ones. - * * @param array $sourceHeadParams SSE parameters used against the source HEAD. SSE-C sources MUST include SSECustomerKey. * @param array $sourceCopyParams CopySource* SSE-C parameters. KMS re-encryption uses destination-side params only. * @throws S3Exception on transport, permission, or provider-compat failure. @@ -1115,9 +1103,9 @@ private function deleteSourceAfterMove(self $source, string $sourcePath, string } /** - * Abort any in-flight MultipartUpload for $targetKey in this bucket. Errors are logged, not - * thrown. Match on exact key: prefix match would cancel unrelated concurrent uploads. - * Paginate to cover buckets with more than 1000 in-flight uploads. + * Errors MUST be logged, not thrown. Match on exact key: a prefix match would cancel + * unrelated concurrent uploads. Paginate to cover buckets with more than 1000 in-flight + * uploads. */ private function abortMultipartUploadForKey(string $targetKey): void { try { diff --git a/apps/files_external/tests/env/start-amazons3-ceph.sh b/apps/files_external/tests/env/start-amazons3-ceph.sh index f57c6aaab52c0..88bd872b39a04 100755 --- a/apps/files_external/tests/env/start-amazons3-ceph.sh +++ b/apps/files_external/tests/env/start-amazons3-ceph.sh @@ -38,7 +38,7 @@ user=test accesskey=aaabbbccc secretkey=cccbbbaaa bucket=testbucket -# bucket2 is created lazily on first S3 call by S3ConnectionTrait::getConnection(). +# bucket2 is created on first use by the peer storage. No docker provisioning step needed. bucket2=testbucket2 port=80 From 3d17f2e645f3972f54d4843d71b68b137e7b6baf Mon Sep 17 00:00:00 2001 From: Tobias Harnickell Date: Thu, 30 Jul 2026 14:31:02 +0200 Subject: [PATCH 3/3] fix(files_external): suppress Psalm ImpureStaticProperty and tidy docblocks Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Tobias Harnickell --- .../lib/Lib/Storage/AmazonS3.php | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/AmazonS3.php b/apps/files_external/lib/Lib/Storage/AmazonS3.php index 68c3018518379..8861a2aeb25bb 100644 --- a/apps/files_external/lib/Lib/Storage/AmazonS3.php +++ b/apps/files_external/lib/Lib/Storage/AmazonS3.php @@ -54,7 +54,11 @@ class AmazonS3 extends Common { private bool $serverSideCopyEnabled; - /** Failure counter keyed by endpoint fingerprint. Static so sibling mounts share state within a request. */ + /** + * Keyed by endpoint fingerprint. Static because sibling mounts share state within a request. + * + * @psalm-suppress ImpureStaticProperty MUST persist across per-mount instances within a request. + */ private static array $serverSideCopyFailureCounter = []; /** Fast path MUST stay disabled for the remainder of the request after this many consecutive S3Exceptions per endpoint. */ @@ -813,7 +817,7 @@ public function writeStream(string $path, $stream, ?int $size = null): int { return $size; } - #[Override] + #[\Override] public function getDirectDownload(string $path): array|false { if (!$this->isUsePresignedUrl()) { return false; @@ -843,7 +847,7 @@ public function getDirectDownload(string $path): array|false { ]; } - #[Override] + #[\Override] public function getDirectDownloadById(string $fileId): array|false { if (!$this->isUsePresignedUrl()) { return false; @@ -853,7 +857,7 @@ public function getDirectDownloadById(string $fileId): array|false { return $this->getDirectDownload($entry->getPath()); } - #[Override] + #[\Override] public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, bool $preserveMtime = false): bool { if ($preserveMtime === true) { return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime); @@ -882,7 +886,7 @@ public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalP } } - #[Override] + #[\Override] public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { $eligibility = $this->evaluateFastPathEligibility($sourceStorage, $sourceInternalPath); if ($eligibility === null) { @@ -959,10 +963,7 @@ private function handleFastPathFailure(S3Exception $exception, string $targetKey } /** - * Returns null when the terminal storage is not an AmazonS3. Otherwise returns the - * unwrapped storage plus the Jail-translated path. - * - * @return array{0: self, 1: string}|null + * @return array{0: self, 1: string}|null Unwrapped storage and Jail-translated path, or null when the terminal storage is not AmazonS3. */ private function unwrapSource(IStorage $sourceStorage, string $sourceInternalPath): ?array { $current = $sourceStorage; @@ -986,10 +987,7 @@ private function isSameS3Endpoint(self $other): bool { } /** - * Normalised endpoint identity tuple. Two AmazonS3 instances with identical fingerprints - * target the same S3 endpoint under the same access-key ID. - * - * @return list + * @return list Endpoint identity tuple including access-key ID so distinct credentials against the same host do not collide. */ private function endpointFingerprint(): array { return [ @@ -1086,8 +1084,7 @@ private function copyObjectFromForeignBucket( } /** - * Delete the source after a successful server-side copy. Roll back the destination on - * cleanup failure to mirror AmazonS3::rename() semantics and to let the caller retry. + * Roll back the destination on cleanup failure to mirror AmazonS3::rename() semantics and let the caller retry. */ private function deleteSourceAfterMove(self $source, string $sourcePath, string $targetPath): bool { $deleted = $source->is_dir($sourcePath) ? $source->rmdir($sourcePath) : $source->unlink($sourcePath);