diff --git a/app/Actions/CreateCommit.php b/app/Actions/CreateCommit.php index 25a726bee..bcde6496d 100644 --- a/app/Actions/CreateCommit.php +++ b/app/Actions/CreateCommit.php @@ -101,8 +101,13 @@ public function handle(User $user, string $body, ?Commit $parent = null, ?Commit } } + // A unique job takes its lock when it is dispatched, even with afterCommit(). With the database cache, a lock + // still held by an earlier check would fail its insert and abort this transaction on Postgres, so the job is + // only dispatched once the commit is saved. if ($commit->status === CommitStatus::Approved) { - dispatch(new CheckUserLevel($user))->afterCommit(); + DB::afterCommit(function () use ($user): void { + dispatch(new CheckUserLevel($user)); + }); } if ($commit->status === CommitStatus::Pending && ! $hasGoodReputation) { diff --git a/app/Actions/UpdateCommitStatuses.php b/app/Actions/UpdateCommitStatuses.php index b8b901c3e..6237f4f95 100644 --- a/app/Actions/UpdateCommitStatuses.php +++ b/app/Actions/UpdateCommitStatuses.php @@ -94,8 +94,15 @@ public function handle(array $commitIds, CommitStatus $status, ?string $reason = } } - User::query()->whereKey($commits->pluck('user_id')->filter()->unique()->values()->all())->get() - ->each(fn (User $author) => dispatch(new CheckUserLevel($author))->afterCommit()); + // Dispatched once the statuses are saved, as CreateCommit does, so a check still holding its unique lock + // can't abort the approval. + $authors = User::query()->whereKey($commits->pluck('user_id')->filter()->unique()->values()->all())->get(); + + DB::afterCommit(function () use ($authors): void { + foreach ($authors as $author) { + dispatch(new CheckUserLevel($author)); + } + }); } }); } diff --git a/tests/Feature/Controllers/CommitApprovalControllerTest.php b/tests/Feature/Controllers/CommitApprovalControllerTest.php index 65ab6d2ef..60f32f1e7 100644 --- a/tests/Feature/Controllers/CommitApprovalControllerTest.php +++ b/tests/Feature/Controllers/CommitApprovalControllerTest.php @@ -5,6 +5,8 @@ use App\Enums\CommitStatus; use App\Models\Commit; use App\Models\User; +use Illuminate\Database\Events\QueryExecuted; +use Illuminate\Support\Facades\DB; beforeEach(function (): void { config(['app.admins' => ['admin']]); @@ -23,6 +25,29 @@ expect($commit->refresh()->status)->toBe(CommitStatus::Approved); }); +it('takes the unique lock of the level check of the author only once the approval is saved', function (): void { + // On Postgres, a lock row left by an earlier check makes the insert fail and aborts the transaction it runs in. + config(['cache.default' => 'database']); + $admin = User::factory()->create(['username' => 'admin']); + $commit = Commit::factory()->pending()->create(); + $outside = DB::transactionLevel(); + $levels = []; + DB::listen(function (QueryExecuted $query) use (&$levels): void { + if (str_contains($query->sql, 'cache_locks')) { + $levels[] = DB::transactionLevel(); + } + }); + + $this->actingAs($admin) + ->fromRoute('moderation.show') + ->post(route('commit-approval.store', $commit)) + ->assertRedirectToRoute('moderation.show'); + + expect($commit->refresh()->status)->toBe(CommitStatus::Approved) + ->and($levels)->not->toBeEmpty() + ->and(array_unique($levels))->toBe([$outside]); +}); + it('may not approve a deleted commit', function (): void { $admin = User::factory()->create(['username' => 'admin']); $commit = Commit::factory()->pending()->create(); diff --git a/tests/Feature/Controllers/CommitControllerTest.php b/tests/Feature/Controllers/CommitControllerTest.php index d0acaf771..7ba91b441 100644 --- a/tests/Feature/Controllers/CommitControllerTest.php +++ b/tests/Feature/Controllers/CommitControllerTest.php @@ -15,6 +15,7 @@ use App\Models\Notification; use App\Models\User; use Illuminate\Database\Eloquent\Factories\Sequence; +use Illuminate\Database\Events\QueryExecuted; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; @@ -35,6 +36,25 @@ expect($user->commits()->sole()->body)->toBe('Hello, devnull!'); }); +it('takes the unique lock of the level check only once the commit is saved', function (): void { + // On Postgres, a lock row left by an earlier check makes the insert fail and aborts the transaction it runs in. + config(['cache.default' => 'database']); + $user = User::factory()->create(); + config(['app.admins' => [$user->username]]); + $outside = DB::transactionLevel(); + $levels = []; + DB::listen(function (QueryExecuted $query) use (&$levels): void { + if (str_contains($query->sql, 'cache_locks')) { + $levels[] = DB::transactionLevel(); + } + }); + + $this->actingAs($user)->post(route('commit.store'), ['body' => 'It was DNS.'])->assertSessionHasNoErrors(); + + expect($levels)->not->toBeEmpty() + ->and(array_unique($levels))->toBe([$outside]); +}); + it('flashes the created commit', function (): void { $user = User::factory()->create();