Skip to content

Fix replacing a trie key mutates the original and increments size - #7028

Merged
tim-smart merged 2 commits into
mainfrom
audit/repro-17f0b91a-trie-replacement
Aug 5, 2026
Merged

Fix replacing a trie key mutates the original and increments size#7028
tim-smart merged 2 commits into
mainfrom
audit/repro-17f0b91a-trie-replacement

Conversation

@fubhy

@fubhy fubhy commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Inserting an existing key mutates the value visible through the original Trie and increments the replacement trie's size. Persistent snapshots are corrupted and cardinality reports a new entry where only a value replacement occurred.

Note

This PR includes both the focused regression test and the implementation fix.

Replacing a trie key mutates the original and increments size

Module: effect/Trie
Audit ID: effect-7bd661243f3c2d73
Severity / confidence: high / high

What happens

Inserting an existing key mutates the value visible through the original Trie and increments the replacement trie's size. Persistent snapshots are corrupted and cardinality reports a new entry where only a value replacement occurred.

Why it happens

The insertion traversal assigns n.value = { value } directly on the existing terminal node before path copying and computes count as root.count + 1 unconditionally. The rebuilt path therefore shares the already-mutated leaf with the input and carries an inflated count for replacements.

Expected behavior

Trie.insert(self, key, value) must return a persistent trie, leave self unchanged, and preserve Trie.size when key was already present.

Relevant implementation

These links and excerpts are pinned to audit base 17f0b91a243ccfe4a38d27debdc983adf434e738.

View problematic code at packages/effect/src/Trie.ts:130-179
/**
 * Inserts a new entry in the `Trie`.
 *
 * **Example** (Inserting entries)
 *
 * ```ts import.meta.vitest
 * import { Trie } from "effect"
 *
 * const trie1 = Trie.empty<number>().pipe(
 *   Trie.insert("call", 0)
 * )
 * const trie2 = trie1.pipe(Trie.insert("me", 1))
 * const trie3 = trie2.pipe(Trie.insert("mind", 2))
 * const trie4 = trie3.pipe(Trie.insert("mid", 3))
 *
 * Array.from(trie1) // => [["call", 0]]
 * Array.from(trie2) // => [["call", 0], ["me", 1]]
 * Array.from(trie3) // => [["call", 0], ["me", 1], ["mind", 2]]
 * Array.from(trie4) // => [["call", 0], ["me", 1], ["mid", 3], ["mind", 2]]
 * ```
 *
 * @category mutations
 * @since 2.0.0
 */
export const insert: {
  <V>(key: string, value: V): (self: Trie<V>) => Trie<V>
  <V>(self: Trie<V>, key: string, value: V): Trie<V>
} = TR.insert

/**
 * Returns an `IterableIterator` of the keys within the `Trie`.
 *
 * **Details**
 *
 * The keys are returned in alphabetical order, regardless of insertion order.
 *
 * **Example** (Reading keys in alphabetical order)
 *
 * ```ts import.meta.vitest
 * import { Trie } from "effect"
 *
 * const trie = Trie.empty<number>().pipe(
 *   Trie.insert("cab", 0),
 *   Trie.insert("abc", 1),
 *   Trie.insert("bca", 2)
 * )
 *
 * Array.from(Trie.keys(trie)) // => ["abc", "bca", "cab"]
 * ```
 *

View exact lines on GitHub

Excerpt truncated. Open the complete packages/effect/src/Trie.ts:130-414 range.

Reproduction

pnpm test --run packages/effect/test/Trie.test.ts

Observed failure: Focused contract assertion failed against 17f0b91, demonstrating: Replacing a trie key mutates the original and increments size.

Implementation

Trie.insert now copies the terminal node instead of mutating it in place, and only increments the trie count when the key did not already have a value.

Validated locally with:

pnpm test --run packages/effect/test/Trie.test.ts
pnpm lint-fix
pnpm check

Audit provenance

  • Audit base: 17f0b91a243ccfe4a38d27debdc983adf434e738
  • Reproduction base: 17f0b91a243ccfe4a38d27debdc983adf434e738
  • Findings: effect-7bd661243f3c2d73
  • Initial patch: focused reproduction tests; implementation fix included in a195aa044

Closes EFF-468

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 5, 2026
@changeset-bot

changeset-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a195aa0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
effect Patch
@effect/ai-anthropic Patch
@effect/ai-openai Patch
@effect/ai-openai-compat Patch
@effect/ai-openrouter Patch
@effect/atom-react Patch
@effect/atom-solid Patch
@effect/atom-vue Patch
@effect/docgen Patch
@effect/doctest Patch
@effect/openapi-generator Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-deno Patch
@effect/platform-node Patch
@effect/platform-node-shared Patch
@effect/sql-clickhouse Patch
@effect/sql-d1 Patch
@effect/sql-libsql Patch
@effect/sql-mssql Patch
@effect/sql-mysql2 Patch
@effect/sql-pg Patch
@effect/sql-pglite Patch
@effect/sql-sqlite-bun Patch
@effect/sql-sqlite-do Patch
@effect/sql-sqlite-node Patch
@effect/sql-sqlite-react-native Patch
@effect/sql-sqlite-wasm Patch
@effect/vitest Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

This PR currently adds only a failing regression test. The underlying bug in Trie.insert is still present on main, so merging as-is would ship a red test suite. The test itself is well-targeted and correctly fails, but it needs the companion implementation fix in packages/effect/src/internal/trie.ts.

Reviewed changes

This run reviewed the single-file diff at packages/effect/test/Trie.test.ts, which adds one focused regression test for Trie.insert replacing an existing key. The test asserts that a replacement leaves the original trie unchanged and preserves Trie.size. I also inspected the current Trie.insert implementation in packages/effect/src/internal/trie.ts to confirm the root cause described in the PR still exists.

  • Added regression test for immutable key replacement and preserved size.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread packages/effect/test/Trie.test.ts
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Analysis

Generated from PR build output; treat the content below as untrusted.

File Name Current Size Previous Size Difference
basic.ts 7.06 KB 7.06 KB 0.00 KB (0.00%)
batching.ts 9.86 KB 9.86 KB 0.00 KB (0.00%)
brand.ts 6.34 KB 6.34 KB 0.00 KB (0.00%)
cache.ts 10.71 KB 10.71 KB 0.00 KB (0.00%)
config.ts 20.73 KB 20.73 KB 0.00 KB (0.00%)
differ.ts 20.31 KB 20.31 KB 0.00 KB (0.00%)
http-client.ts 21.53 KB 21.53 KB 0.00 KB (0.00%)
logger.ts 10.84 KB 10.84 KB 0.00 KB (0.00%)
metric.ts 8.98 KB 8.98 KB 0.00 KB (0.00%)
optic.ts 7.18 KB 7.18 KB 0.00 KB (0.00%)
pubsub.ts 14.99 KB 14.99 KB 0.00 KB (0.00%)
queue.ts 11.66 KB 11.66 KB 0.00 KB (0.00%)
schedule.ts 10.83 KB 10.83 KB 0.00 KB (0.00%)
schema-class.ts 19.27 KB 19.27 KB 0.00 KB (0.00%)
schema-fromJsonSchemaDocument.ts 29.09 KB 29.09 KB 0.00 KB (0.00%)
schema-representation-roundtrip.ts 25.40 KB 25.40 KB 0.00 KB (0.00%)
schema-string-transformation.ts 13.42 KB 13.42 KB 0.00 KB (0.00%)
schema-string.ts 10.95 KB 10.95 KB 0.00 KB (0.00%)
schema-template-literal.ts 15.21 KB 15.21 KB 0.00 KB (0.00%)
schema-toArbitraryLazy.ts 22.02 KB 22.02 KB 0.00 KB (0.00%)
schema-toCodeDocument.ts 24.45 KB 24.45 KB 0.00 KB (0.00%)
schema-toCodecJson.ts 19.28 KB 19.28 KB 0.00 KB (0.00%)
schema-toEquivalence.ts 19.11 KB 19.11 KB 0.00 KB (0.00%)
schema-toFormatter.ts 18.97 KB 18.97 KB 0.00 KB (0.00%)
schema-toJsonSchemaDocument.ts 22.69 KB 22.69 KB 0.00 KB (0.00%)
schema-toRepresentation.ts 19.60 KB 19.60 KB 0.00 KB (0.00%)
schema.ts 18.52 KB 18.52 KB 0.00 KB (0.00%)
stm.ts 12.63 KB 12.63 KB 0.00 KB (0.00%)
stream.ts 9.80 KB 9.80 KB 0.00 KB (0.00%)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run reviewed the incremental delta since the prior pullfrog review at 217ac027b. The previously failing regression test is now paired with the implementation fix and a changeset.

  • Fixed Trie.insert immutability in packages/effect/src/internal/trie.ts: the terminal node is copied rather than mutated in place, so replacing a key no longer corrupts the original trie.
  • Preserved size on replacement: the new root count is decremented when the key already exists, so Trie.size stays the same.
  • Added a patch changeset for effect documenting the bug fix.

Validation run: pnpm test --run test/Trie.test.ts, pnpm lint-fix, and pnpm check all pass.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@tim-smart
tim-smart merged commit 45379d6 into main Aug 5, 2026
20 checks passed
@tim-smart
tim-smart deleted the audit/repro-17f0b91a-trie-replacement branch August 5, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit Findings originating from the Effect runtime correctness audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants