fix: honor repository guard return values in deleteLink and disableLink#11
Merged
Conversation
…eLink
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
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 58445e2 | May 30 2026, 09:03 AM |
There was a problem hiding this comment.
Pull request overview
This PR fixes race-condition edge cases in link disable/delete service methods by honoring repository guard return values and ensuring null repository results are handled safely, with accompanying regression tests.
Changes:
disableLink: handlenullreturn fromLinkRepository.disable()and avoid unsafe non-null assertions.deleteLink: honor boolean return fromLinkRepository.delete()and fail the request when the repository-level guard blocks deletion.- Add Vitest regression tests covering both race scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/services/link-management.ts |
Adds repository-return handling for disableLink and deleteLink to prevent incorrect success responses / runtime throws under races. |
src/__tests__/service/ownership.test.ts |
Adds regression tests validating correct service behavior when repository guards return false/null. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…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.
…nitialized 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.
…eleteLink 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Weekly defect review — 2026-05-29
Two bugs found and fixed. All 892 tests green.
Fix 1:
deleteLinkignores return value ofLinkRepository.delete()What was wrong:
deleteLinkinsrc/services/link-management.tscheckedlink.total_clicks > 0before callingLinkRepository.delete(), but ignored the boolean thatdelete()returns.LinkRepository.delete()performs its own click-count guard internally (a secondgetById+ check). In the window between the service-level check and the repository DELETE, a concurrent click could be recorded. In that case the repository returnedfalse(deletion blocked), but the service discarded the return value and responded{ deleted: true }to the caller. The KV cache entries for the link's slugs were also incorrectly evicted.Impact: API response lied — callers received
200 { deleted: true }for a link that still existed in the database. The KV eviction meant the next redirect fell through to D1 and re-populated KV, so redirect behaviour recovered, but the stated contract was broken.Fix: Check the return value; propagate
fail(400, ...)if the repository guard fired.Regression test:
deleteLink: DB-level guard return value is honored— usesvi.spyOn(LinkRepository, 'delete').mockResolvedValueOnce(false)to simulate the race. Failed before the fix (result.okwastrue); passes after.Fix 2:
disableLinkunsafe non-null assertion onLinkRepository.disable()resultWhat was wrong:
disableLinkcalledLinkRepository.disable()then immediately useddisabled!.slugswith a non-null assertion, without checking for null.LinkRepository.disable()returnsnullwhen the link row does not exist at UPDATE time (the link could be deleted in the window between the service's precedinggetByIdand the repository'sUPDATE). The function would throw aTypeError: Cannot read properties of null (reading 'slugs')instead of returning a structured error.enableLink(the counterpart) already had the correct null-guard (if (!enabled) return fail(404, ...)).disableLinkwas missing the same protection.Impact: Concurrent delete during a disable produced an unhandled 500 error instead of a clean 404.
Fix: Added
if (!disabled) return fail(404, "Link not found")immediately after thedisable()call, matching the pattern inenableLink. Removed the!assertions from subsequent uses ofdisabled.Regression test:
disableLink: null return from repository does not throw— usesvi.spyOn(LinkRepository, 'disable').mockResolvedValueOnce(null). Before the fix the function threwTypeError; after the fix it returns{ ok: false, status: 404 }.https://claude.ai/code/session_01PNGGm7htQKRuHz1nAy9Qib
Generated by Claude Code