Skip to content

[Bug] encodeRefs/decodeRefs never remove from their WeakSet, so it is visited-detection rather than cycle-detection and every repeated object in a cross-node message body arrives as null #946

Description

@pathosDev

Problem

encodeRefs and decodeRefs walk the message body with a WeakSet that is added to on first visit and never removed. That is visited-detection, not cycle-detection: it cannot tell "this object is an ancestor of itself" from "this object appears twice in a tree". The second occurrence of any shared object is replaced with null.

Sharing an object across two fields of a message is not an exotic pattern — it is what you get from const user = …; ref.tell({ author: user, reviewer: user }), from a list built by items.map(() => sameDefaults), from a lookup table referenced from several entries, or from any normalised in-memory graph. encodeRefs runs on every cross-node send (Cluster._sendEnvelope is the single chokepoint) and decodeRefs on every receive, so the corruption applies to every remote tell, every sharded entity message, every pub-sub publication and every singleton delivery. There is no error, no warning, and no way for the receiver to notice: it gets a well-formed object with a null where a value should be.

Correct cycle detection removes the node from the set when its subtree is done — an ancestor path set, not a visited set. That is a one-line change on each walker and preserves the existing protection against genuine cycles.

Evidence

src/cluster/RefCodec.ts:134-147
function walk(value: unknown, encodeRef: RefEncoder, seen: WeakSet<object>): unknown {
  if (value === null || value === undefined) return value;
  if (typeof value !== 'object') return value;
  if (value instanceof ActorRef) return encodeRef(value);
  // Types JSON already handles (or silently lossy): leave alone.
  if (value instanceof Date) return value;
  if (value instanceof Uint8Array) return value;

  if (seen.has(value as object)) return null;  // break cycles
  seen.add(value as object);

  if (Array.isArray(value)) {
    return value.map((v) => walk(v, encodeRef, seen));
  }
src/cluster/RefCodec.ts:166-178
function walkDecode(value: unknown, cluster: Cluster, seen: WeakSet<object>): unknown {
  if (value === null || value === undefined) return value;
  if (typeof value !== 'object') return value;

  if (isWireActorRef(value)) return decodeSingleRef(value, cluster);
  if (value instanceof Date || value instanceof Uint8Array) return value;

  if (seen.has(value as object)) return null;
  seen.add(value as object);

  if (Array.isArray(value)) {
    return value.map((v) => walkDecode(v, cluster, seen));
  }

Neither walker ever calls seen.delete. The comment on :142 says "break cycles"; the code breaks repetition.

Proposal

Make seen an ancestor set rather than a visited set — add before recursing, delete after:

seen.add(value as object);
const out = /* … recurse … */;
seen.delete(value as object);
return out;

That keeps the cycle guard exact (a value is only in seen while it is on the current path) and lets a DAG round-trip. The Map/Set and object branches need the same treatment, and both walkers do.

Worth deciding explicitly while the walkers are open: whether a true cycle should still become null silently, or should throw. Today a cycle and a shared reference produce the same output, so a user cannot distinguish "you sent a cycle" from "the codec dropped your data". Once they are distinguishable, a cycle is a programming error worth reporting.

Note this is a fidelity fix, not a sharing fix: the wire format is JSON, so the receiver gets two structurally equal objects rather than one shared object. That matches JSON.parse(JSON.stringify(x)) and is the documented expectation for cross-node messages; silently substituting null is not.

#247 asks for the instanceof chains in this file to become match() — same file, unrelated concern; the walkers are the dispatch-free part.

Acceptance sketch

  • encodeRefs({ a: shared, b: shared }) produces both values, not { a: {…}, b: null }.
  • The same holds for repeated entries in arrays, Map values and Set members.
  • decodeRefs round-trips the same shapes.
  • A genuine self-referential cycle still terminates.
  • An ActorRef appearing twice in one body encodes to a valid marker both times.

Verification status

Found in the ten-lens production-readiness review of 2026-08-05 (v0.13.0) and re-verified before filing: reproduced by execution, calling the exported functions directly:

encodeRefs  -> {"a":{"id":"S","n":1},"b":null,"list":[null,null]}
decodeRefs  -> {"a":{"id":"S","n":1},"b":null,"list":[null,null]}
decode only -> {"x":{"id":"T"},"y":null}
array dup   -> [{"id":"S","n":1},null]
true cycle  -> {"name":"cyc","self":null}
map dup     -> [["p",{"id":"S","n":1}],["q",null]]

The input was { a: shared, b: shared, list: [shared, shared] } with shared = { id: 'S', n: 1 } — no cycles anywhere. Both walkers drop every occurrence after the first, on plain objects, arrays and Map values alike. The true cycle line is the control: the guard it exists for still works.

Part of the production-readiness review batch — tracked in #913.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority: highTop priority — high impact, plan nextproduction-goalBlocks or defines the path to production readiness

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions