Davis Rollman

tests/core: pytest → python_unittest Summary: # Stack Context pytest is unsupported at Meta. The buck2 e2e test harness (`fbcode/buck2/tests/e2e_util`, used through `buck_e2e.bzl`) ran its ~500 test modules as `python_pytest` targets. This stack moves the harness and every module on it to `python_unittest`, then deletes the pytest support: a harness that runs tests as `unittest` methods (D124484084), the modules converted in four groups (D124484085 `tests/core`, D124484086 the rest of `buck2/tests`, D124484087 the agentcloud guards, D124484088 everything else), and the removal of the pytest path (D124487800). Test bodies are untouched; test ids change from `test_x.py::test_y[case]` to `pkg.test_x.XTest.test_y_case`. `buck2_core_tests` makes `python_unittest`s, and the 322 modules under `tests/core` are converted with `tools/pytest_to_unittest`: each module's tests move into a `<Module>Test(BuckTestCase)` class and gain `self`; `pytest.mark.parametrize`/`pytest.param` → `parametrize`/`param`; `pytest.raises` → `self.assertRaises` (`assertRaisesRegex` for `match=`, `.value` → `.exception`); `pytest.skip` → `self.skipTest`; `pytest.mark.xfail` → `unittest.expectedFailure`. Done by hand: - Modules that generated tests with `globals()[name] = ...` (`build/test_build_report*.py`, `build/test_artifact_path_sketch.py`, `build/test_build_modifiers_report.py`, `completion/*`, `errors/test_command_report.py`, `errors/test_formatting.py`, `test/test_build_report.py`, `test/test_modifiers.py`): the factory returns the decorated method and the class body assigns it, `test_x = build_report_test("test_x", [...])`. `completion_test` is one method parametrized over the shells; a scenario with a bash-specific expectation gets a `test_x_bash` attribute beside `test_x`, so the test ids stay what they were. - `build/actions/test_http_proxy_env.py`: the autouse `monkeypatch` fixture is a `setUp` that patches `os.environ`. - `build/test_artifact_path_sketch.py`, `build/test_detailed_aggregated_metrics.py`: `pytest.approx` → `helper.utils.approx`. - `build/test_modify.py`: `test_modify_genrule_notify` called `test_modify_genrule`; the shared body is a helper. - `docs/test_docs.py`: `test_docs_fail_with_invalid_patterns` was `xfail`ed in 2022 and passes today; `expectedFailure` would fail it for passing, so the mark is gone. Test ids change shape (see the stack context), so test history and by-name disablement start over. A bare `assert` loses pytest's operand introspection: a failure prints `AssertionError` alone unless the assert carries a message. Reviewed By: IanChilds Differential Revision: D124484085 fbshipit-source-id: 3110efb82913b75e1b919d54168e93dc3440c29d

tests/e2e, tests/meta_only, the bxl_test and check_dependencies_test templates: pytest → python_unittest Summary: # Stack Context pytest is unsupported at Meta. The buck2 e2e test harness (`fbcode/buck2/tests/e2e_util`, used through `buck_e2e.bzl`) ran its ~500 test modules as `python_pytest` targets. This stack moves the harness and every module on it to `python_unittest`, then deletes the pytest support: a harness that runs tests as `unittest` methods (D124484084), the modules converted in four groups (D124484085 `tests/core`, D124484086 the rest of `buck2/tests`, D124484087 the agentcloud guards, D124484088 everything else), and the removal of the pytest path (D124487800). Test bodies are untouched; test ids change from `test_x.py::test_y[case]` to `pkg.test_x.XTest.test_y_case`. The rest of the modules under `buck2/tests` (115: `e2e`, `meta_only`, `select_type_params`, and the two `e2e_util` templates behind `bxl_test` and `check_dependencies_test`) are converted with `tools/pytest_to_unittest`, as `tests/core` was. The `buck2_e2e_test.use_unittest` package value in `buck2/tests/PACKAGE` switches every `buck2_e2e_test` under it to `python_unittest`. `bxl_test` and `check_dependencies_test` pass `use_unittest = True` themselves, since their template modules are the converted ones whichever package the target lives in. Users of `buck_e2e.bzl` outside `buck2/tests` stay on pytest until D124484088. Done by hand: - `e2e_util/test_bxl_check_dependencies_template.py`: the three flavors' list-argument helpers are one module-level `pass_list_arg`, so the `if FLAVOR == ...` chain in the class body holds tests only. - `e2e/test/test_testname_formatting.py`: the test was `testname_formatting` (collected on the `test` prefix); it is `test_name_formatting`, and its dead `pytest.xfail` branch is a `skipTest`. - `meta_only/e2e/rules/cxx/test_cxx.py`: `cxx_link_test` wrote a test per target into `globals()` from a loop; it is one method parametrized over the targets. - `meta_only/e2e/rules/link_groups/test_link_groups.py`: `pytest.param` in module-level lists. `meta_only/e2e/rules/nodejs/test_nodejs.py`: `pytest.fail` in a helper. Reviewed By: IanChilds Differential Revision: D124484086 fbshipit-source-id: 21c9bcc0ced3ab8a69bbb09a59d9a05bf070c6a1

e2e harness: `BuckTestCase`, `buck_test` as a method decorator, `parametrize` Summary: # Stack Context pytest is unsupported at Meta. The buck2 e2e test harness (`fbcode/buck2/tests/e2e_util`, used through `buck_e2e.bzl`) ran its ~500 test modules as `python_pytest` targets. This stack moves the harness and every module on it to `python_unittest`, then deletes the pytest support: a harness that runs tests as `unittest` methods (D124484084), the modules converted in four groups (D124484085 `tests/core`, D124484086 the rest of `buck2/tests`, D124484087 the agentcloud guards, D124484088 everything else), and the removal of the pytest path (D124487800). Test bodies are untouched; test ids change from `test_x.py::test_y[case]` to `pkg.test_x.XTest.test_y_case`. The harness learns to run tests as `python_unittest`s. Nothing changes for the pytest modules yet. - A test module becomes a `BuckTestCase` subclass (an `IsolatedAsyncioTestCase`); its tests are `async def test_*(self, buck: Buck, ...)` methods. - `buck_test(...)` on a method does what the pytest `buck` fixture did: runs the method with a `Buck` on a fresh daemon, and a fresh `tmp_path` when the method takes one. `env`, `nobuckd` and `windows_cmd_option` work as before. A sync method that takes neither is left alone, so `buck_test(skip_for_os=...)` still works on a plain `unittest.TestCase`. - `parametrize(argnames, argvalues, ids=)` and `param(..., id=)` replace `pytest.mark.parametrize`/`pytest.param` with the same arguments. `BuckTestCase` expands a parametrized method into one `test_x_<id>` method per case when the class is created; stacked decorators run the product, innermost first, as pytest did; an empty `argvalues` leaves one skipping test. - `expect_failure` no longer needs `pytest.raises`; `helper.utils.approx` replaces `pytest.approx`. `buck_fixture` takes the test id for its isolation prefix instead of reading `PYTEST_CURRENT_TEST`. - `buck_e2e_test(use_unittest = True)` / `buck2_e2e_test(use_unittest = True)` make a `python_unittest`. Static listing is off for them: it reads names off the source and would miss the expanded parametrized cases. - `tools/pytest_to_unittest` is the libcst codemod the later diffs use: it wraps a module's tests into the class, adds `self`, and rewrites the pytest marks and `pytest.raises`/`fail`/`skip`; what it cannot handle it reports. ___ landed-with-radar-review Differential Revision: D124484084 fbshipit-source-id: c3bf98ed909f336d3cd4feb0a3e70952a261208f

Report every buck1 #flavor use by category, with a fix hint Summary: # Stack Context Buck2 still accepts buck1 `//pkg:target#flavor` labels through a compatibility shim that maps a few known flavors to subtargets and drops platform flavors. This stack rewrites every remaining use in fbsource to the buck2 spelling, then makes `#flavor` a hard error and deletes the shim. # This diff Today the shim logs one soft error (`platform_flavor`) that names only `name#flavor`, so the remaining usages cannot be attributed or fixed from logs. The mapping is unchanged; the reporting is split into three categories (`platform_flavor`, `legacy_flavor_subtarget`, `legacy_flavor_unrecognized`), given a low-cardinality logview key, and the message now carries the full label and the replacement to write (`[shared]`, "remove it", or "unsupported"). New e2e test `test_buck1_flavors.py` covers each category. Nothing is user-visible yet: the new categories are quiet. Reviewed By: IanChilds Differential Revision: D124524381 fbshipit-source-id: a5e18b8ba6796902d4a0108b182a0714885d7f3c

Chunk stdout partial results so a huge package can't kill the stream Summary: `buck2 targets --streaming` formats each package into one `String` and writes it with a single `write_all`. `StdoutPartialOutput` wraps a 16 KiB `BufWriter`, but `BufWriter` passes writes larger than its capacity straight through, so `WriterWrapper::write` turned the whole package into a single `StdoutBytes` partial result, i.e. a single gRPC message. tonic allows messages up to 4 GiB and buck2 raises its configurable limits to `usize::MAX`, but `h2` refuses to send a single DATA payload of 2^31-1 bytes or more (`UserError::PayloadTooBig`). hyper propagates that error out of its body-sending future, which drops the `SendStream`, and h2 then sends `RST_STREAM(CANCEL)`. The client sees only `Reset(StreamId(3), CANCEL, Remote)` / "Buck daemon event bus encountered an error" and the daemon logs nothing. `--keep-going` can't help because the failure is in the transport after evaluation already succeeded. This is what broke the BTD v1 base-graph jobs once `fbcode//pyper_models/tp4fb/ads:` crossed ~2 GiB of JSON output (S715091, T291606076). Fix: cap each `StdoutBytes` emitted by `WriterWrapper::write` at 16 MiB, returning a short write so `write_all` loops. The existing `BUCK2_DEBUG_RAWOUTPUT_CHUNK_SIZE` override is shared with the stderr writer (whose 1 MiB default is unchanged) so tests can force tiny chunks on both paths. 16 MiB rather than 1 MiB because jemalloc returns allocations at or above its 8 MiB `oversize_threshold` to the OS immediately on free; see the test plan for the measurements behind that choice. This fixes every command that writes through `StdoutPartialOutput` (targets, uquery/cquery/aquery, audit, starlark, bxl, ...), and as a side effect the client no longer has to buffer a whole package's output as one message (client peak RSS on `targets --streaming fbcode//folly/...` drops from ~4 GiB to ~0.5 GiB). Reviewed By: Wilfred, mzlee Differential Revision: D123515148 fbshipit-source-id: 4d61af5d03194206cf9de5b84536d3d8f90d186d

oss: fmt.py formats Rust and Starlark as arc f does Summary: Stack goal: let GitHub contributors format buck2's Rust and Starlark exactly as `arc f` does, so imported pull requests no longer need to be commandeered and reformatted. This diff is the script and the GitHub check; the configerator diff D125022466 exports its config inputs, and the next two diffs mirror the check internally. Pull requests arrive formatted by whatever the contributor had, so the importer commandeers each diff and runs `arc f`. `python3 fmt.py` does the Rust and Starlark part of that formatting on GitHub, and the new "Format" workflow checks it on every pull request. Rust goes through the nightly `rustfmt` that `RUSTFMT` pins (`.rustfmt-version`, now exported next to `rust-toolchain.toml`), installed through `rustup` on first use; `cargo fmt` would use the toolchain's `rustfmt`, a different version. `rustfmt.toml` gains the one setting (`format_code_in_doc_comments`) the lint engine's `tools/rust/ossconfigs/rustfmt.toml` had and this copy did not. Starlark goes through `tools/starlark_fmt`, built from the tree with `cargo`, with the lint engine's config, which the export now carries to `tools/starlark_fmt/config.json`. File selection and the `generated`/`nolint`/`lint-ignore-every` skips mirror the `RUSTFMT` and `BUCKFORMAT` rulesets. The export rewrites `oss-disable`/`oss-enable` lines (`X # oss-disable` -> `# oss-disable[end= ]: X`), and the formatters do not know the rewritten shape: run on the export directly, `starlark_fmt` wants to change 42 files that are clean internally. So each file is formatted in its internal form -- the rewrite undone (a port of codesync's `ConditionalLinesFilterMutator`), formatted, redone -- which makes the result exactly what the file will be once imported and formatted here. `--check` prints a unified diff and the command to fix it. The check is its own workflow: it is independent of the build and takes about a minute, so it fails fast. Needs the configerator diff that exports `.rustfmt-version` and `tools/starlark_fmt/config.json` to land first (the mapping is in `opensource/shipit_config/facebook/buck2.cconf`), plus ShipIt's fixup for the two files it then lists; until then the "Format" check fails with "`tools/starlark_fmt/config.json` is missing". Reviewed By: ndmitchell Differential Revision: D125022522 fbshipit-source-id: fba89436a395adc0e92c3a1fb92e21749401bf86

Reference registered static heaps by StaticHeapId Summary: ## Problem The OSS `linux-build-and-test` job fails on `pagable::tests::test_globals_roundtrip`: ``` cannot serialize the inner value of a PartialPagableArc backed by DataKey(..) ``` The failure is test-order dependent. `test_bcinstrs_def_round_trip_via_module` pages a module graph through real storage; that graph transitively references the process-wide `REGISTER_LIST_STATICS` static heap, and `page_out_item` stamps a `DataKey` on its shared `PartialPagableArc`. Any later inline (testing-serializer) serialization of a graph referencing the same static heap — `test_globals_roundtrip` reaches it via `register_list` — then fails in `serialize_inner`, which refuses stamped arcs. Whether CI hits it depends purely on thread interleaving, which is why it reproduces on the 4-core CI runner (and locally with `--test-threads=4` or `=1`) but not on wide devservers. The deeper issue: paging a static heap's body is dead weight. Every value allocated in a `globals_static!` / `methods_static!` heap already serializes as a `StaticValueId` and resolves to the live value on page-in, so the paged body is never consulted — but paging it mutates process-wide state (`DataKey` stamp) that couples otherwise independent serialization consumers, and page-in reconstructs a shadow copy that can collide with the live heap under the same `HeapRefId` in a deser scope. ## Fix Complete the static-value design at the heap level: `FrozenHeapRef` gains a wire tag, and references to heaps registered through `StaticHeapEntry` serialize as a dense `StaticHeapId` (tag 2) instead of going through the pagable arc mechanism. Ids are assigned by enumerating the `StaticHeapEntry` inventory in deterministic (file, line) order — the same same-binary index-order assumption `StaticValueId` already makes — so serializer and deserializer in the same binary always agree. Deserialization resolves the id back to the live heap and publishes its graph in the deser scope. Static heaps are therefore never paged, never stamped, and never shadow-copied. Heap names cannot serve as the reference (and `HeapRefId` is a name hash): distinct static heaps can legally share a name — every `#[starlark_module]` expands to a `globals_static!` named `<module_path>::<FN>_STATICS`, so two function-local `fn module` helpers in one file collide. Registration-site ids sidestep the ambiguity entirely, so every static heap — same-named or not — takes the by-id path. (V1 of this diff referenced heaps by name and had to leave same-named heaps on the paged-arc path; this follows nero's suggestion to mirror the `StaticValueId` scheme instead.) The `FrozenHeapRef` wire format change (bool → tag byte) invalidates previously paged data; paged storage is a same-binary cache, matching the existing `StaticValueId` index-order assumption. ## Attribution The stamp-then-refuse symptom dates to D114793220; the same test pair previously failed with "Frozen heap is already registered with a different StarlarkSerState" at least back to D114793219. The colliding test arrived in D108842734. The Aug 18 landings only shifted CI thread scheduling enough to surface it. Reviewed By: Nero5023 Differential Revision: D116795644 fbshipit-source-id: 7e2414c538e0f0eed0269b6a376d761c2e6af928

report the per-thread chunk cache to allocative Summary: Each evaluation thread keeps a small cache of leftover heap chunk parts so the next heap it builds can reuse them. Those parts aren't reachable from any heap, so `buck2 debug allocative` has never seen them. On an adfinder dev-lg daemon that was 89.8 MB of memory with no owner in the profile. This diff adds an allocative root, `cached_chunk_parts`, that reports the total length of everything parked in those caches. Each cache owns an atomic counter that it updates on store and fetch. A process-wide registry holds weak references to the counters and sums them at profile time, pruning entries whose cache has gone away. Only the owning thread ever writes its counter, so the hot path is a relaxed store with no locking, and the counters are cache-line padded so threads don't false-share. A counter zeroes itself on drop, so a profile that races with thread teardown can't report bytes that were already freed. The root is registered lazily the first time a thread creates its cache rather than with `#[allocative::root]`, because that macro's `ctor` expansion needs a direct dependency this crate doesn't otherwise have. The reported number is a lower bound. It counts part lengths, not the full power-of-two allocations those parts keep alive once their sibling parts have been freed. This is observability only: it doesn't change what gets cached and doesn't free anything. A/B against the same stack with the counter removed showed no measurable memory or CPU cost either way (table in the test plan). Reviewed By: JakobDegen Differential Revision: D116711556 fbshipit-source-id: 77e5b6a1f21ec77b4eab7f765979de345d52abd6

Isolate write-hot from read-hot words within table shards Summary: D115652300 stopped different shards of `ShardedLockFreeRawTable` from sharing cache lines, but two write-hot/read-hot conflicts remained *within* each shard: - Every insertion acquires and releases the shard's `write_lock` (two RMWs), and that lock word sat on the same cache line as `current` — the table pointer every lookup must load. - Every insertion increments the table's `size` counter, which sat on the same cache line as the `entries` fat pointer every lookup must read to reach the buckets. In both cases insertions keep invalidating a line that all concurrent lookups (and insert probes) on the shard immediately re-fetch, so a hot shard degrades to coherence-miss speed even though lookups are wait-free. Give each of those words its own 128-byte block (`IsolatedCacheLine`, covering the 64-byte line plus its adjacent-line prefetch buddy), add layout regression tests, and add `:bench`, a multithreaded throughput benchmark for the sharded table. Median Mops/s over 6 interleaved runs of `:bench` (`fbcode//mode/opt`, 96-core host) at the production shard count of 64, D115652300 → this diff: | regime | 1 thread | 16 threads | 48 threads | 96 threads | | --- | --- | --- | --- | --- | | `mixed` (unique inserts + lookups) | 8.6 → 8.6 | 25.9 → 32.9 (+27%) | 50.4 → 72.3 (+43%) | 62.8 → 84.9 (+35%) | | `insert_unique` | 7.2 → 7.4 | 19.9 → 24.1 (+21%) | 25.3 → 29.0 (+15%) | 32.8 → 36.9 (+12%) | | `lookup_hit` (pure reads) | 21.1 → 22.4 | 292 → 293 | 802 → 850 | 1027 → 1174 | Relative to the pre-D115652300 layout, `mixed` at 96 threads is 2.0x overall (42.6 → 84.9). Single-threaded throughput is unchanged and the pure-read control regimes barely move, so the win comes specifically from removing the false sharing rather than from a general code change. One honest caveat from end-to-end validation: the TD-shaped shared-interner scan (`fbsource//arvr/...` at `-j 180`) did not improve beyond D115652300 alone — with both diffs it ran 39-50s wall across four runs, within padding-only's spread. After the layout fixes, that workload's remaining shared-interner cost was dominated by the label `ThinArc` refcount bumps on lookup hits — removed entirely by the label-interning diffs above this one. This diff is a core-structure fix for the table's many shared users rather than an endpoint win on its own. Cost: each padded shard grows from 128 to 256 bytes and each table generation's header from 32 to 256 bytes — a few KiB per sharded table. The benchmark also shows the same `mixed` workload at 256 shards reaches 168.6 Mops/s (vs 84.9 at 64 shards). A 256-shard version of the hot tables was tried and measured end-to-end neutral on real workloads (interning is not a large enough slice there), so the production shard count stays at 64; the microbenchmark headroom is on record if a more contended workload ever appears. Full-stack measurements (adfinder wall/CPU/peak RSS, hardware counters, and the fleet abtests of both the analysis and TD-scan flows) are in D115906158, the stack top. Reviewed By: JakobDegen Differential Revision: D115657342 fbshipit-source-id: 594253164bf150ece069a8fc5a242c1c92fcc507

shrink retained bytecode side tables Summary: Every compiled function keeps a few side tables alive for as long as it exists: the per-instruction spans used for error messages, the statement-location table, and the local names. Two things made them bigger than they needed to be. The tables were built by pushing into `Vec`s and never trimmed, so they carried doubling slack. And every instruction's slow arg embedded a `Vec<FrameSpan>` of "extra" spans that only `DictNPop` ever populates (one per key, so a duplicate or unhashable key error can point at the key); for every other instruction it was 24 bytes of empty vector header. This diff drops that field. Dictionary key spans now live in a single `dictnpop_spans` array on the function's `End` instruction, and each `DictNPop` carries a `u32` offset into it. The writer appends a dictionary's key spans when it emits the instruction, which happens after any nested dictionaries have been compiled, so nested literals get their own disjoint ranges. On the error path the instruction already knows it is the faulting one, so it walks to `End` (the same walk the existing span lookup does) and indexes the array. `finish` also shrinks the slow-arg and statement-location vectors before they're retained. This is the layout jtbraun suggested in review. `DictNPop` grows by 8 bytes for the offset, visible in the updated golden; what it replaces was 24 bytes per instruction plus a heap allocation per dictionary literal. Against the parent diff, retained jemalloc Allocated drops 3.8% on a floor daemon and 1.6% on adfinder, with CPU flat (test plan). Reviewed By: jtbraun Differential Revision: D116723107 fbshipit-source-id: 5dddaeba4a82f9cb18b6d25be4b1497a6ce1f010