Skip to content

Fix the defects that upstream's CI reports on the bun branch (cherry-pick from #3) - #8

Open
robobun wants to merge 7 commits into
bunfrom
robobun/6d0316ee/fix-upstream-ci-failures
Open

robobun wants to merge 7 commits into
bunfrom
robobun/6d0316ee/fix-upstream-ci-failures

Conversation

@robobun

@robobun robobun commented Sep 13, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #7, which is the first commit here. Merge #7 first.

Problem

  • With the actions pinned (ci: pin workflow actions to full commit SHAs #7), the two jobs of ci.yml start and fail on defects that are already on bun (run 34774752554). Test fails in Run Rust tests under RUSTFLAGS=-Dwarnings: error: unused import: Cell and error: hidden lifetime parameters in types are deprecated. Clippy and format fails in Check format: Diff in src/selectors_vm/mod.rs:508.
  • Two more defects are behind these. c-api does not compile, because set_user_data needs Send since 0f44e89. Check semver reports enum_variant_added for RewritingError::Suspended.

Fix

  • The PR cherry-picks three commits by @sosukesuzuki from the branch of Expose the transform-stream layer; UTF-8 text fast path; O(active) end-tag dispatch #3. There, 6166e4c reverted them as unrelated to that PR.
  • eeea10d removes the unused import, writes TextChunk<'_> in the type hint of text!, and applies rustfmt. The behavior does not change.
  • 4afc2ca makes c-api store the user-data pointer in a UserDataPtr newtype that is Send. 021637b runs Check semver only in cloudflare/lol-html.
  • Verified on this PR: Test, Clippy and format, cargo test and semgrep/ci all pass. Check semver shows as skipped. The same steps pass locally (Notes).

Background

  • ci.yml comes from upstream and runs on every pull request here, with warnings as errors. It also builds the c-api and js-api crates. oven-sh/bun builds only lol_html.
  • Check semver compares the public API with the crates.io release of the same version (2.7.2). The fork adds RewritingError::Suspended to a public enum on purpose and is never published. So the check can only fail here.
Notes

Commits

commit here from change
2151cac #7 pin the actions
00871c5 eeea10d src/rewriter/mod.rs (test import), src/rewriter/settings.rs (text!), src/selectors_vm/mod.rs (rustfmt)
9a16a33 4afc2ca c-api/src/*.rs
baeee5f 021637b .github/workflows/ci.yml

The first two cherry-picks are verbatim. 021637b conflicted with the # v2.9 comment from #7 on the adjacent uses: line. The resolution keeps the if: and the comment of 021637b, without its mention of lol_html::transform, which is not on bun before #3 lands.

Where the defects come from

  • 0f44e89 (content-handler suspension): the unused Cell import, the Send bound that c-api does not meet, and the new RewritingError::Suspended variant.
  • The elided lifetime in text! is upstream code (still on upstream main). 0f44e89 exposed the lint, because its tests are the first to use text! inside the crate.
  • ea6125b (selectors_vm): the rustfmt drift.

Merge and rebase

  • "Rebase and merge" keeps the authorship of the three commits. A squash keeps it as Co-authored-by only.
  • ea6125b is upstream 87f7987. At the next rebase onto an upstream tag that contains it, drop ea6125b together with the rustfmt hunk of 00871c5.

Earlier evidence

On the branch of #3 the same four commits made all three workflows green: run 34296114458 (CI), 34296114509 (bun), 34296114485 (Semgrep config).

Local run

Toolchain: nightly-2026-07-20 (CI uses stable 1.98.1). All with RUSTFLAGS=-Dwarnings CARGO_INCREMENTAL=0 CARGO_PROFILE_DEV_DEBUG=0:

cargo fmt -- --check                                                         ok
scripts/test.sh
  library tests: 172 + 3 + 32 + 2 passed
  C API tests: Files=1, Tests=8, Result: PASS
  fuzz/test_case, tools/parser_trace, tools/selectors_ast: cargo check ok
cargo clippy --all --all-targets                                             ok
cargo clippy --all --all-targets --features=integration_test                 ok
cargo clippy --all-targets --manifest-path=c-api/Cargo.toml                  ok
cargo clippy --all-targets --manifest-path=js-api/Cargo.toml                 ok
cargo doc --no-deps --all                                                    ok

robobun and others added 3 commits August 2, 2026 02:58
A handler can return Err(SuspensionRequest) to park the current rewritable
unit on the heap and exit write()/end() with the non-poisoning
RewritingError::Suspended; HtmlRewriter::resume() continues from a
StateMachineBookmark.

- take_owned() on each rewritable unit deep-copies borrowed byte slices so
  the unit outlives the write() stack frame
- Arena::shift advances a start offset instead of memmoving the tail (a
  suspended rewrite re-feeds its unconsumed tail on every resume)
- the pending captured-text flush is hoisted from Dispatcher::handle_tag
  into the lexer actions so every suspension point has a uniform shape
- end_mut() is the non-consuming end() for a rewriter that may suspend in a
  document-end handler

Bun uses this so an async JS content handler can suspend the rewrite and
resume it from a promise reaction instead of spinning a nested event loop
inside write(). See oven-sh/bun#36733.
The upstream ci.yml triggers on push: branches: [master] only, so an
additive bun.yml runs cargo test --features=integration_test on pushes/PRs
to bun without touching ci.yml (keeps rebases onto upstream clean).
…epth

The selector-VM open-element stack had two per-token scans that go
quadratic (or worse) on pathological nesting:

  * Stack::pop_up_to did a full rposition() over the open stack for
    every end tag. A stray end tag that matches nothing still walked
    the whole stack, so N unclosed opens plus N stray closes was O(N^2):
    "<a></b>".repeat(100_000) (~700 KB) burned ~25s of CPU.

  * try_exec_hereditary_jumps_without_attrs walked every ancestor and
    re-executed its hereditary_jumps for every start tag. For a selector
    like "div div" over N nested <div>s every item carries the same
    address range, so the walk is O(depth) per tag and O(N^2) overall;
    k-arm chains ("div div div") compound to O(N^k).

Both paths are reachable from HtmlRewriter with any element-handler
registration, so untrusted HTML can pin a CPU core via sub-MB input.
Memory stays linear so max_allowed_memory_usage does not bound it.

Changes:

  * Stack now keeps open_name_counts: HashMap<LocalName, usize> so
    pop_up_to rejects a name not on the stack in O(1). When the name is
    present the rposition scan length equals the subsequent drain
    length, so total scan work over a document is bounded by total
    pushes.

  * Stack now keeps active_hereditary_jumps: Vec<(AddressRange, usize)>,
    the distinct hereditary-jump ranges currently contributed by any
    open item, each paired with the shallowest depth that introduced it.
    Descendant matching iterates this flat list (bounded by program
    size, not depth) and pop_up_to prunes it by depth. The per-item
    has_ancestor_with_hereditary_jumps flag and the
    HereditaryJumpPtr.stack_offset resume coordinate are removed.

  * LocalName gets a manual Hash impl that ASCII-lowercases the Bytes
    arm so it agrees with the existing case-insensitive PartialEq.
    Without this, keying a HashMap on LocalName is unsound for any name
    that falls off LocalNameHash (custom elements, >12 chars, chars
    outside a-zA-Z1-6). This also fixes the same latent contract
    violation in TypedChildCounterMap.

Re-executing the same address range against the same start tag was
already a no-op after the matched-id set union in add_execution_branch,
so dedup-once execution is observationally equivalent.
@robobun

robobun commented Sep 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

  • Reproduced: on ci: pin workflow actions to full commit SHAs #7 (actions pinned, nothing else), Test fails in Run Rust tests and Clippy and format fails in Check format (run 34774752554).
  • CI on this PR: all four checks pass (run 34774752904). In Clippy and format, Check format, the four clippy steps and Check docs succeed, and Check semver is skipped. In Test, Run Rust tests and Run JS tests succeed.

robobun and others added 4 commits September 13, 2026 18:28
The oven-sh organization only allows actions that are pinned to a
full-length commit SHA. The workflows use tag refs, so every job on
this repository fails at "Set up job" in about 2 seconds:

  The actions actions/checkout@v4, dtolnay/rust-toolchain@stable, and
  actions/cache@v4 are not allowed in oven-sh/lol-html because all
  actions must be pinned to a full-length commit SHA.

Pin each `uses:` in bun.yml, ci.yml and semgrep.yml to the commit that
its tag points to today. The versions do not change.

dtolnay/rust-toolchain is the exception. Its README requires a pinned
commit to be in the history of `master`, because other commits get
garbage-collected. The `stable` rev is one commit off `master` that only
makes `stable` the default of the `toolchain` input. So pin the `v1`
tag, which is on `master`, and pass `toolchain: stable`. Apart from that
default, action.yml at `v1` is identical to action.yml at `stable`.

Co-authored-by: SUZUKI Sosuke <sosuke@bun.com>
With the workflows actually executing again, -Dwarnings turns up three
pre-existing issues on the bun branch: rustfmt drift in
selectors_vm/mod.rs, an unused `Cell` import in the suspension tests, and
an elided lifetime in the `text!` macro's type hint
(elided_lifetimes_in_paths). No behaviour change.

(cherry picked from commit eeea10d)
…d bound

The suspension change made UserData::set_user_data require `Send` data;
the C API still passed the raw `*mut c_void`, so lol_html_c_api no longer
compiled (hidden while CI could not start). Store it in a `UserDataPtr`
newtype that is `Send` -- the C API only stores and returns the pointer,
and thread-safety of what it points to has always been the C caller's
contract. c-api clippy and the c-tests suite pass again.

(cherry picked from commit 4afc2ca)
cargo-semver-checks compares against the published lol_html of the same
version. The bun branch intentionally adds API on top of 2.7.2
(RewritingError::Suspended from the suspension work) and is not published,
so on this fork the check can only fail; keep it for cloudflare/lol-html.

(cherry picked from commit 021637b,
without the mention of lol_html::transform, which is not on bun yet)
@robobun

robobun commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

bun moved to the v3.0.1 base (e1a306d), so this branch does not merge any more. #10 now carries the pin commit of #7 on the new base, and #7 is closed.

The defects this PR fixes are still on bun. With the actions pinned, Test and Clippy and format fail on them in run 35728372762. I checked the three commits of this PR on top of #10, locally, with stable 1.98.1 and the environment of ci.yml:

  • 00871c5 is still needed. Drop its rustfmt hunk for src/selectors_vm/mod.rs. The new base has that code from upstream, already formatted.
  • 9a16a33 is still needed. It applies cleanly.
  • baeee5f is not needed any more. RewritingError is #[non_exhaustive] since v3.0.0, and cargo semver-checks check-release --package lol_html --default-features passes on the new base (223 pass, 31 skip).

With the first two commits, cargo fmt -- --check, the four cargo clippy steps, cargo doc and scripts/test.sh pass. I did not run the wasm-pack JS tests.

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