fix(network): report iOS requests that reused a keep-alive connection - #2433
Conversation
|
Three cases need fixing at f2360bd. The simulator recovery path only merges results when entries are nonempty. If log show returns only reused-task summaries with no known origin, its unnamedRequests count is discarded and the response still says no HTTP traffic was found. Preserve those counts and add a recovery-only unnamed-traffic regression. Connection correlation uses only the C number, not the process identity in the log. A reused connection number after an app restart can inherit an origin from the previous process. Scope the match to the process and connection, and test different PIDs with the same C number; unmatched traffic must stay unnamed. The global trailing-punctuation removal changes valid URLs such as https://example.test/release. into a different endpoint. Remove separators only where the log format establishes that they are separators, and test valid trailing punctuation. The reported live evidence is useful, but these cases are not covered yet. No CI checks are reported on this head. |
|
This head now also conflicts with main. The previously reported code findings still apply; please resolve those and the conflict before rerunning the affected checks. |
f2360bd to
0bc675b
Compare
|
All three fixed, rebased onto Recovery counts discarded. You were right that the merge was gated on entries alone. The recovery pass now merges whenever it observed traffic in either form, and the "none looked like HTTP traffic" note is reserved for a pass that found neither. Regression added: recovery returns only an unresolvable reused summary, and the response reports Cross-process origin inheritance. Agreed, and this was the worst of the three — attributing a request to a host the app never contacted is worse than dropping it. The index is now keyed on the compact log's Trailing punctuation. Correct, Validation. On CI: no checks report because this is a fork PR awaiting the "Approve and run" step — nothing I can trigger from my side. |
|
All three fixed and rebased onto Cross-process origin inheritance. You're right, and this was the worst of the three: attributing a request to a host the app never contacted is worse than dropping it. The index now keys on the compact log's Recovery path discarding counts. Confirmed. The pass merged only when it produced entries, so a recovery window holding nothing but unnameable summaries dropped the count and the response still claimed an empty window — while also emitting "none looked like HTTP traffic", which was untrue. It now merges whenever the pass observed traffic in either form, and that note is reserved for a pass that found neither. Trailing punctuation. Also right, and my justification was too broad. Regressions, each verified to fail without its fix (reverting the fix alone turns the test red):
Re-checked against the original captured simulator log after the key change: unchanged at 47 entries / 8 reported against an origin, and a 450-line window still counts 2 unnameable requests rather than dropping them.
|
|
The three earlier findings are fixed at 0bc675b. One merge case remains: mergeNetworkDumps uses Math.max for unnamedRequests, although the app log and recovery window can contain different requests. Two distinct summaries in one and three in the other are reported as three, while a request resolved by recovery can still be counted as unnamed from the app log. Preserve enough identity to reconcile the windows, or expose this explicitly as a lower-bound observation rather than an exact request count. Add overlapping and disjoint-window regressions. No CI checks are reported on this fork head. |
|
Fixed at You're right that I went with reconciliation rather than relabelling it a lower bound, because the identity is already in the log: every CFNetwork line names its request as
Two unnameable requests in one window and three in the other now report five; the same request seen in both reports once; and a request named in either window is not reported as unnamed by the other. Regressions: disjoint windows, overlapping windows, and named-in-one-window-only. The first and third fail against the old max-based merge — the middle one passes either way, since
Still no CI on this head — it's a fork branch, so I can't trigger it. Let me know if you'd rather I do anything to get those running. |
0bc675b to
eef183c
Compare
|
The earlier classification fixes look addressed at eef183c. One output-bound issue remains: |
|
The local Apple route now keeps task IDs out of the response, but the fix is incomplete at a46bcd5. Limrun ( |
a46bcd5 to
7ed6992
Compare
|
Fixed at You're right, and I'd actually flagged bounded response arrays to myself earlier and then talked myself out of it on the grounds that the count would be small in practice — which is exactly the reasoning that produces an unbounded output. Took your first option. The identities exist only to reconcile two scan windows, and that step finishes before a dump is returned, so they no longer leave the reader:
Reconciliation is unchanged, since the merge still sees the identities and recomputes the count from them: overlapping windows still collapse to one request, disjoint windows still sum. Regression: five unnameable tasks against Validation
That failure is not from this branch. Also still outstanding: the 2 Real-log evidence re-checked after the change: 47 entries / 8 reported against an origin, and truncated windows still count 2 and 1 unnamed. |
CFNetwork logs a request URL only on the `com.apple.network:connection` line that opens a connection. A request that reuses a keep-alive connection emits a task summary carrying status, timing, and byte counts but no URL anywhere in the log, so a URL-keyed reader dropped it and the dump silently omitted a request that did happen. An "assert this endpoint was called on startup" check therefore read as a definite fail. Correlate a reused task summary with the connection it names and report it against that connection's origin, with `pathUnavailable` set, its status, and its timing. The request path is not in the log at all, so the dump also notes how many requests it could not name — a gap in observation now reads as a gap rather than as a negative observation. Also stop a URL parsed out of a log line from carrying the punctuation that follows it, so an entry's `url` compares equal to the endpoint under test instead of failing on a trailing comma. The correlation lives in the reader rather than a sibling module because `packages/capture-kit/src/index.ts` may not grow its eager import closure. Refs callstack#2430
Review of the parent commit found the same definite-negative it fixes, one level down: a reused task summary whose connection was opened before the scanned window resolves to no origin, so it produced no entry and no signal — an empty dump reporting "No HTTP(s) entries were found" for a window that demonstrably carried traffic. Count those in the dump's `unnamedRequests` and say so in the notes, so an unnameable request is still a reported observation. Also order the Apple note builders so the keep-alive note no longer trips the `notes.length === 0` guard that suppresses lifecycle guidance, and give the android-backend test a fixture an Apple dump would actually resolve, so the backend gate it names is the thing it proves.
Review findings on the parent commits: three ways the reader still answers with something other than what it observed. A connection number is only meaningful within one process, but the index keyed on the number alone, so an app that relaunched and reopened the same number inherited the origin its predecessor had contacted — a request attributed to a host it never reached, which is worse than dropping it. Key the index by the compact log's `name[pid]` and the connection number together; a line whose process cannot be read correlates to nothing and its traffic stays unnamed. The simulator recovery pass merged its dump only when it carried entries, so a recovery window holding nothing but unnameable reused-task summaries discarded that count and the response still reported an empty window. Merge whenever the pass observed traffic in either form, and reserve the "none looked like HTTP traffic" note for a pass that found neither. The trailing-separator strip was global, so a valid URL ending in punctuation became a different endpoint. Take the URL from the delimited `url:` field where the format establishes the separator, and leave a bare URL exactly as matched. Regressions cover each: the same connection number under a different pid, an unreadable process identity, recovery-only unnamed traffic, and a path that legitimately ends in a period.
The app log and the simulator recovery pass cover different, sometimes overlapping windows, so taking the larger of their two unnamed counts was wrong in both directions: two unnameable requests in one window and three in the other reported three rather than five, and a request the recovery pass resolved stayed counted as unnamed from the app log. Carry the identities instead of a count. Every CFNetwork line names its request as `Task <UUID>.<seq>`, scoped here to the emitting process, so the same request seen in two windows is recognisable as one. A merge unions the unnamed identities and subtracts anything either window managed to name, and a resolved reused request carries its identity as `packetId` so that subtraction has something to key on. `NetworkDump.unnamedRequests` becomes `unnamedRequestIds`, since a list of identities is what makes the reconciliation exact rather than a lower bound. Regressions cover disjoint windows, overlapping windows, and a request one window named while the other could not.
`unnamedRequestIds` collected every unresolved task in the scan window and was spread straight into the response, so `network dump 1` could answer with thousands of task ids: an output whose size tracked the log rather than the requested entry limit. The identities exist to reconcile two scan windows, which is a step that finishes before a dump is returned. Keep them there. `NetworkDump` carries `unnamedRequests` as a count again, bounded by construction; the identities ride `ScannedNetworkDump`, the internal widening that the reader and the merge speak, and the Apple runtime projects them away with `withoutScanIdentities` on the way out. Reconciliation is unchanged: overlapping windows still collapse to one request and disjoint windows still sum, because the merge still sees the identities and recomputes the count from them. Regression: five unnameable tasks against `maxEntries: 1` reports all five and exposes no identity list.
The Apple route stopped leaking task identities into its response, but
Limrun and WebDriver return the scanner result directly and both serve
Apple sessions, so an iOS `network dump 1` through either still answered
with every unresolved task id in the scan window. Projecting at one
producer was never going to hold: `ScannedNetworkDump` was assignable to
`NetworkDump`, so returning the scanner result compiled everywhere and
each producer had to remember not to.
Take the shape away instead. `readRecentNetworkTrafficFromText` returns a
`NetworkScan` — `{ dump, unnamedRequestIds }` — so identities sit beside
the public dump rather than on it, and `mergeNetworkScans` reconciles the
pair. A route returning `scan.dump` cannot carry them out, and a route that
forgets does not compile. All four producers are updated; the response
shape is unchanged.
Regressions cover the Apple, Limrun and WebDriver routes: five unnameable
tasks against `maxEntries: 1` report the count and expose no identity list.
All three fail if the identities are put back on the dump.
7ed6992 to
3fd6441
Compare
|
Fixed at You're right, and the shape of the miss is worse than the miss: I'd enumerated all four producers before the previous round, fixed one, and treated it as done. Projecting at a single producer could never have held anyway — So I took the shape away rather than covering the routes one by one. Regressions on all three Apple-capable routes — Apple, Limrun ( Validation
I have to correct my last comment on one point. I reported The underlying observation does survive, though, so it may still be worth a look independently: Remaining, both reproducible on a clean |
|
The output-bound issue is fixed at 3fd6441. Scan identities are separate from the public dump, and the Apple, Limrun and WebDriver regressions cover the affected routes. No remaining code findings; the reported local checks pass, but GitHub still has no checks on this fork head, so please approve and run CI before merging. |
thymikee
added
the
ready-for-human
|
Thanks for running it — CI is green on Nothing outstanding from my side. The branch is rebased on |
2 participants
Summary
On iOS,
network dumpsilently omitted any request that reused a keep-alive connection, so "assert this endpoint was called on startup" read as a definite fail. Closes #2441. Also the second half of #2430, whosereact-devtoolshalf is now fixed upstream in callstackincubator/agent-react-devtools#60 and tracked here by #2432, so this should not auto-close #2430.CFNetwork logs a request URL only on the
com.apple.network:connectionline that opens a connection. A reused task emits a summary with status, timing and byte counts but no URL anywhere, so a URL-keyed reader dropped it. From a real simulator log,/initreusing the connection/v4/messages/en_USopened 354ms earlier:Such a request is now reported against its connection's origin with
pathUnavailable: true, its status and its timing (rawkeeps the summary, includingrequest_bytes/response_bytes). The path is genuinely absent from the log, so it is not invented. A reused task whose connection opened before the scan window resolves to nothing at all; those are counted in the newunnamedRequestsand named in the notes, so an empty dump still reports that traffic was observed. Also stops parsed URLs keeping the punctuation that follows them, sourlcompares equal to the endpoint under test. 7 files.Two shaping notes:
pathUnavailableoverloadsurlwith an origin (the alternative — dropping the entry — is the bug), and the correlation lives insidenetwork-traffic.tsrather than a sibling module becausecapture-kit/src/index.tsmay not grow its eager import closure and that gate has no approval path.Validation
f2360bd3.pnpm check:affected --runpasses on that commit; fallow clean. Fullpnpm test:unit: 9438 pass; the 2code-signature.test.tsfailures reproduce on a clean tree and are unrelated.A/B on one captured
app.logfrom a real RN app: before, 39 entries, 1 on:3040(carrying a trailing comma),/initabsent; after, 47 entries, 2 on:3040,/initpresent atstatus=200. Livenetwork dumpon the production path reported 8 such requests plus the note. Truncating the window to 450 scanned lines counted 2 unnameable requests instead of dropping them.Risk: assumes CFNetwork's
connection=Nshares numbering with the[CN]nw connection id. Every captured log is consistent with it; a divergence would attribute a request to the wrong origin, so it is worth a maintainer's eye.