Skip to content

feat(app): implement stacksnipe#502

Open
iamquang95 wants to merge 2 commits into
mainfrom
feat/stacksnipe
Open

feat(app): implement stacksnipe#502
iamquang95 wants to merge 2 commits into
mainfrom
feat/stacksnipe

Conversation

@iamquang95

Copy link
Copy Markdown
Collaborator

fix #71

@iamquang95

Copy link
Copy Markdown
Collaborator Author

@claude review this PR

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @iamquang95's task in 2m 11s —— View job


Review: feat(app): implement stacksnipe

I compared this against the Charon Go source (charon/app/stacksnipe/stacksnipe.go). This is a clean, faithful, and well-documented port. Tests actually exceed the Go coverage. A few notes below — nothing blocking.

✅ Functional equivalence verified

  • Public surface matches Go: Instance, new/new_with_interval, run, snipe, StackComponent, SUPPORTED_VCS, MAYBE_VCS, 15s poll interval.
  • NUL-split cmdline → drop empties → join with spaces, comm trimming, dedup-by-raw-cmdline, and substring-based name derivation (from cmdline, not comm) all match the Go logic.
  • Empty proc_path disables sniping and logs "Stack component sniping disabled". ✔
  • The tracing topic (topic = "stacksnipe") mirrors Go's log.WithTopic. ✔

👍 Good Rust-specific choices

  • First-tick skip (stacksnipe.rs:108-111): tokio::time::interval fires immediately on the first tick, whereas Go's time.NewTicker waits one full interval. Skipping the first tick correctly preserves Go's timing, and the comment explains why. Nice catch.
  • spawn_blocking (stacksnipe.rs:118): the blocking /proc filesystem walk is correctly moved off the async runtime — better than Go's plain goroutine.
  • Making snipe infallible is justified: Go's snipe returns an error, but walkFunc swallows every error and returns nil, so walkErr is effectively never set. filter_map(Result::ok) is the faithful, simpler equivalent.

🟡 Behavioral note — name selection determinism

vc_name (stacksnipe.rs:219-225) returns the first match in SUPPORTED_VCS order via .find(). The Go version iterates a map without break, keeping the last match in non-deterministic iteration order:

for vc := range supportedVCs {
    if strings.Contains(cmdlineString, vc) {
        vcName = vc   // no break → keeps last, map order is random
    }
}

So when a single cmdline contains two supported VC names, Go could report either one (run-to-run), while Rust deterministically reports the first by array order. The Rust behavior is strictly better, but it is a deviation from Go and name_multi_match_deterministic_first bakes it in — worth a one-line comment noting this is an intentional improvement over Go's non-determinism.

🟡 Minor: token vec built before dedup check

In snipe, cli_tokens is allocated at stacksnipe.rs:187 before the dedup check at line 196. Go builds its cmdLine slice after the dedup check, so duplicate cmdlines (e.g. background threads) avoid the per-token String allocations. Reordering — dedup-insert first, then build cli_tokens — would match Go and skip wasted work for duplicates. Purely a micro-optimization. Fix this →

ℹ️ Scope: not yet wired into app startup

Charon wires the sniper into the node lifecycle at app/app.go:170 (stacksnipe.New(conf.ProcDirectory, stackComponents)), where stackComponents writes the Prometheus metric (monitoringapi.go:324). This PR adds the module only — there's no caller of Instance::run and no MetricsFn bridged to a metric. Assuming wiring + the metric are a deliberate follow-up; flagging so it isn't lost.

Tests

Test coverage is thorough and goes beyond Go's single happy-path test (exact-comm-match, name-from-cmdline, dedup, NUL handling, comm newline trim, non-PID dirs, disabled path, nonexistent path). I was unable to execute cargo test in this environment (command not permitted), so please confirm CI is green.


Verdict: Looks good to merge once CI passes. The two 🟡 items are optional polish; the wiring is presumably a follow-up.
· feat/stacksnipe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement app/stacksnipe

1 participant