Skip to content

fix(storage): cancel paginated value reads - #276

Closed
perfloop[bot] wants to merge 14 commits into
mainfrom
perfloop-pr-open-j5x7c844wd
Closed

perfloop[bot] wants to merge 14 commits into
mainfrom
perfloop-pr-open-j5x7c844wd

Conversation

@perfloop

@perfloop perfloop Bot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Canceled cluster value-page requests could leave local and peer scans running until a full page finished. The list path now carries the caller context into storage and stops before returning a partial page, so canceled work can release its iterator and payload resources promptly.

Spilled segment and raw-file values use private interruptible descriptors for paginated reads. Closing a canceled read cannot close a descriptor still owned by the shared cache. Successful pages keep their sorted order, continuation tokens, value lengths, omission behavior, and ordinary read-error handling. The private descriptor open/close work on live spilled-value pages is covered by a continuation-page guard.

The change adds storage coverage for raw and segment values, pre-canceled scans, and a read that never returns. It also adds a cluster benchmark that cancels after local and peer payload reads begin and checks post-cancel rows and bytes along with handler, scan, and payload-reader lifetime. A live benchmark covers the same public operation across continuation pages.

Testing:

  • make test-server
  • make test-storage
  • make test-bench-cache-service-list-with-values-cancellation
  • make test-race-bench-cache-service-list-with-values-cancellation
  • make test-integration TESTRUN='^$'
  • make lint
  • make license-check

Workload: cluster public ListWithValues cancellation after local and peer raw-value reads begin

Metric Before After Change
ns/op 263763917 1082770 99.6% lower
B/op 604718516 16382 100% lower
allocs/op 88276 229 99.7% lower
cancel-to-exit-ns/op 261957510 116722 100% lower
rows-after-cancel/op 1991 0 100% lower
bytes-after-cancel/op 261126144 0 100% lower
active-list-scans-after-cancel/op 2 0 100% lower
active-list-handlers-after-cancel/op 2 0 100% lower
active-payload-readers-after-cancel/op 2 0 100% lower
handler-exit-before-gate/op 0 1 higher
Checks: 7 passed.

Generated by Perfloop. Measurements and checks.


Note

Medium Risk
Changes cancellation and I/O behavior on the hot ListWithValues path across storage, files, and segments; semantics are well tested but regressions could affect pagination or concurrent reads.

Overview
Canceled ListWithValues requests now stop promptly instead of finishing local/peer scans and disk reads. The caller context is threaded from RPC/operations into ListKeyValuesWithPagination, with checks throughout the scan and proto conversion so no partial page or continuation token is returned on cancel.

Spilled raw-file and segment values for list pages use private, interruptible file descriptors (ReadForList / ReadEntryForList, InterruptibleReadCloser) rather than the shared FD cache, so cancellation can Interrupt() blocked reads without affecting other readers. A listPayloadReader ties context cancellation to descriptor interrupt (sync ReadAll fast path plus worker fallback).

Benchmark-only work under ocache_benchmark: payload read blocking/stats in benchio, cluster cancellation test/benchmark with NewBenchmarkCoordinator, Makefile targets, and storage tests for cursors, segments, and stuck reads.

Reviewed by Cursor Bugbot for commit 4a9acf2. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

This PR should not merge until production raw-file and segment reads can return promptly when cancellation occurs during an already-blocked read.

Fix All in Claude CodeFindings

  1. P1 Cancellation Still Waits On Reads ▶

Summary

  • Carries caller context from the service operation into storage scans.
  • Discards canceled pages rather than returning partial pagination state.
  • Adds private raw-file and segment descriptors plus benchmark lifecycle instrumentation.
  • Adds raw/segment pagination, pre-cancellation, blocked-reader, cluster cancellation, and continuation coverage.

Reviews (1) · Last reviewed commit: "cleanup: remove unused list descriptor h..."

Comment thread storage/storage.go
Comment on lines +803 to +804
if readAller, ok := reader.(listPayloadReadAller); ok {
return payloadReader.readSynchronous(ctx, reader, readAller.ReadAll)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Cancellation Still Waits On Reads

The real raw-file and segment readers implement ReadAll, so they always take this synchronous branch. If a disk read stalls after entering the operating system, closing its regular-file descriptor does not reliably interrupt it on Linux or macOS. This branch cannot return until readAll finishes, so a canceled list request can still retain its descriptor and handler indefinitely. The asynchronous fallback test uses a reader without ReadAll, while the benchmark gate cancels before entering the underlying file read, so neither covers this production path.

Knowledge Base Used:

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid and should be fixed. The synchronous ReadAll path is the production path for the raw-file and segment readers, and canceling before ReadAll does not prove cancellation once that read has entered the OS. I will change the production read boundary so cancellation is observed while the payload read is in progress, with cleanup that releases the private descriptor and cached read state, and add a test that blocks the real ReadAll path after entry and verifies the canceled request exits without publishing a partial page. I will keep the live-page ordering, continuation, omission, and error behavior unchanged.

Source feedback

@perfloop

perfloop Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Closing this pull request because the remaining cancellation path depends on synchronous regular-file reads that cannot be safely interrupted by closing the descriptor. The review correctly identified that raw-file and segment readers can retain their read capacity until the syscall returns; the attempted containment does not meet the requested guarantee. I’m not shipping a partial fix that only returns the handler while the underlying read remains active.

@perfloop perfloop Bot closed this Sep 12, 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.

1 participant