Conversation
|
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. |
|
Thank you @pcristin, both for picking this up so quickly and for keeping it focused on the misleading zero. That was exactly the part that hurt us. I built the PR head and ran it against the repro from #2265, plus one extra case. The inbound side works well. I found three rough edges and traced each one to the source, so I hope this saves you some time. Please take whatever is useful. All line links below point to the PR head How I testedLinux x86_64, gcc, export HOME=$(mktemp -d)
B=./build/c/codebase-memory-mcp
R=/path/to/repro # the 9 files from #2265 (+ anidada.js below)
echo "{\"repo_path\":\"$R\",\"mode\":\"full\"}" | $B cli index_repository
P=<project name printed above>
q() { echo "{\"project\":\"$P\",\"function_name\":\"$1\",\"direction\":\"$2\",\"format\":\"json\"}" | $B cli trace_path; }What works
1. Outbound: nested functions inside a factory still report an exact zeroRepro ( import { ayudante } from "./ayudante.js";
export function crearAnidada({ cliente }) {
function interna(id) {
ayudante(id); // resolved → CALLS edge from `interna`
return cliente.buscar(id); // unresolved → recorded with caller `crearAnidada`
}
return { interna };
}The original repro shows the same thing: Why: the two passes disagree on who the caller is.
I checked this with a temporary Possible directions (you know the codebase much better, so these are only ideas):
This is the case that made me open #2265. Our backend builds services with factory functions and injected dependencies, so almost every method we'd trace outbound is a nested function. 2. A resolved call is also recorded as unresolved, twiceIn [{"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"},
{"caller":"…directo.usarDirecto","leaf":"ayudante","start_byte":84,"end_byte":95,"reason":"import_symbol_not_in_registry"}]As a result, Why: the TS LSP runs twice per file, and each run gives a different callee QN for the same site. My trace shows:
Possible fixes:
(Separately, and maybe worth its own issue: the cross-file pass has the correct module QN 3.
|
DeusData
left a comment
There was a problem hiding this comment.
Thank you. This is the follow-up we agreed on in #1682, and persisting unresolved call sites so coverage and trace_path can admit what they do not know is exactly the direction we want. We accept the "unknown" relation value.
@BryanQuiceno, thank you too for that test report. You traced each rough edge to the line that causes it, and that saved us real time. Your three findings overlap with what we were going to ask for, so here they are in one list.
Before it merges:
-
Remove the false
"unknown"s. Two sources drain the signal:- Bryan's finding 2: one call site that resolved (
usarDirecto → ayudante) is also recorded as unresolved, twice, because the per-file and cross-file TS passes use different callee strings. - Inbound matching on any short name: any unresolved site whose leaf matches a visited node's short name marks that trace
"unknown". Common names (get,run,init) would then almost always read"unknown".
Please dedupe coverage rows on
(caller, leaf, start_byte, end_byte), drop an unresolved site whose span already produced a CALLS edge, and link an unresolved site to a traced node only through the caller or a leaf the resolver would actually have considered for it. - Bryan's finding 2: one call site that resolved (
-
Outbound on nested functions must not claim an exact zero (Bryan's finding 1). A factory-built function (
crearAnidada → interna) still reportscallees_total_relation: "eq"while a call is missing. Correcting that"eq"is what this PR is for. Matching the site to the traced function by file and source range, as Bryan suggests in (b), keeps this PR narrow. The deeper fix in the TS LSP walk, setting the enclosing function for nested functions, affects resolved calls too and deserves its own issue and PR, as Bryan proposes. -
Files with unresolved calls must not appear in
skipped[](Bryan's finding 3).add_skipped_summaryshould treatunresolved_callslike the parse-coverage phases, since those files were indexed. -
Real-corpus numbers. Our rule is that a change is shown working on real input before it merges. Please report:
- index time
- peak RSS
- the number and total size of
unresolved_callsrows - the share of
trace_pathcalls that come back"unknown"
Please measure on a Go repository and on the Linux kernel, taken after items 1–3, so they show the signal we will actually ship.
-
A parallel-path test. The two tests use two files, which is below
MIN_FILES_FOR_PARALLEL(50), so only the sequential path runs. Please add a fixture past 50 files, ideally one that forces a result spill, to cover the path most real indexes take. -
yyjson_mut_doc_new(NULL)incbm_pipeline_record_unresolved_callsuses libc's allocator. Please pass the core-backed allocator the rest of the PR already uses.
Bryan has offered to open a PR against your branch with items 1 (dedupe part) and 3, plus tests. That is welcome from our side, and it is your call whether to take it. The 1-based line next to the byte offsets is a good small addition if you have room.
We will call out one behaviour in the release notes on our side: every index built before upgrading reads "unknown" on call traces until it is re-indexed. Also a heads-up: #2294 removes is_test_file() from mcp.c, which this PR calls in new places, so whichever lands second will need a small rebase.
Thank you both. This closes a real honesty gap in the tools.
|
Thank you for keeping the branch current, @pcristin. It saves the next rebase a lot of pain. One heads-up so it doesn't surprise you: #2294 removes |
|
AI-assisted reply on behalf of Pcristin. I’ve reworked coverage capture around the actual call sites. Per-file and cross-file duplicates are collapsed, and a diagnostic is dropped when that exact site emitted a CALLS edge. Spilled results are captured inside the resolver worker before they’re freed. The nested Added sequential, 61-file parallel, and forced-spill regressions, including repeated calls on one line and route registrations. The JSON collector uses the core allocator. Existing indexes need reindexing for coverage version 5. The focused sanitizer suites passed 780 tests. Final changed-line clang-tidy 20 and full CI lint passed; the serialized security fuzz run passed 32/32. The upstream C harness passed all 144 suites: 8222 passed, 0 failed, 8 platform skips. Prometheus v3.0.0 ( On this 4-CPU, 7941 MiB, no-swap machine, full Linux v6.12 ( The default test script remains blocked by the unchanged Scoop version-metadata contract. Existing UBSan warnings in ObjectScript scanners, project listing, and semantic-edge code also remain; the full gate is not claimed as clean. |
|
Thank you, @pcristin. This rework lands nearly everything we asked for:
Thanks, too, for the Prometheus numbers. The one red is our bug, not yours. test-diag, test-lsan-macos and test-unix 2/3 show a 1 KB LeakSanitizer report from One contract decision from the maintainer. On your sample, 121 of 200 outbound traces come back "unknown", because every unresolved call site counts, including stdlib and third-party calls. We'd like outbound "unknown" to count only unresolved sites whose callee name matches an in-project symbol. Then "unknown" means "there may be a project edge we missed", not "this function calls something outside the repository". Inbound is fine as is. We'll take the kernel measurement on our side, since the corpus is here. Also, #2294 is still open: if it lands first, your one |
Signed-off-by: Pcristin <xxxokzxxx@protonmail.com>
Signed-off-by: Pcristin <xxxokzxxx@protonmail.com>
Signed-off-by: Pcristin <xxxokzxxx@protonmail.com>
853192a to
8d80be7
Compare
|
I’ve limited outbound Added regressions for external names, another project's name collision, case-sensitive matching, constructors, and all six container labels. The nested-function fixture now defines a The branch is rebased onto Clang-tidy 20, full CI lint, and the full security audit passed (MCP robustness 32/32). The final MCP suite passed 324 tests with four platform skips; pipeline, incremental and the two affected store suites passed another 609 tests. The default test command still stops at the unchanged Scoop version-metadata contract, and the local sanitizer suites use I’ll pick up your separate fallback-leak fix when it lands. Thanks for taking the kernel measurement. |
…llback handle_trace_call_path first looks the function up by bare name (cbm_store_find_nodes_by_name). find_nodes_generic allocates its initial 16-slot array before stepping the statement and hands it back even when zero rows match. When the bare name misses and the qualified_name fallback then resolves the node, the handler overwrote `nodes` with a fresh one-element array, leaking the empty 1 KB array on every trace_path call made with a qualified name - for the whole lifetime of the daemon. The malloc-failure branch of the fallback lost the same array. Release the zero-row array before the fallback replaces the pointer. The other find_nodes_* callers (get_code_snippet outline, search_code grep classification, detect_changes seeding, Cypher label scans) already free the container on zero rows; the not-found path in this handler did too. Proof (macOS leak lane, Homebrew clang 22.1.8, `make -f Makefile.cbm test-lsan LSAN_SUITES=mcp`): - with the new test, without the fix: 322 passed, then "ERROR: LeakSanitizer: detected memory leaks - Direct leak of 1024 byte(s) in 1 object(s) allocated from find_nodes_generic store.c:2771 <- handle_trace_call_path mcp.c:9176 <- test_tool_trace_call_path_qn_fallback_frees_name_miss", exit 1 (the only leak in the suite). - with the fix: 322 passed, 4 skipped, no leak report, exit 0. The new test pins its own precondition (the bare-name lookup of the qualified name returns zero rows) and that the trace resolves through the fallback, so it keeps exercising this path. LSan (default-on under Linux ASan, test-lsan on macOS) turns any regression red. Found while attributing CI on DeusData#2305. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Preserve unresolved-call coverage alongside upstream route mounts and GraphQL operation identities. Keep the cross-language route helper mutable so emitted CALLS can update coverage.
What does this PR do?
Fixes #2265.
Records unresolved call sites as per-file index coverage, including caller, method leaf, source byte span, one-based line, reason, and the resolver's considered candidate QN.
check_index_coverageexposes those records. Incremental indexing preserves them, and CALLS traces reportunknownwhen relevant unresolved sites or older metadata prevent an exact total.The review amendment reconciles duplicate records by exact call site and removes a diagnostic when that site actually emitted a CALLS edge through any resolver strategy, including route registration. Coverage capture happens while the resolved result is still live, including spilled results. Inbound matching uses the resolver's candidate QN; nested outbound calls use the extractor's enclosing caller. Indexed unresolved files stay out of
skipped[]. The JSON collector uses the core allocator.Nested callers remain distinct from their enclosing functions; the final indexed fixture reports inner
unknownand outereqwhen the unresolved name matches an in-project method. Existing indexes need reindexing for coverage metadata version 5. The deeper TypeScript LSP attribution fix remains separate.The CLI fixture lifetime fix was submitted separately in #2304 and has merged.
Outbound project-symbol amendment
The latest maintainer-requested change limits outbound
unknownto unresolved callee names matching non-container symbols in the same project. File, Folder, Project, Module, Package and Section name collisions do not count. Class/constructor symbols remain eligible. Inbound candidate matching and recorded coverage are unchanged; failed lookups remain conservative.Regressions cover external-only names, another project's name collision, case-sensitive matching, constructors, all six containers, and nested caller attribution with
Client.buscardefined but an untyped receiver.The branch is rebased onto upstream
80eb92a7; both conflicting pipeline suite registrations were retained. PR #2294 remains open. The maintainer owns the separate qualified-name fallback leak fix and full-kernel measurement; the branch can pick up the leak fix once it lands.Current validation:
detect_leaks=0.make -f Makefile.cbm securitypassed, including MCP robustness 32/32.scripts/test.shstopped at the unchanged Step 0x Scoop version-metadata contract. A clean complete default gate is not claimed. The prior full-harness result below applies to the previous published snapshot.Refreshed Prometheus v3.0.0 (
c5d009d57fcccb7247e1191a0b10d74b06295388), full mode, 2 workers, 4096 MiB budget, no forced spill: 23.396 s, 556016 KiB peak worker RSS; 505 unresolved rows / 19006 sites / 3758665 JSON bytes. All 400 sampled function/direction pairs match the previous sample exactly: 43/400 unknown, comprising 1/200 inbound and 42/200 outbound (previously 1 inbound / 121 outbound), with zero errors. The upstream rebase also differs between runs, so no isolated timing improvement is claimed.Current production binary SHA256:
726abe090c53728a603d4c1a60ed7ca72be372a09369c7a741da5f31f727ee09.Previous reconciliation validation (published
853192aa)ASAN_OPTIONS=detect_leaks=0 scripts/run-tests-parallel.sh build/c/test-runner 2: 8222 passed, 0 failed, 8 platform skips, all 144 suites accounted for by the union guard. ASan and UBSan enabled; LeakSanitizer disabled.make -f Makefile.cbm lint-tidy-diff CC=clang-20 CLANG_TIDY=clang-tidy-20: exit 0. Fullscripts/lint.sh --ciwith clang-format 20 and cppcheck 2.20.0, two cppcheck jobs and a build cache: exit 0. Formatting, NOLINT, no-skips policy, and memory-core checks passed.Gate limitations
The default
scripts/test.shstops at the unchanged Scoop version-metadata contract: its 0.11.0 pin conflicts with the newest stable v0.11.0 tag. The full C harness was run separately as described above. Existing UBSan null-argument warnings remain in the ObjectScript scanners, semantic-edge code, and project-listingqsort(records, 0, ...)path. A clean complete default gate is not claimed.Previous pinned corpus measurements (
853192aa)Host: Linux x86_64, 4 reported CPUs, 7941 MiB RAM, no swap. Peak worker RSS uses
/procVmHWM sampled every 100 ms, rather than the CLI process's/usr/bin/timeRSS. No baseline overhead comparison is claimed.Prometheus v3.0.0,
c5d009d57fcccb7247e1191a0b10d74b06295388, full mode, 2 workers, 4096 MiB budget: 31.052 s, worker RSS 552900 KiB; 505 unresolved rows / 19006 sites, 3758665 JSON bytes (3782469 logical path/kind/detail bytes excluding SQLite overhead and project keys). A sample of 200 equally spaced Function/Method QNs in lexical order, depth 1, CALLS only, both directions: 122/400 unknown (1 inbound, 121 outbound), zero query errors. This final-binary run followed the C harness.Full Linux v6.12,
adc218676eef25575469234709c2d87185ca223a, 81924 files: the initial 2-worker/4096 MiB run was OOM-killed after 1881.23 s, at 3522632 KiB worker RSS. Forced-spill runs with 2 workers and 2048/3072 MiB budgets stopped at the memory guard after 173.280/848.803 s, at 2939752/4539200 KiB. The forced-spill runs published no partial graph. A completed full-kernel index, coverage row sizes, and trace share remain unavailable; that measurement needs a larger machine. Linux timings overlapped verification jobs.Final production binary SHA256 for Prometheus and the forced-spill Linux runs:
b3035597e242263093c82d41177e3c2419f797537b67dd3805f82f3b2e0d23e9.Disclosure and checklist
OpenAI Codex assisted with investigation, implementation, testing, and PR preparation. Pcristin reviewed and personally certified the signed-off contributions under DCO 1.1.