Skip to content

fix(serializers): type ByteStorage.retrieve failures and collapse duplicated columnar decode (LAB-2736) - #287

Merged
27Bslash6 merged 3 commits into
mainfrom
lab-2736-byte-storage-error-typing
Sep 19, 2026
Merged

27Bslash6 merged 3 commits into
mainfrom
lab-2736-byte-storage-error-typing

Conversation

@27Bslash6

@27Bslash6 27Bslash6 commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR introduces a distinct exception type for ByteStorage.retrieve() verification failures and consolidates duplicated columnar (DataFrame/Series) decode logic in the AutoSerializer.

Changes

New EnvelopeIntegrityError exception (Rust layer)

  • Adds a new EnvelopeIntegrityError exception (subclass of ValueError) in the Rust bindings, registered on the Python module with a corrected __module__ so it remains picklable across processes.
  • Introduces a retrieve_error_to_py mapping function that translates ByteStorageError variants into the Python exception taxonomy:
    • DeserializationFailed (bytes that were never a ByteStorage envelope) → plain ValueError, preserving the fall-through signal that callers rely on.
    • All other variants (checksum mismatch, decompression bomb/failure, size mismatch, oversized input) → EnvelopeIntegrityError, which must fail closed.

Previously, both "not an envelope" and genuine corruption raised an indistinguishable ValueError, so the deserializer could not reliably tell them apart.

AutoSerializer.deserialize behavior

  • Now catches EnvelopeIntegrityError specifically and re-raises it as SerializationError (fail-closed), rather than falling through to a plain-msgpack/NumPy re-parse of corrupt bytes.
  • Bytes that are genuinely "not an envelope" (e.g. written with integrity checking off) still fall through to the Python-only decode paths.

Collapsed duplicated columnar decode paths (LAB-2736)

  • Merges the separate dataframe and series branches so that, when integrity checking is on, they share a single Rust-envelope retrieve + _decode_columnar path instead of a duplicated copy.
  • Adds a safeguard: when metadata indicates a dataframe/series type but no envelope was used (integrity off) or the envelope wasn't recognized (cross-config read), the bytes are still routed through _decode_columnar to reconstruct the proper DataFrame/Series — preventing a regression where the raw wire dict would be returned instead.
  • Simplifies _deserialize_dataframe and _deserialize_series to accept already-unpacked documents, removing redundant internal msgpack unpacking.

Tests

  • Adds a cross-config regression test (written with integrity off, read with integrity on) verifying columnar reconstruction.
  • Adds tests confirming corrupted payloads surface an "envelope verification" SerializationError, while plain msgpack written with integrity off still falls through and decodes correctly.
  • Updates existing columnar fallback tests to exercise the full serialize/deserialize round-trip (forcing the msgpack-columnar path) instead of calling internal helpers directly.

Summary by CodeRabbit

  • Bug Fixes

    • Corrupted or oversized verified envelopes now fail safely with a dedicated integrity error instead of falling back to other decoding methods.
    • Invalid envelope contents now consistently raise clear serialisation errors.
    • Plain MessagePack data remains readable when integrity checking settings differ.
    • DataFrame and Series round trips now preserve nullable values and missing data more reliably, including PyArrow-backed types.
  • Tests

    • Added coverage for integrity configuration differences, corrupted payloads, and nullable pandas data.

…licated columnar decode (LAB-2736)

ByteStorage.retrieve() flattened every corruption case (checksum mismatch,
decompression bomb/failure, size mismatch) into the same PyValueError as
"not a ByteStorage envelope at all", so AutoSerializer.deserialize() could
not tell a genuinely corrupted cache entry from bytes that were legitimately
written without an envelope (integrity checking off) - a checksum mismatch
either fell through to a confusing "not decodable" error or, on the
DataFrame/Series path, was duplicated into two near-identical retrieve+decode
blocks with their own ad hoc error text.

Add EnvelopeIntegrityError (rust/src/python_bindings.rs), a ValueError
subclass raised for every ByteStorageError variant except
DeserializationFailed (which stays a plain ValueError - the fall-through
signal deserialize() depends on). AutoSerializer.deserialize() catches it
specifically and re-raises as SerializationError without falling through.
Collapse the DataFrame/Series metadata pre-branch and the verified-envelope
branch onto the single _decode_columnar path, and drop the dead
isinstance(data, dict) branches in _deserialize_dataframe/_deserialize_series
now that no caller passes raw bytes.

Expert-panel review caught a regression the collapse introduced: an entry
written with integrity off and read by an integrity-on reader (same
metadata routing to dataframe/series) fell through to the generic msgpack
fallback and returned the raw wire dict instead of failing closed or
reconstructing - fixed by routing that fallback through _decode_columnar
too, with a regression test.
@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Review Change StackReview 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: Team

Run ID: a5abacda-4792-406e-abd9-6be6da752a4f

📥 Commits

Reviewing files that changed from the base of the PR and between 81f97fb and bfe4025.

📒 Files selected for processing (5)
  • rust/src/lib.rs
  • rust/src/python_bindings.rs
  • src/cachekit/serializers/auto_serializer.py
  • tests/unit/test_auto_serializer_mutation_and_corruption.py
  • tests/unit/test_auto_serializer_new_types.py

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.


Walkthrough

The change adds specialised envelope integrity errors, updates AutoSerializer envelope and columnar decoding, and adds regression coverage for corrupted envelopes, direct MessagePack payloads, and public DataFrame and Series round trips.

Changes

Envelope integrity and columnar decoding

Layer / File(s) Summary
Envelope error mapping
rust/src/lib.rs, rust/src/python_bindings.rs
The Rust bindings expose EnvelopeIntegrityError. Retrieval keeps DeserializationFailed as ValueError and maps other envelope failures to EnvelopeIntegrityError.
Serializer envelope and columnar decoding
src/cachekit/serializers/auto_serializer.py
AutoSerializer fails closed for detected envelope verification or decode failures. DataFrame and Series payloads use a shared columnar decode path, while direct MessagePack payloads retain fallback decoding.
Integrity and public round-trip coverage
tests/unit/test_auto_serializer_mutation_and_corruption.py, tests/unit/test_auto_serializer_new_types.py
Tests cover cross-configuration reads, corrupted envelopes, direct MessagePack fallback, nullable values, PyArrow-backed data, and public serializer round trips.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant AutoSerializer
  participant PyByteStorage
  participant ByteStorage
  participant ColumnarDecoder
  AutoSerializer->>PyByteStorage: retrieve envelope
  PyByteStorage->>ByteStorage: retrieve bytes
  ByteStorage-->>PyByteStorage: payload or retrieval failure
  PyByteStorage-->>AutoSerializer: payload or mapped exception
  AutoSerializer->>ColumnarDecoder: decode verified DataFrame or Series payload
Loading

Merge Risk: 🔵 Low · up to bfe40

A narrow class of previously supported direct MessagePack values may fail to deserialize when read with integrity checking enabled. This is bounded but should be tracked before relying on mixed integrity configurations.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description clearly explains the implementation changes, motivation, and tests. However, it does not follow the required template and omits the Type of Change, Security Checklist, Documentation Va… Update the description to include every required template section. Select the applicable Type of Change options. Complete the security, documentation, testing, and backward compatibility checklists. State the motivation explicitly and recor…
Docstring Coverage ⚠️ Warning Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies both primary changes: typed ByteStorage.retrieve() failures and consolidated columnar decoding.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the implementation changes, motivation, and tests. However, it does not follow the required template and omits the Type of Change, Security Checklist, Documentation Validation Checklist, Backward Compatibility, and Additional Notes sections. It also does not record the required test and security checklist results.

Resolution

Update the description to include every required template section. Select the applicable Type of Change options. Complete the security, documentation, testing, and backward compatibility checklists. State the motivation explicitly and record any additional reviewer context.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lab-2736-byte-storage-error-typing

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

@kodus-27b

kodus-27b Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

  • Validate Business Logic: Ask Kody to validate your code against business rules by adding a comment with the @kody -v business-logic command.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ✅

Access your configuration settings here.

​

@codecov

codecov Bot commented Sep 13, 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!

Comment thread tests/unit/test_auto_serializer_mutation_and_corruption.py
@27Bslash6

Copy link
Copy Markdown
Contributor Author

@kody start-review

kodus-27b[bot]
kodus-27b Bot previously approved these changes Sep 13, 2026
Resolve the LAB-3131 (#289) overlap in the columnar decode path:

- src/cachekit/serializers/auto_serializer.py: both sides retired the dead isinstance(data, dict) preamble in _deserialize_dataframe/_deserialize_series; keep main's parameter naming and docstrings. The PR's deserialize() collapse and EnvelopeIntegrityError handling are unchanged.

- tests/unit/test_auto_serializer_new_types.py: both sides adapted the four TestColumnarFallbackExtensionDtypes tests to the decoded-document contract; keep the PR's public serialize()/deserialize() round-trip, which routes through main's _decode_columnar.
@27Bslash6

Copy link
Copy Markdown
Contributor Author

Merged main (284fa7e) into this branch as 0eab9eb. Resolved conflicts in src/cachekit/serializers/auto_serializer.py (kept the _deserialize_dataframe / _deserialize_series signatures and docstrings from #289; this PR's deserialize() changes are intact) and tests/unit/test_auto_serializer_new_types.py (kept this PR's public serialize() / deserialize() round-trip tests). Auto-rebased onto main; CI will re-run.

Resolve the LAB-304 (#264) overlap in src/cachekit/serializers/auto_serializer.py:
one import-block hunk. The PR added EnvelopeIntegrityError to the ByteStorage
import; main added the redact_error_for_log import used by the envelope-fallback
debug log. Keep both (union). Main's redacted logger.debug call and the PR's
deserialize() collapse auto-merged untouched.
@27Bslash6

Copy link
Copy Markdown
Contributor Author

Resolved src/cachekit/serializers/auto_serializer.py (one import-block hunk, union of both sides); auto-rebased onto main; CI will re-run.

@27Bslash6
27Bslash6 merged commit 0aa78ca into main Sep 19, 2026
38 checks passed
@27Bslash6
27Bslash6 deleted the lab-2736-byte-storage-error-typing branch September 19, 2026 13:39
27Bslash6 pushed a commit that referenced this pull request Sep 22, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.19.0](v0.18.0...v0.19.0)
(2026-09-22)


### ⚠ BREAKING CHANGES

* **logging:** remove dead compat surface and Redis branding from
StructuredLogger (LAB-4621)
([#316](#316))
* **logging:** `cachekit.logging.UltraOptimizedStructuredLogger` is now
`StructuredLogger`; the `cachekit.logging.StructuredRedisLogger` alias
is removed; `cachekit.monitoring.pool_monitor.OptimizedPoolMonitor` is
now `PoolMonitor`. No deprecation aliases are provided.
`get_structured_logger()` is unchanged and remains the supported entry
point.
* **logging:** UltraOptimizedStructuredLogger.__init__ no longer accepts
mask_sensitive; get_structured_logger() no longer accepts mask_sensitive
and now keys _logger_instances on name alone; mask_sensitive_patterns is
removed; ProfileConfig.mask_sensitive_data and
ProfileConfig.lazy_pii_masking are removed. All were read by nothing and
toggled no behavior. Constructors/callers passing them now raise
TypeError instead of silently no-op'ing. Same removal shape as
L1CacheConfig.namespace_index in v0.18.0 and
L1CacheConfig.invalidation_enabled in v0.16.0 (LAB-520).

### Features

* **backend:** bound L1 backfill by the server's remaining freshness
(LAB-557)
([#268](#268))
([7bd5abf](7bd5abf))
* **concurrency:** free-threaded CPython support — memory-ordering
fixes, gil_used=false, CI lane (LAB-511)
([#265](#265))
([bda770b](bda770b))


### Bug Fixes

* **ci:** fail loudly on attestation lookup failure; decide the codecov
pair (LAB-2528)
([#270](#270))
([2a8b941](2a8b941))
* **ci:** make the Atheris fuzz job capable of failing + repair its dead
targets (LAB-1140)
([#269](#269))
([6ab0c28](6ab0c28))
* **decorators:** async get hits record serializer/size/hit like the
sync path (LAB-3765)
([#297](#297))
([9b96fd2](9b96fd2))
* **decorators:** async lock double-check L2 hits record get telemetry
(LAB-3769)
([#303](#303))
([683b1c7](683b1c7))
* **decorators:** honour set_default_backend() when called after
decoration (LAB-4457)
([#313](#313))
([d20204c](d20204c))
* **file:** guard eviction unlink against a concurrent rename (LAB-2685)
([#285](#285))
([e917a57](e917a57))
* **file:** write every byte or fail; evict a payload that shrank under
read (LAB-2682)
([#272](#272))
([068adb2](068adb2))
* **logging:** redact raw cache keys on all log paths (LAB-304)
([#264](#264))
([81f97fb](81f97fb))
* **logging:** sanitise error kwarg at the structured cache-operation
sinks (LAB-3666)
([#301](#301))
([d27ec29](d27ec29))
* **redis:** stop lock waiters pinning executor threads (LAB-3596)
([#290](#290))
([ddbeb91](ddbeb91))
* **serializers:** bound forged-entry error echoes; retire columnar dead
code (LAB-3131)
([#289](#289))
([10a1049](10a1049))
* **serializers:** bound untrusted msgpack decode depth and header
allocation (LAB-2503)
([#276](#276))
([f7c087d](f7c087d))
* **serializers:** fail closed on unverified DataFrame/Series envelopes
(LAB-2736)
([#304](#304))
([60da54f](60da54f))
* **serializers:** take the format from the envelope, not the header;
drop the header-gated fall-through (LAB-2736)
([#308](#308))
([69db1c5](69db1c5))
* **serializers:** type ByteStorage.retrieve failures and collapse
duplicated columnar decode (LAB-2736)
([#287](#287))
([0aa78ca](0aa78ca))


### Performance Improvements

* **decorators:** backfill L1 on sync L2 hits; size stats by envelope
length (LAB-348)
([#294](#294))
([c019b26](c019b26))


### Code Refactoring

* **logging:** remove dead compat surface and Redis branding from
StructuredLogger (LAB-4621)
([#316](#316))
([98d616e](98d616e))
* **logging:** remove dead PII-masking knobs (LAB-3797)
([#300](#300))
([705640b](705640b))
* **logging:** rename UltraOptimizedStructuredLogger to
StructuredLogger, OptimizedPoolMonitor to PoolMonitor (LAB-4617)
([#314](#314))
([2f7c979](2f7c979))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: cachekit-release-bot[bot] <247960786+cachekit-release-bot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@cachekit-io cachekit-io deleted a comment from kodus-27b Bot Sep 23, 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