Skip to content

fix: return writable numpy/Series reads and surface DataFrame/Series corruption clearly#191

Merged
27Bslash6 merged 1 commit into
mainfrom
fix/serializer-integrity-numpy-safety
Jun 19, 2026
Merged

fix: return writable numpy/Series reads and surface DataFrame/Series corruption clearly#191
27Bslash6 merged 1 commit into
mainfrom
fix/serializer-integrity-numpy-safety

Conversation

@27Bslash6

@27Bslash6 27Bslash6 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Two robustness fixes in AutoSerializer's deserialize path (the integrity batch from triage).

Closes #157. Closes #156.

#157 — cached numpy / Series reads were read-only and aliased the buffer

Reconstruction via np.frombuffer(...) returns a read-only array that aliases the source bytes — on an L1 hit, the live cached buffer. A caller mutating a cached value then hit ValueError: assignment destination is read-only where an uncached call would not, and mutation-by-view risked corrupting the cache entry (DATA-IS-SACRED adjacency).

Fix: .copy() at all four reconstruction sites (top-level numpy, nested numpy in msgpack, numeric DataFrame column, numeric Series) so reads are writable and own their data. The copy is required for correctness, not gratuitous — it restores the same contract an uncached return value has.

#156 — DataFrame/Series corruption was swallowed, losing the diagnostic

The dataframe/series branches wrapped ByteStorage.retrieve() in a broad except Exception, logged at DEBUG, and fell through to re-parsing the corrupt bytes as raw msgpack. A genuine xxHash3 checksum mismatch (surfaced from Rust as ValueError) was therefore swallowed — the system still failed closed (recompute), but the corruption signal was lost and a confusing TypeError was logged instead.

Fix: a retrieve() failure now raises a clear SerializationError("…corrupted cache entry…"), mirroring StandardSerializer. The unpack/build step moved outside the guard so a genuine post-retrieve error surfaces as itself rather than being mistaken for corruption. The integrity-off (direct msgpack) path is unchanged.

Notes

  • DataFrames route through ArrowSerializer when pyarrow is installed, so the columnar msgpack path (the "dataframe" branch + its frombuffer site) is reached only without pyarrow; Series always use the columnar path. Tests cover both (the no-pyarrow path via a disabled arrow serializer).
  • Scoped out: the no-metadata generic fallback still has the same broad-except shape for other formats — that's the shared path tracked under the AutoSerializer DataFrame/Series deserialize swallows checksum-mismatch (degraded corruption diagnostic) #156 umbrella and needs the Rust binding to distinguish corruption from format errors; this PR fixes the live, metadata-carrying DataFrame/Series branches.

Tests

New tests/unit/test_auto_serializer_mutation_and_corruption.py: writability + non-aliasing for numpy/nested/Series/DataFrame-column reads, and clear SerializationError on corrupted DataFrame/Series. Full suite: 1850 passed, 0 failures; basedpyright 0 errors.

Summary by CodeRabbit

  • Bug Fixes

    • Deserialized numeric arrays and pandas data structures are now writable and do not alias cached buffers, preventing unintended modifications.
    • Enhanced cache integrity checks with clearer error messages for corrupted entries.
  • Tests

    • Added comprehensive test coverage for array mutability safety and cache corruption diagnostics.

…corruption clearly

#157: np.frombuffer reconstruction returned read-only arrays aliasing the source buffer (the L1-cached bytes on a hit); mutating a cached value crashed or risked corrupting the entry. .copy() at all four sites (top-level numpy, nested numpy, numeric DataFrame column, numeric Series) makes reads writable and self-owned.

#156: the DataFrame/Series branches swallowed a checksum mismatch from retrieve() in a broad except and re-parsed the corrupt bytes as raw msgpack, losing the diagnostic. A retrieve() failure now raises a clear SerializationError (mirrors StandardSerializer); unpack/build moved outside the guard so post-retrieve errors surface as themselves. Integrity-off path unchanged.

Closes #156. Closes #157.
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2b656db5-bcda-4db2-a1df-7d0e1daab4c4

📥 Commits

Reviewing files that changed from the base of the PR and between 3b0565f and 1eb625e.

📒 Files selected for processing (2)
  • src/cachekit/serializers/auto_serializer.py
  • tests/unit/test_auto_serializer_mutation_and_corruption.py

Walkthrough

Fixes two deserialization correctness defects in AutoSerializer: all np.frombuffer reconstruction sites now call .copy() to return writable, non-aliasing arrays, and the DataFrame/Series integrity-checking branches now raise SerializationError on ByteStorage.retrieve failures rather than falling back to raw msgpack parsing. A new test file covers both fixes.

Changes

AutoSerializer deserialization correctness

Layer / File(s) Summary
Writable non-aliasing array reconstruction and mutation-safety tests
src/cachekit/serializers/auto_serializer.py, tests/unit/test_auto_serializer_mutation_and_corruption.py
_auto_object_hook, _deserialize_numpy, _deserialize_dataframe, and _deserialize_series each append .copy() after np.frombuffer(...) so returned arrays are writable and do not alias the cached buffer. Mutation-safety tests assert .flags.writeable and verify that mutating a deserialised value does not affect subsequent deserialisations.
Integrity-checking branch tightening and corruption-diagnostic tests
src/cachekit/serializers/auto_serializer.py, tests/unit/test_auto_serializer_mutation_and_corruption.py
The deserialize() DataFrame and Series branches under enable_integrity_checking now raise SerializationError("integrity check failed (corrupted cache entry)") on ValueError/SerializationError from ByteStorage.retrieve, removing the prior broad except Exception fallback to raw msgpack. Corruption tests flip a byte in serialised payloads and assert SerializationError is raised; a _no_arrow() helper forces the columnar msgpack path for DataFrame coverage.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • cachekit-io/cachekit-py#189: Modifies _deserialize_numpy in auto_serializer.py to add checksum-prefixed envelope detection and verification — directly adjacent to the .frombuffer(...).copy() non-aliasing fix applied to the same function in this PR.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: fixing writable numpy/Series reads and surfacing DataFrame/Series corruption clearly, which aligns with the core objectives of issues #157 and #156.
Description check ✅ Passed The description comprehensively covers both issues, explains the problems and solutions, includes linked issue references, and documents testing. It follows the template's spirit with motivation and type of change implied.
Linked Issues check ✅ Passed The code changes directly address both linked issues: #157 applies .copy() at four reconstruction sites to ensure writable, non-aliasing arrays; #156 raises SerializationError on ByteStorage.retrieve() failures.
Out of Scope Changes check ✅ Passed All changes are in-scope: implementation fixes in auto_serializer.py and comprehensive test coverage in the new test file directly address the two linked issues with no extraneous changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/serializer-integrity-numpy-safety

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

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@27Bslash6 27Bslash6 merged commit 46a3c3c into main Jun 19, 2026
32 checks passed
@27Bslash6 27Bslash6 deleted the fix/serializer-integrity-numpy-safety branch June 19, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant