From 0671848bfacbd2a7b5d91bdd53d9e90a4cac9b76 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Thu, 10 Sep 2026 17:23:26 -0300 Subject: [PATCH] Read settings written before the columns they name existed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @apps3000 in #1770. Upgrading a container from 2.0 or 2.1 with external storage configured restart-loops, and says the database is unreachable while the database is fine. A row hydrated from the database does not get the model's column defaults — only a new model does. So a row written before external_storage_settings.provider existed reads that column as null, and the enum match in isConfigured() throws UnhandledMatchError. That would be a small bug anywhere else. It is not here, because PlatformServiceProvider::boot() reads these settings on every process boot, and boot happens before `artisan migrate` runs. During an upgrade the code is new and the schema is still old, so every artisan command in that window dies — including `projectsend:update`, the one that would have added the column. Reordering the entrypoint or using a lighter readiness probe does not help for that reason; the crash is in the bootstrap, not in the probe. current() now applies the model's declared defaults to any column the hydrated row does not have. That closes the window for every column with a default rather than for the one where it was found, and goes inert the moment the schema is current. The match in isConfigured() is left total on purpose: a default arm would swallow a real unhandled case, and the invariant it needs now holds at the one place the row is read. The probe's message is the other half. It boots the whole application, so it fails both when the database is absent and when the application cannot start, and it reported the second as the first — sending an operator off checking credentials that were never wrong. It now prints the error it actually hit and says which of the two it looks like. Verified end to end against a 2.1-shaped database: `artisan migrate` dies with UnhandledMatchError before the change and completes after it, leaving the row reading as S3 with its bucket intact. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS --- .../Settings/ExternalStorageSettings.php | 38 ++++++++++- docker/production/entrypoint.sh | 18 +++++- .../Platform/ExternalStorageSettingsTest.php | 63 +++++++++++++++++++ 3 files changed, 116 insertions(+), 3 deletions(-) diff --git a/app/Modules/Platform/Settings/ExternalStorageSettings.php b/app/Modules/Platform/Settings/ExternalStorageSettings.php index 8aaf248b..f73d7795 100644 --- a/app/Modules/Platform/Settings/ExternalStorageSettings.php +++ b/app/Modules/Platform/Settings/ExternalStorageSettings.php @@ -75,7 +75,38 @@ class ExternalStorageSettings extends Model public static function current(): self { - return static::query()->firstOrNew([]); + $settings = static::query()->firstOrNew([]); + + // Column defaults — the $attributes array above — apply to a NEW + // model, never to one hydrated from a row. So a row written by an + // older release, before one of these columns existed, reads that + // column as null however sensible its default is. + // + // That matters here more than it would anywhere else, because + // PlatformServiceProvider::boot() reads these settings on every + // process boot — and boot happens BEFORE `artisan migrate` runs. + // For the length of an upgrade the code is new and the schema is + // still old, and every artisan command in that window, including + // the one that would run the migrations, boots through here. + // + // A null `provider` made the match in isConfigured() throw + // UnhandledMatchError, which the official image's readiness probe + // reported to the operator as "database unreachable" — on a + // perfectly reachable database, in a container that then + // restart-looped without ever reaching the migration that would + // have fixed it (#1770, upgrading from 2.0/2.1 with external + // storage configured). + // + // Applying the defaults to a hydrated row closes that window for + // every column that has one, rather than for the single column + // where it was found. Inert on any install whose schema is current. + foreach ((new self)->getAttributes() as $column => $default) { + if (! array_key_exists($column, $settings->getAttributes())) { + $settings->setAttribute($column, $default); + } + } + + return $settings; } /** @@ -93,6 +124,11 @@ class ExternalStorageSettings extends Model // authenticate with different things entirely: S3 wants a key and // a secret, GCS wants a service account key file. // + // The match is deliberately left total rather than given a default + // arm: current() guarantees a provider even on a row older than the + // column, and a default arm here would quietly swallow a genuinely + // unhandled case instead of naming it. + // // Unless S3 is being asked to authenticate as the machine it is // running on, in which case there is no credential to fill in at // all and demanding one would leave the disk permanently diff --git a/docker/production/entrypoint.sh b/docker/production/entrypoint.sh index 18e75512..ef7d9890 100755 --- a/docker/production/entrypoint.sh +++ b/docker/production/entrypoint.sh @@ -58,12 +58,26 @@ fi # Wait for the database. `migrate` against a database still starting up is # the single most common first-run failure, and a bare failure here would # restart-loop the container with a stack trace instead of a clear message. +# +# The probe boots the whole application, so it fails for two quite different +# reasons: the database really is not there yet, or it is there and the +# application could not start. Both used to be reported as the first one, +# which sent an operator off checking credentials that were never wrong +# (#1770). The last failure is kept and printed, so whichever it was is on +# screen instead of being guessed at. if [ "$1" = "supervisord" ] || [ "$1" = "/usr/bin/supervisord" ]; then i=0 - until su-exec www-data php artisan db:show --quiet >/dev/null 2>&1; do + until probe_error=$(su-exec www-data php artisan db:show --quiet 2>&1); do i=$((i + 1)) if [ "$i" -ge 60 ]; then - echo "projectsend: database unreachable after 60s — check DB_HOST, DB_DATABASE and credentials" >&2 + echo "projectsend: gave up waiting for the database after 60s." >&2 + echo "projectsend: the last attempt failed with:" >&2 + printf '%s\n' "$probe_error" | tail -n 20 >&2 + echo "projectsend:" >&2 + echo "projectsend: if that names the database host, the connection or the credentials," >&2 + echo "projectsend: check DB_HOST, DB_DATABASE, DB_USERNAME and DB_PASSWORD." >&2 + echo "projectsend: if it is an application error, the database is fine and this is a" >&2 + echo "projectsend: bug — please report it at https://github.com/projectsend/projectsend/issues" >&2 exit 1 fi [ "$i" = 1 ] && echo "projectsend: waiting for the database..." diff --git a/tests/Feature/Platform/ExternalStorageSettingsTest.php b/tests/Feature/Platform/ExternalStorageSettingsTest.php index 44b7e62c..469152c1 100644 --- a/tests/Feature/Platform/ExternalStorageSettingsTest.php +++ b/tests/Feature/Platform/ExternalStorageSettingsTest.php @@ -8,9 +8,12 @@ use App\Modules\Files\Storage\ResolvingUploadDisk; use App\Modules\Platform\Capabilities\Edition; use App\Modules\Platform\Settings\ExternalStorageConfigApplier; use App\Modules\Platform\Settings\ExternalStorageSettings; +use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\Facades\Storage; beforeEach(function () { @@ -239,6 +242,43 @@ test('new uploads go to the external disk with no stored credentials when the se expect($event->disk)->toBe('files_external'); }); +test('booting against a row older than the provider column does not throw', function () { + // #1770. The exact shape of a 2.0/2.1 install with S3 configured, + // upgrading to 2.2 or later: the code is new, the schema is still old, + // and boot happens before `artisan migrate` gets a chance to run. + // + // The failure this pins was not a wrong answer but a fatal one — an + // UnhandledMatchError on a null provider, thrown during + // PlatformServiceProvider::boot(), which took down every artisan + // command including the one that would have added the column. The + // official image reported it as "database unreachable" and + // restart-looped. + legacyStorageRow(); + + app(ExternalStorageConfigApplier::class)->flush(); + app(ExternalStorageConfigApplier::class)->apply(); + + // Read as S3, which is the only thing a row predating the column could + // have been — and is what the migration's own default says. + expect(config('filesystems.disks.files_external.bucket'))->toBe('my-bucket') + ->and(config('filesystems.disks.files_external.key'))->toBe('AKIAEXAMPLE') + ->and(config('filesystems.disks.files_external.driver'))->toBe('s3'); +}); + +test('an upload still reaches the external disk while the schema is mid-upgrade', function () { + // The other half: not throwing is not the same as behaving. An + // installation that was storing files in a bucket before the upgrade + // must not quietly start writing them to local disk during it. + legacyStorageRow(); + + app(ExternalStorageConfigApplier::class)->flush(); + + $event = new ResolvingUploadDisk($this->admin); + Event::dispatch($event); + + expect($event->disk)->toBe('files_external'); +}); + test('staff can save external storage settings', function () { config()->set('projectsend.edition', Edition::Community); @@ -417,6 +457,29 @@ test('a value cached while running as Community cannot leak into Cloud without a expect($event->disk)->toBe('files'); }); +/** + * An external storage row exactly as 2.0/2.1 wrote it: S3 credentials in + * place, and none of the columns added since. + * + * Written with the query builder and then stripped back, rather than + * hand-rolled SQL, so it stays honest if the table changes shape again. + */ +function legacyStorageRow(): void +{ + DB::table('external_storage_settings')->insert([ + 'active' => true, + 'key' => 'AKIAEXAMPLE', + 'secret' => Crypt::encryptString('shh'), + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + 'use_path_style' => false, + ]); + + Schema::table('external_storage_settings', function (Blueprint $table) { + $table->dropColumn(['provider', 'key_file', 'use_instance_role']); + }); +} + /** * @param array $overrides * @return array