feat(io): S3 LIST + footer probe + kvikIO backend, and io-layer fixes - #172
Merged
Merged
Conversation
| [[nodiscard]] std::unique_ptr<datasource> open_datasource(std::shared_ptr<ioctx> io_ctx, | ||
| std::string path); | ||
|
|
||
| /// As above, forwarding @p hint to the backend's io_object resolution so it can, |
Contributor
There was a problem hiding this comment.
we can probably clean up the AI comments after reviews finish. I don't mind the explanations while reviewing.
| /// default covers files up to ~1.4 GB in one GET (the common range); raise it | ||
| /// for multi-GB single files, lower it for many-tiny-file / low-bandwidth | ||
| /// workloads. | ||
| std::size_t footer_probe_bytes{512UL << 10}; // 512 KiB |
Contributor
There was a problem hiding this comment.
If retrieving a larger payload doesn't increase the latency significantly it woyuld be fine to increase this a bit.
| @@ -0,0 +1,139 @@ | |||
| /* | |||
Contributor
There was a problem hiding this comment.
I think this file should be moved into the testing directory
Contributor
There was a problem hiding this comment.
Or is the idea that other projects are going to want to use this authorizer themselves for testing? If so maybe we can make a new target for things that aren't for production but are meant to be included by users of the library for testing?
| // Single-pass unescape of the five predefined XML entities. Unknown sequences | ||
| // (e.g. numeric character references, which S3 does not emit for keys) pass | ||
| // through verbatim. | ||
| std::string xml_unescape(std::string_view s) |
Contributor
There was a problem hiding this comment.
It seems risky to handle xml parsing ourselves. Is there a reason we can't just use a library that handles that for us?
Contributor
Author
There was a problem hiding this comment.
I agree, the alternative is boost
felipeblazing
approved these changes
Jul 29, 2026
Adds three capabilities to the io datasource layer, fixes several latent defects found while auditing it, and gives the layer its first test coverage. Bug fixes - io_request: validate copy_async's host source before forming the pointer. A null host_buffer or an out-of-range [src_off, src_off+size) previously produced UB (nullptr + offset) or a wild in-range pointer that the near-null check could not catch. Now returns cudaErrorInvalidValue. - prefetching_cache: mark the request state load-failed on the prefetch_loop cancel/stop path. Only the chunk states were being marked, leaving the request-level state inconsistent on cancellation. - Four empty catch blocks were silently swallowing exceptions: the rest_reactor and uring_reactor worker loops, and two in templated_ioctx::shutdown. Restoring the log call sites fills them. CUCASCADE_LOG_* are no-ops today, so this costs nothing at runtime. Behavioral fixes - rest_reactor::preferred_prefetching_stage: opportunistic -> just_in_time. prefetching_cache branches on this value, so S3 reads were being eagerly prefetched into cache buffers instead of read straight into the caller's buffer. Network round-trips are high-latency; read ahead on demand rather than prefilling the working set. - uri_parser: S3 object keys are literal bytes. '%', '?' and '#' are ordinary key bytes, matching AWS CLI semantics, so s3://b/a%20b now opens the key a%20b. Previously the key was percent-decoded and split on '?', opening the wrong object. New capability - S3 ListObjectsV2: a hand-rolled fail-closed list parser, presign_url(extra_canonical_query) so caller query params participate in the signature, authorize_list on the authorizer interface and both SigV4 impls, an X-Amz-* injection guard, rest_reactor::list_page, and rest_ioctx's list_objects_paged / list_objects / list_max_matches with guards against non-conforming backends (truncated-without-token, truncated-empty, non-advancing token). - Parquet footer probe: open_hint plus create_io_object(path, hint), a suffix-range GET that resolves object size and stashes the trailing bytes in one round-trip, and a stash-hit fast path in rest_reactor::host_read so the footer reads that follow are served locally. - Known-size open: create_io_object(path, known_size), so a size already learned from a LIST response skips the HEAD entirely. - kvikio_context: a local-file fallback backend built directly on kvikio::FileHandle rather than cudf::io::datasource, keeping the io library cudf-free. Registered as a catch-all that lookup_path defers behind the explicit uring/rest backends. - kvikio_config: optional-per-field tunables (nthreads, task_size, gds_threshold, bounce_buffer_size, O_DIRECT reads and overread, per-block-device pools, compat mode). Unset means "leave kvikIO's env-var default alone", so KVIKIO_* still works as the outer default. Every field except compat_mode is process-global once applied and is documented as such; compat_mode rides the FileHandle constructor so it stays scoped to this ioctx. Cleanup - metadata_store: add the get_metadata(cache_key) overload for callers that know the path but have not built an io_object yet. - Drop object_store_config::s3_use_async_backend: no consumers, and it named types that do not exist. - Rename io_context_registry::lookup to lookup_path. It takes a full path, not a scheme, and the docs claimed otherwise. Tests - New cucascade_io_tests target: 732 assertions across 71 cases covering uri_parser, sigv4, the list parser, both authorizers, static credentials and kvikio_config. The io layer had no tests before this. Builds clean with CUCASCADE_WARNINGS_AS_ERRORS=ON; pre-commit passes. Not verified: the two S3 benchmarks are gated behind CUCASCADE_BUILD_S3_BENCHMARK, which requires aws-sdk-cpp (present only in the s3-bench pixi environment), so they were not compiled. They use only the single-argument open_datasource / open_io_object overloads, which are unchanged since the new behavior was added as overloads rather than signature changes, and nothing switches over io_context_type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vector The stash getter handed out shared_ptr<const std::vector<uint8_t>>, which leaks the container type into the interface: consumers only ever read through it (data / size / subspan), but the signature also advertises the allocator, the growth policy and the whole mutation API. Replace it with shared_byte_span, an alias for shared_ptr<const std::span<const uint8_t>> built with shared_ptr's aliasing constructor. A single make_shared allocates a byte_storage holding both the buffer and a span over it; the returned pointer refers to the span while the control block keeps the buffer alive. One allocation, no copy, and ownership still rides along. byte_storage is non-copyable and non-movable on purpose: `view` points into `bytes`, so a copy would deep-copy the buffer and leave the copy's span aimed at the original's allocation. It is only ever constructed in place. rest_reactor::host_read needs no change — span and vector both answer data() and size() through operator->. Also switch two string-keyed lookups to string_view: - metadata_store::get_metadata now takes string_view. This required giving the underlying map a transparent hash plus std::equal_to<> so C++20 heterogeneous lookup applies; without both, a string_view parameter would just construct a temporary key per call and be strictly worse than string const&. - content_range_total / content_range_start take string_view. Both already converted their argument to a string_view on the first line. Left alone deliberately: make_signer's region and evict_page_cache's path both need an owning string (the latter for c_str()), so a view would only add a conversion. Tests: 12 new cases / 296 assertions covering aliasing lifetime (source vector and first owner both dropped before reading), buffer stability across owners, the host_read access pattern, empty and null stashes, and heterogeneous lookup via string_view, string literal, owning string, and a non-NUL-terminated slice of a longer buffer. Verified under ASan + UBSan + leak detection: the full io suite (1029 assertions, 83 cases) runs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aminaramoon
force-pushed
the
sirius_io_match_up
branch
from
July 29, 2026 17:47
24791a1 to
83f4386
Compare
Contributor
Author
|
/ok to test 83f4386 |
1 similar comment
Contributor
Author
|
/ok to test 83f4386 |
Contributor
Author
|
/merge |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Aug 3, 2026
## What Implements the REST perf-instrumentation follow-up left out of #172. Sirius consumes these counters in its S3 tests and benchmark JSON. Add loopback HTTP range server for REST conformance tests ## Counter behavior - Retry, terminal-failure, device-stream-sync, and payload-byte counters are always enabled. Payload bytes count HTTP response-body bytes across attempts, including retries. - `chunk_get`, `queue_wait`, `h2d_observed`, `blocking_host_get`, and `ttfb_ns` are recorded only when `perf_instrumentation` is enabled. The disabled instrumentation path adds no clock reads. - Despite its retained name, `ttfb_ns` measures GET submission to completion of the first completed GET, not time to the first byte on the wire. - Counters use relaxed atomics. Snapshots can be read while reactors are running without taking reactor locks, but exact comparisons should be made after outstanding requests finish. - `rest_perf_snapshot` is source-pin API rather than a stable binary layout. New fields are appended without reordering existing fields. - cuCascade exposes only the C++ config field. YAML wiring remains the responsibility of the embedding application. ## Validation - Standalone: `pixi run -e cuda-12-stable build` / `test` — 4/4 ctest targets, `cucascade_io_tests` 103 cases / 1169 assertions green, `[rest][perf]` 20 cases / 149 assertions. - Consumer-side: the Sirius integration branch builds against this branch, and its full S3 integration suite and S3 benchmark pass with the perf coverage restored (the counters are consumed by its tests and its benchmark JSON). Authors: - Yu (https://github.com/ran-yuan-rui) Approvers: - Amin Aramoon (https://github.com/aminaramoon) URL: #178
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
2 participants
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.
Adds three capabilities to the
iodatasource layer, fixes several latent defects found while auditing it, and gives the layer its first test coverage.Bug fixes
io_request:copy_asyncvalidates its host source before forming the pointer. A nullhost_buffer, or an out-of-range[src_off, src_off + size), previously produced UB (nullptr + offset) or a wild in-range pointer that the near-null check could not catch. Now returnscudaErrorInvalidValue. The existing asserts compile out in release, so the check has to happen before the pointer is formed.prefetching_cache: mark the request state load-failed on theprefetch_loopcancel/stop path. Only the chunk states were being marked, leaving the request-level state inconsistent on cancellation.catchblocks were silently swallowing exceptions — therest_reactoranduring_reactorworker loops, and two intemplated_ioctx::shutdown. Restoring the log call sites fills them.CUCASCADE_LOG_*are no-ops today, so this costs nothing at runtime but keeps the diagnostics in place for when logging is wired up.Behavioral fixes
rest_reactor::preferred_prefetching_stage:opportunistic→just_in_time.prefetching_cachebranches on exactly this value, so S3 reads were being eagerly prefetched into cache buffers rather than read straight into the caller's buffer. Network round-trips are high-latency; read ahead on demand rather than prefilling the working set.uri_parser: S3 object keys are literal bytes.%,?and#are ordinary key bytes, matching AWS CLI semantics, sos3://b/a%20bnow opens the keya%20b. Previously the key was percent-decoded and split on?, which opened the wrong object.New capability
presign_url(extra_canonical_query)so caller query params participate in the signature (required for S3 to accept a presigned LIST);authorize_liston the authorizer interface and both SigV4 implementations; anX-Amz-*injection guard so callers cannot smuggle or override signing params;rest_reactor::list_page; andrest_ioctx::list_objects_paged/list_objects/list_max_matches. Pagination is guarded against non-conforming backends: truncated-without-token, truncated-but-empty, and non-advancing continuation token all throw rather than loop or silently truncate.open_hintpluscreate_io_object(path, hint); a suffix-range GET that resolves the object size and stashes its trailing bytes in one round-trip; and a stash-hit fast path inrest_reactor::host_readso the footer reads that follow are served from memory instead of costing extra round-trips. Falls back to a plain HEAD on any unusable response (200 full body, 416, missing/unsatisfiedContent-Range).create_io_object(path, known_size), so a size already learned from a LIST response builds the io_object with zero network: no HEAD, no probe.kvikio_context— a local-file fallback backend built directly onkvikio::FileHandlerather thancudf::io::datasource, which keeps the io library cudf-free. Registered as a catch-all thatlookup_pathdefers behind the explicit uring/rest backends, sos3://never resolves to it and a local file still routes to uring first.kvikio_config— optional-per-field tunables:nthreads,task_size,gds_threshold,bounce_buffer_size, O_DIRECT reads and overread, per-block-device pools, and compat mode. An unset field means "leave kvikIO's own default alone", so theKVIKIO_*environment variables keep working as the outer default and this is an explicit in-process override layered on top. Every field exceptcompat_modemaps to a setter on kvikIO's process-globaldefaultssingleton and is documented as such — last context constructed wins.compat_modeis the exception: it rides theFileHandleconstructor, so it scopes to files this ioctx opens and mutates nothing global.Cleanup
metadata_store::get_metadata(cache_key)overload, for callers that know the path but have not built an io_object yet.object_store_config::s3_use_async_backend— no consumers, and it referenced types that do not exist.io_context_registry::lookup→lookup_path. It is passed a full path, not a bare scheme (the checkers parse the URI / stat the filesystem themselves), and the docs claimed otherwise.Tests
New
cucascade_io_teststarget — 732 assertions across 71 cases, coveringuri_parser, sigv4, the list parser, both authorizers, static credentials andkvikio_config. The io layer had no tests at all before this.Full suite: 6,330 assertions / 343 cases, 0 failures.
cucascade_testscucascade_io_tests(new)cucascade_cudf_testscucascade_topology_discovery_testsBuilds clean with
CUCASCADE_WARNINGS_AS_ERRORS=ON;pre-commit run -apasses.Not verified
The two S3 benchmarks are gated behind
CUCASCADE_BUILD_S3_BENCHMARK, which requires aws-sdk-cpp (present only in thes3-benchpixi environment), so they were not compiled. By inspection they use only the single-argumentopen_datasource/open_io_objectoverloads — unchanged, since the new behavior was added as overloads rather than signature changes — and nothing switches overio_context_type, so the newkvikioenumerator cannot trip-Werror=switch.Follow-up
REST perf instrumentation (per-chunk timings, queue wait, TTFB, retry/terminal counters, pool aggregation) is deliberately not part of this PR. The control-plane paths added here leave room for it and it layers on cleanly; the existing S3 benchmarks are its natural consumer.
🤖 Generated with Claude Code