seed the scope from what is already running - #3
Merged
Conversation
Two independent bugs, both in how a process gets into the traced set, so both lost whole subtrees rather than scattered events. Following a process tree through fork is the premise of the tool, and one missing link at the top silently removed everything below it. The traced set only ever spread FORWARD, through fork. That is right for launch mode, where the target is parked with SIGSTOP before it execs and there is no history to miss, but every other front door attaches to something that already exists. --pid never saw the children the target forked before we got there, and unscoped mode seeded pid 1 alone — which was not "whole host" at all, since a login shell and its whole chain back to pid 1 predate attach. Nothing typed into a terminal ever appeared, however long it ran: ancestry decided visibility, not duration. procTable() and descendantsOf() in lib/scope.js enumerate what is running from the process graph and seed it, then fork propagation carries on as before. descendantsOf assigns each pre-existing process the generation distance the kernel would have given it, so a pre-existing subtree indents correctly instead of flattening onto the root. Launch mode is untouched, so its "nothing is missed" promise still holds literally. The second is one line of BPF. in_scope() reads `traced` by tgid, but on_fork looked the parent up by ctx->parent_pid, which the sched tracepoints report as a *tid*. Identical for a single-threaded process, so it looked correct — but in a threaded app membership propagated only by accident, when the forking thread happened to be created after the seed. A thread that already existed at attach was never in the map, and every process it spawned was invisible: measured 0 of 50 execs for a pre-existing worker thread, which is the shape of a JVM, a Node or Go service, or a build system with a thread pool. The fork tracepoint runs in the parent's context, so pid_tgid gives us the tgid directly. This also fixes depth, which had been inherited through the thread hop (a child of a worker thread reported depth 2 instead of 1). The status lines changed with it, because the old ones were now wrong in both directions — they claimed pre-existing children were untracked when they now are, and container mode claimed "cgroup-scoped" when nothing calls setCgroup, so target_cgid stays 0 and it has always been a plain pid subtree. test/capture.sh covers the seam both bugs lived in. heuristics.test.mjs replays fixtures through lib/model.js with no kernel, which structurally cannot reach kernel -> traced set -> ring buffer -> normalized record. The new test runs a workload whose exec count is known exactly (a distinct copy of /bin/true per phase, so comm makes each fold unambiguous) and asserts counts rather than presence, since a half-seeded scope still shows the row and undercounts. Both pre-existing subtrees announce themselves from inside themselves and the test blocks on that before attaching: thread creation otherwise raced capture startup, and on runs where the thread won that race a tid-keyed lookup still worked, so the test passed against broken code. Verified in both directions — PASS at 186 execs with the fixes, and with either fix reverted the checks that depend on it fail deterministically. Still open, and neither explains these misses: setCgroup is unwired, and bpf_ringbuf_output's return value is discarded, so a burst past 4 MiB drops execs with nothing on screen to say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent bugs, both in how a process gets into the traced set, so both lost whole subtrees rather than scattered events. Following a process tree through fork is the premise of the tool, and one missing link at the top silently removed everything below it.
The traced set only ever spread FORWARD, through fork. That is right for launch mode, where the target is parked with SIGSTOP before it execs and there is no history to miss, but every other front door attaches to something that already exists. --pid never saw the children the target forked before we got there, and unscoped mode seeded pid 1 alone — which was not "whole host" at all, since a login shell and its whole chain back to pid 1 predate attach. Nothing typed into a terminal ever appeared, however long it ran: ancestry decided visibility, not duration. procTable() and descendantsOf() in lib/scope.js enumerate what is running from the process graph and seed it, then fork propagation carries on as before. descendantsOf assigns each pre-existing process the generation distance the kernel would have given it, so a pre-existing subtree indents correctly instead of flattening onto the root. Launch mode is untouched, so its "nothing is missed" promise still holds literally.
The second is one line of BPF. in_scope() reads
tracedby tgid, but on_fork looked the parent up by ctx->parent_pid, which the sched tracepoints report as a tid. Identical for a single-threaded process, so it looked correct — but in a threaded app membership propagated only by accident, when the forking thread happened to be created after the seed. A thread that already existed at attach was never in the map, and every process it spawned was invisible: measured 0 of 50 execs for a pre-existing worker thread, which is the shape of a JVM, a Node or Go service, or a build system with a thread pool. The fork tracepoint runs in the parent's context, so pid_tgid gives us the tgid directly. This also fixes depth, which had been inherited through the thread hop (a child of a worker thread reported depth 2 instead of 1).The status lines changed with it, because the old ones were now wrong in both directions — they claimed pre-existing children were untracked when they now are, and container mode claimed "cgroup-scoped" when nothing calls setCgroup, so target_cgid stays 0 and it has always been a plain pid subtree.
test/capture.sh covers the seam both bugs lived in. heuristics.test.mjs replays fixtures through lib/model.js with no kernel, which structurally cannot reach kernel -> traced set -> ring buffer -> normalized record. The new test runs a workload whose exec count is known exactly (a distinct copy of /bin/true per phase, so comm makes each fold unambiguous) and asserts counts rather than presence, since a half-seeded scope still shows the row and undercounts. Both pre-existing subtrees announce themselves from inside themselves and the test blocks on that before attaching: thread creation otherwise raced capture startup, and on runs where the thread won that race a tid-keyed lookup still worked, so the test passed against broken code. Verified in both directions — PASS at 186 execs with the fixes, and with either fix reverted the checks that depend on it fail deterministically.
Still open, and neither explains these misses: setCgroup is unwired, and bpf_ringbuf_output's return value is discarded, so a burst past 4 MiB drops execs with nothing on screen to say so.