fix: let the build job deliver its output to the builds device - #3
Conversation
Source PR: appwrite#13465 Source head: e19ddfb
⛔ Shipwright · BlockedRecommendation: do not merge PR #3 · Tier
Findings (18)
Fireworks usage: 30,434 input · 2,404 output · 32,838 total tokens · $0.0083 · 32s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| function getDevice(string $root, string $connection = ''): Device | ||
| { | ||
| $connection = ! empty($connection) ? $connection : System::getEnv('_APP_CONNECTIONS_STORAGE', ''); | ||
| $configuredDevice = DeviceType::tryFrom(strtolower(System::getEnv('_APP_STORAGE_DEVICE', DeviceType::Local->value))) ?? DeviceType::Local; |
There was a problem hiding this comment.
Shipwright · CRITICAL
getDevice and objectUrl disagree on bucket resolution.
Impact: getDevice and objectUrl disagree on bucket resolution. getDevice skips _APP_CONNECTIONS_STORAGE when generic S3 config is present, but objectUrl can still derive the bucket from the legacy connection DSN. The device can read from one bucket while the sidecar uploads to another, making remote builds produce artifacts that ready() cannot find.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| DeviceType::Backblaze => 'BACKBLAZE', | ||
| DeviceType::Linode => 'LINODE', | ||
| DeviceType::Wasabi => 'WASABI', | ||
| DeviceType::S3, DeviceType::AwsS3, DeviceType::Local => null, |
There was a problem hiding this comment.
Shipwright · CRITICAL
objectUrl infers path-style addressing solely from the presence of _APP_STORAGE_S3_ENDPOINT.
Impact: objectUrl infers path-style addressing solely from the presence of _APP_STORAGE_S3_ENDPOINT. The actual addressing mode is a device configuration property, not equivalent to endpoint presence. A virtual-host device with an endpoint will omit the bucket, while a path-style device without an endpoint will prepend it, uploading to the wrong object key.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| ], | ||
| ], | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Shipwright · CRITICAL
buildPath already returns a device path that may include the bucket for path-style S3 devices, and objectUrl can prepend the bucket again for virtual-host devices.
Impact: buildPath already returns a device path that may include the bucket for path-style S3 devices, and objectUrl can prepend the bucket again for virtual-host devices. There is no normalization, so the artifact can be uploaded under a double-bucket key that deviceForBuilds never reads.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| $bucket = $prefix === null ? '' : System::getEnv("_APP_STORAGE_{$prefix}_BUCKET", ''); | ||
| } | ||
|
|
||
| return 's3://' . \ltrim(($bucket !== '' ? "/{$bucket}" : '') . $path, '/'); |
There was a problem hiding this comment.
Shipwright · CRITICAL
objectUrl builds s3:// URLs by raw concatenation of bucket and path without URL-encoding.
Impact: objectUrl builds s3:// URLs by raw concatenation of bucket and path without URL-encoding. Bucket or path values containing spaces, '#', '%', or non-ASCII characters produce malformed artifact URLs, causing uploads/downloads to fail or target the wrong object. This is a new URL-construction path introduced by the diff.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| $bucket = $prefix === null ? '' : System::getEnv("_APP_STORAGE_{$prefix}_BUCKET", ''); | ||
| } | ||
|
|
||
| return 's3://' . \ltrim(($bucket !== '' ? "/{$bucket}" : '') . $path, '/'); |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'objectUrl' method builds an s3:// URL by concatenating the bucket and path without URL-encoding either component.
Impact: The new 'objectUrl' method builds an s3:// URL by concatenating the bucket and path without URL-encoding either component. If the bucket or path contains characters that are not valid in a URL path (e.g. spaces, '#', '%', or non-ASCII characters), the resulting artifact URL will be malformed and the sidecar upload/download will fail or target the wrong object. The previous code did not construct s3:// URLs, so this…
Suggested fix: Fix the review finding before release.
| function getDevice(string $root, string $connection = ''): Device | ||
| { | ||
| $connection = ! empty($connection) ? $connection : System::getEnv('_APP_CONNECTIONS_STORAGE', ''); | ||
| $configuredDevice = DeviceType::tryFrom(strtolower(System::getEnv('_APP_STORAGE_DEVICE', DeviceType::Local->value))) ?? DeviceType::Local; |
There was a problem hiding this comment.
Shipwright · HIGH
In 'getDevice', the fallback to '_APP_CONNECTIONS_STORAGE' is skipped when the configured device is S3/AwsS3 and 'hasS3Configuration' is true, but the code still reads '_APP_CONNEC
Impact: In 'getDevice', the fallback to '_APP_CONNECTIONS_STORAGE' is skipped when the configured device is S3/AwsS3 and 'hasS3Configuration' is true, but the code still reads '_APP_CONNECTIONS_STORAGE' later in 'objectUrl' to determine the bucket. This creates an inconsistency: 'getDevice' may construct an S3 device from the generic S3 variables while 'objectUrl' may derive the bucket from the legacy connection DSN, causin…
Suggested fix: Fix the review finding before release.
| DeviceType::Backblaze => 'BACKBLAZE', | ||
| DeviceType::Linode => 'LINODE', | ||
| DeviceType::Wasabi => 'WASABI', | ||
| DeviceType::S3, DeviceType::AwsS3, DeviceType::Local => null, |
There was a problem hiding this comment.
Shipwright · HIGH
The 'objectUrl' method uses 'System::getEnv('_APP_STORAGE_S3_ENDPOINT', '') !== ''' to decide whether to omit the bucket for path-style S3 devices.
Impact: The 'objectUrl' method uses 'System::getEnv('_APP_STORAGE_S3_ENDPOINT', '') !== ''' to decide whether to omit the bucket for path-style S3 devices. However, the device's actual path-style behavior is determined by the device configuration, not merely the presence of an endpoint. If an S3-compatible endpoint is configured but the device is not path-style, the bucket will be omitted and the upload will target the wron…
Suggested fix: Fix the review finding before release.
| ], | ||
| ], | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Shipwright · HIGH
The 'storage' method now constructs 'UploadArtifact' objects with 'out' set to 'static::objectUrl($device, static::buildPath($projectId, $deploymentId))'.
Impact: The 'storage' method now constructs 'UploadArtifact' objects with 'out' set to 'static::objectUrl($device, static::buildPath($projectId, $deploymentId))'. The 'buildPath' method calls 'static::device($projectId)->getPath(...)', which may return a path that already includes the bucket for path-style S3 devices. The 'objectUrl' method then prepends the bucket again for virtual-host devices, resulting in a double-bu…
Suggested fix: Fix the review finding before release.
| DeviceType::Backblaze => 'BACKBLAZE', | ||
| DeviceType::Linode => 'LINODE', | ||
| DeviceType::Wasabi => 'WASABI', | ||
| DeviceType::S3, DeviceType::AwsS3, DeviceType::Local => null, |
There was a problem hiding this comment.
Shipwright · HIGH
The 'objectUrl' method uses 'System::getEnv('_APP_STORAGE_S3_ENDPOINT', '') !== ''' to determine path-style addressing, but the 'getDevice' function does not pass the endpoint to t
Impact: The 'objectUrl' method uses 'System::getEnv('_APP_STORAGE_S3_ENDPOINT', '') !== ''' to determine path-style addressing, but the 'getDevice' function does not pass the endpoint to the device constructor. This means the device may be configured with a different endpoint than the one used to construct the artifact URL, causing the upload to target a different storage backend than the one the device reads from. The diff…
Suggested fix: Fix the review finding before release.
| function getDevice(string $root, string $connection = ''): Device | ||
| { | ||
| $connection = ! empty($connection) ? $connection : System::getEnv('_APP_CONNECTIONS_STORAGE', ''); | ||
| $configuredDevice = DeviceType::tryFrom(strtolower(System::getEnv('_APP_STORAGE_DEVICE', DeviceType::Local->value))) ?? DeviceType::Local; |
There was a problem hiding this comment.
Shipwright · HIGH
hasS3Configuration requires access key, secret, and bucket but omits region.
Impact: hasS3Configuration requires access key, secret, and bucket but omits region. An empty region can construct an S3 device with an invalid or default region, causing authentication or endpoint failures; provider-specific paths previously required region.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| ], | ||
| ], | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Shipwright · HIGH
storage() performs a synchronous $device->exists($cachePath) remote call on every submission.
Impact: storage() performs a synchronous $device->exists($cachePath) remote call on every submission. A slow or unavailable storage backend blocks deployment submission and can cause timeouts or failures; the previous code had no remote existence check.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| return $deployment; | ||
| } | ||
|
|
||
| if ($deviceForBuilds->getType() !== DeviceType::Local && $cache->load('jobs-output-' . $deploymentId, self::DEDUPE_TTL) === false) { |
There was a problem hiding this comment.
Shipwright · HIGH
ready() requires the jobs-output marker based on the worker's current deviceForBuilds type, while submission-time storage() decides whether an output artifact is generated.
Impact: ready() requires the jobs-output marker based on the worker's current deviceForBuilds type, while submission-time storage() decides whether an output artifact is generated. If device configuration changes between submission and callback processing, the deployment can hang waiting for a marker that was never produced, or finalize without required output.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| DeviceType::Wasabi => 'WASABI', | ||
| DeviceType::S3, DeviceType::AwsS3, DeviceType::Local => null, | ||
| }; | ||
| $bucket = $prefix === null ? '' : System::getEnv("_APP_STORAGE_{$prefix}_BUCKET", ''); |
There was a problem hiding this comment.
Shipwright · HIGH
objectUrl parses _APP_CONNECTIONS_STORAGE with new DSN($connection) without handling malformed input.
Impact: objectUrl parses _APP_CONNECTIONS_STORAGE with new DSN($connection) without handling malformed input. An invalid legacy DSN can throw during deployment submission, a new failure mode introduced by this diff.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| ], | ||
| ], | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'storage' method now calls '$device->exists($cachePath)' on every build submission.
Impact: The 'storage' method now calls '$device->exists($cachePath)' on every build submission. For remote devices, this performs a synchronous network request to the storage backend before the job is submitted. If the storage backend is slow or unavailable, this will block the deployment submission and may cause timeouts or failures. The previous code did not perform any remote existence check. This is a concrete perfor…
Suggested fix: Fix the review finding before release.
| // callback explicitly because complete is emitted after artifacts but | ||
| // the queue can deliver those callbacks out of order. | ||
| if (($data['artifactId'] ?? '') === 'output') { | ||
| if (($data['status'] ?? '') === 'failed') { |
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'onArtifact' method now treats any artifact with 'artifactId' equal to ''output'' as the remote output delivery marker, but the 'storage' method only creates an 'UploadArtifact
Impact: The 'onArtifact' method now treats any artifact with 'artifactId' equal to ''output'' as the remote output delivery marker, but the 'storage' method only creates an 'UploadArtifact' with id ''output'' for remote devices. For local devices, no such artifact is created, so the 'jobs-output-' cache marker is never set. However, 'ready' only checks for this marker when '$deviceForBuilds->getType() !== DeviceType::Loc…
Suggested fix: Fix the review finding before release.
| return $deployment; | ||
| } | ||
|
|
||
| if ($deviceForBuilds->getType() !== DeviceType::Local && $cache->load('jobs-output-' . $deploymentId, self::DEDUPE_TTL) === false) { |
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'ready' method now checks '$deviceForBuilds->getType() !== DeviceType::Local' to decide whether to require the 'jobs-output-' marker.
Impact: The 'ready' method now checks '$deviceForBuilds->getType() !== DeviceType::Local' to decide whether to require the 'jobs-output-' marker. However, '$deviceForBuilds' is injected into the worker and may not reflect the same device configuration used at submission time. If the storage device configuration changes between submission and callback processing, the worker may require an output marker that was never gene…
Suggested fix: Fix the review finding before release.
| DeviceType::Wasabi => 'WASABI', | ||
| DeviceType::S3, DeviceType::AwsS3, DeviceType::Local => null, | ||
| }; | ||
| $bucket = $prefix === null ? '' : System::getEnv("_APP_STORAGE_{$prefix}_BUCKET", ''); |
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'objectUrl' method uses 'new DSN($connection)' to parse the legacy connection string, but it does not handle the case where the DSN is invalid or malformed.
Impact: The 'objectUrl' method uses 'new DSN($connection)' to parse the legacy connection string, but it does not handle the case where the DSN is invalid or malformed. If '_APP_CONNECTIONS_STORAGE' contains an invalid DSN, the constructor may throw an exception, causing the entire deployment submission to fail. The previous code did not parse the DSN in this path, so this is a new failure mode introduced by the diff.
Suggested fix: Fix the review finding before release.
| function getDevice(string $root, string $connection = ''): Device | ||
| { | ||
| $connection = ! empty($connection) ? $connection : System::getEnv('_APP_CONNECTIONS_STORAGE', ''); | ||
| $configuredDevice = DeviceType::tryFrom(strtolower(System::getEnv('_APP_STORAGE_DEVICE', DeviceType::Local->value))) ?? DeviceType::Local; |
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'getDevice' function now reads 'APP_STORAGE_S3*' variables and uses them to construct the device, but it does not validate that the S3 region is provided.
Impact: The 'getDevice' function now reads 'APP_STORAGE_S3*' variables and uses them to construct the device, but it does not validate that the S3 region is provided. The 'hasS3Configuration' check only requires access key, secret, and bucket, omitting region. If the region is empty, the S3 device may be constructed with an invalid or default region, causing authentication or endpoint failures. The previous code required…
Suggested fix: Fix the review finding before release.
1 participant
Summary
Reworks appwrite#13443. That fix had the Jobs worker copy the finished build artifact off the builds volume onto the storage device in a new
Deployments::store(), which Cloud then had to override with an empty method. One decision (where build output lands) was split across two overridable methods that had to agree.This moves the remote branch of Cloud's storage strategy down into CE and selects it by the builds device:
build.shwrites straight tobuildPath.build.shwrites into the job workspace and the sidecar moves output and cache overs3://upload/download artifacts, signed with the orchestrator'sS3_*credentials. The job delivers the artifact; the worker only checksbuildPathexists.buildPath()/cachePath()return the builds device's path viagetDevice()->getPath(). On the local device and on a virtual-host device that is the same string as before; on a path-style store with a bucket (MinIO with_APP_STORAGE_S3_ENDPOINT) it is keyed under the bucket, which was the original bug.objectUrl()maps a device path to thes3://bucket/keyURL, from the same configurationgetDevice()reads.docker-compose.ymlpasses_APP_STORAGE_S3_*through to the orchestrator asS3_*. Orchestrator 1.9.2 already signss3://artifacts from those, so nothing changes there.store(), the device constructor parameter, and theDeploymentsinjection into the Jobs worker are removed.Jobs.phpand both factories are back to their pre-fix: publish build output to the builds device when storage is not local appwrite/appwrite#13443 shape.Unlike Cloud, compression stays in
build.sh(_APP_COMPUTE_BUILD_COMPRESSION); the sidecar archive step remains Cloud's own override.Cloud
Cloud
maincompiles and behaves the same against this branch: the constructor is back to four arguments, Cloud'sstorage()override is untouched, andbuildPath()/cachePath()return identical strings on its virtual-host AWS device. The new CE helper isobjectUrl()rather thanurl()because Cloud has aprivate static url(), which would otherwise be a fatal visibility clash. appwrite-labs/cloud#5612 becomes unnecessary; a follow-up Cloud PR drops its privateurl()in favour ofobjectUrl().Verification
composer lint, PHPStan on the four PHP files,composer refactor:checkpass._APP_STORAGE_DEVICE=s3, executor on the same bucket): thes3grouptestDeploymentBuildOutputIsServedFromTheBuildsDevicepasses (build ready, execution completed, output and source downloaded through the device), and SitestestCreateDeploymentpasses. The cache squashfs lands understorage/builds/app-{project}/cache/in the bucket.S3 StorageCI group and MinIO service from fix: publish build output to the builds device when storage is not local appwrite/appwrite#13443 stay and cover this in CI.Source merge-base:
e6a468a11fe70378596d168b0b4e5329b5f18ba2Source head:
e19ddfb33b4db47b0b0a0c44e13d72b6fcb65560