Fix remaining panics from #400 in encode_ordinary/_encode_bytes/encode_with_unstable - #587
Open
yanmo42 wants to merge 1 commit into
Open
Fix remaining panics from #400 in encode_ordinary/_encode_bytes/encode_with_unstable#587yanmo42 wants to merge 1 commit into
yanmo42 wants to merge 1 commit into
Conversation
Issue openai#400 reported that pathological input (a huge whitespace run) makes fancy_regex's backtracking engine exceed its hard-coded MAX_STACK = 1_000_000 step counter. fancy_regex reports this as a *recoverable* Err(RuntimeError::StackOverflow) rather than blowing the native stack, but several encode paths threw that safety valve away by calling .unwrap(), turning it into a panic that crosses the PyO3 boundary as a PanicException. That is not catchable as a normal `except Exception` in Python, so callers can't defend against untrusted input. The original openai#400 repro -- encode(text, disallowed_special=()) -- was already fixed upstream: eedc856 ("Partial sync of codebase", 2025-08-08), released in 0.13.0, rewrote encode()'s inner find_iter loop to return EncodeError instead of unwrapping. So `encode()` no longer panics. The catch is that the sibling paths never got that same treatment and still panic identically on the very same input: - encode_ordinary() (its own find_iter().unwrap()) - _encode_bytes() (delegates to encode_ordinary / encode) - encode_with_unstable() (delegates to encode + encode_ordinary) This change finishes what eedc856 started, applying the exact same Err -> EncodeError conversion to those paths so every user-facing entry point degrades gracefully: - encode_ordinary(): extract a fallible encode_ordinary_native() that returns Result; the public infallible method delegates via .expect(), so its signature and today's panic-on-pathological-input behavior are preserved for pure-Rust callers. - _encode_unstable_native(): same pattern via a fallible _encode_unstable_native_impl(). - encode_with_special_tokens(): swap .unwrap() for .expect() with a clear message (still infallible, Rust-only public API). - encode(): also propagate the special_regex.find_from_pos error. This is defensive only -- the special-tokens regex is an alternation of escaped literals and can't backtrack pathologically -- but it removes the last .unwrap() from the function for consistency. The PyO3 bindings (py.rs) now call the fallible cores directly and map Err to PyValueError, matching the existing py_encode() pattern, so py_encode_ordinary, _encode_bytes, and py_encode_with_unstable now raise a catchable ValueError instead of panicking. No public Rust signature changes, so this is not a breaking change for downstream crate consumers. Tests: - tests/test_encoding.py gains a regression test asserting the three previously-panicking Python entry points now raise ValueError on the openai#400 input. Verified it fails on unpatched main (PanicException escapes pytest.raises) and passes here. - Rust unit tests deterministically trip the fancy_regex limit via the cl100k_base pattern and assert Err. The encode() case is a characterization test (already green on main); encode_ordinary_native() is the one that actually regressed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes the fix for #400 by applying the same
Err→EncodeErrorconversion (already used byencode()) to the sibling encode paths that were still panicking on the same pathological input.Why
fancy_regex's backtracking engine deliberately returnsErr(RuntimeError::StackOverflow)when input drives it past its hard-codedMAX_STACK = 1_000_000step counter — a recoverable safety valve, not a native stack overflow. Several encode paths discarded that safety valve via.unwrap(), turning it into a Rust panic that crosses the PyO3 boundary as aPanicException, which is not catchable via a normalexcept Exceptionin Python.#400's original repro —
encode(text, disallowed_special=())— was already fixed upstream in eedc856 (2025-08-08, released in 0.13.0), which rewroteencode()'s innerfind_iterloop to returnEncodeErrorinstead of unwrapping. That part of #400 is resolved onmaintoday.What's still broken, verified against current
main(08a5f3b):encode_ordinary(),_encode_bytes(), andencode_with_unstable()never got the same treatment and panic identically on the exact same input, since they all ultimately call the same unwrapped regex/encode()paths.How
encode_ordinary(): extracted a fallibleencode_ordinary_native()returningResult. The existing public infallibleencode_ordinary()delegates via.expect(...), so its signature and today's panic-on-pathological-input behavior are unchanged for pure-Rust callers (no breaking change)._encode_unstable_native(): same pattern via a fallible_encode_unstable_native_impl().encode_with_special_tokens(): swapped.unwrap()for.expect(...)with a clear message (Rust-only public API, kept infallible).encode(): also propagated thespecial_regex.find_from_poserror for consistency — defensive only, since the special-tokens regex is an alternation of escaped literals and can't backtrack pathologically.src/py.rs:py_encode_ordinary,_encode_bytes, andpy_encode_with_unstablenow call the fallible cores directly and mapErrtoPyValueError, matching the existingpy_encode()pattern — so Python callers get a catchableValueErrorinstead of a panic.No public Rust signature changes, so this isn't a breaking change for downstream
rlibconsumers.Testing
tests/test_encoding.py::test_pathological_regex_input_raises— new regression test asserting all three previously-panicking Python entry points raiseValueErroron a 2M-space pathological input againstcl100k_base. Verified it fails with an uncaughtPanicExceptionon unpatchedmain, and passes here.fancy_regexlimit and assertErr. Theencode()case is a characterization test (already green onmain);encode_ordinary_native()is the one that actually regressed.cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings,cargo testall clean. Full Python suite: 34/34 passing.Disclosure
I used AI assistance (Claude, via two different models cross-checking each other) to help investigate the root cause and draft this patch. I reviewed the diff, verified the claims (including independently re-confirming against the live
mainbranch that the bug is still present today), and take responsibility for the change.Closes part of #400 (the
encode()portion was already resolved upstream).