Repository navigation
fix: abort only the query's own request on re-execution, and stop submit() from mutating params - #116
Conversation
Re-executing a query with other params aborted the `abort` stored on its old hash, and a hash is shared by every query with the same params: the request of another query could be the one aborted, and that query ended idle with no data and no error. Each action now keeps a lease on the request it waits on and releases only its own. A read() that joins a request in flight takes a lease too, so a shared request is aborted only when no query waits on it any more; a clean up still never aborts a request. In the read() join: - the query joins the hash before awaiting, so it shows the request as loading, and no longer registers again once the request ends: that re-registration enabled again a query cleaned up meanwhile, sent a query shared by name back to the old params, and cleared the handles of a newer request on the hash, which could then stay loading for ever; - with a persistence shorter than a request, a query releases its older request on the same hash and waits on the newer one with its own lease. An aborted request sets its hash back to idle only while the hash still holds that request, identified by its response promise: the hashes of submit() and remove() now hold it while loading too. Fixes #111 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
submit() set the key of a single payload on the params object it got, the caller's own one on the execution started by the call and on the ones of autoExecute. A later execute() with the payload of another item kept the first key and updated the first item, and with reactive params and autoExecute the write triggered the watcher again, so the same update was sent twice. execute() now works on a copy of the params. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds lease-based request ownership for reads, submits, and removals. Shared reads remain active while a query still holds a lease. Submit execution also copies selected parameters before adding a payload key. ChangesRequest ownership and execution
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ReadQueryA
participant ReadQueryB
participant PendingRequest
participant Repository
ReadQueryA->>Repository: Start read request
Repository-->>ReadQueryA: Return response promise
ReadQueryA->>PendingRequest: Acquire request lease
ReadQueryB->>PendingRequest: Acquire lease for shared promise
ReadQueryA->>PendingRequest: Release its lease
PendingRequest-->>ReadQueryB: Keep request active while a lease remains
Repository-->>ReadQueryB: Resolve shared response
Merge Risk: ⚪ Minimal · up to The change improves request ownership and prevents submit parameter mutation. No confirmed merge-blocking issue remains; the documented aborted-request join limitation remains unchanged. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Cancellation is better isolated between queries, and submit no longer modifies the caller’s top-level parameters. One compatibility uncertainty remains: refreshing a read could cancel work still awaited elsewhere if the repository reuses an active response promise and its cancellation authority. The demonstrated scope is shared work within an application store, not a verified privilege or tenant-boundary compromise. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes address the main Resolution Prevent Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit checks the requests in flight, Comment |
…ted one Follow-up to the CodeRabbit review of #116. A repository may hand back the request already in flight for the same key, with the same response promise. A read() that asked for it again created a second pending request for that promise and released its older lease before taking the new one, so it could abort the request it was about to wait on. The leases of a response promise are now counted on one pending request, and a query takes its new lease before it releases the older ones. A read() with the params of a request that was just aborted, before it settled, joined it and resolved as aborted. It now sends a new request instead, without the repository key: RepositoryHttp deduplicates reads by key and would hand back the aborting request until it settles. A pending request is marked aborted only when it had an `abort` to call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai thanks for the review. All three points are addressed in cf771e5; each fix was first reproduced with a failing test. 1. Retained concern: a repository that hands back the same response promise. Confirmed. With a repository that returns the request in flight for the same key, a persistence-expired re-read created a second pending request for that promise and released its older lease first, aborting the request it was about to wait on. The leases of a response promise are now counted on one pending request, and a query takes its new lease before it releases the older ones. Test: 2. Linked issues check: joining an aborted request before it settles. Confirmed and fixed. A 3. Docstring coverage. Added docstrings to the new Checks on cf771e5: 97/97 tests, lint, type-check and build are clean, and the emitted |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-patch --format='%h %s' cf771e5
ast-grep outline src/index.ts --items all --match 'read|PendingRequest' --view expanded
ast-grep outline src/utilities.ts --items all --match 'PendingRequest' --view expanded
git show --format=short --stat cf771e5
git show --format= --unified=12 cf771e5 -- src/index.ts src/utilities.ts test/read-advanced.test.tsLength of output: 12175
These changes address the three review points. Keeping the surrounding comment style is reasonable; no threshold-only docstrings are needed. I did not run the checks independently. Your reported results cover 97 tests, lint, type-check, build, and unchanged ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |



Fixes #111, plus a second bug found while writing its tests. One commit each, plus a follow-up commit for the CodeRabbit review.
1. Re-executing a query aborts only its own request (#111)
Re-executing a query with other params called the
abortstored on its old hash. A hash is shared by every query with the same params and keeps theabortof its last request, so the request of another query could be the one aborted: that query endedidle, with no data and no error, and nothing retried it.Fix. Each action keeps a lease on the request it waits on (
PendingRequestinsrc/utilities.ts) and releases only its own when it moves to other params. Aread()that joins a request in flight takes a lease too, so a shared request is aborted only when no query waits on it any more. A clean up still never aborts a request, as before.Also fixed in the
read()join, all with regression tests:idle;nameback to the old params, and cleared theabort/promiseof a newer request on the hash, which could then stayloadingfor ever;persistenceshorter than a request, a query releases its older request on the same hash and waits on the newer one with its own lease.An aborted request now sets its hash back to
idleonly while the hash still holds that request, identified by its response promise. Before, an aborted request could reset toidlethe result of another query with the same params.Behavior changes (in the CHANGELOG under "Changed"):
read()that moves to params already cached or already loading now aborts its own previous request too, as it does when it sends a new one (unless another query still waits on it);submit()andremove()now hold thepromiseof their request while it is loading, as the ones ofread()do;aborted: true, also when its request keeps running for another query that joined it.Follow-up from the CodeRabbit review (third commit):
read()with the params of a request that was just aborted, before it settled, joined it and resolved as aborted (this already happened ondevelop). It now sends a new request withkey: false:RepositoryHttpdeduplicates reads by key and would hand back the aborting request until it settles.2.
submit()no longer writes the payload key into the caller's paramssubmit()set the key of a single payload on the params object it got, the caller's own one on the execution started by the call and on theautoExecuteones. A laterexecute()with the payload of another item kept the first key and updated the first item (submit({ id: '1' })thenexecute({ id: '2' })sentPUT /1). With reactive params andautoExecute, the write triggered the watcher again and the samePUTwas sent twice.execute()now works on a copy.Dependencies (
ncu)Nothing applied:
typescriptstays at6.0.3: 7.0.2 is still the latest, and TS 7 drops the JavaScript Compiler API until 7.1, which breaks declaration emit inunplugin-dts;viteststays at5.0.2: 5.0.3 was published 2026-09-30 11:30 UTC and was still under pnpm's one-day minimum release age when this branch was prepared.Review
Every review finding was first reproduced with a failing test, then fixed. Each new test fails on
developand passes here.Checks
pnpm test: 97/97 passed (93/93 on the first commit on its own, 95/95 on the second)CI=true pnpm lint: cleanpnpm type-check: cleanpnpm build: clean, and the emittedindex.d.tsis byte for byte identical to the one ondevelopCHANGELOG: new
[2.1.1]entry.🤖 Generated with Claude Code