Skip to content

fix: abort only the query's own request on re-execution, and stop submit() from mutating params - #116

Merged
alessandrobellesia merged 3 commits into
developfrom
fix/abort-own-request
Oct 1, 2026
Merged

alessandrobellesia merged 3 commits into
developfrom
fix/abort-own-request

Conversation

@alessandrobellesia

@alessandrobellesia alessandrobellesia commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

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 abort stored on its old hash. A hash is shared by every query with the same params and keeps the abort of its last request, so the request of another query could be the one aborted: that query ended idle, with no data and no error, and nothing retried it.

Fix. Each action keeps a lease on the request it waits on (PendingRequest in src/utilities.ts) and releases only its own when it moves to other params. 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, as before.

Also fixed in the read() join, all with regression tests:

  • the joining query registers on the hash before awaiting, so it shows the request as loading instead of idle;
  • it no longer registers again once the request ends. That second registration enabled again a query cleaned up meanwhile (its query then stayed in the store for ever), sent a query shared by name back to the old params, and cleared the abort/promise 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 now sets its hash back to idle only while the hash still holds that request, identified by its response promise. Before, an aborted request could reset to idle the result of another query with the same params.

Behavior changes (in the CHANGELOG under "Changed"):

  • a 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);
  • the hashes of submit() and remove() now hold the promise of their request while it is loading, as the ones of read() do;
  • an execution replaced by one with other params still resolves with aborted: true, also when its request keeps running for another query that joined it.

Follow-up from the CodeRabbit review (third commit):

  • a repository may hand back the request already in flight for the same key, with the same response promise: its leases are now counted on one pending request, and a query takes its new lease before it releases the older ones, so asking for that request again no longer aborts it;
  • a read() with the params of a request that was just aborted, before it settled, joined it and resolved as aborted (this already happened on develop). It now sends a new request with key: false: RepositoryHttp deduplicates 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 params

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 autoExecute ones. A later execute() with the payload of another item kept the first key and updated the first item (submit({ id: '1' }) then execute({ id: '2' }) sent PUT /1). With reactive params and autoExecute, the write triggered the watcher again and the same PUT was sent twice. execute() now works on a copy.

Dependencies (ncu)

Nothing applied:

  • typescript stays at 6.0.3: 7.0.2 is still the latest, and TS 7 drops the JavaScript Compiler API until 7.1, which breaks declaration emit in unplugin-dts;
  • vitest stays at 5.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 develop and 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: clean
  • pnpm type-check: clean
  • pnpm build: clean, and the emitted index.d.ts is byte for byte identical to the one on develop

CHANGELOG: new [2.1.1] entry.

🤖 Generated with Claude Code

Alessandro Bellesia and others added 2 commits October 1, 2026 12:07
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>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: eca3251b-cf75-4124-9b85-73d5d7c99c41

📥 Commits

Reviewing files that changed from the base of the PR and between dc63c9f and 6f264c2.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • src/index.ts
  • src/utilities.ts
  • test/read-advanced.test.ts
  • test/remove-advanced.test.ts
  • test/submit-advanced.test.ts
  • test/utils.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The 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.

Changes

Request ownership and execution

Layer / File(s) Summary
Lease-based read sharing
src/utilities.ts, src/index.ts, test/utils.ts, test/read-advanced.test.ts
Reads track leases for requests they send or join. A request is aborted when no leases remain, and an aborted response does not reset a hash that now points to a newer promise. Tests cover shared reads, parameter changes, and query cleanup.
Submit and remove request ownership
src/index.ts, test/submit-advanced.test.ts, test/remove-advanced.test.ts, CHANGELOG.md
Submit and remove executions track their request leases and guard hash updates against older responses. Submit copies selected parameters before adding a payload key. Tests cover overlapping requests and submit parameter handling; the changelog records these changes.

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
Loading

Merge Risk: ⚪ Minimal · up to 6f264

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 Review

Security architecture risk: 🔵 Low · up to 6f264

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

  • Medium · reliability · inferred: If a repository returns the same active response promise with shared abort authority, a persistence-expired same-hash read creates a replacement lease manager, then releases its older leases before acquiring the new lease. The older manager can therefore cancel work still awaited by the replacement or another query. Base did not cancel on same-hash re-execution. This is a conditional failure-containment regression; the external repository contract needed to establish production reachability is unavailable.
Security review details

Security Blast Radius

  • inferred — The conditional cancellation path requires a caller able to repeat reads with matching hashes and bypass the cached hash through persistence expiry, plus a repository that reuses response identity and abort authority. The evidenced failure domain is the shared request and its waiters within one store instance. Wider tenant, service, or data-store exposure depends on application and repository behavior not established here.

Trust Boundaries and Controls

  • observed — The new control governs cancellation authority through lease ownership and aborted-response identity checks. Caller parameters and payloads continue through the existing injected repository methods; these changed paths do not establish downstream authorization or tenant-isolation guarantees.

Resilience and Maintainability Implications

  • observed — Read cleanup disables the query without releasing its request lease, preserving the documented non-aborting cleanup behavior. Regression tests express the intended shared-request isolation, but the manual repository allocates a fresh promise per request and therefore does not resolve the response-promise-reuse uncertainty.

Hardening Proposals

  • proposed — Make the repository’s response-identity and cancellation-sharing contract explicit. If active promises can be reused, preserve their lease manager and acquire the replacement interest before releasing older interest. Validate same-hash refresh and parameter changes while another query still waits.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The changes address the main #111 race. They add per-query request leases, register read joiners before waiting, release only the caller's lease, guard hash reset by the latest promise, and add regres… Prevent read() from joining an already-aborted pending request, or provide a new refcounted pending request for the joiner in the repository layer. Add a regression test that joins during the aborted-before-settled window and verifies suc…
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The request-lease implementation and its tests directly support #111. The submit() parameter-copy change is explicitly part of the PR scope and has focused regression tests. The changelog documents …
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: isolating request abortion during re-execution and preventing submit() from mutating caller-owned parameters.
Full details: Linked Issues check

Explanation

The changes address the main #111 race. They add per-query request leases, register read joiners before waiting, release only the caller's lease, guard hash reset by the latest promise, and add regression tests for shared reads, submit, and remove. However, the reported behavior still allows read() to join a RepositoryHttp request after that request was aborted but before it settled. The joiner then resolves as aborted instead of receiving the result. This leaves the #111 requirement unmet for that in-flight join case.

Resolution

Prevent read() from joining an already-aborted pending request, or provide a new refcounted pending request for the joiner in the repository layer. Add a regression test that joins during the aborted-before-settled window and verifies successful completion.

Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

A rabbit checks the requests in flight,
Each lease keeps shared work right.
When all let go, the calls may rest,
Fresh params travel un-distressed.
I nibble greens and cheer the test!

Comment @coderabbitai help to get the list of available commands.

…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>
@alessandrobellesia

Copy link
Copy Markdown
Member Author

@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: a repository that hands back the request in flight keeps it for the query asking again.

2. Linked issues check: joining an aborted request before it settles. Confirmed and fixed. A read() no longer joins a request whose pending request was aborted: it sends a new one with key: false, because RepositoryHttp deduplicates reads by key and would hand back the aborting request until it settles. Test: a read does not join a request aborted but not settled yet, which also fails if only the key: false override is removed. Strictly, this window is outside #111 (nobody waits on the request when it is aborted, and develop behaves the same), but the outcome is the same kind of lost read, so it is fixed here.

3. Docstring coverage. Added docstrings to the new PendingRequest API (aborted, lease()). The other touched functions keep the comment style of the surrounding code; I did not add docstrings only to reach the threshold.

Checks on cf771e5: 97/97 tests, lint, type-check and build are clean, and the emitted index.d.ts is unchanged.

@sonarqubecloud

sonarqubecloud Bot commented Oct 1, 2026

Copy link
Copy Markdown

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown

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.ts

Length of output: 12175


@alessandrobellesia Thanks for the fixes and the clarification. I inspected cf771e5.

  • The read path reuses one PendingRequest per response promise and acquires the new lease before releasing older leases.
  • The read path rejects an aborted request as a join target and overrides the repository key with key: false.
  • Both regression tests are present. The PendingRequest.aborted and PendingRequest.lease() docstrings describe the new API.

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 index.d.ts.


✏️ Learnings added
Learnt from: alessandrobellesia
URL: https://github.com/volverjs/query-vue/pull/116

Timestamp: 2026-10-01T10:39:27.299Z
Learning: In volverjs/query-vue, RepositoryHttp deduplicates reads by key and can return an aborted request until it settles. The read path in src/index.ts must bypass that deduplication with key: false when replacing an aborted, unsettled request. This window also exists on develop and is outside issue #111's shared-request ownership case.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


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.

@alessandrobellesia
alessandrobellesia merged commit 869b912 into develop Oct 1, 2026
9 checks passed
@alessandrobellesia
alessandrobellesia deleted the fix/abort-own-request branch October 1, 2026 10:41
@alessandrobellesia alessandrobellesia mentioned this pull request Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-executing a query can abort a request that belongs to another query

1 participant