Repository navigation
feat(error-tracking): scope Flutter AOT source uploads - #124249
Conversation
|
😎 Merged successfully - details. |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| File | Comment lines | Added lines |
|---|---|---|
rust/cymbal/src/core/types/langs/native.rs |
6 | 55 |
cli/src/debug_symbols/mod.rs |
3 | 28 |
cli/src/debug_symbols/upload.rs |
1 | 25 |
rust/cymbal/src/core/symbolication/symbol_store/native.rs |
1 | 60 |
This check does not block merging. It updates on every push and clears when the share drops.
|
Risk: No findings This follow-up cleans up the CLI source-path filtering (consolidating it into filter_source_paths_for_upload with unchanged behavior), replaces the Flutter DWARF test fixtures with redacted synthetic-path versions plus an in-test guard against home-directory leaks, and adds a dart-exact-pc hash-domain to frame IDs. No new security issues; the previously reported home-directory leak in the fixtures is fixed and verified. Sentinel reviewed |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 2cfd329: 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| let lookup_addr = if self.lang.as_deref() == Some("dart") { | ||
| relative_addr | ||
| } else { | ||
| relative_addr.saturating_sub(1) |
There was a problem hiding this comment.
P2 · Include the Dart lookup mode in the frame cache identity
When otherwise identical native frames arrive with and without lang: "dart", this branch resolves different addresses, but RawNativeFrame::frame_id omits the lookup mode. LocalSymbolResolver therefore reuses the first frame's memory/database result, including a symbol miss at a function entry, for the other frame. Include the Dart lookup mode in the frame identity.
There was a problem hiding this comment.
Fixed in 23ae44f: the frame cache key now separates Dart exact-PC lookups from other native return-address lookups. The key remains stable across ASLR slides, and existing non-Dart keys stay unchanged.
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| // Dart AOT reports direct program counters, which can point to a | ||
| // function entry. Other native traces report return addresses, so | ||
| // look up the preceding call instruction for those frames. | ||
| let lookup_addr = if self.lang.as_deref() == Some("dart") { |
There was a problem hiding this comment.
question: Does every Dart AOT frame really report an exact PC? posthog-flutter#677 tags every frame it parses from an AOT trace with lang: "dart", so this branch skips the -1 for all of them, not just the top one.
For caller frames the printed pc is normally a return address: the instruction after the bl. Looking that up without -1 returns the line after the call, or the next function when the call is the last instruction. That wrong function name then feeds the issue fingerprint.
The tests can't tell the two modes apart. In the Android fixture, 0x205210 (resolver test), 0x205217 (parser test), and 0x205218 (the SDK's own test) all resolve to main.dart:7.
If only some frames carry exact PCs (async awaiter frames, say), could the SDK mark those frames explicitly instead of the server keying off lang? That also keeps lang a display hint and makes the frame_id cache gap easy to close. Either way, a resolver test at a mid-stack return address where -1 and exact lookup give different lines would establish which behavior is correct.
There was a problem hiding this comment.
I compiled an invented Dart AOT program with separate DWARF and captured synchronous caller and asynchronous frames. Each reported PC resolved with atos to the expected Dart function and line. On these ARM64 caller frames, PC and PC minus one mapped to the same location, so that capture alone does not distinguish the policies. The existing function-entry regression does: subtracting one there resolves the preceding function. Dart's native_stack_traces decoder uses the reported PC directly. This PR keeps exact lookup for Dart and now separates its cache key; a captured Flutter event-to-UI check is still pending.
| let filtered = source_bundle::filter_source_paths(&all_paths); | ||
| let filtered = match source_root { | ||
| Some(root) => { | ||
| source_bundle::filter_source_paths_under_root(&all_paths, root) |
There was a problem hiding this comment.
suggestion: The choice between filter_source_paths_under_root and filter_source_paths, plus the "No readable sources found inside the source root" warning, lives in two places: here and in cli/src/debug_symbols/mod.rs:141-144 (warning at :91-96). The copies differ only in the variable they pass. Any change to the root rule (such as a different warning or adding --source-root to dsym upload) has to be made twice.
Move the choice into source_bundle and call it from both sites:
pub fn filter_source_paths_scoped<'a>(paths: &'a [String], root: Option<&Path>) -> Vec<&'a str> {
match root {
Some(root) => filter_source_paths_under_root(paths, root),
None => filter_source_paths(paths),
}
}The empty-result warning can move in too if you don't need the binary path in the message.
There was a problem hiding this comment.
Done in 23ae44f. Both upload paths now use source_bundle::filter_source_paths_for_upload for root selection, counts, and the empty-result warning. Loose native symbols keep their existing toolchain exclusions.
| .find(|f| f.debug_id == "850c70a2-6592-a70c-3e49-c0e443794d23") | ||
| .unwrap(); | ||
| let upload = file.into_upload(None, false).unwrap(); | ||
| let upload = file.into_upload(None, false, None).unwrap(); |
There was a problem hiding this comment.
suggestion: Nothing tests that --source-root actually limits what gets uploaded. Every into_upload and package_dsym_bundles call in this file passes include_source=false and None, and the new unit test calls filter_source_paths_under_root directly. If into_upload or zip_dwarf_binary dropped the root and passed None, framework sources would ship again and the suite would stay green.
go_dwarf_source_extraction_uses_line_table_paths already builds a DWARF fixture with real sources on disk. Copy it with include_source=true and a root that excludes one of those files, then assert on __source/manifest.json in the packaged zip:
let upload = file.into_upload(None, true, Some(&root)).unwrap();
// read __source/manifest.json out of upload.data
assert!(!manifest.files.is_empty());
assert!(manifest.files.keys().all(|p| Path::new(p).canonicalize().unwrap().starts_with(&root)));There was a problem hiding this comment.
Added to the existing ELF packaging test in 23ae44f. It creates readable sources inside and outside the selected root, calls into_upload(None, true, Some(root)), then checks the packaged ZIP manifest includes only the in-root source and its original bytes. The CLI test suite and clippy pass.
hpouillot
added
the
cross-lane-change
hpouillot
removed
the
cross-lane-change
Problem
Flutter developers using obfuscated Android and iOS builds cannot inspect Dart stack frames or app source in error tracking (SDK request).
The existing source collector can include Flutter framework files from the build machine when it packages Dart debug symbols.
Changes
posthog-cli symbol-sets upload --include-source --source-root <app-directory>.--source-rootpreserves existing uploads.Note
This change pairs with posthog-flutter#677. Android event-to-UI behavior is verified locally; iOS AOT runtime verification remains pending.
How did you test this code?
hogli ci:preflight --stricton a branch based on currentmaster.main.dart@33with uploaded app source.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
cli/README.mddocuments the new flag. No existingdocs/guide covers CLI source-root scoping; Flutter onboarding remains unchanged until the coordinated SDK and backend changes are ready to release.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: pi, gpt-6-sol
f4faad3e-b1d7-4e72-90f1-eab640e344b9(no public session link available). Tools: read, edit, write, bash, codemode./pr,/writing-pr-descriptions,/writing-tests,/writing-code-comments,/writing-user-facing-copy,/running-ci-preflight,/announcing-behavior-changes.