Fix retained Amp network sessions#2138
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 8:42 PM ET / July 14, 2026, 00:42 UTC. Summary Reproducibility: yes. at source level: current main has two temporary Amp session creation paths without invalidation, and the proposed tests directly exercise finalization on every relevant exit mode. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the defer-based lifecycle fix once the remaining exact-head check is green, retaining the focused hermetic tests as regression coverage. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main has two temporary Amp session creation paths without invalidation, and the proposed tests directly exercise finalization on every relevant exit mode. Is this the best way to solve the issue? Yes: defer-based finalization directly beside session construction is the narrowest maintainable repair and does not alter Amp authentication, redirects, parsing, or fallback semantics. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 315ff65f7b2a. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Summary
URLSessionafter its request completes or fails.Root cause
Both Amp fetch paths created ephemeral sessions with redirect delegates but never invalidated them. Repeated refreshes could retain delegates, URL-cache state, and transport resources beyond each one-shot request.
Validation
swift test --filter 'AmpUsageFetcherTests|AmpUsageParserTests'make checkmake test: 639/639 selections, 54/54 groups passed on first attempt; 0 failures, retries, or timeouts.Screenshots and live account probes are not applicable. The transport lifecycle is covered hermetically with injected
URLProtocolresponses.