Conversation
0f9e8a1 to
26391f4
Compare
shark-cli gains a new `ai-investigate` command backed by a FIFO daemon architecture. Run `shark-ai-investigate --hprof <file>` to load a heap dump and receive a session shortcode; the wrapper then starts the daemon in the background. Commands are sent via `ai-investigate-cmd <shortcode> <command>` over named pipes. The daemon embeds a full algorithm guide in its `--help` output so an AI agent can drive the investigation autonomously. Daemon commands: - `trace`: leak traces as structured JSON (with `key`, `firstLeakingObjectId`) - `fields` / `string` / `array`: inspect heap objects - `mark-leaking` / `mark-not-leaking`: supply leaking-status context - `retained-size @<id>`: memory retained by an object - `human-readable-trace`: plain-text leak summary - `ping` / `close-session` New in shark: - `SingleObjectRetainedSizeCalculator`: retained size via exclude-and-reach two-BFS algorithm (provably correct for any reference graph) - `HeapClass.readInstanceFieldCount()`: reads instance field count as a single unsigned short without allocating field records - `ShallowSizeCalculator` fixes: array objects now include the ART object header omitted from HPROF records; class size uses static field values plus ArtField metadata instead of the raw HPROF record size Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add timestamped log entries for startup, every command, and notes - Startup line calls out the daemon starting with the heap dump path - Add `note TEXT` command to append free-form observations/hypotheses to the session log; update instructions to require notes continuously - Add required `reason` parameter to every command without one; daemon rejects commands missing a reason - Extract heap dump timestamp and metadata (AndroidMetadataExtractor) during analysis and include them in `trace` and `human-readable-trace` responses, matching the HeapAnalysisSuccess format - Move PrettyPrintJson to its own file with unit tests; streaming pretty-printer with no parse/re-encode round trip - Update AiInvestigateDaemonTest to pass reasons in all commands Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tcmulcahy
left a comment
There was a problem hiding this comment.
Some important feedback inline, but it's all optional, so I'm approving.
|
|
||
| ## Unreleased | ||
|
|
||
| `shark-cli` gains a new **AI-driven leak investigation skill**: run `shark-ai-investigate --hprof <file>` to load a heap dump and start a session. A shell wrapper prints a session shortcode, then starts the `ai-investigate` daemon in the background. An AI agent (or a human) can then send commands to the daemon via `ai-investigate-cmd <shortcode> <command>` — all over named pipes. Commands include `trace` (leak traces as structured JSON), `fields` / `string` / `array` (inspect objects), `mark-leaking` / `mark-not-leaking` (supply context), `retained-size` (memory retained by an object), and `human-readable-trace` (plain-text summary). The skill embeds a full algorithm guide in its `--help` output so an AI agent can drive the investigation autonomously. |
There was a problem hiding this comment.
I like this design, but I think the name ai-investigate is misleading, even if it is how this tool is likely to be used. I'd suggest shark-daemon start --hprof <file> and shark-daemon stop --shortcode ...
| done | ||
|
|
||
| # --no-help is consumed by this wrapper and must not be forwarded to shark-cli. | ||
| # Rebuild $@ without it using a POSIX-safe eval loop. |
There was a problem hiding this comment.
Do you really need POSIX compat? Ancient bash (like the version that ships with Mac) supports arrays, so you can write much simpler code:
new_args=()
for arg in "$@"; do
[[ $arg == --no-help ]] || new_args+=("$arg")
done
set -- "${new_args[@]}"
| # (e.g. a task runner) would block until the daemon exits, because the daemon | ||
| # holds the write end of the pipe open. The daemon communicates via named | ||
| # pipes, not stdout, so nothing useful is lost. | ||
| "$SHARK_CLI" "$@" ai-investigate --session "$shortcode" >/dev/null 2>&1 & |
There was a problem hiding this comment.
Note that the daemon will still be tied to the session - if you close the window within which you ran this, the daemon will die too. You can fix this with nohup if desired.
| # pipes, not stdout, so nothing useful is lost. | ||
| "$SHARK_CLI" "$@" ai-investigate --session "$shortcode" >/dev/null 2>&1 & | ||
|
|
||
| # Wait until the daemon creates its named pipes. |
There was a problem hiding this comment.
I don't see any code to capture the pid of the daemon - how to shut it down after?
|
|
||
| // Create named pipes after analysis so the wrapper's pipe-existence check | ||
| // doubles as a signal that the heap is loaded and the daemon is ready. | ||
| ProcessBuilder("mkfifo", inPath, outPath).start().waitFor() |
There was a problem hiding this comment.
This approach has some drawbacks. The main one is that if more than one client attempts to connect to the same daemon, things will be totally broken in a way that's really hard to debug. Also, FIFOs are just annoying (e.g. you have to be careful to avoid deadlocking)
For this reason, I think a Unix Domain Socket approach would be better. This allows traditional networking patterns, but without the security implications of an actual udp/tcp server.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
ai-investigatecommand toshark-clidesigned to be driven by an AI agent exploring Android heap dumpsLeakInvestigationSessionto split the expensive BFS path-finding (runs once) from the cheap inspection+bisecting step (re-runs on each status override)trace,node N,fields,instances,string,referrers,mark-leaking,mark-not-leaking,mark-unknown,select-group,select-trace,summary,help,exit--help)Key design
Split pipeline: BFS path-finding runs exactly once per session via
RealLeakTracerFactory.findLeaksWithCachedPaths(). Re-inspection callsreinspectPath()which skips BFS entirely and only re-runs the cheap ObjectInspector + bisecting stages. This makes override round-trips instantaneous.Override precedence: Status overrides are injected as an
ObjectInspectorappended last in the inspector list. It clears the opposing reason set before adding its own reason, so overrides always win over standard inspectors (e.g.AndroidObjectInspectors) without any special-casing.WIN detection:
leakFound: truein the JSON when no UNKNOWN nodes remain.culpritReferenceIndexpoints to the bad reference — the crossing from the last NOT_LEAKING node to the first LEAKING node.Agent workflow (printed at startup):
trace— scannodes[]for"leakingStatus": "UNKNOWN"fields Nfor runtime evidencemark-leaking/mark-not-leaking) with a concrete reason citing field valuesleakFound: trueJSON schema (trace commands)
{ "group": 0, "traceInstance": 0, "overrides": 2, "suspectWindowStart": 3, "leakFound": false, "culpritReferenceIndex": -1, "nodes": [ { "index": 0, "objectId": 12345, "className": "com.example.MyActivity", "leakingStatus": "NOT_LEAKING", "leakingStatusReason": "Activity#mDestroyed is false", "isSuspect": false } ] }Test plan
./gradlew :shark:shark:compileKotlin :shark:shark-cli:compileKotlinpasses./gradlew :shark:shark:test --tests shark.LeakInvestigationSessionTest— 11 tests pass.hproffilemark-not-leaking 0 "test"→ verify trace updates;mark-unknown 0→ verify revertleakFoundflips totrueandculpritReferenceIndexis set after all UNKNOWNs are resolved🤖 Generated with Claude Code