diff --git a/crates/bashkit/src/builtins/jq/compat.rs b/crates/bashkit/src/builtins/jq/compat.rs index a4632d879..f10cfad16 100644 --- a/crates/bashkit/src/builtins/jq/compat.rs +++ b/crates/bashkit/src/builtins/jq/compat.rs @@ -15,6 +15,15 @@ //! `truncate_stream`: jq 1.7's definitions (capture groups in `scan`, //! null for an unmatched group, null joins as "", non-strings pass //! through the trim filters). +//! - `join`: jq's own definition folds with `(if . == null then "" else +//! . + $x end) + $elem`, which keeps the accumulator reachable while the +//! right operand is evaluated. A shared accumulator cannot be appended to +//! in place, so every element copied the whole result and `join` was +//! quadratic: 68 s for 300k elements. The definition below coerces the +//! elements first and then folds with the accumulator as the left operand +//! of a single add (`. + ($x + $s)`), which is linear -- 14 s for the same +//! 300k. See knowledge/runtimes/jaq-json-vendor.md for the measurements +//! and the rule; `join_follows_jq` pins the observable behaviour. //! - `halt_error`: jq prints a string as is, null as nothing, and other //! values as JSON plus a newline, then halts (the run loop turns the //! halt into the command's exit code, never a process exit). @@ -83,12 +92,25 @@ def capture(re; flags): match(re; flags) | reduce (.captures[] | select(.name != null)) as $c ({}; . + {($c.name): $c.string}); def capture(re): capture(re; ""); +# `join`: fold with the accumulator as the left operand of ONE add, and with +# the right operand fully parenthesized, or the accumulator is shared at the +# add and every element copies it (quadratic -- see the module docs). The +# leading separator the fold prepends is sliced off at the end; arrays of 0 or +# 1 element never reach the fold, so `["a"] | join(1)` still succeeds like jq, +# and `$p[0] + $x` reproduces jq's error (and its operand order) for a +# non-addable separator as soon as a second element exists. +def _bk_joinstr: + if . == null then "" + elif type == "string" then . + elif type == "boolean" or type == "number" then tojson + else "" + . end; def join($x): - reduce .[] as $i (null; - (if . == null then "" else . + $x end) - + ($i | if . == null then "" - elif type == "boolean" or type == "number" then tojson - else . end)) // ""; + [.[] | _bk_joinstr] as $p + | if ($p | length) <= 1 then ("" + ($p[0] // "")) + else ($p[0] + $x) as $_sep_is_addable + | reduce $p[] as $s (""; . + ($x + $s)) + | .[($x | length):] + end; def _bk_ltrimstr($x): ltrimstr($x); def _bk_rtrimstr($x): rtrimstr($x); def ltrimstr($x): diff --git a/crates/bashkit/src/builtins/jq/tests.rs b/crates/bashkit/src/builtins/jq/tests.rs index 730e3ef82..4fc3e38cd 100644 --- a/crates/bashkit/src/builtins/jq/tests.rs +++ b/crates/bashkit/src/builtins/jq/tests.rs @@ -1316,6 +1316,7 @@ const JQ_BANNED: &[&str] = &[ "JQ_COMPAT_DEFS", "def setpath", "def leaf_paths", + "def _bk_joinstr", "def @tsv:", "def @csv:", "def env:", @@ -1372,6 +1373,17 @@ jq_no_leak!( ); jq_no_leak!(no_leak_iterate_over_null, r#"echo 'null' | jq '.[]'"#); jq_no_leak!(no_leak_add_array_and_number, r#"echo '[1,2]' | jq '. + 1'"#); +// `join` fails inside the compat prelude, so its diagnostics are the ones +// most likely to carry prelude source out to the caller. +jq_no_leak!( + no_leak_join_non_addable_separator, + r#"echo '["a","b"]' | jq 'join(1)'"# +); +jq_no_leak!( + no_leak_join_nested_array, + r#"echo '[[1]]' | jq 'join(",")'"# +); +jq_no_leak!(no_leak_join_over_string, r#"echo '"abc"' | jq 'join(",")'"#); // New: malformed $ARGS / file flags jq_no_leak!(no_leak_invalid_jsonargs, "jq -n . --jsonargs 'not json'"); @@ -2109,6 +2121,51 @@ async fn string_builtins_follow_jq() { ); } +/// `join` is defined in the compat prelude rather than vendored, so its +/// parity with real jq is only as good as that definition. Every case here +/// was captured from jq 1.7, including the wording and operand order of the +/// two error messages: the separator is only ever added once the array holds +/// a second element, so `["a"] | join(1)` succeeds while `["a","b"] | join(1)` +/// reports the accumulated string first. +#[tokio::test] +async fn join_follows_jq() { + let cases = [ + (r#"[1, null, "a", true] | join("-")"#, "\"1--a-true\""), + (r#"[null] | join(",")"#, "\"\""), + (r#"[] | join(",")"#, "\"\""), + (r#"{} | join(",")"#, "\"\""), + (r#"[false] | join(",")"#, "\"false\""), + (r#"["a"] | join(1)"#, "\"a\""), + (r#"["a","b","c"] | join("")"#, "\"abc\""), + (r#"["a","b"] | join("<>")"#, "\"a<>b\""), + ( + r#"["\u00e9","\u00fc"] | join("\u2014")"#, + "\"\u{e9}\u{2014}\u{fc}\"", + ), + (r#"{"a":"x","b":"y"} | join("-")"#, "\"x-y\""), + ( + r#"try ([[1]] | join(",")) catch ."#, + r#""string (\"\") and array ([1]) cannot be added""#, + ), + ( + r#"try (["a","b"] | join(1)) catch ."#, + r#""string (\"a\") and number (1) cannot be added""#, + ), + ( + r#"try ([1,2] | join(2)) catch ."#, + r#""string (\"1\") and number (2) cannot be added""#, + ), + ( + r#"try ("abc" | join(",")) catch ."#, + r#""Cannot iterate over string (\"abc\")""#, + ), + ]; + for (filter, want) in cases { + let out = run_jq_with_args(&["-nc", filter], "").await.unwrap(); + assert_eq!(out, format!("{want}\n"), "{filter}"); + } +} + #[tokio::test] async fn unmatched_group_is_reported_like_jq() { let out = run_jq_with_args(&["-c", "[match(\"(a)(z)?\").captures[]]"], "\"ab\"") diff --git a/crates/bashkit/tests/integration/memory_growth_security_tests.rs b/crates/bashkit/tests/integration/memory_growth_security_tests.rs index b2a2baa88..ca4874a3e 100644 --- a/crates/bashkit/tests/integration/memory_growth_security_tests.rs +++ b/crates/bashkit/tests/integration/memory_growth_security_tests.rs @@ -313,6 +313,21 @@ mod jq { /// `join` appends in place; metering must not turn it quadratic (300k /// joins took over 30 s when every append copied the string). + /// + /// This is the only test here whose guard is *throughput* rather than a + /// cap, so the 600 s budget has to sit between the linear cost and the + /// quadratic one on the slowest build we run. The nightly + /// AddressSanitizer job is what makes that interval narrow, and it caught + /// a real regression: the prelude's `join` shared its accumulator at the + /// add, so every element copied the whole result. 300k elements cost 68 s + /// unoptimized natively -- fine against 600 s, but past it once ASAN's + /// instrumentation was on top (nightly #247). The fold is linear again + /// (14 s for the same 300k), which both restores the headroom and keeps + /// the budget discriminating: a quadratic `join` overruns it under ASAN, + /// a linear one finishes well inside it. Do not shrink the element count + /// to buy headroom -- the quadratic penalty scales with it, so a smaller + /// array would fit the budget either way and the guard would stop + /// guarding. #[tokio::test] async fn ordinary_workloads_still_fit() { let r = run( diff --git a/knowledge/operations/testing.md b/knowledge/operations/testing.md index 7cb4129df..e8ca96836 100644 --- a/knowledge/operations/testing.md +++ b/knowledge/operations/testing.md @@ -207,6 +207,20 @@ Only a test that asserts a timeout sets a short deadline, and it sets it explicitly. Before adding a cap test, confirm the cap is the sole limit that can stop the script. +The same file holds one test whose guard is **throughput**, not a cap +(`ordinary_workloads_still_fit`), and it has the opposite constraint: its +budget has to sit *between* the linear cost and the quadratic one on the +slowest build, so it must be loose enough for ASAN and tight enough to still +fail. That interval is real and it found a real bug -- a quadratic `join` +(68 s for 300k elements unoptimized) passed against 600 s natively and only +overran it under ASAN, which is how nightly #247 surfaced it. Two rules follow: + +- Size a throughput guard from the quadratic penalty, not from convenience. + Shrinking the workload to buy headroom shrinks the penalty with it, and a + small enough array fits the budget whether the code is linear or not. +- Make the linear path fast rather than the budget generous. Raising the + budget to fit a slow linear run also lets a quadratic run through. + ## Public capability parity contract [`contracts/capability-parity.json`](../../contracts/capability-parity.json) is the diff --git a/knowledge/runtimes/jaq-json-vendor.md b/knowledge/runtimes/jaq-json-vendor.md index 5446e3d62..86391d97a 100644 --- a/knowledge/runtimes/jaq-json-vendor.md +++ b/knowledge/runtimes/jaq-json-vendor.md @@ -71,6 +71,30 @@ same rewrite applies to every release: uniquely held string is appended to in place (`. + $x`, `join`). An owner wrapper made every append copy, and `join` over 300k strings went from 0.5 s to over 30 s. +- In-place appending only survives if the accumulator is **uniquely held at + the add**, which constrains how a folding filter may be written, not just + how the meter is built. `Val::add` reuses the left operand's buffer through + `BytesMut::from(Bytes)`, which copies unless that `Bytes` is unique, so a + fold whose accumulator is also reachable from somewhere else in the same + expression is quadratic. Measured in an unoptimized build, 100k -> 300k + iterations (linear would be 3.0x): + + | filter body | 100k | 300k | ratio | + | --- | --- | --- | --- | + | `. + "x"` | 0.28 s | 0.83 s | 2.9x | + | `. + ("," + "x")` | 0.48 s | 1.79 s | 3.7x | + | `. + "," + "x"` | 0.91 s | 5.20 s | 5.7x | + | `. + "x" + "y"` | 1.02 s | 7.49 s | 7.4x | + + Rule: the accumulator appears **once**, as the left operand of a **single** + add, with the right operand fully parenthesized. `(. + a) + b` keeps `.` + alive to evaluate `b`, so the inner add sees a shared buffer and copies it; + `. + (a + b)` does not. Testing the accumulator in a condition + (`if . == null then ... else . + $x end`) shares it the same way. This is + why the prelude's `join` (`builtins/jq/compat.rs`) coerces the elements into + an array first and then folds with `. + ($x + $s)`, instead of jq's own + `(if . == null then "" else . + $x end) + ...` shape: that shape cost 68 s + for 300k elements where the fold above costs 14 s and scales linearly. - Array and object bodies are `Metered>` / `Metered`: charged on creation and clone, re-charged by `resync()` after every in-place mutation, released on drop.