Skip to content

fix(filter): unblock Tests (core-and-rest) — internally tagged recursive enum stalls rustc - #784

Open
ohdearquant wants to merge 2 commits into
ruvnet:mainfrom
ohdearquant:fix/filter-tagged-enum-recursion
Open

fix(filter): unblock Tests (core-and-rest) — internally tagged recursive enum stalls rustc#784
ohdearquant wants to merge 2 commits into
ruvnet:mainfrom
ohdearquant:fix/filter-tagged-enum-recursion

Conversation

@ohdearquant

@ohdearquant ohdearquant commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What this fixes

Tests (core-and-rest) on main does not fail, it runs out of clock. In the most
recent Workspace CI run on main (run 30767906155, head cc602dc2) every other job
is green and that one job ran 21:29:04Z to 01:29:26Z and was cancelled by the
240-minute timeout-minutes. It never reaches the test phase; it is still compiling
when the limit hits.

The compile that never finishes is ruvector-filter's test target. cargo check -p ruvector-filter --all-targets finishes in about 10 seconds; cargo test -p ruvector-filter --no-run on the same tree does not finish. Sampling the running
rustc (three independent samples on main) shows a single thread at ~98% CPU with
39k-60k rustc_infer::infer::relate::generalize::Generalizer /
structurally_relate_tys frames on the stack. It is a type-inference blowup, not slow
codegen.

Cause

FilterExpression is internally tagged and recursive:

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(tag = "type", rename_all = "snake_case")]
pub enum FilterExpression {
    Eq { field: String, value: Value },
    // ...
    And(Vec<FilterExpression>),
    Or(Vec<FilterExpression>),
    Not(Box<FilterExpression>),
}

serde's derive serializes an internally tagged newtype variant by wrapping the
outer serializer in serde::private::ser::TaggedSerializer<S>. When the newtype
payload transitively contains Self, the instantiation is
TaggedSerializer<TaggedSerializer<...>> with no bound, so the compiler walks a type
tower that only stops at recursion_limit. The crate already carried
#![recursion_limit = "2048"], raised to 4096 in April to clear an E0275
(&mut Vec<u8>: std::io::Write overflow while instantiating the serde_json
Serializer). Raising the limit converted a fast error into a long compile; it did not
remove the tower.

Measured on main, one variable at a time:

tree cargo test -p ruvector-filter --no-run
main as-is no completion (killed after the filter rustc held the CPU 150s straight)
only expression.rs's #[cfg(test)] module stubbed out 10s
only test_serialization kept, everything else in that module removed no completion
that test reduced to just serde_json::to_string(&filter) no completion
#[serde(tag = "type")] removed, recursion and tests unchanged 5s

The last row is the isolating one: the recursion is fine, the internal tag is fine, the
combination of an internal tag with a self-referential newtype variant is what the
compiler cannot close.

The same shape is also a runtime error, independent of build time. serde cannot encode
an internally tagged newtype variant whose payload is a sequence:

E::Xs(vec![1, 2])   => Err("cannot serialize tagged newtype variant E::Xs containing a sequence")
E::Inner(Box::new(Leaf { a: 1 })) => Ok("{\"type\":\"inner\",\"a\":1}")

So FilterExpression::And and FilterExpression::Or could never be serialized at all.
Not is not better off, only differently broken: its payload encodes as a map, so the
tag is merged into it and the result carries two type keys, which serde_json then
refuses to read back.

Not(Eq { field: "status" })  =>  Ok("{\"type\":\"not\",\"type\":\"eq\",\"field\":\"status\"}")
round trip                   =>  Err("duplicate field `type` at line 1 column 20")

None of the three logical variants had a working round trip. The existing
test_serialization only covered an Eq value, so nothing caught it.

The change

The three logical variants become struct variants:

And { exprs: Vec<FilterExpression> },
Or  { exprs: Vec<FilterExpression> },
Not { expr: Box<FilterExpression> },

A struct variant serializes its fields directly through the outer serializer, so no
wrapper type nests. The wire encoding of every comparison variant is unchanged
({"type":"eq","field":...,"value":...}), and and/or/not keep their signatures,
so callers that construct through the builders are untouched. ruvector-node,
ruvector-wasm and the doc examples all go through those builders and needed no
changes. Direct pattern matches on the three variants are updated in
evaluator.rs.

With the tower gone the crate builds at the default recursion limit, so the
explicit #![recursion_limit] attribute is dropped rather than lowered.

test_logical_operator_round_trip is added: it asserts the encoded value of a nested
and/not/or expression against the exact expected JSON object, then decodes it and checks
that it re-encodes to the same object. On the previous shape that test fails during
serialization.

Compatibility

This is a breaking change to a public type and should be released as one.

  • Rust callers that construct or pattern-match the tuple forms
    (FilterExpression::And(v), Or(v), Not(b)) must move to the named-field forms.
    The and, or and not builders are unchanged. No code in this repository outside
    ruvector-filter itself constructs or matches the three changed variants:
    ruvector-wasm and the crate's doc examples go through the builders, and the Node
    binding only uses the comparison forms, which are untouched.
  • The JSON encoding of the three logical variants changes, but no previously valid
    document is affected: And and Or could not be produced at all, and a produced
    Not document could not be read back. Comparison variants (eq, ne, gt, gte,
    lt, lte, range, in, match, geo_radius, geo_bounding_box, exists,
    is_null) are byte-identical before and after.

Verification

  • cargo test -p ruvector-filter --no-run: no completion before, 10s after, at the
    default recursion limit.
  • cargo test -p ruvector-filter: 17 tests plus 2 doc-tests pass.
  • cargo clippy -p ruvector-filter --all-targets -- -D warnings: clean. cargo fmt
    applied.
  • cargo check -p ruvector-node --all-targets and cargo check -p ruvector-wasm --features collections: both build.
  • The full core-and-rest shard argument list from ci.yml (--workspace plus its 99
    --excludes) with --no-run: 227 test executables link, zero errors. On main that
    same job on CI never reaches the test phase.

Note on this PR's own CI

Tests (core-and-rest) will not go green on this branch alone. Two other things keep
that job from reporting: its --exclude list never reaches cargo (#786), and
ruvector-delta-index deadlocks in a test once the job gets that far (#787). All three
are needed before the job produces a result rather than a timeout.

`FilterExpression` is internally tagged (`#[serde(tag = "type")]`) and
recursive: `And`/`Or`/`Not` carried `Vec<FilterExpression>` and
`Box<FilterExpression>` as newtype variants. serde's derive routes an
internally tagged newtype variant through `TaggedSerializer<S>`, so a
self-referential newtype variant asks the compiler for
`TaggedSerializer<TaggedSerializer<...>>` with no bound. Building the crate's
test target spins in type inference rather than failing fast; the
`recursion_limit` escalations (2048, then 4096) only moved the wall further
out.

The same shape is a runtime error independent of the build: serde cannot
serialize an internally tagged newtype variant whose payload is a sequence,
so `And` and `Or` values could never be encoded ("cannot serialize tagged
newtype variant ... containing a sequence").

Convert the three logical variants to struct variants (`And { exprs }`,
`Or { exprs }`, `Not { expr }`). Struct variants serialize their fields
directly through the outer serializer, so no wrapper type nests and the
encoding of the comparison variants is unchanged. The `and`/`or`/`not`
builders keep their signatures, so callers constructing through them are
unaffected.

The crate now builds at the default recursion limit, so the explicit
`recursion_limit` attribute is dropped. Adds a round-trip test over a nested
and/not/or expression, which previously failed at serialization time.
…ize note

`test_logical_operator_round_trip` asserted only the root tag and the field
set, so it would still have passed with a nested variant encoded under the
wrong field name. It now compares `serde_json::to_value` against the complete
expected nested object and re-checks that the decoded value re-encodes to the
same object, which pins `{"type":"and","exprs":[...]}`,
`{"type":"not","expr":{...}}` and `{"type":"or","exprs":[...]}`.

The workspace `RUST_MIN_STACK` comment justified itself with
`ruvector-filter`'s `#![recursion_limit]` and the trait-resolution recursion
behind it, both of which this branch removes. The comment now records that,
and records the measurement: `cargo test -p ruvector-filter` builds and passes
with `RUST_MIN_STACK=2097152`, well under the 8 MB default. The setting itself
is left in place, since nothing here measures whether the rest of the
workspace still needs it.
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