fix(api): guard lease renew/release updates with state predicates#382
Conversation
Plan 007. renewLease/releaseLease did check-then-act: SELECT the lease, branch in JS, then UPDATE with no state guard in the WHERE. A concurrent release or expiry-driven re-acquire between the select and the update could let renewLease stamp a fresh future expires_at onto an already-released lease, returning a misleading success. Add releasedAt/expiresAt predicates to both UPDATEs and detect 0-row writes via .returning(); on a lost race, re-select once and map to the existing NOT_FOUND/LEASE_EXPIRED codes so client-visible semantics are unchanged. Mutual exclusion on acquire was already safe via the partial unique index; this is a correctness-of-response fix. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Plan 006 is superseded: main shipped a stronger fix for the same issue (#289/#290) in aaf05ac (PR #355) after this plan's ce9a1fa baseline. The Idempotency-Key claim is now embedded atomically in the same d1.batch() as the mutation, rather than plan 006's separate claim-then-mutate step. Plan 007 was unaffected (#355 guarded a different code path) and ships as #382. Plan 012's hard blocker dissolved: PR #380 implemented #344 and wrote docs/webhooks.md with the timestamped signature scheme in the same PR, so the docs were written against the correct format. Plan 012 is now UI-only. Three plans went stale within hours of being written, each differently: 008 was wrong on arrival, 006 was overtaken by unrelated work, 012 by its own dependency being solved better elsewhere. The drift check guards only the first, and only when the executor's baseline is the pinned commit. Also records that #370 and #380 both edit createConversation/deleteConversation and will conflict, plus a suggested merge order. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
* docs(plans): track the plans directory and add a status index The 10 implementation plans were untracked, so they existed only on one machine while every PR referenced them by path, and no worktree could see them. Each plan also instructs its executor to "update this plan's row in plans/README.md" — a file that never existed, which three separate executors hit and correctly flagged rather than fabricating. Adds plans/README.md: status table with issue links, the cross-plan dependency graph, and a note that plan 008 is superseded (its target design preserves the /watch deadlock in #360, so its own regression test would hang). Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> * docs(plans): add plans 011-014 for the held product/architecture issues Plans for the four issues deliberately held out of the parallel fix batch because they are product decisions rather than defined fixes: - 011 dashboard UI for the 4 unexposed primitives (#284) - 012 webhooks docs + dashboard UI (#285) - 013 demo sandbox / pre-signup try-it (#287) - 014 write atomicity via db.batch() (#342) 014 confirms D1 batch() semantics against Cloudflare docs: statements execute sequentially as one all-or-nothing SQL transaction, but all statements must be prepared before the call — no reading a result mid-batch and branching on it. All three call sites are batchable as-is; updateConversationMessageCount is already a blind relative SQL update rather than a read-then-write. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> * docs(plans): mark 006 superseded and 012 docs-done; record merge order Plan 006 is superseded: main shipped a stronger fix for the same issue (#289/#290) in aaf05ac (PR #355) after this plan's ce9a1fa baseline. The Idempotency-Key claim is now embedded atomically in the same d1.batch() as the mutation, rather than plan 006's separate claim-then-mutate step. Plan 007 was unaffected (#355 guarded a different code path) and ships as #382. Plan 012's hard blocker dissolved: PR #380 implemented #344 and wrote docs/webhooks.md with the timestamped signature scheme in the same PR, so the docs were written against the correct format. Plan 012 is now UI-only. Three plans went stale within hours of being written, each differently: 008 was wrong on arrival, 006 was overtaken by unrelated work, 012 by its own dependency being solved better elsewhere. The drift check guards only the first, and only when the executor's baseline is the pinned commit. Also records that #370 and #380 both edit createConversation/deleteConversation and will conflict, plus a suggested merge order. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net> --------- Co-authored-by: duyetbot <bot@duyet.net>
1 participant
Closes #367
Summary
Implements plan 007 (
plans/007-lease-update-guards.md).renewLeaseandreleaseLeasedid check-then-act: SELECT the lease,branch on
releasedAt/expiresAtin JS, thenUPDATE ... WHERE id = ?with no state guard in the
WHERE. Between select and update, aconcurrent release or expiry-driven re-acquire could change the row —
renewLeasewould then stamp a freshexpiresAtonto analready-released lease and return a misleading success for a
coordination primitive.
UPDATEs now carryreleasedAt IS NULL(+expiresAt > nowforrenew) guards and use
.returning()to detect a lost race.NOT_FOUND/LEASE_EXPIREDcodes — client-visible semantics areunchanged; only the internal write is now correct under concurrency.
already race-safe — this is a correctness-of-response fix, not a
lock-safety hole.
Plan 006 (idempotency claim-first, #365) is NOT included — it was
already fixed on
mainby PR #355 (commitaaf05ac, closing #289/#290)with a stronger design (idempotency claim embedded atomically in the
same D1 batch as the mutation, plus a 60s orphan-reservation reclaim).
This branch is rebased onto current
main(includes #355 and #282), soit does not duplicate or conflict with that fix.
services/leases.ts'srenewLease/releaseLease— this PR's target — were untouched by #355(#355's lease fix guards a different function, the write-time lease gate
inside
upsertState/deleteState), so no overlap there.Test plan
bunx tsc --noEmit -p packages/api/tsconfig.json— exit 0bunx biome check packages/api/src/— clean except onepre-existing, unrelated formatting issue in
lib/validation.tsthat predates this branch (confirmed present on unmodified
main)cd packages/api && bunx vitest run— 364/364 pass, run afterthe rebase onto current main (not carried over from before)
expires_atunchanged; concurrent renew/release race → responsealways matches the actual write (no silent success without an
actual write, no reported failure with a silent write)