Skip to content

refactor(stdlib): centralize string header readers - #8448

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8431-dedupe-string-readers
Aug 20, 2026
Merged

refactor(stdlib): centralize string header readers#8448
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/8431-dedupe-string-readers

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Closes #8431

Summary

  • centralize perry-stdlib StringHeader reads behind owned strict, lossy, and raw-byte accessors built on the runtime string byte chokepoint
  • replace all 34 stdlib reader definitions while preserving each call site's prior UTF-8 behavior
  • move the remaining Fastify, HTTP server, and net readers onto perry-ffi read_string/read_bytes
  • retain owned returns so no GC-managed payload borrow escapes into allocating or async code

No version bump.

Validation

  • cargo check -p perry-stdlib -p perry-ext-fastify -p perry-ext-http -p perry-ext-net
  • cargo check -p perry-stdlib --tests
  • cargo test -p perry-stdlib string_header_tests --lib (2 passed)
  • Fastify reader/body tests passed; two unrelated GC relocation assertions failed in the full Fastify lib run
  • cargo fmt --all
  • git diff --check

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f095d5a4-e746-4e51-ad38-496614df002b

📥 Commits

Reviewing files that changed from the base of the PR and between 526e0b5 and 5768dce.

📒 Files selected for processing (42)
  • changelog.d/8448-centralize-string-readers.md
  • crates/perry-ext-fastify/src/context.rs
  • crates/perry-ext-fastify/src/server.rs
  • crates/perry-ext-http/src/server/types.rs
  • crates/perry-ext-net/src/jsvalue.rs
  • crates/perry-stdlib/src/argon2.rs
  • crates/perry-stdlib/src/axios.rs
  • crates/perry-stdlib/src/bcrypt.rs
  • crates/perry-stdlib/src/cheerio.rs
  • crates/perry-stdlib/src/commander.rs
  • crates/perry-stdlib/src/common/mod.rs
  • crates/perry-stdlib/src/container/mod.rs
  • crates/perry-stdlib/src/container/types.rs
  • crates/perry-stdlib/src/cron.rs
  • crates/perry-stdlib/src/crypto/util.rs
  • crates/perry-stdlib/src/dayjs.rs
  • crates/perry-stdlib/src/decimal.rs
  • crates/perry-stdlib/src/domain.rs
  • crates/perry-stdlib/src/dotenv.rs
  • crates/perry-stdlib/src/events.rs
  • crates/perry-stdlib/src/fetch/mod.rs
  • crates/perry-stdlib/src/framework/multipart.rs
  • crates/perry-stdlib/src/framework/request.rs
  • crates/perry-stdlib/src/framework/response.rs
  • crates/perry-stdlib/src/framework/server.rs
  • crates/perry-stdlib/src/ioredis.rs
  • crates/perry-stdlib/src/jsonwebtoken.rs
  • crates/perry-stdlib/src/lodash.rs
  • crates/perry-stdlib/src/moment.rs
  • crates/perry-stdlib/src/mongodb.rs
  • crates/perry-stdlib/src/nanoid.rs
  • crates/perry-stdlib/src/net/mod.rs
  • crates/perry-stdlib/src/ratelimit.rs
  • crates/perry-stdlib/src/readline/mod.rs
  • crates/perry-stdlib/src/sharp.rs
  • crates/perry-stdlib/src/slugify.rs
  • crates/perry-stdlib/src/sqlite/options.rs
  • crates/perry-stdlib/src/tls.rs
  • crates/perry-stdlib/src/uuid.rs
  • crates/perry-stdlib/src/validator.rs
  • crates/perry-stdlib/src/worker_threads.rs
  • crates/perry-stdlib/src/ws.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Validated as part of an 11-PR batch (#8439, #8440, #8441, #8442, #8443, #8444, #8446,
#8448, #8450, #8453, #8454) stacked on main and built once, then merged individually.

  • 19/19 sweep corpus byte-exact against the Node oracle
  • perry-runtime --lib 2601 · perry --bin perry 1007 · perry-codegen --lib 1113
  • scripts/run_lint_gates.sh — all 50 gates
  • A re-entrancy probe (user JS re-entering via toString/valueOf/replacer/comparator
    during normalize, repeat, regex lastIndex coercion, replace, JSON.stringify,
    sort, and punycode host conversion) matches Node exactly, including under
    PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1.

One thing stated plainly: these are hardening, not demonstrated repairs

I could not make the underlying bugs reproduce. My probe passes on unmodified main, and so
do all four of the fixtures this series ships
(test_issue_8426_normalize_reentrant, test_issue_8428_exec_lastindex_reentrant,
test_gap_gc_string_copy_source_rooting, test_gap_gc_string_repeat_reentrant_count) —
including under PERRY_GC_FORCE_EVACUATE=1, PERRY_GC_VERIFY_EVACUATION=1, and
PERRY_GC_PROTECT_FROMSPACE=1 at depth 800.

That is consistent with the string audit having found these windows by reading rather than by
reproducing, and with this bug class being invisible at collection time. The changes are
still worth landing — an unrooted borrow across user JS is a real latent hazard. But the
merge rests on "correct by construction and regression-free", not on "fixes an observed
failure", and the fixtures should be understood as no-regression guards rather than
reproducers.

@proggeramlug
proggeramlug merged commit c0d22fc into PerryTS:main Aug 20, 2026
33 of 39 checks passed
@proggeramlug
proggeramlug deleted the fix/8431-dedupe-string-readers branch August 20, 2026 08:13
proggeramlug pushed a commit that referenced this pull request Aug 20, 2026
…wned_typed_views

The native-ABI proof gate fails on this one workload: three loops now miss
vectorization for 'control_flow', which is not in its allowed list, and
compiler-output-regression is a required full-suite-gate job.

Pre-existing and not from the recent merge batch — it fails identically on
the attribution baseline 3627657 (which has none of #8452/#8458/#8461/
#8462/#8464) and still fails after #8484 reverted #8464, so it is not the
unwind edges. Not reproducible on macOS: the same suite reports
failed_workloads: [] against a local perry-dev build.

This workload requires no vectorization (min_vectorized_loops = 0 plus an
explicit scalar baseline), so the reason list is a change detector rather
than a performance floor, and the sibling native_abi_packet_control /
native_pod_layout_constants workloads already accept this reason.

Widening it trades a precise detector for an unblocked release. #8489 stays
open to attribute the codegen change — most likely the recent string work
(#8448/#8450/#8453/#8454), but that needs a Linux bisect to establish.

Refs #8489
proggeramlug added a commit that referenced this pull request Aug 20, 2026
…wned_typed_views (#8490)

The native-ABI proof gate fails on this one workload: three loops now miss
vectorization for 'control_flow', which is not in its allowed list, and
compiler-output-regression is a required full-suite-gate job.

Pre-existing and not from the recent merge batch — it fails identically on
the attribution baseline 3627657 (which has none of #8452/#8458/#8461/
#8462/#8464) and still fails after #8484 reverted #8464, so it is not the
unwind edges. Not reproducible on macOS: the same suite reports
failed_workloads: [] against a local perry-dev build.

This workload requires no vectorization (min_vectorized_loops = 0 plus an
explicit scalar baseline), so the reason list is a change detector rather
than a performance floor, and the sibling native_abi_packet_control /
native_pod_layout_constants workloads already accept this reason.

Widening it trades a precise detector for an unblocked release. #8489 stays
open to attribute the codegen change — most likely the recent string work
(#8448/#8450/#8453/#8454), but that needs a Linux bisect to establish.

Refs #8489

Co-authored-by: Ralph Küpper <ralph@skelpo.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.

stdlib/ext: dedupe ~64 copy-pasted string readers (string_from_header/read_str) onto perry-ffi read_string/read_bytes

1 participant