Comprehensive audit: 20 proven bugs fixed, HTTP/2 enabled, 3-10x performance gains - #88
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors HAMT persistence and mutation, updates sharded Zarr cache and legacy-array behavior, improves KuboCAS retries, ranges, redirects, and event-loop lifecycle handling, and adds broad regression coverage. ChangesHAMT core behavior
Array-aware sharded Zarr storage
HTTP content-addressed storage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 012fe0cca7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 8 8
Lines 2621 2983 +362
==========================================
+ Hits 2621 2983 +362 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
tests/test_z2_resize_remap.py (1)
13-50: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftProperty-test resize remapping across axes and geometries.
Generate dimensionality, resized axis, chunk shape, and grow/shrink sizes. The current two-dimensional examples can miss equivalent remapping defects in higher dimensions or other trailing axes.
As per coding guidelines, “Use Hypothesis property-based testing in the test suite.”
Also applies to: 108-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_z2_resize_remap.py` around lines 13 - 50, Expand the resize-remapping coverage around test_append_along_non_first_dimension_preserves_existing_chunks and the related tests into Hypothesis property-based tests. Generate array dimensionality, resized axis, chunk shapes, and valid grow/shrink sizes, then verify existing data remains correctly mapped after resizing across axes and geometries, including higher-dimensional and trailing-axis cases.Source: Coding guidelines
tests/test_z3_shard_cache_eviction.py (1)
76-109: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftProperty-test eviction access sequences.
Once these keys genuinely use shards, generate write/read order, cache capacity, and flush boundaries to cover repeated eviction and reload combinations.
As per coding guidelines, “Use Hypothesis property-based testing in the test suite.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_z3_shard_cache_eviction.py` around lines 76 - 109, Expand test_no_lost_acknowledged_writes_under_cache_pressure into a Hypothesis property-based test that generates key write/read orders, cache capacities, and flush boundaries. Exercise repeated shard eviction and reload sequences while preserving the assertions that acknowledged payloads remain readable before and after reopening the store.Source: Coding guidelines
tests/test_sharded_zarr_store.py (1)
788-796: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftProperty-test byte ranges and chunk-key classification.
Generate valid/invalid range boundaries and array paths instead of covering only
10:50and four hard-coded prefixes. This is especially valuable for zero-length ranges, optional ends, coordinate dimensionality, and primary/non-primary paths.As per coding guidelines, “Use Hypothesis property-based testing in the test suite.”
Also applies to: 989-999
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sharded_zarr_store.py` around lines 788 - 796, Expand the tests around the metadata range read and chunk-key classification to use Hypothesis-generated valid and invalid byte-range boundaries, including zero-length ranges and optional ends, rather than the fixed 10:50 case. Generate array paths covering varying coordinate dimensionality and primary/non-primary classifications, and assert the implementation’s expected behavior for each case while preserving the existing full-versus-ranged byte comparison where applicable.Source: Coding guidelines
tests/test_z4_coord_allowlist.py (1)
11-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGenerate coordinate and data-variable names with Hypothesis.
The prior defect was name-classification dependent, but coverage currently checks only
x,y,lat, andtime. Generate valid non-allowlisted names and collisions with historical allowlist names.As per coding guidelines, “Use Hypothesis property-based testing in the test suite.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_z4_coord_allowlist.py` around lines 11 - 57, Extend the tests around test_non_allowlisted_coordinate_names_round_trip and test_legacy_manifest_derives_primary_array_structurally to use Hypothesis strategies for valid coordinate and data-variable names. Generate both non-allowlisted names and names colliding with historical allowlist entries, then assert the existing round-trip and structural manifest behavior remains correct for each generated case.Source: Coding guidelines
tests/test_h1_delete_canonicalization.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix RUF005 lint warning.
Ruff flags the tuple concatenation; use unpacking instead to satisfy
ruff check.🔧 Proposed fix
-KEY_POOL = COLLIDING_KEYS + ("0", "1", "2", "alpha", "omega") +KEY_POOL = (*COLLIDING_KEYS, "0", "1", "2", "alpha", "omega")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_h1_delete_canonicalization.py` at line 14, Update the KEY_POOL tuple definition to use tuple unpacking for COLLIDING_KEYS and the additional string values instead of tuple concatenation, resolving the RUF005 warning while preserving the same element order and contents.Source: Linters/SAST tools
tests/test_h3_failed_set_atomicity.py (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant blind
except Exceptiontriggers BLE001.If
hamt.get("a")raises, pytest already fails the test with a full traceback; catchingExceptionjust to callpytest.failadds no value and trips the ruff BLE001 rule.🔧 Proposed simplification
- try: - committed_value = await hamt.get("a") - except Exception as error: - pytest.fail( - "previously committed key 'a' lost after failed set: " - f"{type(error).__name__}: {error}" - ) - - assert committed_value == b"value-a" + assert await hamt.get("a") == b"value-a", ( + "previously committed key 'a' lost after failed set" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_h3_failed_set_atomicity.py` around lines 24 - 32, Remove the broad try/except around hamt.get("a") in the failed-set atomicity test, allowing any exception to propagate naturally with its traceback. Keep the committed_value assignment and subsequent equality assertion unchanged.Source: Linters/SAST tools
py_hamt/store_httpx.py (1)
374-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid reading HTTPX internals here
client._transport,_pool, and the_max_*fields are private HTTPX details, so an upgrade can silently fall back to the hard-coded defaults and drop a user’s configured limits. If that best-effort behavior is intentional, document it; otherwise pass the limits through explicitly instead of introspecting the client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/store_httpx.py` around lines 374 - 387, The _copy_client_limits method relies on private HTTPX transport and pool fields that can silently lose configured limits after upgrades. Remove this introspection and pass the client’s configured httpx.Limits explicitly through the relevant callers and APIs, preserving user-provided connection and keepalive settings.py_hamt/sharded_zarr_store.py (2)
899-899: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve exception context.
Chain the re-raised
RuntimeErrorto satisfy Ruff B904.♻️ Proposed change
- raise RuntimeError(f"Timeout waiting for shard {shard_idx} to load.") + raise RuntimeError( + f"Timeout waiting for shard {shard_idx} to load." + ) from None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/sharded_zarr_store.py` at line 899, Update the RuntimeError raised in the shard-loading timeout path to explicitly chain it from the caught exception using the appropriate exception context syntax, satisfying Ruff B904 while preserving the existing timeout message and behavior.Source: Linters/SAST tools
1171-1178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid blind
except Exceptionand preserve chaining.Catching bare
Exceptionhere (Ruff BLE001) also swallows things likeasyncio.CancelledError/KeyboardInterrupton some paths and discards the original traceback (B904). Narrow the catch or at least chain the cause.♻️ Proposed change
- except Exception as e: - raise RuntimeError(f"Failed to save data for key {key}: {e}") + except Exception as e: + raise RuntimeError(f"Failed to save data for key {key}: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/sharded_zarr_store.py` around lines 1171 - 1178, Update the exception handling around CAS persistence in the method containing _set_pointer_cid: catch only the expected save/pointer-update exceptions, preserving cancellation and interruption behavior, and chain the original exception when raising RuntimeError. If a broader catch is required, explicitly re-raise excluded control-flow exceptions before wrapping the remaining error.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py_hamt/sharded_zarr_store.py`:
- Around line 906-911: Ensure the pending-load event created in the
shard-loading flow around _fetch_and_cache_full_shard is removed in a finally
block, including when fetch or decoding raises. Preserve the existing
successful-load behavior while allowing subsequent accesses to retry immediately
after failure.
- Around line 178-180: Change ShardedZarrStore.__contains__ from async def to a
synchronous def so Python’s in operator performs an actual membership check
rather than receiving a coroutine. Preserve the cache-locking and shard_idx
membership behavior, using a synchronization approach compatible with
synchronous execution or provide a separately named async helper if locking
cannot remain asynchronous.
In `@py_hamt/store_httpx.py`:
- Around line 28-49: Update the retry-delay calculation in the backoff helper so
a valid Retry-After value is treated as the minimum wait, not capped by
backoff_delay; replace the min-based return with logic that waits at least the
server-directed duration while preserving jitter for cases without a valid
header. If a maximum is required, apply a separate explicit sane limit rather
than using backoff_delay as the ceiling.
In `@tests/test_k3_per_loop_semaphore.py`:
- Around line 18-28: Rename the format parameter in GatewayHandler.log_message
to avoid ruff A002’s shadowed built-in violation, while preserving the method
signature’s positional behavior and the existing no-op implementation.
In `@tests/test_z1_multivar_index.py`:
- Around line 101-122: Update test_multivar_chunks_have_per_array_indexes and
the related regression path around lines 125-147 to exercise distinct per-array
chunk grids by passing different_chunk_grids=True. Convert the coverage to a
Hypothesis property-based test using compatible distinct grid configurations,
while preserving the existing round-trip array assertions and data-chunk index
validation.
In `@tests/test_z3_shard_cache_eviction.py`:
- Around line 34-41: Rename the first parameter of CIDInMemoryCAS.load from id
to identifier and update its use in the _normalize_cid call; leave the method’s
positional behavior and all other parameters unchanged.
In `@tests/test_z4_coord_allowlist.py`:
- Around line 79-82: Add an explicit isinstance check confirming
root_obj["chunks"] is a dict before calling pop in the legacy_root_cid setup,
while preserving the existing root_obj mapping assertion and mutation behavior.
In `@tests/test_z6_store_contract.py`:
- Around line 124-139: Update test_root_level_chunk_key_no_silent_clobber to
catch only ValueError from the store.set call, matching
_validate_chunk_write_key’s rejection contract. Preserve the existing assertions
for accepted root-level writes, and make the ValueError handling explicitly
document that rejection is the expected acceptable outcome rather than silently
masking unrelated exceptions.
---
Nitpick comments:
In `@py_hamt/sharded_zarr_store.py`:
- Line 899: Update the RuntimeError raised in the shard-loading timeout path to
explicitly chain it from the caught exception using the appropriate exception
context syntax, satisfying Ruff B904 while preserving the existing timeout
message and behavior.
- Around line 1171-1178: Update the exception handling around CAS persistence in
the method containing _set_pointer_cid: catch only the expected
save/pointer-update exceptions, preserving cancellation and interruption
behavior, and chain the original exception when raising RuntimeError. If a
broader catch is required, explicitly re-raise excluded control-flow exceptions
before wrapping the remaining error.
In `@py_hamt/store_httpx.py`:
- Around line 374-387: The _copy_client_limits method relies on private HTTPX
transport and pool fields that can silently lose configured limits after
upgrades. Remove this introspection and pass the client’s configured
httpx.Limits explicitly through the relevant callers and APIs, preserving
user-provided connection and keepalive settings.
In `@tests/test_h1_delete_canonicalization.py`:
- Line 14: Update the KEY_POOL tuple definition to use tuple unpacking for
COLLIDING_KEYS and the additional string values instead of tuple concatenation,
resolving the RUF005 warning while preserving the same element order and
contents.
In `@tests/test_h3_failed_set_atomicity.py`:
- Around line 24-32: Remove the broad try/except around hamt.get("a") in the
failed-set atomicity test, allowing any exception to propagate naturally with
its traceback. Keep the committed_value assignment and subsequent equality
assertion unchanged.
In `@tests/test_sharded_zarr_store.py`:
- Around line 788-796: Expand the tests around the metadata range read and
chunk-key classification to use Hypothesis-generated valid and invalid
byte-range boundaries, including zero-length ranges and optional ends, rather
than the fixed 10:50 case. Generate array paths covering varying coordinate
dimensionality and primary/non-primary classifications, and assert the
implementation’s expected behavior for each case while preserving the existing
full-versus-ranged byte comparison where applicable.
In `@tests/test_z2_resize_remap.py`:
- Around line 13-50: Expand the resize-remapping coverage around
test_append_along_non_first_dimension_preserves_existing_chunks and the related
tests into Hypothesis property-based tests. Generate array dimensionality,
resized axis, chunk shapes, and valid grow/shrink sizes, then verify existing
data remains correctly mapped after resizing across axes and geometries,
including higher-dimensional and trailing-axis cases.
In `@tests/test_z3_shard_cache_eviction.py`:
- Around line 76-109: Expand
test_no_lost_acknowledged_writes_under_cache_pressure into a Hypothesis
property-based test that generates key write/read orders, cache capacities, and
flush boundaries. Exercise repeated shard eviction and reload sequences while
preserving the assertions that acknowledged payloads remain readable before and
after reopening the store.
In `@tests/test_z4_coord_allowlist.py`:
- Around line 11-57: Extend the tests around
test_non_allowlisted_coordinate_names_round_trip and
test_legacy_manifest_derives_primary_array_structurally to use Hypothesis
strategies for valid coordinate and data-variable names. Generate both
non-allowlisted names and names colliding with historical allowlist entries,
then assert the existing round-trip and structural manifest behavior remains
correct for each generated case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4d08240-71a1-450c-a83b-11a377cc5bd7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
py_hamt/encryption_hamt_store.pypy_hamt/hamt.pypy_hamt/hamt_to_sharded_converter.pypy_hamt/sharded_zarr_store.pypy_hamt/store_httpx.pypy_hamt/zarr_hamt_store.pypyproject.tomltests/fixtures/z1_legacy_multivar_store.jsontests/fixtures/z1_legacy_single_var_store.jsontests/test_h1_delete_canonicalization.pytests/test_h2_enable_write_idempotent.pytests/test_h3_failed_set_atomicity.pytests/test_h4_keys_iteration_deadlock.pytests/test_h5_read_cache_growth.pytests/test_h6_write_perf_restore.pytests/test_hamt.pytests/test_k10_http2_enable.pytests/test_k1_lazy_client_binding.pytests/test_k2_user_client_multiloop.pytests/test_k3_per_loop_semaphore.pytests/test_k4_range_correctness.pytests/test_k5_follow_redirects.pytests/test_k6_retry_policy.pytests/test_k7_url_normalization.pytests/test_k8_respect_client_timeout.pytests/test_k9_semaphore_scope.pytests/test_kubo_cas.pytests/test_p1_direct_blake3.pytests/test_p2_relink_o1_hash_once.pytests/test_p4_cid_objects.pytests/test_p5_concurrent_vacate.pytests/test_sharded_zarr_store.pytests/test_z1_multivar_index.pytests/test_z2_resize_remap.pytests/test_z3_shard_cache_eviction.pytests/test_z4_coord_allowlist.pytests/test_z5_delete_stale_metadata_cache.pytests/test_z6_store_contract.pytests/testing_utils.py
… deleted metadata from its read cache Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d metadata from its read cache Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…allowlist crash or corrupt ShardedZarrStore Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…es all existing chunks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng its own load; lost-write window for clean shards Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…own load; lost-write window for clean shards Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (ShardedZarrStore) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…edZarrStore) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rough base32 strings (~75% of warm-get CPU) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ase32 strings (~75% of warm-get CPU) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rite each others chunks (per-array indexing) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…de an event loop Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vent loop Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… loop breaks documented multi-loop support Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…reaks documented multi-loop support Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d loop: auth silently dropped + internal client leaked Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… auth silently dropped + internal client leaked Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y overrides the user clients timeout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ides the user clients timeout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(dweb.link, 4everland, w3s.link unusable) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ink, 4everland, w3s.link unusable) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iling slash) builds /ipfs/ipfs/ Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in write mode destroys buffered data Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e mode destroys buffered data Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ly committed keys Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…itted keys Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a broad correctness + performance overhaul across the HAMT core, Zarr store layers, and the HTTP-backed CAS (KuboCAS), aiming to eliminate known data-corruption/atomicity issues, improve determinism, and enable HTTP/2 with robust retry semantics.
Changes:
- Refactors
HAMTinternals for faster hashing, bounded/concurrent vacate, failure-atomicset()/delete(), canonical post-delete structure, and deadlock-free key iteration. - Hardens sharded Zarr storage semantics (multi-variable indexing, resize remapping, cache correctness) and expands contract/compatibility test coverage (incl. fixtures).
- Improves KuboCAS behavior (range correctness, redirects, retry/backoff, per-event-loop clients/semaphores, HTTP/2 enablement) and adds
trustmefor TLS testing.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
py_hamt/hamt.py |
Core HAMT changes: hashing, caching, vacate concurrency, atomicity/determinism, and iteration/len behavior. |
py_hamt/zarr_hamt_store.py |
Metadata cache correctness on set/delete in the HAMT-backed Zarr store. |
py_hamt/encryption_hamt_store.py |
Align encrypted store metadata cache update timing with underlying write success. |
pyproject.toml |
Adds trustme to dev dependencies for TLS/HTTP2 regression testing. |
tests/testing_utils.py |
Adds a CID-returning in-memory CAS for offline tests and CID normalization helper. |
tests/test_sharded_zarr_store.py |
Updates shard-cache containment checks and adds metadata range expectations. |
tests/test_sharded_zarr_store_coverage.py |
Adds coverage for cache update/dirty-guard branches. |
tests/test_z1_multivar_index.py |
Tests multi-variable per-array indexing + legacy fixture readability. |
tests/test_z2_resize_remap.py |
Tests correct remapping on resize/append across non-leading dimensions. |
tests/test_z3_shard_cache_eviction.py |
Regression tests for shard eviction safety and acknowledged-write durability. |
tests/test_z4_coord_allowlist.py |
Ensures coordinate names are handled structurally (no hardcoded allowlist). |
tests/test_z5_delete_stale_metadata_cache.py |
Ensures delete purges metadata read cache (plain + encrypted stores). |
tests/test_z6_store_contract.py |
Contract tests for byte ranges + listing semantics. |
tests/test_z7_delete_legacy_metadata.py |
Tests legacy metadata cleanup behavior and mutation sequences. |
tests/test_z8_cas_save_cid_validation.py |
Validates CAS save() must return a CID (rejects raw multihash bytes). |
tests/test_z9_cache_contains.py |
Ensures __contains__ works via the in operator for the LRU cache. |
tests/test_z10_concurrent_flush.py |
Tests concurrent shard flush behavior + failure cleanup/no orphan tasks. |
tests/test_z11_v1_unrecorded_coord_chunk.py |
Tests v1 coordinate-chunk routing across write-order permutations. |
tests/test_z12_v1_legacy_primary_inference.py |
Property tests for conservative v1 primary-array inference behavior. |
tests/test_z13_inferred_primary_write_stability.py |
Tests inferred-primary write semantics match recorded-primary behavior. |
tests/test_p1_direct_blake3.py |
Golden digest + “no multiformats wrapper in hot path” performance guard. |
tests/test_p2_relink_o1_hash_once.py |
Ensures relinking/hashing stays bounded and root IDs remain stable. |
tests/test_p4_cid_objects.py |
Ensures warm get/set paths preserve CID objects (avoid encode/decode churn). |
tests/test_p5_concurrent_vacate.py |
Verifies sibling subtree vacate concurrency + golden root stability. |
tests/test_p6_bounded_vacate.py |
Ensures vacate bounded concurrency + no orphan save tasks on failure. |
tests/test_p7_len_without_snapshot.py |
Ensures len() doesn’t materialize a key snapshot via keys(). |
tests/test_p8_bounded_task_creation.py |
Ensures bounded task creation for both HAMT vacate and shard flush. |
tests/test_h1_delete_canonicalization.py |
Property test: operation history shouldn’t affect root CID determinism. |
tests/test_h2_enable_write_idempotent.py |
Ensures redundant enable_write() calls are safe/idempotent. |
tests/test_h3_failed_set_atomicity.py |
Ensures failed bucket split doesn’t corrupt previously committed keys. |
tests/test_h4_keys_iteration_deadlock.py |
Ensures keys iteration no longer deadlocks under nested get/set. |
tests/test_h5_read_cache_growth.py |
Ensures read caching is effective and doesn’t grow unbounded on reads. |
tests/test_h6_write_perf_restore.py |
Guards against deepcopy regressions and validates atomic failure behavior. |
tests/test_h7_delete_failure_atomicity.py |
Ensures delete remains atomic across CAS load failures during collapse. |
tests/test_hamt.py |
Adjusts cache vacate assertions to account for concurrent repopulation. |
tests/test_kubo_cas.py |
Updates retry behavior expectations (transient status retries). |
tests/test_k1_lazy_client_binding.py |
Ensures KuboCAS(client=...) doesn’t require a running loop at init. |
tests/test_k2_user_client_multiloop.py |
Ensures headers/auth propagate across loops; internal clients get closed. |
tests/test_k3_per_loop_semaphore.py |
Ensures contended loads work across sequential event loops. |
tests/test_k4_range_correctness.py |
Ensures safe handling when gateways ignore Range + zero-length/suffix cases. |
tests/test_k5_follow_redirects.py |
Ensures internal clients follow redirects. |
tests/test_k6_retry_policy.py |
Tests retry policy including Retry-After parsing and 404 fail-fast. |
tests/test_k7_url_normalization.py |
Tests gateway URL normalization of /ipfs path variants. |
tests/test_k8_respect_client_timeout.py |
Ensures user-provided httpx timeouts aren’t overridden by defaults. |
tests/test_k9_semaphore_scope.py |
Ensures semaphore is released during retry backoff (prevents head-of-line blocking). |
tests/test_k10_http2_enable.py |
TLS raw-h2 GOAWAY regression + verify HTTP/2 negotiation on HTTPS. |
tests/test_k12_cross_loop_client_policy.py |
Documents/reinforces cross-loop client policy + factory constraints. |
tests/test_k13_per_loop_aclose.py |
Ensures per-loop client cleanup works even when loops are already closed. |
tests/fixtures/z1_legacy_single_var_store.json |
Adds a pinned legacy fixture for sharded-store backward compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| if isinstance(recorded_path, str): | ||
| primary_path_is_exclusive = True | ||
| effective_primary_path = recorded_path | ||
| elif self._primary_array_path: |
There was a problem hiding this comment.
HIGH
Persist routing based on an inferred V1 primary. If a writable legacy root infers temp, then stores a same-geometry precip chunk as metadata, the inferred path is not persisted. On reopen both arrays are inference candidates, so inference becomes ambiguous and precip/c/... is parsed against the shard index, silently returning temp data instead of its metadata-backed value.
| if self._manifest_version == SHARDED_ZARR_V1: | ||
| chunk_info = self._root_obj["chunks"] | ||
| if "primary_array_path" not in chunk_info: | ||
| self._primary_array_path = parsed_chunk.array_path |
There was a problem hiding this comment.
HIGH
Do not let a named write rebind a legacy root-level primary. An inferred root path is "", which is indistinguishable from “not inferred” in the truthiness check. A subsequent named chunk is therefore treated as the primary, overwrites the root shard slot, and records the named path here, making existing root-array data inaccessible or incorrect. Track inference separately so an empty primary path remains exclusive.
| ) | ||
| evicted = True | ||
| break | ||
| self._evict_if_needed_locked() |
There was a problem hiding this comment.
MEDIUM
Pin shards populated during resize before running eviction. _snapshot_shards_for_resize() calls _fetch_and_cache_full_shard() directly without a pin. With an over-budget cache whose older entries are dirty or pinned, this eviction pass can immediately remove the newly inserted clean shard; the following snapshot lookup then raises RuntimeError. Route resize loads through the pinned loader or pin cache_key around population.
| chunk_prefix = ( | ||
| "c" if not self._primary_array_path else f"{self._primary_array_path}/c" | ||
| ) | ||
| keys.add(chunk_prefix) |
There was a problem hiding this comment.
MEDIUM
Make V1 list_dir() descend into chunk directories. This synthetic candidate lets list_dir("temp") report c, but list_dir("temp/c") remains empty because _is_v2_chunk_listing_prefix() rejects V1 and the candidate set contains no coordinate keys. Enumerate _iter_chunk_keys() for V1 chunk prefixes as well.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
py_hamt/store_httpx.py (2)
724-739: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn an empty result for valid out-of-bounds ranges.
A gateway returns HTTP 416 when the requested offset is at or beyond EOF, but
raise_for_status()currently raises. This diverges fromInMemoryCASand the HTTP-200 fallback’s slice semantics. Validate the 416Content-Rangetotal and returnb""when the start is beyond it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/store_httpx.py` around lines 724 - 739, The HTTP gateway fetch flow around the retry loop and response.raise_for_status() must handle valid out-of-bounds range responses. Before raising for status, detect HTTP 416, parse and validate its Content-Range total, and return b"" when the requested offset is at or beyond EOF; preserve existing behavior for valid ranges and malformed or unrelated 416 responses.
477-504: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize first-use client binding across loop threads.
Two loops can simultaneously observe
_supplied_clientbefore either clears it, binding the sameAsyncClientto both loops. Protect client-map creation and supplied-client consumption with a thread-safe state lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/store_httpx.py` around lines 477 - 504, The client selection and per-loop binding logic must be serialized to prevent concurrent loops from consuming the same supplied client. Add or reuse a thread-safe state lock around the critical section in the client-acquisition method containing `_supplied_client`, `_client_factory`, and `_client_per_loop`, including supplied-client consumption, client creation, and map assignment; keep client use outside the lock where possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py_hamt/store_httpx.py`:
- Around line 536-545: Update the client cleanup logic around
_close_client_on_stopped_loop so the run_coroutine_threadsafe result cannot wait
indefinitely if owner_loop stops after the running check. Bound the await of
close_future, cancel the pending future on timeout, and invoke the
stopped/dead-loop fallback to ensure the client is closed.
In `@tests/test_k13_per_loop_aclose.py`:
- Line 2: Update test_k13_per_loop_aclose to assert the promised RuntimeWarning
directly during transport cleanup, rather than accepting any warning or
warning-level log; preserve the existing cleanup scenario and verify the warning
type and relevant message.
In `@tests/test_p8_bounded_task_creation.py`:
- Around line 178-257: Parameterize the four concurrency-regression
tests—test_vacate_bounds_simultaneously_live_save_tasks,
test_shard_flush_bounds_simultaneously_live_save_tasks,
test_failed_vacate_cancels_bounded_work_without_orphans, and
test_failed_shard_flush_cancels_bounded_work_without_orphans—with Hypothesis
bounded strategies for HAMT key count, shard count, and failing save ordinal.
Thread the generated values through the existing build helpers and Delayed*CAS
fail_on_save setup, while preserving the concurrency-limit and
orphan-cancellation assertions for every generated case.
In `@tests/test_z13_inferred_primary_write_stability.py`:
- Around line 214-220: Update the assertions in
tests/test_z13_inferred_primary_write_stability.py at lines 214-220 and 410-417:
require inferred.persisted_primary is None at the first site, and require
"primary_array_path" not in chunk_info at the second site, ensuring foreign
writes never persist the inferred "temp" primary.
---
Outside diff comments:
In `@py_hamt/store_httpx.py`:
- Around line 724-739: The HTTP gateway fetch flow around the retry loop and
response.raise_for_status() must handle valid out-of-bounds range responses.
Before raising for status, detect HTTP 416, parse and validate its Content-Range
total, and return b"" when the requested offset is at or beyond EOF; preserve
existing behavior for valid ranges and malformed or unrelated 416 responses.
- Around line 477-504: The client selection and per-loop binding logic must be
serialized to prevent concurrent loops from consuming the same supplied client.
Add or reuse a thread-safe state lock around the critical section in the
client-acquisition method containing `_supplied_client`, `_client_factory`, and
`_client_per_loop`, including supplied-client consumption, client creation, and
map assignment; keep client use outside the lock where possible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb351348-1be7-4b94-b718-2ba477c370c0
📒 Files selected for processing (14)
py_hamt/hamt.pypy_hamt/sharded_zarr_store.pypy_hamt/store_httpx.pytests/test_h7_delete_failure_atomicity.pytests/test_k12_cross_loop_client_policy.pytests/test_k13_per_loop_aclose.pytests/test_p6_bounded_vacate.pytests/test_p7_len_without_snapshot.pytests/test_p8_bounded_task_creation.pytests/test_sharded_zarr_store.pytests/test_z11_v1_unrecorded_coord_chunk.pytests/test_z12_v1_legacy_primary_inference.pytests/test_z13_inferred_primary_write_stability.pytests/test_z7_delete_legacy_metadata.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_p7_len_without_snapshot.py
- tests/test_sharded_zarr_store.py
- tests/test_k12_cross_loop_client_policy.py
- tests/test_p6_bounded_vacate.py
- tests/test_h7_delete_failure_atomicity.py
| @pytest.mark.asyncio | ||
| async def test_vacate_bounds_simultaneously_live_save_tasks( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| cas = DelayedBytesCAS() | ||
| hamt = await build_wide_hamt(cas) | ||
| node_store = cast(InMemoryTreeStore, hamt.node_store) | ||
| buffered_node_count = len(node_store.buffer) | ||
| assert buffered_node_count > _VACATE_CONCURRENCY | ||
| cas.arm() | ||
| tracker = TaskCreationTracker() | ||
|
|
||
| with monkeypatch.context() as scoped_monkeypatch: | ||
| tracker.install(scoped_monkeypatch) | ||
| await hamt.cache_vacate() | ||
|
|
||
| assert tracker.peak_live_tasks <= _VACATE_CONCURRENCY, ( | ||
| "vacate must not create the entire save wave as pending tasks: " | ||
| f"peak={tracker.peak_live_tasks}, limit={_VACATE_CONCURRENCY}, " | ||
| f"buffered_nodes={buffered_node_count}" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_shard_flush_bounds_simultaneously_live_save_tasks( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| cas = DelayedCIDCAS() | ||
| store = await build_dirty_sharded_store(cas) | ||
| cas.arm() | ||
| tracker = TaskCreationTracker() | ||
|
|
||
| with monkeypatch.context() as scoped_monkeypatch: | ||
| tracker.install(scoped_monkeypatch) | ||
| await store.flush() | ||
|
|
||
| assert tracker.peak_live_tasks <= _FLUSH_CONCURRENCY, ( | ||
| "shard flush must not create every dirty shard as a pending task: " | ||
| f"peak={tracker.peak_live_tasks}, limit={_FLUSH_CONCURRENCY}, " | ||
| f"dirty_shards={_SHARD_COUNT}" | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_failed_vacate_cancels_bounded_work_without_orphans( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| cas = DelayedBytesCAS(fail_on_save=3) | ||
| hamt = await build_wide_hamt(cas) | ||
| total_nodes = len(cast(InMemoryTreeStore, hamt.node_store).buffer) | ||
| cas.arm() | ||
| tracker = TaskCreationTracker() | ||
|
|
||
| with monkeypatch.context() as scoped_monkeypatch: | ||
| tracker.install(scoped_monkeypatch) | ||
| with pytest.raises(ConnectionError, match="simulated HAMT save failure"): | ||
| await hamt.cache_vacate() | ||
|
|
||
| assert cas.started_saves < total_nodes, "failure must stop unscheduled node saves" | ||
| assert cas.in_flight_saves == 0 | ||
| assert tracker.pending_tasks() == [], "vacate save tasks survived the failure" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_failed_shard_flush_cancels_bounded_work_without_orphans( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| cas = DelayedCIDCAS(fail_on_save=3) | ||
| store = await build_dirty_sharded_store(cas) | ||
| cas.arm() | ||
| tracker = TaskCreationTracker() | ||
|
|
||
| with monkeypatch.context() as scoped_monkeypatch: | ||
| tracker.install(scoped_monkeypatch) | ||
| with pytest.raises(ConnectionError, match="simulated shard save failure"): | ||
| await store.flush() | ||
|
|
||
| assert cas.started_saves < _SHARD_COUNT, "failure must stop unscheduled shard saves" | ||
| assert cas.in_flight_saves == 0 | ||
| assert tracker.pending_tasks() == [], "shard save tasks survived the failure" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Locate the target test file and nearby test patterns\n'
git ls-files 'tests/**/*.py' | sed -n '1,120p'
printf '\n---\n'
ast-grep outline tests/test_p8_bounded_task_creation.py --view expanded || true
printf '\n## Inspect the relevant file sections\n'
wc -l tests/test_p8_bounded_task_creation.py
sed -n '1,320p' tests/test_p8_bounded_task_creation.py
printf '\n## Search for Hypothesis usage and async property-based test patterns\n'
rg -n "hypothesis|`@given`|strategies as st|settings\\(|pytest\\.mark\\.asyncio|pytest\\.mark\\.hypothesis" tests . -g '*.py' -g '!**/.venv/**'
printf '\n## Search for repository-wide Hypothesis configuration or async handling hooks\n'
rg -n "hypothesis|register_profile|settings\\.load_profile|deadline=None|event_loop|pytest_plugins|asyncio" . -g '*.py' -g 'pytest.ini' -g 'pyproject.toml' -g 'conftest.py' -g '!**/.venv/**'Repository: dClimate/py-hamt
Length of output: 50372
Parameterize these concurrency-regression tests with Hypothesis.
Use bounded strategies for the HAMT key count, shard count, and failing save ordinal so these cases cover more wave sizes and cancellation points instead of a single fixed layout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_p8_bounded_task_creation.py` around lines 178 - 257, Parameterize
the four concurrency-regression
tests—test_vacate_bounds_simultaneously_live_save_tasks,
test_shard_flush_bounds_simultaneously_live_save_tasks,
test_failed_vacate_cancels_bounded_work_without_orphans, and
test_failed_shard_flush_cancels_bounded_work_without_orphans—with Hypothesis
bounded strategies for HAMT key count, shard count, and failing save ordinal.
Thread the generated values through the existing build helpers and Delayed*CAS
fail_on_save setup, while preserving the concurrency-limit and
orphan-cancellation assertions for every generated case.
Source: Coding guidelines
| # later flush of a writable open cannot seal a wrong inference. | ||
| # The flag keeps the (possibly empty-string) inferred primary | ||
| # exclusive so foreign chunk writes cannot rebind over it. | ||
| self._primary_array_path = next(iter(matching_paths)) |
There was a problem hiding this comment.
HIGH
_primary_array_path is inferred only in memory. If this writable V1 store then adds a second array with identical shape/chunk geometry, its chunk is metadata-routed; after flush/reopen both arrays match inference, routing becomes ambiguous, and the second chunk is read from the primary array's global shard slot instead of its metadata CID. Persist an explicit routing decision or reject/migrate such writes before flushing.
| else: | ||
| # V1 emits a single primary chunk tree, rooted at "c" or | ||
| # "<primary>/c" to match the recorded/inferred primary path. | ||
| yield ( |
There was a problem hiding this comment.
MEDIUM
For an unrecorded V1 root whose inferred primary path is "", this yields c, while _iter_chunk_keys() emits /c/.... Because list_dir() strips the input slash and compares against c/, list_dir("c") returns empty even though list_dir("") advertises c and chunks exist. Normalize the legacy root key and prefix consistently.
| content = response.content | ||
| response_bytes = len(content) | ||
| final_retry_count = retry_count | ||
| if headers and response.status_code == httpx.codes.OK: |
There was a problem hiding this comment.
MEDIUM
Handle valid out-of-bounds ranges. A compliant gateway returns HTTP 416 when offset is at or beyond EOF, so raise_for_status() runs before this HTTP-200 fallback and raises even though InMemoryCAS and the documented Python-slice semantics return b"". Recognize a valid Content-Range: bytes */N response and return empty when offset >= N.
|
|
||
| clone.array_indices = self.array_indices | ||
| clone._primary_array_path = self._primary_array_path | ||
| clone._primary_inferred = self._primary_inferred |
There was a problem hiding this comment.
HIGH
Copying _primary_inferred into a writable clone leaves the inferred V1 path only in memory. If with_read_only(False) then writes a same-geometry secondary array, flush/reopen sees two inference candidates and can return the primary shard's bytes for the secondary key instead of its metadata CID. Persist chunks.primary_array_path and mark the root dirty when creating the writable clone, or reject this transition.
| # compliant gateway; return b"" to match Python-slice | ||
| # semantics (and InMemoryCAS) instead of raising. | ||
| if ( | ||
| offset is not None |
There was a problem hiding this comment.
MEDIUM
Handle suffix ranges on empty objects. A compliant gateway may answer Range: bytes=-N for a zero-byte object with 416 Content-Range: bytes */0, but this branch only recognizes 416 responses when offset is set. Nonzero suffix reads therefore raise while InMemoryCAS returns b""; handle the suffix/zero-size case consistently.
| content = response.content | ||
| response_bytes = len(content) | ||
| final_retry_count = retry_count | ||
| if headers and response.status_code == httpx.codes.OK: |
There was a problem hiding this comment.
MEDIUM
Validate partial responses before trusting them. raise_for_status() accepts any 2xx response, while this branch only corrects status 200. A gateway returning a malformed 206 with a missing or mismatched Content-Range can therefore supply the full or wrong byte window, silently corrupting partial reads. Verify Content-Range and body length for 206 responses and reject unexpected successful statuses.
| # only true-primary chunks reach this point anyway. | ||
| chunk_info = self._root_obj["chunks"] | ||
| if "primary_array_path" not in chunk_info: | ||
| self._primary_array_path = parsed_chunk.array_path |
There was a problem hiding this comment.
HIGH
Recording the V1 primary here is not used when processing later array metadata. _register_array_metadata_from_bytes() still resizes the global V1 index for any same-rank array whose shape differs, so secondary metadata can remap—or when shrinking, discard—the primary array's chunks. Gate V1 resizing on the metadata path matching _primary_array_path; the added test_secondary_array_metadata_does_not_resize_primary_geometry currently exercises this failure.
| try: | ||
| data_cid_obj = await self.cas.save(raw_data_bytes, codec="raw") | ||
| await self._set_pointer(key, str(data_cid_obj), register_metadata=False) | ||
| if not isinstance(data_cid_obj, CID): |
There was a problem hiding this comment.
MEDIUM
This narrows the documented ContentAddressedStore.save() contract from any immutable IPLDKind to multiformats.CID. Backends returning a valid CID string previously worked because _set_pointer() decoded it, but now every set() fails with RuntimeError. Normalize CID strings with CID.decode() instead of rejecting them.
| chunk_info = self._root_obj["chunks"] | ||
| if "primary_array_path" not in chunk_info: | ||
| self._primary_array_path = parsed_chunk.array_path | ||
| chunk_info["primary_array_path"] = parsed_chunk.array_path |
There was a problem hiding this comment.
HIGH
Recording the V1 primary here does not protect it from later metadata writes. _register_array_metadata_from_bytes() still calls _resize_store_unlocked() for any same-rank array with a different shape, so writing secondary-array metadata can remap or discard the primary array's chunks. Gate V1 resizing on array_path == _primary_array_path.
| try: | ||
| data_cid_obj = await self.cas.save(raw_data_bytes, codec="raw") | ||
| await self._set_pointer(key, str(data_cid_obj), register_metadata=False) | ||
| if not isinstance(data_cid_obj, CID): |
There was a problem hiding this comment.
MEDIUM
This rejects valid CID strings returned by custom ContentAddressedStore implementations, despite save() permitting any immutable IPLDKind; the previous _set_pointer() path decoded such strings successfully. Normalize strings with CID.decode() before validating the result.
Comprehensive audit: 20 proven bugs fixed, HTTP/2 enabled, 3–10x performance gains
This PR is the output of a full library audit followed by a strict TDD pipeline. Every item below was implemented as a pair of commits — a red commit with a failing test that reproduces the defect, then a green commit with the fix — so reviewers can check out any red commit and watch the test fail. Every fix was then reviewed by two independent reviewers (Codex implemented under a verifying harness; Grok and an adversarial Claude reviewer audited each diff, with blocking findings driving remediation commits).
Results: 215 tests pass (up from 142 on
main), ruff/format clean, mypy improved (7 errors vs 8 pre-existing). Two silent data-corruption bugs inShardedZarrStoreare fixed,set()is now failure-atomic, HTTP/2 is enabled (closes the investigation in #73, resolves #79 by staying on httpx), and bulk writes are ~10x faster thanmain.Benchmarks (same machine, 20k-key bulk write + flush,
InMemoryCAS)main(e932534)Reads: ~3x faster hashing (P1), ~30x faster sharded warm gets (P4), instrumentation overhead when disabled reduced from 18–22% to ≈0 (P2/P5 measurements below per item).
1. Core HAMT (
py_hamt/hamt.py)enable_write()while already writable replaced the buffer store unconditionally, orphaningroot_node_idand destroying all buffered data. Now an idempotent no-op inside the lock.set()that failed mid-bucket-split (hash bits exhausted with short hash functions) destroyed previously committed keys: the parent bucket was replaced with a link to an empty node before evicted KVs were re-inserted.set()is now atomic on failure — prior state stays fully readable.keys()held the non-reentrant asyncio lock acrossyields; callingget/set/deleteinside the iteration deadlocked silently forever. Iteration now snapshots under the lock and yields lock-free.InMemoryTreeStore.load()added every CAS-loaded node to the write buffer under a fresh uuid nobody could look up: unbounded memory growth on pure reads and a 0% cache hit rate. Loads are now cached by CAS id with dirty/clean separation.blake3_hashfnspent ~99% of its time in multiformats wrap/unwrap validation (~134µs vs 0.5µs raw). Calls blake3 directly; golden-digest tests prove byte-identical output (CIDs unchanged). 3.7x write / 3.2x read._reserialize_and_linklocated children via O(256) scans per level (6.2Mget_linkcalls per 50k writes) and re-hashed keys it already hashed. Descent path is now threaded through for O(1) relinking; hashes computed once. Structural call-count test + golden root CIDs.vacate()flushed buffered nodes one await at a time. Sibling subtrees now flush concurrently (children strictly before parents); 30x faster flush at 20ms simulated RTT, byte-identical root CIDs.set()(−40% throughput net of P1/P2). Rewritten to build/validate-before-mutate: overflow subtrees are constructed in detached buffer nodes so the only failure point fires before any existing node is mutated; happy path does zero deepcopies. Adversarial review ran differential CID tests across 7 workloads with hundreds of injected failures — byte-identical trees, zero atomicity holes. Result: 3,625 → 59,506 sets/s.2. Zarr stores (
sharded_zarr_store.py,zarr_hamt_store.py, converter, encryption)temp/c/0/0/0andprecip/c/0/0/0mapped to the same slot — last write won, reads returned the other variable's data with no error. Each array now gets its own index space under a versioned root format; stores written by currentmainremain readable (missing version ⇒ legacy layout). The HAMT→sharded converter builds per-array indexes and round-trips multi-variable datasets.resize_storerecomputed geometry without remapping existing row-major entries: growing any non-first dimension reflowed the index and scrambled all previously written chunks (append_dim="lon"silently corrupted data; time-appends only worked by coincidence). Resizes now remap old→new linear indices through chunk coordinates; only the primary array's metadata may trigger a resize.RuntimeErrorin a 6-op sequential sequence), and the clean-until-mark_dirtywindow could silently drop acknowledged writes at flush. Shards are now pinned while in use and marked dirty atomically with mutation; the cache overflows gracefully instead of raising.time, lat, lon, …); any other coordinate name (y,x,level,station) crashed writes or fell into the Z1 collision. Keys are now classified structurally against the store's actual primary array — the allowlist is gone. (Remediation commit fixes a legacy-manifest regression the adversarial reviewer proved with a failing script.)ZarrHAMTStore.delete()skipped the metadata read-cache purge (it was commented out), soget()resurrected deleted keys whileexists()said False. Cache is purged on delete; the encrypted subclass is covered by the same tests.Storecontract violations: metadataget()rejected byte ranges,list()/list_prefix()never yielded chunk keys,list_dir()raisedNotImplementedError, and root-level arrays silently bloated the root object. All implemented/made explicit with contract tests.3. HTTP layer / KuboCAS (
store_httpx.py) — includes #73 / #79load()sentRangeheaders but never checked for a 206: a gateway that ignoresRangereturned the full body, which flowed as-if-ranged straight into zarr buffers. Responses are now validated (200 bodies sliced locally); zero-length/zero-suffix requests returnb""instead of emitting invalidbytes=-0headers, consistently acrossKuboCASandInMemoryCAS.Retry-Afterignored) while permanently dead endpoints were retried with full backoff and re-raised as a misleadingTimeoutException. Now retries transient statuses honoringRetry-After, fails fast on 404, and always re-raises the original exception type.httpx[http2]). The #73ConnectionTerminatedfailure was empirically re-verified on httpx 0.28.1 and is absorbed by the K6/K9 retry layer — proven by a new regression test against a raw-h2 TLS server that sends GOAWAY every N streams (all concurrent loads succeed). Plaintext daemon URLs stay HTTP/1.1 via ALPN. A niquests migration (#79) was evaluated hands-on and declined: itsurllib3-futuredependency hijacks theurllib3namespace, and the measured benefit doesn't justify the migration risk.KuboCAS(client=...)calledget_running_loop()in__init__— crashed in sync code. Client now binds lazily.asyncio.run()→RuntimeError. Semaphores are now per-loop like clients.Nonedefaults) and that internal client leaked. Per-loop clients now inherit the user client's configuration and are all closed byaclose().timeout=60.0per request silently overrode user-configured client timeouts (0.05s configured → 60s actual). The default lives on internal clients only; user timeouts win.raise_for_status()treated 301 as fatal — dweb.link, 4everland.io and w3s.link (which 301 path-style/ipfs/CID) were entirely unusable.follow_redirects=Trueon internal clients./ipfs(no trailing slash) built…/ipfs/ipfs/. URLs are normalized; all four spellings (host,host/,host/ipfs,host/ipfs/) work.Compatibility notes (suggest a major version bump)
mainare still readable. Old readers cannot read new multi-variable stores.h2dependency viahttpx[http2];trustmeadded to dev deps for the GOAWAY TLS test.How this was verified
sys.settracefuzzing that would have broken the repo's 100%-coverage gate.pytest215 passed / 3 skipped (IPFS-daemon and live-gateway tests excluded in this environment — please runbash run-checks.shwith a daemon before merging),ruff checkandruff format --checkclean, mypy 7 pre-existing errors (was 8).Addresses #73, #79; makes substantial progress on #56 (request amplification measured: sharded store = 2 cold RTTs vs HAMT's 3–4; CPU is no longer the bottleneck toward 1Gb/s) and #20/#21 (atomicity/determinism hardening).
🤖 Generated with Claude Code
Summary by CodeRabbit
client_factoryoption for HTTP CAS clients.keys()snapshot,len()without snapshot).