Repository navigation
Conversation
walk_yaml_mapping (dotted key paths, 256 frames) and scan_yaml_for_infra_bindings (512 nodes) kept their pending nodes in fixed-size stacks and stopped pushing once full. A mapping with more than 256 nested sibling mappings lost the full key_path of every leaf past the cap: `k299.url` was only recorded as the shorter path that the walk of a nested block emits on its own. The infra Route pass decides on those paths. A `data:` payload block with 300 entries lost `data.svcN.webhook_url` for N >= 256, the payload deny never saw those values, and 44 of them were minted as infra Route nodes. Both walkers now start on the same-sized on-stack chunk and grow through cbm_walk_stack_reserve / cbm_walk_stack_release (DeusData#2375). If the stack cannot grow, the failure is logged as extract.walk_stack_alloc_failed and the walk stops, instead of truncating silently. Part of DeusData#2388. Signed-off-by: Gaurav Dubey <[email protected]>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you very much for this, Gaurav - a careful and honest piece of work. You found a second walker with the same defect that the #2388 list had missed (walk_yaml_mapping), traced it to its real consequence (payload values under data: being minted as infra Routes once the key-path walk dropped frames), and fixed both walkers with the existing cbm_walk_stack_reserve helper from #2375. Main had moved since your CI run (another change touched the same test file), so we merged current main into your branch (one signed-off merge commit, 50ed9c3; your commit is untouched) and CI is re-running. If you push to this branch again, please pull first. We will merge once the fresh checks are green. |
|
Thanks for the review and for merging main into the branch. Noted on 50ed9c3: I'll pull before any further push. Happy to adjust anything if the fresh checks turn something up. |
DeusData
added
bug
Signed-off-by: Martin Vogel <[email protected]>
|
Thanks again, @gaurav0107. Several other pull requests landed on main this evening, including changes in files this one also touches, so we merged current main into your branch once more (one signed-off merge commit, abdd281; your changes are untouched). That keeps the required checks honest against today's tree; we will merge once they are green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you for your patience, @gaurav0107. CI went green on this branch, but tonight's merges on main touched tests/test_extraction.c, which this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, 8012a75; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again. |
|
Merged as e425666 - thank you, @gaurav0107! The YAML walkers now grow their stacks instead of stopping at a fixed size, so deeply nested YAML keeps all of its content in the graph. Thank you for picking up another piece of #2388 and for your patience. |
What does this PR do?
Part of #2388. It fixes the
extract_unified.csite in the issue's list,scan_yaml_for_infra_bindings(a 512-node stack), and the walker next to it that has the same defect but isn't in the list:walk_yaml_mapping, the dotted key-path walk (a 256-frame stack). The LSP sites,extract_imports.cand the Nix helper are left for separate PRs, so the issue stays open.walk_yaml_mappingpushes one frame for each nested mapping value. Once 256 frames were pending it stopped pushing. In a mapping with more than 256 keys whose values are mappings, every entry past the cap lost its fullkey_path. Only the shorter path survived, the one the walk of the nested block emits by itself (urlinstead ofk299.url).The infra Route pass decides on those paths.
infra_payload_keydenies a URL underdata:,stringData:orbinaryData:by its full path (#521), and with deny-wins-by-value one denied ref is enough to keep the value from being minted. With 300 entries underdata:, entries 256 to 299 had nodata.…ref left, so nothing denied them, and theirwebhook_urlvalues were minted as infra Route nodes.Both walkers now use
cbm_walk_stack_reserve/cbm_walk_stack_releasefrom #2375. Each starts on an on-stack chunk the same size as the old cap, so ordinary files never allocate. Wider trees spill to the memory core (class EXTRACT). If the stack cannot grow, the failure is logged asextract.walk_stack_alloc_failedand the walk stops, instead of truncating silently. Traversal order is unchanged.Evidence
Machine: Amazon Linux 2023, x86_64, clang 15.0.7. gcc here has no
libasan, so the sanitizer build uses clang.extraction: yaml_key_paths_wide_mapping_exceed_old_stack_cap(300 keys, eachkN: {url: …}; everykN.urlref must exist)lang_contract: contract_edge_no_infra_routes_from_wide_payload_block_issue2388(300 entries underdata:; no infra Route)minted 44 infra routes (expected 0)extraction: yaml_infra_bindings_wide_sequence_exceed_old_stack_cap(600-item sequence of{topic, push_endpoint})The third test does not fail on main, and I don't want to imply it does. The unified cursor walk calls
scan_infra_bindingson everyblock_mapping, and that re-scans each mapping the capped document-level walk dropped. So the 512 cap never cost a binding under the default walk budget. The test pins the invariant now that the cap is gone.For the RED run I put main's
extract_unified.cback under the new tests:End to end with product builds (
make -f Makefile.cbm cbm) of main @ ff28841 and this branch, on a one-file repo (service.yaml), countingMATCH (r:Route) RETURN count(r):service.yamldata:with 300svcN: {webhook_url: https://hN.example.com/hook}h256…h299)hooks:(control: real endpoints)Not verified locally
scripts/lint.sh: cppcheck and clang-format-20 aren't installed here.git diff -U0 upstream/main..HEAD | clang-format-diff -p1(clang-format 15) is empty locally, and thelintCI job (cppcheck + clang-format-20) passed on this PR.Checklist
git commit -s):scripts/check-dco.sh upstream/main..HEADOKmake -f Makefile.cbm test): defaultscripts/test.shvenue run green (see above)make -f Makefile.cbm lint-ci): not run locally; thelintCI job passed (see above)🤖 Opened with Superhuman, an open-source contribution agent.