Skip to content

Commit b33cc5c

Browse files
sunnylqmclaude
andcommitted
fix(telemetry): stop reporting rescued network errors as patch_fail
A download that succeeded on a later strategy sent an extra patch_fail carrying every earlier error, so a pdiff timeout rescued by the full package counted as a patch failure, and a real patch failure rescued the same way was counted twice (downloadFallback plus the extra report). The extra report is gone: downloadFallback alone carries the patch-health signal, only when a strategy failed with PATCH_FAILED, and its detail now holds only those patch errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent cf6a019 commit b33cc5c

2 files changed

Lines changed: 68 additions & 8 deletions

File tree

‎src/__tests__/client.test.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1001,12 +1001,14 @@ describe('downloadUpdate fallback chain', () => {
10011001
downloadFullUpdate = mock(() => Promise.resolve()),
10021002
addProgressListener = mock(() => ({ remove: mock(() => {}) })),
10031003
setLocalHashInfo = mock(() => {}),
1004+
fetchWithTimeout = mock(() => Promise.resolve()),
10041005
}: {
10051006
downloadPatchFromPpk?: ReturnType<typeof mock>;
10061007
downloadPatchFromPackage?: ReturnType<typeof mock>;
10071008
downloadFullUpdate?: ReturnType<typeof mock>;
10081009
addProgressListener?: ReturnType<typeof mock>;
10091010
setLocalHashInfo?: ReturnType<typeof mock>;
1011+
fetchWithTimeout?: ReturnType<typeof mock>;
10101012
} = {}) => {
10111013
setupClientMocks({
10121014
downloadPatchFromPpk,
@@ -1029,7 +1031,7 @@ describe('downloadUpdate fallback chain', () => {
10291031
? Math.min(100, Math.max(0, Math.floor((received / total) * 100)))
10301032
: 0,
10311033
DEFAULT_FETCH_TIMEOUT_MS: 5000,
1032-
fetchWithTimeout: mock(() => Promise.resolve()),
1034+
fetchWithTimeout,
10331035
info: mock(() => {}),
10341036
joinUrls: (paths: string[], fileName?: string) =>
10351037
fileName ? paths.map((p) => `${p}/${fileName}`) : undefined,
@@ -1043,9 +1045,19 @@ describe('downloadUpdate fallback chain', () => {
10431045
downloadPatchFromPpk,
10441046
downloadPatchFromPackage,
10451047
downloadFullUpdate,
1048+
fetchWithTimeout,
10461049
};
10471050
};
10481051

1052+
// The telemetry POSTs a download sent, as { type, detail } pairs.
1053+
const serverReports = (fetchWithTimeout: ReturnType<typeof mock>) =>
1054+
fetchWithTimeout.mock.calls
1055+
.filter(([url]: any[]) => String(url).includes('/report/'))
1056+
.map(([, init]: any[]): { type: string; detail?: string } => {
1057+
const { type, detail } = JSON.parse(String(init.body));
1058+
return { type, detail };
1059+
});
1060+
10491061
const updateInfo = {
10501062
update: true as const,
10511063
hash: 'new-hash',
@@ -1555,6 +1567,50 @@ describe('downloadUpdate fallback chain', () => {
15551567
);
15561568
});
15571569

1570+
test('a download rescued after a network error is not a patch failure', async () => {
1571+
const timeout: any = Error('请求超时。');
1572+
timeout.code = 'DOWNLOAD_FAILED';
1573+
const { fetchWithTimeout } = setupDownloadMocks({
1574+
downloadPatchFromPpk: mock(() => Promise.reject(timeout)),
1575+
});
1576+
const { Pushy, sharedState } = await importFreshClient(
1577+
'dl-fallback-transport-report'
1578+
);
1579+
sharedState.downloadedHash = undefined;
1580+
const client = new Pushy({ appKey: 'demo-app', maxRetries: 0 });
1581+
1582+
expect(await client.downloadUpdate(updateInfo)).toBe('new-hash');
1583+
1584+
expect(serverReports(fetchWithTimeout)).toEqual([
1585+
{ type: 'download_success' },
1586+
]);
1587+
});
1588+
1589+
test('a rescued patch failure is reported once, with only the patch error', async () => {
1590+
const patchError: any = Error('hpatch failed');
1591+
patchError.code = 'PATCH_FAILED';
1592+
const timeout: any = Error('请求超时。');
1593+
timeout.code = 'DOWNLOAD_FAILED';
1594+
const { fetchWithTimeout } = setupDownloadMocks({
1595+
downloadPatchFromPpk: mock(() => Promise.reject(patchError)),
1596+
downloadPatchFromPackage: mock(() => Promise.reject(timeout)),
1597+
});
1598+
const { Pushy, sharedState } = await importFreshClient(
1599+
'dl-fallback-patch-report'
1600+
);
1601+
sharedState.downloadedHash = undefined;
1602+
const client = new Pushy({ appKey: 'demo-app', maxRetries: 0 });
1603+
1604+
expect(await client.downloadUpdate(updateInfo)).toBe('new-hash');
1605+
1606+
// The test i18n renders bare keys: the pdiff timeout is not in detail.
1607+
expect(
1608+
serverReports(fetchWithTimeout).filter(
1609+
({ type }) => type === 'patch_fail'
1610+
)
1611+
).toEqual([{ type: 'patch_fail', detail: 'error_diff_failed' }]);
1612+
});
1613+
15581614
test('a `[PATCH_FAILED] ` message prefix classifies like a code property (bridge contract)', async () => {
15591615
// Harmony's bridge cannot set `code` on a rejection and prefixes the
15601616
// message instead; the strategy loop must treat it as the same

‎src/client.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -827,13 +827,11 @@ export class Pushy {
827827
).catch((e: any) => {
828828
log('telemetry report failed:', e?.message || e);
829829
});
830+
// A download rescued by a later strategy reports its patch-health
831+
// signal through downloadFallback, which only counts patch-apply
832+
// failures; downloadSuccess's data.error (any earlier failure, network
833+
// errors included) stays local.
830834
send(serverType, message || undefined);
831-
// A download that only succeeded after an incremental patch failed is
832-
// still a patch_fail signal server-side (diff quality), carried in
833-
// data.error alongside the downloadSuccess event.
834-
if (serverType === 'download_success' && data.error) {
835-
send('patch_fail', String(data.error));
836-
}
837835
} catch (e: any) {
838836
log('telemetry error:', e?.message || e);
839837
}
@@ -1493,6 +1491,9 @@ export class Pushy {
14931491
// downloadFallback report carries that as its code even when a later,
14941492
// unrelated transport failure happened to be the last error seen.
14951493
let patchFailed = false;
1494+
// The errors of the strategies that failed at the patch stage. A strategy
1495+
// that failed to apply is never retried, so these do not repeat.
1496+
const patchErrorMessages: string[] = [];
14961497

14971498
// The ordered attempts come from decideDownload (the pure decision layer);
14981499
// this side only executes them: probe candidate URLs, run the matching
@@ -1586,6 +1587,7 @@ export class Pushy {
15861587
log(errorMessage);
15871588
if (lastError.code === 'PATCH_FAILED') {
15881589
patchFailed = true;
1590+
patchErrorMessages.push(errorMessage);
15891591
}
15901592
if (!isMirrorRetryableCode(lastError.code)) {
15911593
// The bytes arrived but could not be applied (patch/manifest
@@ -1625,7 +1627,9 @@ export class Pushy {
16251627
newVersion: hash,
16261628
succeeded,
16271629
},
1628-
message: errorMessages.join(';'),
1630+
// Only the patch failures: a mirror's timeout alongside them would
1631+
// otherwise be read as the cause server-side.
1632+
message: (patchFailed ? patchErrorMessages : errorMessages).join(';'),
16291633
});
16301634
}
16311635
if (!succeeded) {

0 commit comments

Comments
 (0)