Repository navigation
Commit 3585be3
authored
fix(record): keep a failed capture's evidence for the retry (#2591)
* fix(record): keep a failed recording's evidence for the next stop
ADR 0024 rule 6, shipped first. `record stop` that failed to produce its export
ran compensating forced cleanup anyway. On Android that reaches
`cleanupVerifiedAndroidEvidence`, which stops the recorders and removes the
remote chunks and the device-side manifest: an `adb pull` that failed once under
load left nothing to pull twice, and the manifest the next stop would re-drive
from was gone with it.
The shared coordinator now asks the kind's own definition what a failed finish
may do (`failedFinishPolicy`), and never infers disposal from "finish threw".
Screen recording declares `preserve-retry-material`, so a failed collect leaves
the record `open`, keeps the live handle with the session, and lets the next stop
re-collect from the artifacts still on the device. Forced cleanup stays exactly
where rule 6 puts it: session teardown and failed-start rollback.
App-log, audio-probe, and perf-capture declare `dispose-on-failed-finish` and
keep today's behavior unchanged until their own failed-finish test states what
their retry needs.
* test(app-log): state what an app-log retry needs from a failed finish
ADR 0024 rule 6 earns its scope per kind. App-log's compensating cleanup is not a
disposal of anything a retry reads: `createAppLogLiveHandleFromFinish` derives it
from the same idempotent finish transaction, and the captured log file at the
canonical artifact path is never on its delete path.
So app-log declares `dispose-on-failed-finish` and this test names why: a stop
whose first finish attempt fails is retried by that very cleanup (two finish
calls, one error returned to the caller), the stream's bytes stay on disk, and the
record terminalizes so a replacement stream may start on the same device.
* test(audio-probe): state what an audio-probe retry needs from a failed finish
ADR 0024 rule 6, per kind. Audio-probe's whole capture is the status file its
helper publishes; the kind's cleanup terminates the sampler by exact identity and
has no path that removes that file. A helper that died before publishing its
terminal checkpoint cannot be revived by keeping it alive, so nothing a retry
reads is destroyed by disposing here.
Audio-probe therefore declares `dispose-on-failed-finish`, and this test drives the
real host capture operations through a failed finish to show what that guarantees:
the sampler is terminated exactly once by the compensating cleanup, the last
checkpoint stays on disk for `audio probe status` and recovery to read, and the
record terminalizes so a replacement probe may start.
* fix(perf): let a perf stop re-pull the trace its failed stop preserved
ADR 0024 rule 6, per kind. Perf-capture's retry needs the profiler artifact:
`stopAndroidNativePerfSession` pulls the device trace and only then removes it, and
the refusal it raises on a failed pull tells the caller to retry stop with the same
session. The compensating cleanup was the `rm -f` that emptied that promise.
Perf-capture declares `preserve-retry-material`, and the live handle stops
memoizing a refused finish, mirroring the recording handle: a stop the profiler
refused has to be re-driven by the next `perf stop`, which is what re-pulls the
trace the record now keeps. The test drives a pull failure through the daemon's
own perf record and shows the trace surviving the first stop, the record staying
open, and the second stop collecting and releasing it.
* docs(record): say what a record stop that fails keeps
Versioned help owned nothing about the failed-stop path, so an operator reading
`agent-device help record` after a lost export had no signal that the Android
artifact and its native manifest were still on the device, that the session's
recording manifest stayed open, or that `record stop` in the owning session is the
retry — and that only closing the session disposes them.
* refactor(capture-kit): keep the failed-finish policy type at its declaration
The package-boundary re-export had no consumer: every durable kind names the
literal where it builds its definition, and the one type-level consumer — the
capture-kit fixture that builds a preserving definition — resolves the declaration
module directly. The dead-code audit on the changed diff flagged the export.
* fix(capture): make session teardown a disposal finish
A finish now says why it was asked for. A `capture` finish tries to produce the
export and may keep retry material under a preserving kind's policy; a `disposal`
finish is session teardown handing the resource back for good, and disposes what a
failed finish left no matter what the kind preserves. Without the distinction, the
policy I just added to recording and perf-capture reached teardown and left a
closing session unable to give its recorder back — the coordinator skipped the forced
cleanup that ADR 0024 rule 6 assigns to teardown, and the two teardown tests that
watch for it failed.
`finishLiveDurableCapture` and `finishDurableCaptureHandle` require the intent rather
than defaulting it, so every caller states which authority it acts for; the three
teardown wrappers in `session-teardown.ts` are the only disposal callers.
* test(record): state what a failed record stop owes the retry
Three tests watched the coordinator compensate a failed `record stop` by force-cleaning
the recording and terminalizing its manifest, and one allowed a replacement `record start`
over the top of it. Those were the behavior ADR 0024 rule 6 retired, so they now assert
the rule: the finish error stays primary, the live handle and the open manifest survive,
phase rests at `completing` rather than `cleanup-pending`, and the next `record stop`
re-enters the recorder through the same handle — twice called, zero forced cleanups.
The two macOS provider scenarios keep their original point that an unfinalized recording
is never published as a completed artifact; only the retained state changed.
* docs(changelog): a failed record stop keeps what its retry reads back (#2591)
* fix(record): make a refused Apple record stop retryable
Two Apple facts made the retry my rule-6 change promises impossible, and the review
caught both.
The runner path memoized `stopRunner` with `??=`, so once the runner rejected a stop the
same rejection came back to every later `record stop` and the runner was never asked
again. Only an in-flight or completed stop is shared now, which is what the live handle
already does for a refused finish, so the next stop re-enters `recordStop`.
The simulator path threw on a non-zero `simctl recordVideo` exit, which made the settled
exit code the permanent answer and threw away a recording the recorder had already
written. An exit is an observation about the recorder, not about the export (ADR 0024
2.2), and the finalizer is what answers whether the file is a video: the exit is now
disclosed — `exited with code N`, or `was killed by SIGKILL` — and collection proceeds.
Simulator and macOS recordings that used to fail here now export; a refusal that really
stands is re-driven by the next stop instead of being remembered.
* fix(record): say how to clear a recording that never became playable
A preserved recording can reach a file no retry will ever turn into a video — the
simulator recorder SIGKILLed mid-write leaves zero bytes behind. That failure said only
what the file is not, with the generated "retry with --debug" hint, while the manifest
stays open and `record start` keeps refusing until the session closes. The error now
carries `recording-output-unplayable`, stays retriable for the case a retry does fix
(a recorder still finalizing its file), and names session close as the way out.
* docs(changelog): the Apple paths a preserved recording depends on to be retryable
* fix(record): say which side of a simulator stop failed
Two wrong blame assignments in the disclosure I added, both from reading the process result
as though the host had not already interpreted it.
`describeSimctlRecorderExit` checked `signal` first, and the simulator host normalizes a
termination record stop *asked for* to exit code 0 with the signal still attached — so our own
SIGINT-to-SIGTERM escalation came out as the recorder being "killed by SIGTERM before record
stop". The exit code is the normalized fact, so it answers first now.
And dropping the exit from an unreadable recording was a regression: the old error named the
exit code, the new one named only the file, and every retry waits again on bytes that cannot
change. The simulator finish now puts the exit in the error it raises, carries stderr and the
exit code/signal under `processExitError`, keeps the unplayable reason, clears a retriable flag
it no longer deserves, and points at closing the session.
The re-drive test used a clean exit, which main already re-drove, so it uses a non-zero one.
* fix(daemon): label the finishes that make room as disposals
Three finishes ask for a completion nobody reads: `audio start` settles the probe it replaces,
`audio status` settles a sampler that already exited, and `logs clear --restart` settles the
stream whose files it deletes a line later. They are disposals — whatever runs next expects the
record settled, not left open for a retry — and labelling them `capture` made them a trap for
any kind that later decides its retry needs material. Audio-probe and app-log dispose on a
failed finish either way, so nothing changes today.
The intent's own doc said session teardown was the only disposal caller, which is the claim that
just narrowed.
* docs(changelog): the simulator exit a lost recording now names
* fix(record): let only an unreadable file end the recording
The wrap I added explained too much. It took every error the export path raised — a telemetry
write that failed once, a finalize hiccup — gave it `retriable: false` and the "close this
session" hint, and dropped the original `hint` along the way, so a failure a retry would fix told
the user to give up. It now fires only on `recording-output-unplayable`, keeps the details it was
handed and adds the exit facts to them; anything else the export path raises comes through exactly
as it was.
The reason the two layers match on is now one exported constant instead of a string each, and the
exit details come from `execFailureDetails`, whose parameter accepts the null exit code a process
killed by a signal reports — the shape `AppErrorDetails.exitCode` already allows.1 parent b6fdbb1 commit 3585be3
37 files changed
Lines changed: 1196 additions & 53 deletions
File tree
- packages
- capture-kit/src/durable-capture
- contracts/src
- host-kit/src/internal
- platform-android/src/recording
- platform-apple/src/recording
- src
- commands/recording
- daemon
- __tests__
- handlers
- __tests__
- session-observability/internal
- test/integration/provider-scenarios
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
44 | 68 | | |
45 | 69 | | |
46 | 70 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
32 | 57 | | |
33 | 58 | | |
34 | 59 | | |
| |||
38 | 63 | | |
39 | 64 | | |
40 | 65 | | |
| 66 | + | |
41 | 67 | | |
42 | 68 | | |
43 | 69 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| 15 | + | |
15 | 16 | | |
16 | 17 | | |
17 | 18 | | |
| |||
47 | 48 | | |
48 | 49 | | |
49 | 50 | | |
| 51 | + | |
50 | 52 | | |
51 | 53 | | |
52 | 54 | | |
| |||
57 | 59 | | |
58 | 60 | | |
59 | 61 | | |
| 62 | + | |
60 | 63 | | |
61 | 64 | | |
62 | 65 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
| 73 | + | |
73 | 74 | | |
74 | 75 | | |
75 | 76 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
41 | | - | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
42 | 47 | | |
43 | 48 | | |
44 | 49 | | |
| |||
64 | 69 | | |
65 | 70 | | |
66 | 71 | | |
67 | | - | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
68 | 78 | | |
69 | 79 | | |
70 | 80 | | |
| |||
95 | 105 | | |
96 | 106 | | |
97 | 107 | | |
98 | | - | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
99 | 114 | | |
100 | 115 | | |
101 | 116 | | |
| |||
111 | 126 | | |
112 | 127 | | |
113 | 128 | | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
15 | | - | |
| 16 | + | |
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| |||
21 | 22 | | |
22 | 23 | | |
23 | 24 | | |
24 | | - | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
25 | 31 | | |
26 | 32 | | |
27 | 33 | | |
| |||
31 | 37 | | |
32 | 38 | | |
33 | 39 | | |
| 40 | + | |
34 | 41 | | |
35 | 42 | | |
36 | 43 | | |
| |||
53 | 60 | | |
54 | 61 | | |
55 | 62 | | |
| 63 | + | |
56 | 64 | | |
57 | 65 | | |
58 | 66 | | |
| |||
65 | 73 | | |
66 | 74 | | |
67 | 75 | | |
68 | | - | |
| 76 | + | |
69 | 77 | | |
70 | 78 | | |
71 | 79 | | |
| |||
74 | 82 | | |
75 | 83 | | |
76 | 84 | | |
77 | | - | |
| 85 | + | |
78 | 86 | | |
79 | 87 | | |
80 | 88 | | |
81 | 89 | | |
82 | 90 | | |
83 | | - | |
| 91 | + | |
84 | 92 | | |
85 | 93 | | |
86 | 94 | | |
87 | 95 | | |
| 96 | + | |
88 | 97 | | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
89 | 102 | | |
90 | 103 | | |
91 | 104 | | |
| |||
134 | 147 | | |
135 | 148 | | |
136 | 149 | | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
137 | 166 | | |
138 | 167 | | |
139 | 168 | | |
| |||
143 | 172 | | |
144 | 173 | | |
145 | 174 | | |
146 | | - | |
| 175 | + | |
147 | 176 | | |
148 | 177 | | |
149 | 178 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
12 | 15 | | |
13 | 16 | | |
14 | 17 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
646 | 646 | | |
647 | 647 | | |
648 | 648 | | |
649 | | - | |
| 649 | + | |
| 650 | + | |
650 | 651 | | |
651 | 652 | | |
652 | | - | |
| 653 | + | |
653 | 654 | | |
654 | 655 | | |
655 | 656 | | |
| |||
0 commit comments