Skip to content

Fix remaining panics from #400 in encode_ordinary/_encode_bytes/encode_with_unstable - #587

Open
yanmo42 wants to merge 1 commit into
openai:mainfrom
yanmo42:fix/encode-result-unwrap-panic
Open

Fix remaining panics from #400 in encode_ordinary/_encode_bytes/encode_with_unstable#587
yanmo42 wants to merge 1 commit into
openai:mainfrom
yanmo42:fix/encode-result-unwrap-panic

Conversation

@yanmo42

@yanmo42 yanmo42 commented Jul 30, 2026

Copy link
Copy Markdown

What

Completes the fix for #400 by applying the same ErrEncodeError conversion (already used by encode()) to the sibling encode paths that were still panicking on the same pathological input.

Why

fancy_regex's backtracking engine deliberately returns Err(RuntimeError::StackOverflow) when input drives it past its hard-coded MAX_STACK = 1_000_000 step 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 a PanicException, which is not catchable via a normal except Exception in Python.

#400's original repro — encode(text, disallowed_special=()) — was already fixed upstream in eedc856 (2025-08-08, released in 0.13.0), which rewrote encode()'s inner find_iter loop to return EncodeError instead of unwrapping. That part of #400 is resolved on main today.

What's still broken, verified against current main (08a5f3b): encode_ordinary(), _encode_bytes(), and encode_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 fallible encode_ordinary_native() returning Result. The existing public infallible encode_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 the special_regex.find_from_pos error 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, and py_encode_with_unstable now call the fallible cores directly and map Err to PyValueError, matching the existing py_encode() pattern — so Python callers get a catchable ValueError instead of a panic.

No public Rust signature changes, so this isn't a breaking change for downstream rlib consumers.

Testing

  • tests/test_encoding.py::test_pathological_regex_input_raises — new regression test asserting all three previously-panicking Python entry points raise ValueError on a 2M-space pathological input against cl100k_base. Verified it fails with an uncaught PanicException on unpatched main, and passes here.
  • Rust unit tests deterministically trip the fancy_regex limit and assert Err. The encode() case is a characterization test (already green on main); encode_ordinary_native() is the one that actually regressed.
  • cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, cargo test all 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 main branch that the bug is still present today), and take responsibility for the change.

Closes part of #400 (the encode() portion was already resolved upstream).

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

1 participant