From d430aed45d3892b0337ffac93aca813eccca5cca Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 19:19:36 +0000 Subject: [PATCH 1/4] fix: honor return value of repository guards in deleteLink and disableLink deleteLink silently returned { deleted: true } even when LinkRepository.delete() returned false (the DB-level click-count guard can fire in the window between the service-level check and the repository delete, e.g. a concurrent click). Added a check on the boolean return so callers get a 400 instead. disableLink used a non-null assertion (disabled!) on the result of LinkRepository.disable(), which returns null when the link is deleted between the preceding getById and the UPDATE inside disable(). That made the function throw a TypeError instead of returning a structured error. Added the same null-guard that enableLink already had, returning 404 on null. Regression tests added for both cases using vi.spyOn to simulate the narrow race window. https://claude.ai/code/session_01PNGGm7htQKRuHz1nAy9Qib --- src/__tests__/service/ownership.test.ts | 46 ++++++++++++++++++++++++- src/services/link-management.ts | 12 ++++--- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/__tests__/service/ownership.test.ts b/src/__tests__/service/ownership.test.ts index 70cf81d..bc154d6 100644 --- a/src/__tests__/service/ownership.test.ts +++ b/src/__tests__/service/ownership.test.ts @@ -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 } from "../../db"; @@ -272,3 +272,47 @@ describe("List links by owner", () => { } }); }); + +describe("deleteLink: DB-level guard return value is honored", () => { + it("returns 400 when the repository delete guard fires after the service-level check", async () => { + // Simulate the race where a click is recorded between the service's + // total_clicks check and the underlying DB delete: the repository + // re-checks inside delete() and returns false. The service must surface + // that as a 400, not silently report { deleted: true }. + const link = await createOwnedLink(); + + const spy = vi.spyOn(LinkRepository, "delete").mockResolvedValueOnce(false); + const result = await deleteLink(env as any, link.id, OWNER); + spy.mockRestore(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(400); + } + }); + + it("link is not in DB after a successful deleteLink", async () => { + const link = await createOwnedLink(); + const result = await deleteLink(env as any, link.id, OWNER); + expect(result.ok).toBe(true); + expect(await LinkRepository.getById(env.DB, link.id)).toBeNull(); + }); +}); + +describe("disableLink: null return from repository does not throw", () => { + it("returns 404 rather than throwing when the link is concurrently deleted", async () => { + // Simulate the narrow race: the link exists at getById time but is + // deleted before the UPDATE inside LinkRepository.disable(). Without the + // null check the function would crash with a TypeError on disabled!.slugs. + const link = await createOwnedLink(); + + const spy = vi.spyOn(LinkRepository, "disable").mockResolvedValueOnce(null); + const result = await disableLink(env as any, link.id, OWNER); + spy.mockRestore(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(404); + } + }); +}); diff --git a/src/services/link-management.ts b/src/services/link-management.ts index 264710b..27071cd 100644 --- a/src/services/link-management.ts +++ b/src/services/link-management.ts @@ -153,18 +153,19 @@ export async function disableLink(env: Env, id: number, identity: string): Promi if (!link) return fail(404, "Link not found"); if (link.created_by !== identity) return fail(403, "Only the link owner can disable this link"); const disabled = await LinkRepository.disable(env.DB, id); + if (!disabled) return fail(404, "Link not found"); await Promise.all( - disabled!.slugs.map((s) => + disabled.slugs.map((s) => SlugCache.put(env.SLUG_KV, s.slug, { - url: disabled!.url, + url: disabled.url, disabled_at: s.disabled_at, - expires_at: disabled!.expires_at, + expires_at: disabled.expires_at, }), ), ); - return ok(disabled!); + return ok(disabled); } export async function enableLink(env: Env, id: number, identity: string): Promise> { @@ -195,7 +196,8 @@ export async function deleteLink(env: Env, id: number, identity: string): Promis if (link.total_clicks > 0) return fail(400, "Cannot delete a link with clicks, disable it instead"); const slugsToDelete = link.slugs.map((s) => s.slug); - await LinkRepository.delete(env.DB, id); + const deleted = await LinkRepository.delete(env.DB, id); + if (!deleted) return fail(400, "Cannot delete a link with clicks, disable it instead"); await Promise.all(slugsToDelete.map((s) => SlugCache.delete(env.SLUG_KV, s))); return ok({ deleted: true }); From 1956bfbc70dd9057675c8d799b31fbc1e06d79c2 Mon Sep 17 00:00:00 2001 From: Dennis Alund Date: Sat, 30 May 2026 16:36:56 +0800 Subject: [PATCH 2/4] fix: return 404 from deleteLink when link vanishes before repository delete Copilot review on PR #11 flagged that deleteLink mapped every false from LinkRepository.delete() to a 400 'Cannot delete a link with clicks'. The repository returns false for two cases: a concurrent request removed the link, or a concurrent click pushed total_clicks past the lifetime guard. Re-check existence after a failed delete and return 404 for a vanished link, 400 otherwise. Add a regression test covering the concurrent-delete path, and wrap the spy usages in the deleteLink and disableLink tests in try/finally so a thrown call cannot leak the mock into later tests. --- src/__tests__/service/ownership.test.ts | 40 ++++++++++++++++++++++--- src/services/link-management.ts | 8 ++++- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/__tests__/service/ownership.test.ts b/src/__tests__/service/ownership.test.ts index bc154d6..6c34175 100644 --- a/src/__tests__/service/ownership.test.ts +++ b/src/__tests__/service/ownership.test.ts @@ -282,8 +282,12 @@ describe("deleteLink: DB-level guard return value is honored", () => { const link = await createOwnedLink(); const spy = vi.spyOn(LinkRepository, "delete").mockResolvedValueOnce(false); - const result = await deleteLink(env as any, link.id, OWNER); - spy.mockRestore(); + let result; + try { + result = await deleteLink(env as any, link.id, OWNER); + } finally { + spy.mockRestore(); + } expect(result.ok).toBe(false); if (!result.ok) { @@ -291,6 +295,30 @@ describe("deleteLink: DB-level guard return value is honored", () => { } }); + it("returns 404 when the link is concurrently deleted before the repository delete", async () => { + // The link exists at the service's getById and ownership checks, then + // vanishes before the repository delete runs. delete() returns false for a + // missing row exactly as it does for the click guard, so the service must + // re-check existence and surface 404 rather than a misleading 400. + const link = await createOwnedLink(); + + const spy = vi.spyOn(LinkRepository, "delete").mockImplementationOnce(async (db, id) => { + await db.prepare("DELETE FROM links WHERE id = ?").bind(id).run(); + return false; + }); + let result; + try { + result = await deleteLink(env as any, link.id, OWNER); + } finally { + spy.mockRestore(); + } + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(404); + } + }); + it("link is not in DB after a successful deleteLink", async () => { const link = await createOwnedLink(); const result = await deleteLink(env as any, link.id, OWNER); @@ -307,8 +335,12 @@ describe("disableLink: null return from repository does not throw", () => { const link = await createOwnedLink(); const spy = vi.spyOn(LinkRepository, "disable").mockResolvedValueOnce(null); - const result = await disableLink(env as any, link.id, OWNER); - spy.mockRestore(); + let result; + try { + result = await disableLink(env as any, link.id, OWNER); + } finally { + spy.mockRestore(); + } expect(result.ok).toBe(false); if (!result.ok) { diff --git a/src/services/link-management.ts b/src/services/link-management.ts index 27071cd..438690f 100644 --- a/src/services/link-management.ts +++ b/src/services/link-management.ts @@ -197,7 +197,13 @@ export async function deleteLink(env: Env, id: number, identity: string): Promis const slugsToDelete = link.slugs.map((s) => s.slug); const deleted = await LinkRepository.delete(env.DB, id); - if (!deleted) return fail(400, "Cannot delete a link with clicks, disable it instead"); + if (!deleted) { + // delete() returns false for two reasons: a concurrent request removed the + // link, or a concurrent click pushed total_clicks past the lifetime guard. + // Disambiguate so the caller gets 404 for a vanished link, 400 otherwise. + if (!(await LinkRepository.getById(env.DB, id))) return fail(404, "Link not found"); + return fail(400, "Cannot delete a link with clicks, disable it instead"); + } await Promise.all(slugsToDelete.map((s) => SlugCache.delete(env.SLUG_KV, s))); return ok({ deleted: true }); From 1a35f637fe013b499b6c1430f5c1b3e190f90c25 Mon Sep 17 00:00:00 2001 From: Dennis Alund Date: Sat, 30 May 2026 16:50:10 +0800 Subject: [PATCH 3/4] test: restore delete/disable spies via promise finally instead of uninitialized let Copilot review flagged the uninitialized 'let result;' in the try/finally spy blocks as an implicit any. tsc --noEmit passes regardless (evolving-let control flow infers the type), but dropping the bare let removes the noise. Restore the spy with a .finally() on the awaited call: restoration still runs on both resolve and reject, and result is a typed const. --- src/__tests__/service/ownership.test.ts | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/__tests__/service/ownership.test.ts b/src/__tests__/service/ownership.test.ts index 6c34175..0db44e1 100644 --- a/src/__tests__/service/ownership.test.ts +++ b/src/__tests__/service/ownership.test.ts @@ -282,12 +282,7 @@ describe("deleteLink: DB-level guard return value is honored", () => { const link = await createOwnedLink(); const spy = vi.spyOn(LinkRepository, "delete").mockResolvedValueOnce(false); - let result; - try { - result = await deleteLink(env as any, link.id, OWNER); - } finally { - spy.mockRestore(); - } + const result = await deleteLink(env as any, link.id, OWNER).finally(() => spy.mockRestore()); expect(result.ok).toBe(false); if (!result.ok) { @@ -306,12 +301,7 @@ describe("deleteLink: DB-level guard return value is honored", () => { await db.prepare("DELETE FROM links WHERE id = ?").bind(id).run(); return false; }); - let result; - try { - result = await deleteLink(env as any, link.id, OWNER); - } finally { - spy.mockRestore(); - } + const result = await deleteLink(env as any, link.id, OWNER).finally(() => spy.mockRestore()); expect(result.ok).toBe(false); if (!result.ok) { @@ -335,12 +325,7 @@ describe("disableLink: null return from repository does not throw", () => { const link = await createOwnedLink(); const spy = vi.spyOn(LinkRepository, "disable").mockResolvedValueOnce(null); - let result; - try { - result = await disableLink(env as any, link.id, OWNER); - } finally { - spy.mockRestore(); - } + const result = await disableLink(env as any, link.id, OWNER).finally(() => spy.mockRestore()); expect(result.ok).toBe(false); if (!result.ok) { From 58445e2657ca300458e3800aec4e4e665c1c71ce Mon Sep 17 00:00:00 2001 From: Dennis Alund Date: Sat, 30 May 2026 17:02:30 +0800 Subject: [PATCH 4/4] fix: evict delete-time slug set and use cheap existence re-check in deleteLink Copilot review raised two issues on deleteLink: - KV leak under a concurrent custom-slug add. The service evicted the slug list captured by its own initial getById, but LinkRepository.delete cascades every slug row at delete time. A slug added in the window was removed from D1 yet left in KV (no TTL), so redirects kept resolving for a deleted link. delete() now returns the slug set it removed (string[] on success, false when blocked or gone, preserving the existing guard contract), and the service evicts exactly those. - The failed-delete disambiguation called getById, loading the full link plus slugs and click aggregation only to test existence. Added LinkRepository.exists (SELECT 1) and use it instead. Tests: repository returns-removed-slugs and exists cases; a service regression proving a slug added between the read and the delete is still evicted from KV. --- .../repository/link-repository.test.ts | 19 ++++++++++++++++ src/__tests__/service/ownership.test.ts | 22 +++++++++++++++++++ src/db/link-repository.ts | 14 ++++++++++-- src/services/link-management.ts | 13 ++++++----- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/src/__tests__/repository/link-repository.test.ts b/src/__tests__/repository/link-repository.test.ts index ded2a01..e28b72e 100644 --- a/src/__tests__/repository/link-repository.test.ts +++ b/src/__tests__/repository/link-repository.test.ts @@ -398,4 +398,23 @@ describe("LinkRepository click_count options", () => { expect(removed).toBe(false); }); + + it("delete returns every slug it removed on success", async () => { + const link = await LinkRepository.create(env.DB, { url: "https://example.com", slug: "primary" }); + await SlugRepository.addCustom(env.DB, link.id, "secondary"); + + const removed = await LinkRepository.delete(env.DB, link.id); + + expect(removed).not.toBe(false); + if (removed !== false) { + expect([...removed].sort()).toEqual(["primary", "secondary"]); + } + }); + + it("exists reports whether a link row is present", async () => { + const link = await LinkRepository.create(env.DB, { url: "https://example.com", slug: "present" }); + + expect(await LinkRepository.exists(env.DB, link.id)).toBe(true); + expect(await LinkRepository.exists(env.DB, 999999)).toBe(false); + }); }); diff --git a/src/__tests__/service/ownership.test.ts b/src/__tests__/service/ownership.test.ts index 0db44e1..064bf17 100644 --- a/src/__tests__/service/ownership.test.ts +++ b/src/__tests__/service/ownership.test.ts @@ -2,6 +2,7 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { env } from "cloudflare:test"; import { applyMigrations, resetData } from "../setup"; import { LinkRepository } from "../../db"; +import { SlugCache } from "../../kv"; import { createLink, disableLink, @@ -315,6 +316,27 @@ describe("deleteLink: DB-level guard return value is honored", () => { expect(result.ok).toBe(true); expect(await LinkRepository.getById(env.DB, link.id)).toBeNull(); }); + + it("evicts a custom slug added between the read and the repository delete", async () => { + // The link is read, then a custom slug lands before the repository delete + // cascades the slug rows. The repository reports the slug set at delete + // time, so the service must evict the raced slug from KV. Otherwise the + // KV entry (no TTL) keeps the deleted link resolving on redirects. + const link = await createOwnedLink(); + const systemSlug = link.slugs[0].slug; + const racedSlug = "raced-slug"; + + const realDelete = LinkRepository.delete; + const spy = vi.spyOn(LinkRepository, "delete").mockImplementationOnce(async (db, id) => { + await addCustomSlugToLink(env as any, id, { slug: racedSlug }); + return realDelete(db, id); + }); + const result = await deleteLink(env as any, link.id, OWNER).finally(() => spy.mockRestore()); + + expect(result.ok).toBe(true); + expect(await SlugCache.get(env.SLUG_KV, systemSlug)).toBeNull(); + expect(await SlugCache.get(env.SLUG_KV, racedSlug)).toBeNull(); + }); }); describe("disableLink: null return from repository does not throw", () => { diff --git a/src/db/link-repository.ts b/src/db/link-repository.ts index d613fed..502d2aa 100644 --- a/src/db/link-repository.ts +++ b/src/db/link-repository.ts @@ -137,17 +137,27 @@ export class LinkRepository { return LinkRepository.getById(db, id); } - static async delete(db: D1Database, id: number): Promise { + static async exists(db: D1Database, id: number): Promise { + const row = await db.prepare("SELECT 1 FROM links WHERE id = ?").bind(id).first(); + return row !== null; + } + + static async delete(db: D1Database, id: number): Promise { // Lifetime guard: a link with any historical clicks (bots, self-referrers, // or real users) is preserved so analytics history is not silently dropped. const link = await LinkRepository.getById(db, id); if (!link) return false; if (link.total_clicks > 0) return false; + // Capture the slug set at delete time. The caller evicts these from KV; + // sourcing them here (not from the caller's earlier read) covers a custom + // slug added in the window before this delete cascades the slug rows. + const slugs = link.slugs.map((s) => s.slug); + await db.prepare("DELETE FROM clicks WHERE slug IN (SELECT slug FROM slugs WHERE link_id = ?)").bind(id).run(); await db.prepare("DELETE FROM slugs WHERE link_id = ?").bind(id).run(); await db.prepare("DELETE FROM links WHERE id = ?").bind(id).run(); - return true; + return slugs; } static async search( diff --git a/src/services/link-management.ts b/src/services/link-management.ts index 438690f..2dd0b9f 100644 --- a/src/services/link-management.ts +++ b/src/services/link-management.ts @@ -195,16 +195,17 @@ export async function deleteLink(env: Env, id: number, identity: string): Promis if (link.created_by !== identity) return fail(403, "Only the link owner can delete this link"); if (link.total_clicks > 0) return fail(400, "Cannot delete a link with clicks, disable it instead"); - const slugsToDelete = link.slugs.map((s) => s.slug); - const deleted = await LinkRepository.delete(env.DB, id); - if (!deleted) { + const deletedSlugs = await LinkRepository.delete(env.DB, id); + if (!deletedSlugs) { // delete() returns false for two reasons: a concurrent request removed the // link, or a concurrent click pushed total_clicks past the lifetime guard. - // Disambiguate so the caller gets 404 for a vanished link, 400 otherwise. - if (!(await LinkRepository.getById(env.DB, id))) return fail(404, "Link not found"); + // A cheap existence check disambiguates: 404 for a vanished link, 400 otherwise. + if (!(await LinkRepository.exists(env.DB, id))) return fail(404, "Link not found"); return fail(400, "Cannot delete a link with clicks, disable it instead"); } - await Promise.all(slugsToDelete.map((s) => SlugCache.delete(env.SLUG_KV, s))); + // Evict the slug set the repository removed at delete time, not the slugs from + // the read above, so a custom slug added in between is not orphaned in KV. + await Promise.all(deletedSlugs.map((s) => SlugCache.delete(env.SLUG_KV, s))); return ok({ deleted: true }); }