Skip to content

[BUG]: CopyOnWrite containers lose or duplicate data in un-overridden inherited methods (serialization, deepcopy, operators) #152

Description

@araujof

Current Behavior

CopyOnWriteList and CopyOnWriteDict override only a subset of their base-class methods. Every un-overridden inherited method reads the parent list/dict storage directly — and that storage stays empty until _materialize() runs on the first write. So on an unmaterialized (i.e. read-only, the common case) CoW container, those methods silently operate on nothing.

This is the same root cause as #54 / #55 (CopyOnWriteDict.__eq__) and #135 / #136 (CopyOnWriteList.__eq__). Both of those fixed a single operator. This issue tracks the remaining surface.

Unifying property: all of these behave correctly after any write. The defect only affects containers that were never mutated.

Critical — silent data loss through serialization and copy

These are the reachable, high-impact cases. wrap_payload_for_isolation() is the real production path (ManagerBase._isolate_payload):

class P(PluginPayload):
    server_id: str
    items: list = Field(default_factory=list)

iso = wrap_payload_for_isolation(P(server_id="s", items=["a", "b", "c"]))

list(iso.items)              # ['a', 'b', 'c']          reads fine
iso.model_dump()             # {'server_id': 's', 'items': []}     <-- data gone
iso.model_dump_json()        # {"server_id":"s","items":[]}        <-- data gone
copy.deepcopy(iso).items     # ['a','b','c','a','b','c']           <-- DUPLICATED
iso.model_copy(deep=True)    # ['a','b','c','a','b','c']           <-- DUPLICATED

RootModel payloads are affected regardless of policy, because _isolate_payload isolates them unconditionally (isinstance(effective_payload, RootModel)):

iso = wrap_payload_for_isolation(HttpHeaderPayload({"authorization": "Bearer x", "x-tenant": "acme"}))
dict(iso.root.items())   # {'authorization': 'Bearer x', 'x-tenant': 'acme'}
iso.model_dump()         # {}      <-- every header stripped

This matters because external plugins receive the payload via model_dump():

  • cpex/framework/external/grpc/client.py:228json_format.ParseDict(payload.model_dump(), payload_struct)
  • cpex/framework/external/unix/client.py:303-304 — same
  • cpex/framework/external/mcp/server/server.py:229

Call path: _isolate_payload() (manager.py:405, manager.py:692) → execute_plugin() → external client → payload.model_dump(). An out-of-process plugin would therefore see items: [] / args: {} / no headers, while an in-process plugin reading the same payload sees the real contents.

Note on scope: I confirmed the serialization/copy behavior directly and traced the call path by reading the code, but did not execute a live external-plugin round-trip. Worth confirming before assigning final severity — it's possible something downstream re-materializes. _safe_deepcopy(CopyOnWriteList([...])) also duplicates, which is the same mechanism.

Root cause of each: model_dump and copy/deepcopy both bypass the overridden accessors. Pydantic and json.dumps read the C-level base storage (empty → []/{}); copy reconstructs via __reduce_ex__, which yields the logical items and carries _original over in __dict__, so extend on the new object materializes 3 items and then appends 3 more.

json.dumps differs between the two types — CopyOnWriteDict{}, but CopyOnWriteList happens to serialize correctly because the list encoder iterates.

Moderate — operators and lookups on CopyOnWriteList

With CopyOnWriteList(["a", "b", "c"]), unmaterialized:

Expression Actual Expected
cow < ["a"] True False
cow > ["a"] False True
cow <= [] True False
cow + ["d"] ["d"] ["a","b","c","d"]
cow * 2 [] 6 items
cow += ["d"] no-op (["a","b","c"]) ["a","b","c","d"]
cow *= 2 no-op (3 items) 6 items
cow.index("b") ValueError 1
cow.count("a") 0 1
list(reversed(cow)) [] ["c","b","a"]

cow += [...] silently discarding an append is the nastiest of these for plugin authors: it looks like a mutation and materializes nothing.

Moderate — CopyOnWriteDict

Expression Actual Expected
cow | {} {} {"a": 1, "b": 2}
cow.popitem() KeyError a (key, value) pair
list(reversed(cow)) [] ["b", "a"]
json.dumps(cow) {} {"a": 1, "b": 2}

Already correct on both types: __getitem__ (incl. slices), __len__, __iter__, __contains__, bool(), copy(), keys()/values()/items(), and all mutators that call _materialize(). Double-wrapping (CopyOnWriteList(CopyOnWriteList([...]))) is also safe.

Expected Behavior

Every read on a CoW container reflects the logical sequence/mapping, whether or not it has been materialized — and serializing or copying an isolated payload preserves its contents exactly.

Steps to Reproduce

import copy, json
from cpex.framework.memory import CopyOnWriteList, CopyOnWriteDict

cow = CopyOnWriteList(["a", "b", "c"])
assert list(cow) == ["a", "b", "c"]        # passes
assert cow.count("a") == 1                 # FAILS: 0
assert cow + ["d"] == ["a","b","c","d"]    # FAILS: ["d"]
assert copy.deepcopy(cow) == ["a","b","c"] # FAILS: duplicated

d = CopyOnWriteDict({"a": 1})
assert json.dumps(d) == '{"a": 1}'         # FAILS: {}

For the isolation path, see the wrap_payload_for_isolation / HttpHeaderPayload snippets above.

Logs / Error Output

Not applicable — every failure mode here is silent. No exception, no log line. That is the main hazard: a stripped payload is indistinguishable from a legitimately empty one, which is exactly how #135 stayed hidden in production.

Environment

Additional Context

Patching operators one at a time is what left this surface open twice already (#55, then #136). Two structural options:

  1. Route every read through _source() and override the full list/dict API, including __reduce_ex__/__deepcopy__ (for copy) and whatever hook Pydantic serialization needs. Keeps CoW laziness; broad surface to get right and to keep right as the base types gain methods.
  2. Materialize eagerly on any un-overridden access — e.g. implement __getattr__/__reduce_ex__ to _materialize() first. Gives up some laziness on unusual access patterns but makes the class correct by construction, which is the property the last two fixes each failed to deliver.

Option 2 seems the better trade for a security-relevant container: the failure mode of the current design is silent data loss on an authorization path, and read-only performance only matters if it is also correct. Whichever route, a test that asserts the logical contents survive model_dump(), model_dump_json(), deepcopy(), and model_copy(deep=True) would have caught the whole class.

Related: #54, #55 (CopyOnWriteDict.__eq__), #135, #136 (CopyOnWriteList.__eq__).

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingtriage

Type

Projects

Status
In progress

Relationships

None yet

Development

No branches or pull requests

Issue actions