Skip to content

Commit 27b7d52

Browse files
malysclaude
andcommitted
fix(chargepilot): the OTA retry answers to the right question
3bc115a made UpdateHook retry only on Unreachable, but got the boundary wrong in both directions. check() read any non-200 — a 403 unauthenticated rate limit, a 502 during a GitHub incident — as a definitive "no update", which silently switched the retry off for exactly the case it was added for: GitHub reachable but not actually answering. Only 404 (no `unstable` tag yet) is genuinely definitive; everything else non-200 now retries. download() had the opposite problem: every failure — a disallowed host, an oversized asset, a removed release, a dropped connection — was retried alike. It now carries the same split as check() (DownloadResult: Downloaded/Refused/Unreachable), so a refusal that retrying cannot change stops on the first try and only a genuine network failure or a 5xx spends the two 20s retries. Not compiled here: this workspace has no JDK and no Android SDK. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 3bc115a commit 27b7d52

2 files changed

Lines changed: 39 additions & 14 deletions

File tree

‎app/src/unstable/java/com/evsuite/chargepilot/update/OtaUpdater.kt‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,13 @@ internal object OtaUpdater {
143143
}
144144
if (connection.responseCode != 200) {
145145
AppLogger.w(TAG, "Release API returned ${connection.responseCode}")
146-
return CheckResult.Answered(null)
146+
// 404 is the one status GitHub gives for real: no `unstable` tag exists yet.
147+
// Everything else — 403 unauthenticated rate limit, 5xx, 429 — is GitHub itself
148+
// being unreachable in every way that matters, and is exactly the condition the
149+
// retry exists for; answering it with a definitive "no update" would silently
150+
// turn every one of those into the outcome the retry was added to avoid.
151+
return if (connection.responseCode == 404) CheckResult.Answered(null)
152+
else CheckResult.Unreachable
147153
}
148154
val json = JSONObject(
149155
connection.inputStream.bufferedReader(StandardCharsets.UTF_8).use { it.readText() }
@@ -175,21 +181,32 @@ internal object OtaUpdater {
175181
}
176182
}
177183

184+
sealed interface DownloadResult {
185+
data class Downloaded(val file: File) : DownloadResult
186+
187+
/** A deliberate no: a disallowed host, an oversized or empty body, too many redirects.
188+
* Trying again in 20s changes nothing about any of those. */
189+
object Refused : DownloadResult
190+
191+
/** The request itself never completed — worth trying again once the network is up. */
192+
object Unreachable : DownloadResult
193+
}
194+
178195
/**
179196
* Downloads into app-private cache, following redirects by hand so that every hop is
180-
* re-checked. Returns null on any refusal or failure; the partial file never survives.
197+
* re-checked. The partial file never survives a refusal or a failure.
181198
*/
182-
fun download(context: Context, update: Update): File? {
199+
fun download(context: Context, update: Update): DownloadResult {
183200
if (!isAllowedUrl(update.apkUrl)) {
184201
AppLogger.w(TAG, "Refusing to download from ${update.apkUrl}")
185-
return null
202+
return DownloadResult.Refused
186203
}
187204
val temporary = File.createTempFile(CACHE_PREFIX, ".apk", context.cacheDir)
188205
var current = update.apkUrl
189206
var kept = false
190207
try {
191208
repeat(MAX_REDIRECTS) {
192-
if (!isAllowedUrl(current)) return null
209+
if (!isAllowedUrl(current)) return DownloadResult.Refused
193210
val connection = (URL(current).openConnection() as HttpURLConnection).apply {
194211
instanceFollowRedirects = false
195212
setRequestProperty("User-Agent", "EVChargePilot-Android")
@@ -199,20 +216,24 @@ internal object OtaUpdater {
199216
try {
200217
val status = connection.responseCode
201218
if (status in 300..399) {
202-
val location = connection.getHeaderField("Location") ?: return null
219+
val location = connection.getHeaderField("Location")
220+
?: return DownloadResult.Refused
203221
// Resolved against the current URL: a `Location` may be relative, and
204222
// a bare path would otherwise fail the allowlist for the wrong reason.
205223
current = URI(current).resolve(location).toString()
206224
return@repeat
207225
}
208226
if (status != HttpURLConnection.HTTP_OK) {
209227
AppLogger.w(TAG, "Update download returned $status")
210-
return null
228+
// A 404/410 on the asset itself is permanent for this run; a 5xx or a
229+
// rate limit is the same "GitHub, not us" case check() already retries.
230+
return if (status in 500..599) DownloadResult.Unreachable
231+
else DownloadResult.Refused
211232
}
212233
// The declared length is a claim; the transferred length is the fact.
213234
// Both are capped, so neither a lying header nor a chunked response can
214235
// fill the head unit's storage.
215-
if (connection.contentLengthLong > MAX_APK_BYTES) return null
236+
if (connection.contentLengthLong > MAX_APK_BYTES) return DownloadResult.Refused
216237
var written = 0L
217238
connection.inputStream.use { input ->
218239
temporary.outputStream().use { output ->
@@ -221,24 +242,24 @@ internal object OtaUpdater {
221242
val read = input.read(buffer)
222243
if (read == -1) break
223244
written += read
224-
if (written > MAX_APK_BYTES) return null
245+
if (written > MAX_APK_BYTES) return DownloadResult.Refused
225246
output.write(buffer, 0, read)
226247
}
227248
output.fd.sync()
228249
}
229250
}
230-
if (written == 0L) return null
251+
if (written == 0L) return DownloadResult.Refused
231252
kept = true
232-
return temporary
253+
return DownloadResult.Downloaded(temporary)
233254
} finally {
234255
connection.disconnect()
235256
}
236257
}
237258
AppLogger.w(TAG, "Update download exceeded $MAX_REDIRECTS redirects")
238-
return null
259+
return DownloadResult.Refused
239260
} catch (e: Exception) {
240261
AppLogger.w(TAG, "Update download failed: ${e.message}")
241-
return null
262+
return DownloadResult.Unreachable
242263
} finally {
243264
// Only a returned file survives. A partial body left in the cache after a refused
244265
// redirect or a dropped connection is an unverified archive on disk, which is

‎app/src/unstable/java/com/evsuite/chargepilot/update/UpdateHook.kt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,11 @@ object UpdateHook {
127127
val existing = File(directory, OtaUpdater.fileName(update.versionName))
128128
if (existing.isFile && existing.length() > 0) return Attempt.Ready(existing)
129129

130-
val downloaded = OtaUpdater.download(context, update) ?: return Attempt.Retry
130+
val downloaded = when (val result = OtaUpdater.download(context, update)) {
131+
OtaUpdater.DownloadResult.Unreachable -> return Attempt.Retry
132+
OtaUpdater.DownloadResult.Refused -> return Attempt.Nothing
133+
is OtaUpdater.DownloadResult.Downloaded -> result.file
134+
}
131135
val published = OtaUpdater.publish(context, downloaded, update.versionName)
132136
?: return Attempt.Nothing
133137
AppLogger.i(TAG, "Update ${update.versionName} ready at $published")

0 commit comments

Comments
 (0)