Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/__tests__/repository/slug-repository.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { beforeAll, beforeEach, describe, expect, it } from "vitest";
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
import { env } from "cloudflare:test";
import { applyMigrations, resetData } from "../setup";
import { LinkRepository, SlugRepository } from "../../db";
Expand Down Expand Up @@ -219,4 +219,25 @@ describe("SlugRepository.remove", () => {
const primary = updated!.slugs.find((s) => s.is_primary);
expect(primary!.slug).toBe("abc");
});

it("guard holds when a click lands between the pre-read and the batch delete", async () => {
// The pre-read reports click_count = 0, but a click record exists in the
// DB by the time the DELETE runs. The NOT EXISTS guard inside the batch
// must block the delete so the click record is not orphaned.
const link = await LinkRepository.create(env.DB, { url: "https://example.com", slug: "rmrace" });
await SlugRepository.addCustom(env.DB, link.id, "rmrace-c");
await env.DB.prepare("INSERT INTO clicks (slug, clicked_at, link_mode) VALUES (?, ?, 'link')")
.bind("rmrace-c", Math.floor(Date.now() / 1000)).run();

const spy = vi
.spyOn(SlugRepository, "findByValue")
.mockResolvedValueOnce({ ...(await SlugRepository.findByValue(env.DB, "rmrace-c"))!, click_count: 0 });
const removed = await SlugRepository.remove(env.DB, "rmrace-c").finally(() => spy.mockRestore());
Comment thread
DennisAlund marked this conversation as resolved.
Outdated

expect(removed).toBe(false);
const clickRow = await env.DB.prepare("SELECT 1 FROM clicks WHERE slug = 'rmrace-c'").first();
const slugRow = await env.DB.prepare("SELECT 1 FROM slugs WHERE slug = 'rmrace-c'").first();
expect(clickRow).not.toBeNull();
expect(slugRow).not.toBeNull();
});
Comment thread
DennisAlund marked this conversation as resolved.
});
17 changes: 13 additions & 4 deletions src/db/slug-repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,22 +114,31 @@ export class SlugRepository {
// Lifetime guard: never drop a slug that has recorded any click, so
// analytics rows are not orphaned. Filter options would mask historical
// bot traffic and let real history be deleted.
const row = await db.prepare(`SELECT ${slugSelect()} FROM slugs s WHERE slug = ?`).bind(slug).first<Slug>();
// findByValue is used (rather than a bare db.prepare) so the pre-read
// is a named static method that tests can spy on.
const row = await SlugRepository.findByValue(db, slug);
if (!row) return false;

if (!row.is_custom) return false;

if (row.click_count > 0) return false;

// Primary handover and delete run in one transactional batch.
// NOT EXISTS re-checks atomically that no click arrived since the pre-read:
// FK cascade is not active (no PRAGMA foreign_keys), so a click arriving
// in the window would otherwise orphan its clicks row.
const statements = [];
Comment thread
DennisAlund marked this conversation as resolved.
Outdated
if (row.is_primary) {
statements.push(
db.prepare("UPDATE slugs SET is_primary = 1 WHERE link_id = ? AND is_custom = 0").bind(row.link_id),
);
Comment on lines 150 to 159

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e7686db. The handover UPDATE now carries AND EXISTS (SELECT 1 FROM slugs WHERE slug = ? AND is_primary = 1) on the slug being removed, so it only promotes the random slug when this slug still holds primary inside the transaction. A concurrent setPrimary that moved primary to another custom slug now leaves the handover a no-op, and the delete still fires. Added a regression test that drives a stale pre-read (is_primary = 1) against a real setPrimary to a second custom slug; it fails on the old code with two primaries and passes now. Verified both guards on the UPDATE (click NOT EXISTS and still-primary EXISTS) are independently load-bearing.

}
statements.push(db.prepare("DELETE FROM slugs WHERE slug = ?").bind(slug));
await db.batch(statements);
return true;
statements.push(
db.prepare("DELETE FROM slugs WHERE slug = ? AND NOT EXISTS (SELECT 1 FROM clicks WHERE slug = ?)")
.bind(slug, slug),
);
const results = await db.batch(statements);
const deleteResult = results[results.length - 1];
return (deleteResult.meta.changes ?? 0) > 0;
Comment thread
DennisAlund marked this conversation as resolved.
Outdated
Comment thread
DennisAlund marked this conversation as resolved.
}
}