Skip to content

fix(downgrader): clear the 3.1 to 3.0 nullable-merge lists when conversion throws - #99

Merged
dinwwwh merged 1 commit into
mainfrom
claude/project-thread-fpuvkk
Oct 25, 2026
Merged

dinwwwh merged 1 commit into
mainfrom
claude/project-thread-fpuvkk

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Oct 24, 2026

Copy link
Copy Markdown
Member

Requested by dinwwwh · project thread

Problem

downgradeV30 in src/v3.1-to-v3.0.ts keeps two module-level lists, twoBranchAnyOfs and anyOfTargets, for the nullable merge. They were cleared at the start of a call and at its successful end, but not when conversion threw (for example the RangeError on very deep nesting that the README lists). Until the next call, they kept the input and the partial output alive: on a 50k-schema doc, about 61.7 MB stayed retained after a throw versus 4.2 MB after a success.

Fix

Wrap the body in try/finally and clear both lists once, in finally. Since every call now ends with them empty, the clear at the start is dropped. The loop iterates twoBranchAnyOfs directly instead of splice(0), because mergeNullable never pushes to it.

Checks

  • pnpm test: 262 tests pass; pnpm run check clean.
  • Output byte-identical to main on 80 real 3.1 documents.
  • Checked by hand that a call after a deep-nesting RangeError still merges anyOf: [string, null] into nullable: true. No gc-based unit test, since that would be flaky.

Generated by Claude Code

…rsion throws

downgradeV30 keeps two module-level lists, twoBranchAnyOfs and anyOfTargets,
that were only cleared on success. A throw (such as the RangeError on very deep
nesting) left them holding the input and partial output until the next call.
Wrap the body in try/finally and clear them once there.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NerLPPYvjVTGwnV3s1in2V
@dinwwwh dinwwwh self-assigned this Oct 24, 2026
@codecov

codecov Bot commented Oct 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Oct 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 21 untouched benchmarks


Comparing claude/project-thread-fpuvkk (167541a) with main (6a906e9)

Open in CodSpeed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

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

  • Wrap downgradeV30 in try/finally — clears twoBranchAnyOfs and anyOfTargets even when conversion throws, so a failed call no longer retains the input and partial output until the next call.
  • Drop the start-of-call clear — now redundant since every call ends with both lists empty.
  • Iterate twoBranchAnyOfs directly instead of splice(0) — valid because mergeNullable never pushes to the list.

The direct iteration is safe: mergeNullable only calls isOnlyNullSchema, isRecord, getRef, find, rejectsNull, and Object.assign, none of which re-enter conversion. downgradeV30 is also not re-entrant — the chained 3.2→3.1→3.0 path calls the two downgraders sequentially — so removing the defensive start-of-call clear cannot leak state between calls.

I confirmed the fix empirically with a throwaway probe (200k two-branch-anyOf schemas plus a >1000-deep schema to force the documented RangeError): the base retains ~142 MB after the throw, while this branch retains ~0 MB. A successful call retains ~0 MB on both. pnpm test passes (262 tests).

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@dinwwwh
dinwwwh merged commit ba10381 into main Oct 25, 2026
11 checks passed
dinwwwh added a commit that referenced this pull request Oct 25, 2026
…lish, in 3.1 to 3.0 (#102)

<!-- claude-projects-attribution: {"github_login":"dinwwwh"} -->
_Requested by **dinwwwh** · [project
thread](https://claude.ai/code/project/chan_01UcKMz91tMarPV1kWLVgLYE?thread=cmsg_01UcKMz91tMarPV1kWLVgLYE3x6dbnc7PCFZZ9Y1ggLhEU)_

## Problem

Effect's `S.NullishOr(X)` and `S.optional(S.NullOr(X))` come out of
`@orpc/effect` as `anyOf: [X, { type: "null" }, { type: "null" }]`. The
nullable merge from #87/#88 only looks at two-branch anyOfs, so 3.0
keeps the anyOf.

NSwag 14.7 on an oRPC Effect model (`addr: S.NullishOr(S.Struct({ city
}))`, `tags: S.optional(S.NullOr(S.Array(S.String)))`, `count:
S.NullishOr(S.Int)`, `name: S.optional(S.NullOr(S.String))`): on main it
makes empty wrapper classes, and the C# client fails on both a filled
payload ("Cannot deserialize the current JSON array into type 'Tags'")
and an all-null one ("Required property 'addr' expects a value but got
null"). With this PR it makes a nullable class, `ICollection<string>`,
`long?` and `string`, and both payloads deserialize. Kiota reads the
kept anyOf as a string.

## Change

- `finishSchema` queues every anyOf with more than one branch (the list
is renamed `twoBranchAnyOfs` to `nullableAnyOfs`).
- `mergeNullable` merges when all branches but one are only-null.
- When the pair becomes a `oneOf`, it keeps the other branch and one
null branch, in their order, since a `oneOf` rejects null when it
matches two branches. It picks by position, not identity, so a doc that
uses one null object twice still gets null once.
- Tests: the #87 test that pinned `[string, null, null]` as a kept anyOf
moves to the merge test, plus a `oneOf` case for each order and
`[string, number, null]` staying an anyOf. One README row.

## Checks

- 258 tests pass, `pnpm run check` clean.
- 1,000 random Effect docs stay valid with no meaning lost. Zod/Valibot
fuzz output and 79 of 80 real docs are byte-identical to main.
- Speed (esbuild bundles, one process, gc and shuffled order, 250
rounds): generated api 100, path item chain and the corpus are within
noise of main. A synthetic doc with 6,000 Effect nullish fields is about
15% slower, which is the cost of the merge it now gets.

Note: #99 also edits `downgradeV30`, so whichever merges second needs a
small rebase for the rename.

---
_Generated by [Claude
Code](https://claude.ai/code/session_01KxDhzz5VuQrnkwczU91Bv6)_

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants