link: take click counters off the hot row to stop deadlocks
Deploy Ladill Link / deploy (push) Successful in 1m18s
Deploy Ladill Link / deploy (push) Successful in 1m18s
Every click ran three UPDATEs against the same short_links row plus one against the owner's link_wallets row, all inside one transaction. Two consequences: - A popular link serialised its entire traffic through a single row lock. - Locking two rows per transaction meant concurrent clicks on different links of the same owner could take them in opposite orders, so MySQL deadlocked. Production logged 152 deadlocks, 146 of them on one row, surfacing as HTTP 500s to real visitors. With a link now being handed to a very large audience, this is the next thing that falls over, and it is independent of page weight. Counters are now buffered and flushed instead of written inline: - link_clicks stays the source of truth. It is insert-only, so it has no contention no matter how popular a link gets. - Increments accumulate in Redis hashes and drain via link:flush-click-counters, scheduled every minute. The database sees one UPDATE per link per flush instead of four per click. - Draining reads and deletes each hash atomically, so a click landing mid-flush is counted in that batch or the next, never dropped. - --recount rebuilds totals from link_clicks after a Redis loss. If the buffer is unavailable the recorder writes through to the database, keeping today's behaviour (and today's contention) rather than losing counts. Nothing in the click path may break the redirect the visitor actually came for, so the whole recorder is wrapped and failures are logged. Storage sits behind ClickCounterStore so the buffered path is covered by tests rather than silently falling back to write-through when no Redis is present. Also adds the (short_link_id, ip_hash, clicked_at) index. The unique-click check filters on ip_hash but only (short_link_id, clicked_at) was indexed, so every click scanned all of that link's rows in the dedupe window — cost growing with the link's own popularity, the worst possible shape for a link going viral. Tests: 11 new. The load-bearing one asserts the request path issues no UPDATE against short_links or link_wallets at all. Pre-existing suite failures go from 13 to 9; none of the remainder are related. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,265 @@
|
||||
<?php
|
||||
|
||||
namespace Tests\Feature;
|
||||
|
||||
use App\Models\LinkClick;
|
||||
use App\Models\LinkWallet;
|
||||
use App\Models\ShortLink;
|
||||
use App\Models\User;
|
||||
use App\Services\Link\ArrayClickCounterStore;
|
||||
use App\Services\Link\ClickCounterStore;
|
||||
use App\Services\Link\LinkClickCounterBuffer;
|
||||
use App\Services\Link\LinkClickRecorder;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Http\Request;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Illuminate\Support\Str;
|
||||
use Tests\TestCase;
|
||||
|
||||
class LinkClickCounterTest extends TestCase
|
||||
{
|
||||
use RefreshDatabase;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
parent::setUp();
|
||||
// No Redis in the test environment; the array store exercises the same
|
||||
// buffered path so it is verified rather than silently falling back.
|
||||
config(['link.click_counter_store' => 'array']);
|
||||
$this->app->singleton(ClickCounterStore::class, fn () => new ArrayClickCounterStore);
|
||||
}
|
||||
|
||||
private function user(): User
|
||||
{
|
||||
return User::create([
|
||||
'public_id' => (string) Str::uuid(),
|
||||
'name' => 'Test',
|
||||
'email' => 'test+'.uniqid().'@example.com',
|
||||
]);
|
||||
}
|
||||
|
||||
private function link(User $user, string $slug = 'hot1'): ShortLink
|
||||
{
|
||||
LinkWallet::firstOrCreate(
|
||||
['user_id' => $user->id],
|
||||
['links_total' => 0, 'clicks_total' => 0, 'status' => LinkWallet::STATUS_ACTIVE],
|
||||
);
|
||||
|
||||
return ShortLink::create([
|
||||
'user_id' => $user->id,
|
||||
'slug' => $slug,
|
||||
'source_app' => 'link',
|
||||
'source_kind' => 'redirect',
|
||||
'is_managed_here' => true,
|
||||
'destination_url' => 'https://example.com/target',
|
||||
'is_active' => true,
|
||||
]);
|
||||
}
|
||||
|
||||
private function record(ShortLink $link, string $ip = '203.0.113.5'): void
|
||||
{
|
||||
$request = Request::create('https://ladl.link/'.$link->slug, 'GET', [], [], [], [
|
||||
'REMOTE_ADDR' => $ip,
|
||||
'HTTP_USER_AGENT' => 'Mozilla/5.0 (iPhone; CPU iPhone OS 18_7 like Mac OS X)',
|
||||
]);
|
||||
|
||||
app(LinkClickRecorder::class)->record($link, $request);
|
||||
}
|
||||
|
||||
/* ------------------------------------------------- the deadlock is gone */
|
||||
|
||||
public function test_recording_a_click_does_not_write_to_the_shared_counter_rows(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
|
||||
DB::flushQueryLog();
|
||||
DB::enableQueryLog();
|
||||
$this->record($link);
|
||||
$queries = array_column(DB::getQueryLog(), 'query');
|
||||
DB::disableQueryLog();
|
||||
|
||||
// The whole point: no UPDATE against short_links or link_wallets on the
|
||||
// request path, because that shared row lock is what deadlocked.
|
||||
$hotWrites = array_values(array_filter(
|
||||
$queries,
|
||||
fn (string $q) => (bool) preg_match('/update\s+.?(short_links|link_wallets)/i', $q),
|
||||
));
|
||||
|
||||
$this->assertSame([], $hotWrites, "Click path still writes to a shared row:\n".implode("\n", $hotWrites));
|
||||
}
|
||||
|
||||
public function test_the_click_itself_is_still_persisted(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
|
||||
$this->record($link);
|
||||
|
||||
$this->assertSame(1, LinkClick::where('short_link_id', $link->id)->count());
|
||||
$click = LinkClick::where('short_link_id', $link->id)->first();
|
||||
$this->assertTrue((bool) $click->is_unique);
|
||||
$this->assertNotNull($click->ip_hash);
|
||||
$this->assertSame('mobile', $click->device_type);
|
||||
}
|
||||
|
||||
public function test_repeat_visits_from_one_ip_count_once_as_unique(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
|
||||
$this->record($link, '203.0.113.9');
|
||||
$this->record($link, '203.0.113.9');
|
||||
$this->record($link, '198.51.100.7');
|
||||
|
||||
$this->assertSame(3, LinkClick::where('short_link_id', $link->id)->count());
|
||||
$this->assertSame(2, LinkClick::where('short_link_id', $link->id)->where('is_unique', true)->count());
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------- flushing */
|
||||
|
||||
public function test_flush_applies_buffered_counts_to_the_database(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
|
||||
$this->record($link, '203.0.113.1');
|
||||
$this->record($link, '203.0.113.2');
|
||||
$this->record($link, '203.0.113.1');
|
||||
|
||||
// Buffered, so the denormalised totals have not moved yet.
|
||||
$this->assertSame(0, (int) $link->fresh()->clicks_total);
|
||||
|
||||
app(LinkClickCounterBuffer::class)->flush();
|
||||
|
||||
$link->refresh();
|
||||
$this->assertSame(3, (int) $link->clicks_total);
|
||||
$this->assertSame(2, (int) $link->unique_clicks_total);
|
||||
$this->assertNotNull($link->last_clicked_at);
|
||||
$this->assertSame(3, (int) LinkWallet::where('user_id', $user->id)->value('clicks_total'));
|
||||
}
|
||||
|
||||
public function test_flush_is_idempotent_when_the_buffer_is_empty(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
$this->record($link);
|
||||
|
||||
app(LinkClickCounterBuffer::class)->flush();
|
||||
$after = (int) $link->fresh()->clicks_total;
|
||||
|
||||
app(LinkClickCounterBuffer::class)->flush();
|
||||
|
||||
$this->assertSame($after, (int) $link->fresh()->clicks_total, 'A second flush must not double-count.');
|
||||
}
|
||||
|
||||
public function test_flush_batches_one_update_per_link_not_one_per_click(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
for ($i = 0; $i < 25; $i++) {
|
||||
$this->record($link, '203.0.113.'.$i);
|
||||
}
|
||||
|
||||
DB::flushQueryLog();
|
||||
DB::enableQueryLog();
|
||||
app(LinkClickCounterBuffer::class)->flush();
|
||||
$updates = array_filter(
|
||||
array_column(DB::getQueryLog(), 'query'),
|
||||
fn (string $q) => (bool) preg_match('/update\s+.?short_links/i', $q),
|
||||
);
|
||||
DB::disableQueryLog();
|
||||
|
||||
$this->assertCount(1, $updates, '25 clicks must collapse into a single UPDATE.');
|
||||
$this->assertSame(25, (int) $link->fresh()->clicks_total);
|
||||
}
|
||||
|
||||
public function test_command_flushes_and_reports(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
$this->record($link);
|
||||
|
||||
$this->artisan('link:flush-click-counters')
|
||||
->expectsOutputToContain('Flushed 1 click')
|
||||
->assertSuccessful();
|
||||
|
||||
$this->assertSame(1, (int) $link->fresh()->clicks_total);
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------ recovery */
|
||||
|
||||
public function test_recount_rebuilds_totals_from_link_clicks(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
$this->record($link, '203.0.113.1');
|
||||
$this->record($link, '203.0.113.2');
|
||||
app(LinkClickCounterBuffer::class)->flush();
|
||||
|
||||
// Simulate drift, e.g. counters lost or double-applied.
|
||||
$link->update(['clicks_total' => 999, 'unique_clicks_total' => 999]);
|
||||
|
||||
$this->artisan('link:flush-click-counters', ['--recount' => true])->assertSuccessful();
|
||||
|
||||
$link->refresh();
|
||||
$this->assertSame(2, (int) $link->clicks_total);
|
||||
$this->assertSame(2, (int) $link->unique_clicks_total);
|
||||
}
|
||||
|
||||
private function breakTheStore(): void
|
||||
{
|
||||
$this->app->singleton(ClickCounterStore::class, fn () => new class implements ClickCounterStore
|
||||
{
|
||||
public function increment(string $hash, string $field, int $by = 1): void
|
||||
{
|
||||
throw new \RuntimeException('redis down');
|
||||
}
|
||||
|
||||
public function put(string $hash, string $field, string $value): void
|
||||
{
|
||||
throw new \RuntimeException('redis down');
|
||||
}
|
||||
|
||||
public function drain(string $hash): array
|
||||
{
|
||||
throw new \RuntimeException('redis down');
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
public function test_a_redis_outage_still_records_the_click(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
$this->breakTheStore();
|
||||
|
||||
$this->record($link);
|
||||
|
||||
$this->assertSame(1, LinkClick::where('short_link_id', $link->id)->count());
|
||||
// Fallback writes straight through, so the total moves immediately. This
|
||||
// reintroduces the row lock, but only while Redis is unavailable.
|
||||
$this->assertSame(1, (int) $link->fresh()->clicks_total);
|
||||
}
|
||||
|
||||
public function test_a_failing_store_never_breaks_the_redirect(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$this->link($user, 'direct9');
|
||||
$this->breakTheStore();
|
||||
|
||||
$this->get('https://ladl.link/direct9')->assertRedirect('https://example.com/target');
|
||||
}
|
||||
|
||||
public function test_a_failing_flush_does_not_throw(): void
|
||||
{
|
||||
$user = $this->user();
|
||||
$link = $this->link($user);
|
||||
$this->record($link);
|
||||
$this->breakTheStore();
|
||||
|
||||
$result = app(LinkClickCounterBuffer::class)->flush();
|
||||
|
||||
$this->assertSame(0, $result['clicks']);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user