Possible race condition for feature insert to DB

[nathanaelytj]

Pennant Version

1.3.1

Laravel Version

10.13.0

PHP Version

8.2.5

Database Driver & Version

(PostgreSQL) 14.8 (Ubuntu 14.8-0ubuntu0.22.04.1)

Description

There is possible race condition if a JS Frontend hit a multiple API route that use Feature middleware.

Pennant attempt to insert but failed because of duplicate value constraint.

Below is the exception that sent to slack:

{ "class": "Illuminate\\Database\\QueryException", "message": "SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint \"features_name_scope_unique\"\nDETAIL: Key (name, scope)=(App\\Features\\<FEATURE_NAME>, App\\Models\\User|<REDACTED_ID>) already exists. (Connection: pgsql, SQL: insert into \"features\" (\"created_at\", \"name\", \"scope\", \"updated_at\", \"value\") values (2023-06-06 01:35:18, App\\Features\\<FEATURE_NAME>, App\\Models\\User|<REDACTED_ID>, 2023-06-06 01:35:18, true))", "code": 23505, "file": "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Database/Connection.php:793", "trace": [ "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Database/Connection.php:753", "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Database/Connection.php:567", "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Database/Connection.php:531", "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Database/Query/Builder.php:3303", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/Drivers/DatabaseDriver.php:134", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/Drivers/Decorator.php:209", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/Drivers/Decorator.php:232", "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Collections/Traits/EnumeratesValues.php:694", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/Drivers/Decorator.php:232", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/PendingScopedFeatureInteraction.php:70", "/var/www/vhosts/localhost/vendor/laravel/framework/src/Illuminate/Collections/Traits/EnumeratesValues.php:694", "/var/www/vhosts/localhost/vendor/laravel/pennant/src/PendingScopedFeatureInteraction.php:70", "/var/www/vhosts/loca

Steps To Reproduce

1. Using JS Framework (I'm using Vue) in frontend, and do multiple axios call.
2. The call must be at the same time, in parallel and hit a API route that use pennant middleware.
3. On rare case, if feature not already in database, it will get created from first request, but second request that hit will fail because of duplicate value constraint.

[timacdonald]

Hey @nathanaelytj,

Could you try creating the following middleware in your application and letting me know how it goes? This will wrap the feature resolution in an atomic lock for you to ensure only one request resolves the features, for the given user, at a time.

It may need to be more fine-grained, depending on your application, e.g., you might need a different lock for each individual feature the middleware has applied, but that will really be up to your application specifically.

<?php

namespace App\Http\Middleware;

use Closure;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Cache;
use Laravel\Pennant\Feature;
use Laravel\Pennant\Middleware\EnsureFeaturesAreActive as BaseMiddleware;
use Symfony\Component\HttpFoundation\Response;

class EnsureFeaturesAreActive extends BaseMiddleware
{
    /**
     * Handle an incoming request.
     *
     * @param  \Closure(\Illuminate\Http\Request): (\Symfony\Component\HttpFoundation\Response)  $next
     */
    public function handle(Request $request, Closure $next, string ...$features): mixed
    {
        $name = "pennant:user:{$request->user()?->id}:features:".implode(',', $features);

        Cache::lock($name, 2)->block(3, fn () => Feature::loadMissing($features));

        return parent::handle($request, $next, ...$features);
    }
}

[timacdonald]

I'm closing this for now, as I don't really see race-conditions as a bug. If we get similar reports we can consider adding a cache lock to the main middleware or adding a different middleware to support this.

[emilklindt]

@timacdonald We encountered this issue in our production environment as well. We addressed it by implementing a custom database driver for Pennant, which instead of inserting uses the upsert strategy (ON DUPLICATE KEY UPDATE).

A potential caveat with the upsert approach: When using non-deterministic flags, such as usage with the Lottery class across multiple servers, a user might have a feature disabled on one server and concurrently enabled on another due to the upsert. This inconsistency could lead to unpredictable feature behaviors. For our use case, that is acceptable.

I'm not proposing the upsert as a default strategy, but it could be beneficial as an option. Given our results, would you be open to a pull request introducing this strategy? Or do you believe users will generally be better off using Cache locks to address this?

[timacdonald]

@emilklindt right not I believe that a cache lock is the best approach.

I'm going to think on an approach we could potentially build into the package to help out with this, but for now I recommend using the cache lock approach to have consistent results.