Skip to content

feat: serialize documents iteratively - #81

Merged
nikku merged 1 commit into
mainfrom
nikku-moddle-xml-advisory-review
Aug 25, 2026
Merged

nikku merged 1 commit into
mainfrom
nikku-moddle-xml-advisory-review

Conversation

@nikku

@nikku nikku commented Aug 24, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Related to GHSA-x3vc-q6mj-47vp.

Writing previously recursed once per document level (in both serializer construction and output), so deeply nested documents grew the call stack with depth and eventually threw RangeError: Maximum call stack size exceeded — exhausting resources on large inputs. The reader is already iterative and unaffected.

This reworks the writer to walk an explicit stack instead of recursing, in both phases:

  • build → buildTree: an element serializer enters (registers namespaces, tag, attributes) and returns its children as ordered, deferred work; buildTree drives that work and runs exit (generic attributes) once a subtree is built. Pre-order traversal preserves document-order namespace logging.
  • serializeTo → serializeTree: output is emitted from a frame stack rather than nested recursion.

Deeply nested documents now stay flat and scale linearly. Output is byte-identical — guarded by the existing exact-string writer suite and verified with a differential fuzz against the previous implementation (byte-for-byte match across diverse structures).

How to try out

New test/spec/performance.js exercises write, read and round-trip at depth 50000 under a 5s timeout — a stack-recursive or quadratic regression crashes or blows past it.

npm run all

Checklist

  • Contribution meets our definition of done
  • Pull request establishes context
    • Link to related issue(s): Related to GHSA-x3vc-q6mj-47vp
    • Brief textual description of the changes
    • Screenshots or short videos showing UI/UX changes — n/a (no UI)
    • Steps to try out: npm run all

@bpmn-io-tasks bpmn-io-tasks Bot added the in progress Currently worked on label Aug 24, 2026
@nikku

nikku commented Aug 24, 2026 •

Copy link
Copy Markdown
Member Author

Note

🤖 run analysis. Numbers are reproducible via steps below.

Performance assessment

Setup: Node v24.16.0. before = 54407ac (v12.1.0, recursive writer), after = this branch (iterative). Both built via npm run build and measured on identical inputs.

1. Small-file regression — full existing suite (180 tests)

Existing reader + writer + roundtrip specs (all small documents), 3 runs, mocha-reported execution time:

run 1 run 2 run 3 min
before 273 ms 265 ms 254 ms 254 ms
after 257 ms 272 ms 265 ms 257 ms

No regression — within noise (~±3%). Everyday small-document serialization is unaffected.

2. Scaling — deeply nested write (depth kept below the old crash point)

Self-nested props:ComplexNesting, median of 25 runs, warm:

depth before (ms) after (ms) speedup
100 0.56–0.86 0.41–0.48 ~1.4–1.8×
250 1.85–2.09 0.47–0.53 ~4.0×
500 6.60–7.09 0.89–0.97 ~7.3×
800 15.4–15.5 1.23–1.24 ~12.5×

The old writer scales super-linearly (~quadratic: doubling depth ~4×s the time) because each node re-walks the namespace parent chain (O(depth²) total). The iterative writer memoizes that walk and scales linearly, so the gap widens with depth.

3. Stack safety

  • before: throws RangeError: Maximum call stack size exceeded at depth 2000.
  • after: the same depth 2000 serializes in ~19 ms; the dedicated perf spec round-trips at depth 50 000 well under the 5 s gate (write ~390 ms / read ~230 ms / round-trip ~375 ms).

Summary

Same speed on small documents, linear instead of quadratic on large ones, and no depth ceiling. Output is byte-identical (182 exact-string tests + differential fuzz against the previous implementation).

@nikku
nikku marked this pull request as ready for review August 24, 2026 16:23
@nikku
nikku requested review from a team and a lite review from Copilot August 24, 2026 16:23
@bpmn-io-tasks bpmn-io-tasks Bot added needs review Review pending and removed in progress Currently worked on labels Aug 24, 2026
Walk an explicit stack when building and writing serializers so deeply
nested documents stay stack-safe and scale linearly. Output is
byte-identical.

Ref: GHSA-x3vc-q6mj-47vp

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@nikku
nikku force-pushed the nikku-moddle-xml-advisory-review branch from 6cae991 to b79409e Compare August 24, 2026 16:27

Copilot AI 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.

Pull request overview

This PR updates the moddle-xml writer implementation to avoid stack overflows when serializing deeply nested documents (related to GHSA-x3vc-q6mj-47vp) by replacing recursive build/serialize walks with explicit stack-driven traversals.

Changes:

  • Refactor serializer tree construction to an explicit stack walk (buildTree) using enter/exit phases.
  • Refactor XML output emission to an explicit frame stack (serializeTree) instead of recursive serializeTo calls.
  • Add a performance/regression test that exercises write/read/round-trip at depth 50,000 within a 5s timeout, plus a changelog entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test/spec/performance.js Adds deep nesting performance/regression coverage for writer/reader/round-trip at depth 50,000.
lib/write.js Reworks writer internals to iterative build + iterative serialization; adds namespace scope helpers.
CHANGELOG.md Notes the new stack-safe serialization behavior in the unreleased section.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@barmac barmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good and indeed the error can be reproduced when write.js changes are reverted.

@nikku
nikku merged commit 3083e48 into main Aug 25, 2026
3 checks passed
@bpmn-io-tasks bpmn-io-tasks Bot removed the needs review Review pending label Aug 25, 2026
@nikku
nikku deleted the nikku-moddle-xml-advisory-review branch August 25, 2026 08:09
pull Bot pushed a commit to Mu-L/camunda that referenced this pull request Sep 15, 2026
This PR contains the following updates:

| Package | Change |
[Age](https://docs.renovatebot.com/merge-confidence/) |
[Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [bpmn-js](https://redirect.github.com/bpmn-io/bpmn-js) | [`18.25.1` →
`18.27.0`](https://renovatebot.com/diffs/npm/bpmn-js/18.25.1/18.27.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/bpmn-js/18.27.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/bpmn-js/18.25.1/18.27.0?slim=true)
|
| [bpmn-js](https://redirect.github.com/bpmn-io/bpmn-js) | [`18.16.1` →
`18.27.0`](https://renovatebot.com/diffs/npm/bpmn-js/18.16.1/18.27.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/bpmn-js/18.27.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/bpmn-js/18.16.1/18.27.0?slim=true)
|
| [bpmn-moddle](https://redirect.github.com/bpmn-io/bpmn-moddle) |
[`10.1.0` →
`10.2.0`](https://renovatebot.com/diffs/npm/bpmn-moddle/10.1.0/10.2.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/bpmn-moddle/10.2.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/bpmn-moddle/10.1.0/10.2.0?slim=true)
|
| [bpmn-moddle](https://redirect.github.com/bpmn-io/bpmn-moddle) |
[`10.0.0` →
`10.2.0`](https://renovatebot.com/diffs/npm/bpmn-moddle/10.0.0/10.2.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/bpmn-moddle/10.2.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/bpmn-moddle/10.0.0/10.2.0?slim=true)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the [Dependency
Dashboard](..camunda/issues/12605) for more information.

---

### Release Notes

<details>
<summary>bpmn-io/bpmn-js (bpmn-js)</summary>

###
[`v18.27.0`](https://redirect.github.com/bpmn-io/bpmn-js/blob/HEAD/CHANGELOG.md#18270)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-js/compare/v18.26.0...v18.27.0)

- `FEAT`: add tooltip with title and shortcut on palette entries
([#&camunda#8203;2465](https://redirect.github.com/bpmn-io/bpmn-js/pull/2465))
- `FIX`: point label link to the closest point of the connection
([#&camunda#8203;2493](https://redirect.github.com/bpmn-io/bpmn-js/pull/2493))
- `DEPS`: update to `diagram-js@15.26.0`

###
[`v18.26.0`](https://redirect.github.com/bpmn-io/bpmn-js/blob/HEAD/CHANGELOG.md#18260)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-js/compare/v18.25.1...v18.26.0)

- `FEAT`: give resize handle a border radius
([bpmn-io/diagram-js#1100](https://redirect.github.com/bpmn-io/diagram-js/pull/1100))
- `FEAT`: give segment dragger a border radius
([bpmn-io/diagram-js#1100](https://redirect.github.com/bpmn-io/diagram-js/pull/1100))
- `FEAT`: add `--accent-color` theming token
([#&camunda#8203;2492](https://redirect.github.com/bpmn-io/bpmn-js/pull/2492))
- `FIX`: use WCAG AA compliant primary accent color
([#&camunda#8203;2492](https://redirect.github.com/bpmn-io/bpmn-js/pull/2492))
- `DEPS`: update to `diagram-js@15.25.0`
- `DEPS`: update to `bpmn-moddle@10.2.0`
- `DEPS`: update to `bpmn-font@0.13.0`

</details>

<details>
<summary>bpmn-io/bpmn-moddle (bpmn-moddle)</summary>

###
[`v10.2.0`](https://redirect.github.com/bpmn-io/bpmn-moddle/blob/HEAD/CHANGELOG.md#1020)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-moddle/compare/v10.1.0...v10.2.0)

- `FEAT`: serialize deeply nested documents in a scalable, stack-safe
manner
([bpmn-io/moddle-xml#81](https://redirect.github.com/bpmn-io/moddle-xml/pull/81))
- `DEPS`: update to `moddle-xml@12.2.0`

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- Branch creation
- At 08:00 PM through 11:59 PM and 12:00 AM through 08:59 AM, Monday
through Friday (`* 20-23,0-8 * * 1-5`)
  - Only on Sunday and Saturday (`* * * * 0,6`)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/camunda/camunda).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yNDIuMiIsInVwZGF0ZWRJblZlciI6IjQ0LjYwLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbImFyZWEvZnJvbnRlbmQiLCJhdXRvbWVyZ2UiLCJkZXBlbmRlbmNpZXMiXX0=-->
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.

3 participants