fix: atomic NOT EXISTS click guard in SlugRepository.remove by DennisAlund · Pull Request #14 · oddbit/shrtnr

DennisAlund · 2026-06-12T19:18:45Z

Summary

Weekly defect review. One confirmed new bug found; all other candidates were refuted or pre-existing.

What was wrong

SlugRepository.remove checked click_count > 0 via a pre-read SELECT, then ran the batch DELETE unconditionally. A click arriving between the pre-read and the DELETE would produce an orphaned clicks row: PRAGMA foreign_keys is not set anywhere in the codebase (confirmed by search), so D1's FK cascade is inactive and clicks.slug ON DELETE CASCADE never fires.

LinkRepository.delete was hardened against the same race in PR #13 by embedding AND NOT EXISTS (SELECT 1 FROM clicks ...) inside the transactional batch. SlugRepository.remove was left unguarded by that pass.

The method's own comment states: "never drop a slug that has recorded any click, so analytics rows are not orphaned" — the pre-read alone does not enforce this under concurrent load.

The fix

Added AND NOT EXISTS (SELECT 1 FROM clicks WHERE slug = ?) to the DELETE inside the batch, matching the pattern used in LinkRepository.delete.
Checked deleteResult.meta.changes and return false when the guard blocks the delete. The service layer (removeSlug) maps this to 400 "Cannot remove a slug with clicks, disable it instead", matching the existing pre-read guard and deleteLink; a slug that left this link concurrently maps to 404.
Refactored the pre-read to call SlugRepository.findByValue (an existing public static method) instead of an inline db.prepare, making it spyable for the race-condition test.

The regression test

src/__tests__/repository/slug-repository.test.ts — "guard holds when a click lands between the pre-read and the batch delete":

Inserts a click record for the slug.
Spies on findByValue to return click_count: 0 (simulating the race window where the pre-read predates the click).
Calls remove and asserts it returns false.
Asserts both the slug row and the click row are still present.

On the old code (no NOT EXISTS), the DELETE would fire and remove would return true, failing the assertion. On the new code the guard blocks the delete.

Test suite: 933 tests, all pass.

https://claude.ai/code/session_01RCqBLUKrf78eW9EdrqD9Mi

Generated by Claude Code

The previous code checked click_count > 0 via a pre-read SELECT and then ran the batch DELETE unconditionally. A click arriving between the pre-read and the DELETE would be orphaned: FK cascade is not active (no PRAGMA foreign_keys in D1), so the clicks row would reference a deleted slug with no cascade cleanup. LinkRepository.delete already guards against this race by embedding AND NOT EXISTS (SELECT 1 FROM clicks ...) inside the transactional batch. SlugRepository.remove was left unguarded by the same 0.35.3 hardening pass. Fix: add the same NOT EXISTS re-check to the DELETE statement and check the result (meta.changes) to detect when the guard blocks the delete. Use SlugRepository.findByValue for the pre-read so the method is spyable in tests. Regression test: mocks findByValue to return click_count=0 while an actual click row exists in the DB; asserts remove() returns false and both the slug and click records survive. https://claude.ai/code/session_01RCqBLUKrf78eW9EdrqD9Mi

cloudflare-workers-and-pages · 2026-06-12T19:19:42Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	shrtnr	`94643de`	Jun 15 2026, 09:10 AM

Copilot

Pull request overview

This PR hardens SlugRepository.remove against a race where a click can arrive between the pre-read and the transactional delete, preventing orphaned clicks rows when FK cascade is inactive in D1.

Changes:

Refactors the pre-read in SlugRepository.remove to call SlugRepository.findByValue (enabling spying in tests).
Adds an atomic NOT EXISTS (SELECT 1 FROM clicks ...) guard to the slug DELETE inside the transactional batch and returns false when the guard blocks deletion.
Adds a regression test that simulates the race window by spying on findByValue to report click_count: 0 while a click row exists at DELETE time.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/db/slug-repository.ts`	Adds an atomic `NOT EXISTS` guard to prevent deleting slugs that have click history under concurrent load.
`src/__tests__/repository/slug-repository.test.ts`	Adds a regression test for the race window between the pre-read and transactional delete.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The handover UPDATE that flips the random slug to primary ran unconditionally inside the batch. When a click landed in the race window the guarded DELETE was blocked but the UPDATE still fired, leaving the link with two primaries while remove returned false. Carry the NOT EXISTS click guard on the UPDATE so the handover only runs when the delete fires, matching how LinkRepository.delete self-gates follow-on work within the transaction. Strengthen the race regression test to assert the primary set is left untouched when the delete is blocked.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…ment[] The empty const statements = [] inferred an evolving any[]. Annotating it as D1PreparedStatement[] keeps db.batch() statically checked and matches the type the other batch arrays in this file already infer from their initializers.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

The spy argument called findByValue while the spy was already installed, so the test relied on vi.spyOn's call-original default and on the mock queue being empty during that read. Both pre-read and the click > 0 early return leave identical final state, so the test could pass for the wrong reason without exercising the batch NOT EXISTS guard. Capture the real row before installing the spy, then mock the single pre-read inside remove. Assert the spy was called exactly once (before mockRestore clears the history) so a false result is proven to come from the batch guard, not an early return. Verified the test fails when either the DELETE guard or the handover guard is removed.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

…ccess removeSlug ignored SlugRepository.remove's boolean and always returned { removed: true }, evicting the cache unconditionally. When a click lands between the service pre-read and the repository's transactional delete, the NOT EXISTS guard blocks the delete and remove returns false, but the API still reported success and dropped the cache entry for a slug that is still resolving. Capture the return and mirror deleteLink: on false, an existence check disambiguates 404 (slug vanished) from 400 (click history), and the cache is evicted only after a confirmed delete. Adds service tests for both the guard-blocked (400, no eviction) and vanished (404) races.

The schema declares clicks.slug REFERENCES slugs(slug) ON DELETE CASCADE, and whether D1 enforces it can vary by environment, so claiming the cascade is definitively inactive was misleading. Reframe the comment around the invariant that holds regardless: a slug with clicks must never be deleted, since the delete would either orphan or cascade-drop its clicks rows, both losing analytics history.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

DennisAlund · 2026-06-15T07:34:10Z

      statements.push(
-        db.prepare("UPDATE slugs SET is_primary = 1 WHERE link_id = ? AND is_custom = 0").bind(row.link_id),
+        db
+          .prepare(
+            "UPDATE slugs SET is_primary = 1 WHERE link_id = ? AND is_custom = 0 AND NOT EXISTS (SELECT 1 FROM clicks WHERE slug = ?)",
+          )
+          .bind(row.link_id, slug),
      );


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.

The handover UPDATE fired on row.is_primary from the pre-read without re-checking that the slug was still primary at batch time. A concurrent setPrimary moving primary to another custom slug between the pre-read and the batch would let the handover promote the random slug alongside the new primary, leaving the link with two primaries once the delete fired. Add AND EXISTS (SELECT 1 FROM slugs WHERE slug = ? AND is_primary = 1) to the handover so it only promotes the random slug when this slug still holds primary inside the transaction. Adds a regression test driving a stale pre-read against a concurrent setPrimary; verified each guard on the UPDATE (click NOT EXISTS, still-primary EXISTS) is independently load-bearing.

disable carried the same stale-primary handover as remove: it promoted the random slug based on row.is_primary from the pre-read without re-checking that the slug still held primary at batch time. A concurrent setPrimary moving primary to another custom slug in that window left the link with two primaries. Apply the same guard remove uses: promote the random slug only while this slug still holds primary inside the batch (promote first, then demote), with AND EXISTS (SELECT 1 FROM slugs WHERE slug = ? AND is_primary = 1). Read the pre-read through findByValue so the race is spyable, matching remove. Adds a regression test; verified the guard is load-bearing.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

DennisAlund · 2026-06-15T09:09:56Z

+    // remove() returns false for two reasons: a concurrent request already
+    // removed the slug, or a click landed between the pre-read above and the
+    // transactional delete (the NOT EXISTS guard then blocks it). A cheap
+    // existence check disambiguates: 404 for a vanished slug, 400 otherwise.
+    if (!(await SlugRepository.exists(env.DB, slug))) return fail(404, "Slug not found on this link");
+    return fail(400, "Cannot remove a slug with clicks, disable it instead");
+  }


Fixed in 94643de. The disambiguation no longer uses the global SlugRepository.exists; it re-reads via findByValue and compares link_id, so a slug freed from this link and re-claimed by another in the race window is correctly reported as 404 rather than the click-history 400. (deleteLink is safe with a global id check because link ids are autoincrement and never re-claimed; slug text is, which is exactly the case you flagged.) Added a regression test that mocks findByValue to a different link_id and asserts 404; verified it fails on the old global-exists check (returns 400).

DennisAlund · 2026-06-15T09:09:57Z

+    // transactional delete (the NOT EXISTS guard then blocks it). A cheap
+    // existence check disambiguates: 404 for a vanished slug, 400 otherwise.
+    if (!(await SlugRepository.exists(env.DB, slug))) return fail(404, "Slug not found on this link");
+    return fail(400, "Cannot remove a slug with clicks, disable it instead");


400 is intended. It matches the existing pre-read guard (Cannot remove a slug with clicks, disable it instead) and deleteLink's has-clicks behavior, so the test asserting 400 is correct. The PR description's 409 was wrong; updated it to state 400 for has-clicks and 404 when the slug left this link concurrently.

The fallback used SlugRepository.exists, a global slug-value check. Slug values are globally unique and can be re-claimed by another link once freed, so a slug removed from this link and re-claimed elsewhere in the race window would read as existing and return the click-history 400 instead of 404. (deleteLink avoids this because link ids are not re-claimable; slug text is.) Disambiguate with findByValue and compare link_id: 404 when the slug no longer belongs to this link, 400 otherwise. Updates the vanished test to the new mechanism and adds a re-claimed-by-another-link test; verified the re-claim test fails on the old global check.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

PR #14 extends the 0.35.3 concurrency work to slug removal and disable: the click guard moved inside the delete transaction, the primary handover re-checks current primary status, and removeSlug reports the real outcome (400/404) instead of a false success. OpenAPI surface unchanged. The version bump re-embeds info.version, so the recorded spec hash is refreshed in all three SDK manifests in this commit to keep the sdk-spec-drift gate green; no SDK code or version changes.

Copilot AI review requested due to automatic review settings June 12, 2026 19:18

Copilot started reviewing on behalf of DennisAlund June 12, 2026 19:19 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

Comment thread src/db/slug-repository.ts Outdated

Comment thread src/__tests__/repository/slug-repository.test.ts

DennisAlund requested a review from Copilot June 15, 2026 05:30

Copilot started reviewing on behalf of DennisAlund June 15, 2026 05:30 View session