Add read-through filesystems for gradual storage migrations - #620
Conversation
Add Laravel's read-through disk with primary writes, fallback reads, optional promotion, deletion on both disks, fallback copy/move and visibility handling. Port the current upstream tests and documentation. Adapt Flysystem operations to pooled disk lifetimes. Streams retain their leases until closed; fallback copies release the source before borrowing the primary, allowing both sides to share a capacity-one pool. Materialize pooled and native cloud listings before returning them. Preserve native cloud stream options and range requests while applying the composite disk's failure policy once, including through Sentry decorators. Preserve outer and side prefixes for paths and URLs, honor composite URL callbacks, reject circular construction with coroutine-local state, and close owned resources on errors and cancellation. Upstream: laravel/framework#61140 laravel/framework#61155 laravel/framework#61272 laravel/framework#61375 Deletion follow-up: a833cea6ad444f64833d65f075b2a73e34b8872b Source: laravel/framework master cd6e81dff3ba7a4564ac88c3949698c728d20109 Docs: laravel/docs 13.x eff8739e9090c2a0216fefac8e33dacdd689f8f6 Validation: full parallel suite, full static analysis and formatting pass. After final corrections, the affected parallel suite also passes, including pooled and non-pooled cloud reads, cleanup, Sentry storage and generated facade checks.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: hypervel/components/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (16)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis pull request adds read-through filesystem disks with configurable primary and fallback sides. It adds optional promotion of fallback content, pooled stream and range-read support, disk construction and URL integration, documentation, and tests. ChangesRead-through filesystem
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReadThroughFilesystemAdapter
participant PrimaryFilesystem
participant FallbackFilesystem
Caller->>ReadThroughFilesystemAdapter: Read path
ReadThroughFilesystemAdapter->>PrimaryFilesystem: Check file existence
ReadThroughFilesystemAdapter->>FallbackFilesystem: Read when absent from primary
ReadThroughFilesystemAdapter->>PrimaryFilesystem: Promote fallback content when copy is enabled
ReadThroughFilesystemAdapter-->>Caller: Return file content
Merge Risk: ⚪ Minimal · up to This change adds read-through disks for gradual storage migrations, with fallback reads, optional copying to the primary disk, and pooled cloud stream support. No concrete merge-blocking defect has been established. One unconfirmed concern remains: promoting a file to an S3 or GCS primary disk might fail if the cloud SDK closes the temporary stream after upload. The owner may want to confirm this against a real cloud primary. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Read-through storage enables online migration, but concurrent operations or a failed move can make old file contents visible again. The risk is limited to disks configured for this feature; deployment permissions and external access paths are not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.72% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 109 functions across 14 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@cubic-dev-ai review |
@binaryfire I have started the AI code review. It will take a few minutes to complete. |
PR Summary by QodoAdd read-through filesystems for gradual storage migrations
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review by Qodo
1. Promoted private files become public
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 16 files
Confidence score: 2/5
- In
ReadThroughFilesystemAdapter.php, promotion can overwrite a concurrent write or recreate a deleted file, and a failed promotion can leave a corrupt primary object that later reads prefer; serialize promotion and remove partial objects before swallowing failures. - In
ReadThroughFilesystemAdapter.php, promotion may apply the primary disk’s public visibility to a private fallback file, exposing it; preserve fallback visibility across write, stream, and copy promotions. - In
FilesystemManager.php, the missinghypervel/contextdependency can break standalone construction, while assuming everyClouddisk hasgetOperator()can break scoped or custom disks; declare the dependency and avoid relying on that method for all implementations. - In
ReadThroughFilesystem.php, temporary URL support can be advertised when the selected disk cannot sign URLs, and URL generation may add a network existence check on every call; gate support on signing capability and avoid the repeated probe where possible.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/filesystem/src/FilesystemManager.php">
<violation number="1" location="src/filesystem/src/FilesystemManager.php:10">
P1: Declare `hypervel/context` in `src/filesystem/composer.json`'s `require` section; standalone installations otherwise cannot resolve `CoroutineContext` when constructing read-through disks.
(Based on your team's feedback about split-package runtime dependencies.)</violation>
<violation number="2" location="src/filesystem/src/FilesystemManager.php:529">
P2: Do not assume every `Cloud` implementation provides `getOperator()`; scoped and custom cloud disks can satisfy the contract but fail here during construction.</violation>
</file>
<file name="src/filesystem/src/ReadThroughFilesystemAdapter.php">
<violation number="1" location="src/filesystem/src/ReadThroughFilesystemAdapter.php:84">
P1: This promotion can race a primary write or delete: another coroutine can change the path after `fileExists()` returns false, then this unconditional write overwrites new contents or recreates a deleted file. Serialize promotions with writes and deletes, or use an atomic conditional promotion.</violation>
<violation number="2" location="src/filesystem/src/ReadThroughFilesystemAdapter.php:84">
P1: Preserve the fallback file's visibility on promotion writes; otherwise a primary configured with public visibility can expose a private fallback file. Apply the same inheritance to stream promotion and fallback copy/move.</violation>
<violation number="3" location="src/filesystem/src/ReadThroughFilesystemAdapter.php:86">
P1: Remove or invalidate a partially written primary object before swallowing a promotion failure; otherwise later reads prefer the corrupt object over the intact fallback.</violation>
</file>
<file name="src/filesystem/src/ReadThroughFilesystem.php">
<violation number="1" location="src/filesystem/src/ReadThroughFilesystem.php:50">
P3: `url()` and `temporaryUrl()` route through `readerFor()`, which performs a live `$this->primary->fileExists()` on every call. For S3/GCS this adds a network HEAD request per URL, and on pooled primary disks it borrows a client each time; for fallback-only files it is a HEAD on the primary plus the fallback URL lookup. Consider caching the side decision per request or documenting that URL generation costs an existence check.</violation>
<violation number="2" location="src/filesystem/src/ReadThroughFilesystem.php:58">
P2: Advertise temporary URL support only when every selected disk can sign URLs, unless the composite callback handles the request; otherwise fallback-only files can pass this check and fail in `temporaryUrl()`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| use Aws\S3\S3Client; | ||
| use Closure; | ||
| use Google\Cloud\Storage\StorageClient as GcsClient; | ||
| use Hypervel\Context\CoroutineContext; |
There was a problem hiding this comment.
P1: Declare hypervel/context in src/filesystem/composer.json's require section; standalone installations otherwise cannot resolve CoroutineContext when constructing read-through disks.
(Based on your team's feedback about split-package runtime dependencies.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/FilesystemManager.php, line 10:
<comment>Declare `hypervel/context` in `src/filesystem/composer.json`'s `require` section; standalone installations otherwise cannot resolve `CoroutineContext` when constructing read-through disks.
(Based on your team's feedback about split-package runtime dependencies.) </comment>
<file context>
@@ -7,6 +7,7 @@
use Aws\S3\S3Client;
use Closure;
use Google\Cloud\Storage\StorageClient as GcsClient;
+use Hypervel\Context\CoroutineContext;
use Hypervel\Contracts\Container\Container;
use Hypervel\Contracts\Filesystem\Cloud;
</file context>
There was a problem hiding this comment.
Added hypervel/context as a direct filesystem dependency in c78a17d. It was already installed transitively through hypervel/coroutine, so the standalone resolution failure did not occur, but the direct declaration is appropriate.
| } | ||
|
|
||
| try { | ||
| $this->primary->write($path, $contents); |
There was a problem hiding this comment.
P1: This promotion can race a primary write or delete: another coroutine can change the path after fileExists() returns false, then this unconditional write overwrites new contents or recreates a deleted file. Serialize promotions with writes and deletes, or use an atomic conditional promotion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/ReadThroughFilesystemAdapter.php, line 84:
<comment>This promotion can race a primary write or delete: another coroutine can change the path after `fileExists()` returns false, then this unconditional write overwrites new contents or recreates a deleted file. Serialize promotions with writes and deletes, or use an atomic conditional promotion.</comment>
<file context>
@@ -0,0 +1,340 @@
+ }
+
+ try {
+ $this->primary->write($path, $contents);
+ } catch (FilesystemException $exception) {
+ $this->handlePromotionFailure($path, $exception);
</file context>
There was a problem hiding this comment.
The driver does not provide atomic operations across two disks. The docs now explicitly require coordinating concurrent writes and deletions while a path is copied. A worker-local lock cannot protect other workers or external migration processes; a portable atomic promotion would require a different storage contract. Keeping Laravel behavior without adding an incomplete synchronization guarantee.
| try { | ||
| $this->primary->write($path, $contents); | ||
| } catch (FilesystemException $exception) { | ||
| $this->handlePromotionFailure($path, $exception); |
There was a problem hiding this comment.
P1: Remove or invalidate a partially written primary object before swallowing a promotion failure; otherwise later reads prefer the corrupt object over the intact fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/ReadThroughFilesystemAdapter.php, line 86:
<comment>Remove or invalidate a partially written primary object before swallowing a promotion failure; otherwise later reads prefer the corrupt object over the intact fallback.</comment>
<file context>
@@ -0,0 +1,340 @@
+ try {
+ $this->primary->write($path, $contents);
+ } catch (FilesystemException $exception) {
+ $this->handlePromotionFailure($path, $exception);
+ }
+
</file context>
There was a problem hiding this comment.
Not deleting the destination after a failed write. That cleanup could remove a newer valid file from another writer, and cannot identify which bytes belong to this attempt. Write publication and partial-write behavior are properties of the underlying adapter; the read-through disk does not promise rollback.
| } | ||
|
|
||
| try { | ||
| $this->primary->write($path, $contents); |
There was a problem hiding this comment.
P1: Preserve the fallback file's visibility on promotion writes; otherwise a primary configured with public visibility can expose a private fallback file. Apply the same inheritance to stream promotion and fallback copy/move.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/ReadThroughFilesystemAdapter.php, line 84:
<comment>Preserve the fallback file's visibility on promotion writes; otherwise a primary configured with public visibility can expose a private fallback file. Apply the same inheritance to stream promotion and fallback copy/move.</comment>
<file context>
@@ -0,0 +1,340 @@
+ }
+
+ try {
+ $this->primary->write($path, $contents);
+ } catch (FilesystemException $exception) {
+ $this->handlePromotionFailure($path, $exception);
</file context>
There was a problem hiding this comment.
Removed the shipped public visibility overrides for S3 and GCS in c78a17d, with the matching skeleton correction. Explicit public writes still work. The docs now state that copied files use the primary disk configuration. Source ACL inheritance is not added because visibility does not map consistently across local and cloud storage and some stores do not support ACL queries.
| ); | ||
| } | ||
|
|
||
| return $disk instanceof FilesystemAdapter ? $disk->getDriver() : $disk->getOperator(); // @phpstan-ignore method.notFound (Pooled decorators forward the borrow-safe accessor.) |
There was a problem hiding this comment.
P2: Do not assume every Cloud implementation provides getOperator(); scoped and custom cloud disks can satisfy the contract but fail here during construction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/FilesystemManager.php, line 529:
<comment>Do not assume every `Cloud` implementation provides `getOperator()`; scoped and custom cloud disks can satisfy the contract but fail here during construction.</comment>
<file context>
@@ -434,6 +440,95 @@ public function createS3Driver(array $config): Cloud
+ );
+ }
+
+ return $disk instanceof FilesystemAdapter ? $disk->getDriver() : $disk->getOperator(); // @phpstan-ignore method.notFound (Pooled decorators forward the borrow-safe accessor.)
+ }
+
</file context>
There was a problem hiding this comment.
Documented the supported composition. Static configured scoped disks work through configuration resolution. A dynamic scoped proxy should wrap the read-through disk, rather than be an individual side; it deliberately rejects raw internal access to protect its prefix. Arbitrary Cloud implementations do not necessarily supply the additional Flysystem and adapter capabilities this feature requires. No unsafe raw accessor was added.
| */ | ||
| public function temporaryUrl(string $path, DateTimeInterface $expiration, array $options = []): string | ||
| { | ||
| return isset($this->temporaryUrlCallback) |
There was a problem hiding this comment.
P2: Advertise temporary URL support only when every selected disk can sign URLs, unless the composite callback handles the request; otherwise fallback-only files can pass this check and fail in temporaryUrl().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/ReadThroughFilesystem.php, line 58:
<comment>Advertise temporary URL support only when every selected disk can sign URLs, unless the composite callback handles the request; otherwise fallback-only files can pass this check and fail in `temporaryUrl()`.</comment>
<file context>
@@ -0,0 +1,129 @@
+ */
+ public function temporaryUrl(string $path, DateTimeInterface $expiration, array $options = []): string
+ {
+ return isset($this->temporaryUrlCallback)
+ ? ($this->temporaryUrlCallback)($path, $expiration, $options)
+ : $this->readerFor($path)->temporaryUrl($this->readThroughPrefixer->prefixPath($path), $expiration, $options); // @phpstan-ignore method.notFound
</file context>
There was a problem hiding this comment.
Keeping Laravel's OR behavior. This method has no path argument and reports whether signing is available, not whether every possible file can be signed. AND would incorrectly hide primary signing support when the fallback cannot sign. The per-file method selects the containing disk, and a composite callback can handle both.
| */ | ||
| public function url(string $path): string | ||
| { | ||
| return $this->readerFor($path)->url($this->readThroughPrefixer->prefixPath($path)); |
There was a problem hiding this comment.
P3: url() and temporaryUrl() route through readerFor(), which performs a live $this->primary->fileExists() on every call. For S3/GCS this adds a network HEAD request per URL, and on pooled primary disks it borrows a client each time; for fallback-only files it is a HEAD on the primary plus the fallback URL lookup. Consider caching the side decision per request or documenting that URL generation costs an existence check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/filesystem/src/ReadThroughFilesystem.php, line 50:
<comment>`url()` and `temporaryUrl()` route through `readerFor()`, which performs a live `$this->primary->fileExists()` on every call. For S3/GCS this adds a network HEAD request per URL, and on pooled primary disks it borrows a client each time; for fallback-only files it is a HEAD on the primary plus the fallback URL lookup. Consider caching the side decision per request or documenting that URL generation costs an existence check.</comment>
<file context>
@@ -0,0 +1,129 @@
+ */
+ public function url(string $path): string
+ {
+ return $this->readerFor($path)->url($this->readThroughPrefixer->prefixPath($path));
+ }
+
</file context>
There was a problem hiding this comment.
Keeping the existence check: it selects the disk that actually contains the file. Caching that decision can return a stale location during migration. The docs now include URLs among the operations that consult the containing disk without copying the file.
Keep the read-through promotion stream open when an upload adapter closes its input, including the Google Cloud Storage client. Give uploads a separate resource handle and detach its wrapper after use. Close streams conditionally in fallback copy/move and putFileAs so a completed upload cannot fail during cleanup. Use one pooled filesystem operator implementation for both client and whole-driver pools. Preserve native cloud reads, range requests, HTTP options and stream leases without applying the inner disk's error policy. Declare the filesystem package's direct context dependency. Remove the shipped public visibility overrides for S3 and GCS, keeping private adapter defaults and explicit public opt-in. Clarify destination visibility, dynamic scoping, concurrent mutation and promotion failure semantics in the read-through documentation. Extend coverage for closing upload adapters, actual Google client stream ownership with a fake HTTP transport, whole-driver cloud ranges and leases, shipped-config write ACLs, and cancellation cleanup. Keep these corrections together because their regression cases share complete test files. Follow-up to laravel/framework#61140 Source: laravel/framework master cd6e81dff3ba7a4564ac88c3949698c728d20109 Validation: changed test files and affected parallel filesystem, storage, Sentry, facade and foundation suites pass. Full static analysis, formatting, facade regeneration and diff checks pass.
| 'storage_api_uri' => env('GOOGLE_CLOUD_STORAGE_API_URI', null), // see: Public URLs below | ||
| 'api_endpoint' => env('GOOGLE_CLOUD_STORAGE_API_ENDPOINT', null), // set storageClient apiEndpoint | ||
| 'visibility' => 'public', // optional: public|private | ||
| 'visibility_handler' => null, // optional: set to \League\Flysystem\GoogleCloudStorage\UniformBucketLevelAccessVisibility::class to enable uniform bucket level access |
There was a problem hiding this comment.
public setting makes ordinary writes create private objects. Applications that relied on those files being publicly accessible will find that their existing public URLs cannot retrieve new uploads. Preserve the prior default or provide an explicit migration path.
There was a problem hiding this comment.
This default change is intentional. Hypervel 0.4 is unreleased, so preserving earlier application behavior is not a compatibility requirement. Cloud files should be private unless the application explicitly requests public visibility. The existing visibility option and public-write APIs remain available, and the shipped configuration tests cover both the private default and an explicit public override. We will keep the private default; no migration layer is needed.
|
@coderabbitai @greptile-apps @cubic-dev-ai @qodo-free-for-open-source-projects The latest changes:
Regression coverage includes real Google client uploads with a fake HTTP transport, stream cleanup, both pool types, and private/default versus explicit public writes. |
Thanks for the update. The changes cover the key risks: stream ownership during promotion and uploads, pooled native cloud reads, private-by-default cloud writes, and scoped-disk/concurrency behavior. The added regression coverage appears aligned with those cases. I don’t have any Qodo findings to resolve on this PR. |
|
❌ Action failedReview failed.
|
|
Thanks for the update. The changes address the previously raised risks around stream ownership during promotion and uploads, pooled native cloud reads, private-by-default cloud writes, and scoped-disk and concurrency behavior. The added regression coverage matches those cases, so I have no further findings to resolve on this PR. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
1 participant
This adds a
read-throughfilesystem driver for moving files between storage disks without taking the application offline. New files are written to the primary disk. Reads use the primary when the file is present and fall back to the old disk otherwise. By default, a fallback read also copies the file to primary; settingcopytofalseleaves migration to an external process.The feature comes from laravel/framework#61140, with its follow-up changes for optional copying, copy and move operations, and visibility handling. Deletes remove files from both disks so an old fallback copy cannot reappear on the next read. Directory listings come from primary only.
Operations across the two disks are not atomic. Applications must coordinate writes and deletions to a path while it is being copied.
Adapting it to pooled connections
Hypervel's cloud disks borrow connections from a pool. An open download stream keeps its connection until the stream closes. Passing that stream directly into an upload can leave the upload waiting for a connection the download is still holding. This can happen when two disks share a pool, including disks with different buckets or prefixes, or when concurrent transfers exhaust the available connections.
Fallback transfers therefore finish reading into a temporary stream and release the source connection before borrowing the destination connection. Laravel already does this when a read copies a file to primary; Hypervel also applies it to fallback copy and move operations. Streams and borrowed connections are released on failure and coroutine cancellation. Streams returned to callers remain open until the caller closes them.
Directory listings from pooled and native cloud sides are fully loaded before being returned. This lets a caller read, copy or delete files while processing the listing without retaining the connection used to fetch it.
Resource tradeoffs
Buffering avoids holding two pooled connections at once, but it adds temporary I/O and makes the download and upload sequential. PHP's
php://tempkeeps small contents in memory and spills to the system temporary directory after its default memory threshold. Large transfers therefore need temporary space for the whole file. That space grows with concurrent transfers; the threshold is not a limit on total memory or temporary storage. A memory-backed temporary directory uses RAM for spilled files too.Fully loaded listings use memory in proportion to the number of entries. Ordinary file-listing helpers already collect their results, but callers using raw
listContents()must account for this behavior. With Hypervel's normal Swoole hooks, network transfers and temporary-file reads and writes yield to other coroutines while waiting for I/O; the calling coroutine still waits for its transfer to complete.Other integration details
Includes filesystem documentation and coverage for upstream behavior, shared pools, stream cleanup, cloud reads, prefixes and URL handling. The full parallel test suite, static analysis and formatting checks passed; affected tests also passed after the final corrections.