Repository navigation
refactor(ios): give each snapshot capture step its penalty authority - #3947
Conversation
EffectiveSnapshotCapturePlan carried a backend array, the channel state, a tree-slice budget override, and a pin in parallel. Whether the first step was the periodic probe was inferred from position, and the penalty recorder accepted a channel state defaulted to .normal, so a caller that ran a bounded recovery attempt but forgot its state could re-arm the penalty it was recovering from - the #3896 bug class, still reachable at the interface. A factory-built SnapshotCaptureStep now carries the backend together with its execution mode. captureWithBackend returns the attempt carrying that step; the recorder takes the attempt plus the warmup exemption and no other policy arguments. Only an ordinary attempt has an arming branch; only an accepted completed bounded recovery can clear the penalty early. Plan shapes, the selection-cause slots from #3916, the in-band claim after the occupied skip, and SnapshotXCTestChannelPenalty as the sole expiry owner are unchanged. The channel policy and its verdict helpers move to a named policy file.
Size Report
Startup median (7 runs, lower is better):
|
Bind the periodic-probe claim to an XCTest-backed leading step at the factory, so the mode cannot hand claim authority to a non-XCTest step. Align the warmup-exemption and claim-ordering comments with the code, and drop repeated narration of the step-carries-authority constraint.
Collapse the step's execution mode into the step itself, drop the accessors the step made redundant, and trim the moved policy's comments to constraints the code cannot show. Net production lines vs main now shrink.
Merge the plan construction into one site, drop the step factory and the plan accessor the steps made redundant, and cut narration to the constraints the code cannot show.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotCapturePlan.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotCapturePlan.swift:55">
P3: This comment incorrectly implies that every pin avoids arming the penalty; a tree pin remains an ordinary attempt and can arm it. Qualify the statement to private-AX pins and document the tree-pin exception.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| /// if any — that narrowed it. A pin shares the deferred ordering with a breaker deferral without | ||
| /// arming the penalty, so the selection cause stays separate from each step's mode. |
There was a problem hiding this comment.
P3: This comment incorrectly implies that every pin avoids arming the penalty; a tree pin remains an ordinary attempt and can arm it. Qualify the statement to private-AX pins and document the tree-pin exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+SnapshotCapturePlan.swift, line 55:
<comment>This comment incorrectly implies that every pin avoids arming the penalty; a tree pin remains an ordinary attempt and can arm it. Qualify the statement to private-AX pins and document the tree-pin exception.</comment>
<file context>
@@ -52,9 +52,8 @@ enum SnapshotTierOutcome: Equatable {
-/// if any — that narrowed it. The selection cause stays separate from the step's execution mode:
-/// an explicit pin, a custom-action pin, and a breaker deferral can share the deferred ordering
-/// while their verdict explanations differ, and a pin is never an armed penalty.
+/// if any — that narrowed it. A pin shares the deferred ordering with a breaker deferral without
+/// arming the penalty, so the selection cause stays separate from each step's mode.
struct EffectiveSnapshotCapturePlan {
</file context>
| /// if any — that narrowed it. A pin shares the deferred ordering with a breaker deferral without | |
| /// arming the penalty, so the selection cause stays separate from each step's mode. | |
| /// if any — that narrowed it. A private-AX pin shares the deferred ordering with a breaker deferral | |
| /// without arming the penalty; a tree pin remains an ordinary attempt. Selection cause stays separate | |
| /// from each step's mode. |
There was a problem hiding this comment.
Valid — a tree pin stays an ordinary attempt and can arm the penalty; only the private-AX pin takes the deferred route. Removed the sentence instead of qualifying it: the step mode (SnapshotCaptureStep.ordinary vs .boundedRecovery) is what decides arming authority, so the struct doc should not restate it. Fixed in 1e39504.
A tree pin stays an ordinary attempt and can arm the penalty; only the private-AX pin defers. The step mode owns that, so the struct says nothing.
|
This PR is ready at 1e39504. I found no problems in the code, and all 16 checks pass at this commit. The Apple Smoke, iOS Conformance and Runner Full XCTest lanes exercise the changed Swift files. There are no conflicts. Not blocking, and you can take or leave these: the I did not rerun the Swift tests or mutations, and the red/green and 356/13 counts come from the PR body. The live simulator run was at 0ae929c, not at head. I judged head equivalent by reading the diff from 0ae929c to 1e39504, not by rerunning it. The outdated Cubic thread no longer applies, because 1e39504 deletes the sentence it questioned (CapturePlan.swift:54-55). It can be resolved: #3947 (comment) |
thymikee
added
the
ready-for-human
|
Summary
Refactor #3 from the iOS snapshot-performance plan, following #3896 and #3916. Each capture step now carries its own penalty authority: a new
SnapshotCaptureStep(an enum over.ordinary(backend)/.boundedRecovery(backend, claimsPeriodicProbe:)) is built once by the plan factory and travels insideSnapshotBackendAttempt. The penalty recorder reads the attempt's step instead of a re-derived plan position plus a defaulted.normalchannel-state argument — the seam that made #3896's probe self-renewal reachable is now unrepresentable. All XCTest-channel policy owners (penalty value, plan reorder, verdict reasons, evidence gates) move to one file,RunnerTests+SnapshotChannelPolicy.swift, whose plan construction is a single site. Behavior-preserving.Touched files: 6 (3 production, 3 tests). No scope expansion, no interface added.
Production lines (three files,
wc -l): 1057 → 1053 vs main.SnapshotCapturePlan.swift872→630,SnapshotTiming.swift185→143, newSnapshotChannelPolicy.swift280. Comments net −13; the 5-line code delta replaces the removed defaulted argument and duplicated evidence gate with the step enum's payload.Validation
Tested SHA
1e39504de9(prior SHAs0ae929c0eacarried the full validation below; the later commits only consolidate plan construction and trim comments).pnpm check:affected --run --headpassed (receipt.tmp/validation/run-GPl9i8/receipt.json; iOS/macOS Swift lanes GitHub-authoritative). Full macOS host unit lane: 356 passed,TEST EXECUTE SUCCEEDED. 13 focused iOS-lane tests passed on a dedicated simulator. Historical red/green: the new regression test fails at1e7092c09a^(tree_backend_timeoutrenewal) and passes at head. Four mutations (drop recovery mode, omit probe claim, sweep-added step, post-response probe) each fail an existing test; the guards remain structurally present after the consolidation.pnpm check:xctest-selectionandcheck:packaged-runner-swiftpass.Live on an owned simulator with the production runner: hostile captures armed
PENALIZED, deferred plans stampedDEFERRED, the due capture claimedRECOVERY_PROBEinside the plan and did not renew the penalty, and the next ordinary probe re-armed only from its own hostile evidence (queries_backend_timeout).Independent review of the full range found no blockers; findings were fixed across
f873db3629..1e39504de9(probe claim bound to XCTest-backed steps at construction, single plan-construction site, constraint-only comments).CI obligations: Apple Smoke / iOS Conformance / Runner Full XCTest lanes are authoritative for the Swift suites.
🤖 Generated with Claude Code