Repository navigation
fix: preserve pending backups across restart - #1689
ben-kaufman wants to merge 5 commits into
Conversation
|
Triage: 3.1.0. It fixes shipped backup handling, and 3.1.0 is due 28 Oct. |
 [Medium impact] The changes affect wallet backup and recovery work in the Android app. Fix the restart gap before merging so edits made during old cleanup are not silently left out of backups. Findings
SummaryThis PR makes reset wait for retained backup work and keeps explicit backups separate from observation.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Stop observation] --> B[Cancel old observer jobs]
B --> C[Restart requested]
C --> D[Wait for old cleanup]
D --> E[Start new data listeners]
E --> F[Drop initial values]
G[User edits settings during wait] --> D
G --> F
F --> H[Edit never marks backup required]
Reviews (1) · Last reviewed commit: "fix: preserve pending backups across res..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Moves every backup task under a reset-owned scope with a generation fence, so reset drains interrupted setup, uploads and status writes before clearing the VSS clients, and a stopped or restarted observer keeps pending categories. Reviewed a8f0810, full tier, reasoned from the code and CI (not built here).
What I checked, and 6 candidates I ruled out
Read in full: the BackupRepo.kt diff and its lifecycle paths (reset, startObservingBackups, scheduleBackup, withBackupOperation, performBackup, retryAfterTemporaryFailure), PaykitSdkOperationLock.kt, PaykitSdkService.withWalletWipe / exportBackupState, WipeWalletUseCase.kt, BackupsViewModel.kt, UsdtService.recoveryBackup
Call sites traced: reset (only WipeWalletUseCase.stopNode), triggerBackup, persistUsdtRecoveryBackup, scheduleFullBackup, start/stopObservingBackups
CI: build (includes testDevDebugUnitTest) and lint green; e2e jobs still running at review time
Ruled out
- Reset hangs the wipe:
resetjoins pending work underNonCancellablewhilewithWalletWipeholds the Paykit lock. A background WALLET export blocked there is inacquire, whoseawaitis cancellable and removes its waiter; the SDK call's ownNonCancellablesection cannot be entered while the wipe holds the lock. Covered bywipe admission drains a backup queued at the real SDK operation barrier. - Lost update while a scheduled job is mid-debounce: the old cancel-and-restart is gone, but the status observer skips because
runningis true, the job reads the latest status after the debounce, and a change during upload bumpsrequiredpastbackupRequired, so the job'sfinallyreschedules.full backup follows newer required data after observers coalescecovers the follow-up. - Reschedule loop in the job's
finally: a permanent failure setsfailedBackupRequired, soshouldBackupstops it;WalletBackupSnapshotChangedre-runs as it did on the base through therunningtrue to false transition. - Lock inversion:
categoryMutexesthenwalletBackupMutex/usdtBackupMutexon the scheduled path, the inner mutex alone onpersist*;lifecycleLockis only taken in non-suspending blocks, including inside the DataStore transform. - Restart dropped during reset:
startObservingBackupsreturns whileresetting, but the base already returned while wiping, and the only reset caller restarts observation itself on failure. - Greptile's restart gap: real, but the class predates this PR. On the base an edit made while observation is stopped is already discarded by the listeners'
drop(1)on restart; this change extends that window by the old observation's cancellation cleanup, which is short (cancellable uploads plus oneNonCancellablestatus write).
Merge confidence: 4/5, no findings, but the change is reasoned from code and CI rather than built here, and the e2e jobs had not finished.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (not exposed to the reviewing session)
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- A backup is marked synced only after the put returns success. Stop, restart and wipe keep consumed pending markers. Only the fully-stopped window drops edits, which was already discussed here.
- After a wipe and restore, the old wallet's pending data can't upload under the new wallet: the generation fence and the drained join stop it.
BackupItemStatusis persisted in the same format as v3.0.0, so nothing to migrate.- Twin synonymdev/bitkit-ios#1123 serialises the observer lifecycle the same way.
Performance-relevant (observation restarts on every background/foreground cycle and now waits for the old cleanup). The gate measures the gap between Stopped observing and Start observing per cycle, and counts Backup starting for per single edit (expected 1), head vs master.
Device gate (partial): e585fcc — not run yet; pending that measurement. Device results will follow in a separate review.
|
@ben-kaufman please resolve conflicts. |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for e585fcc: Android 15 (API 35), arm64. I built the reviewed head and drove an isolated fault fixture using the app's real DataStores and native encrypted VSS transport. Local hooks hold operations and expose their results; a proxy supplies a controlled VSS conflict. Recovery payloads are synthetic, so these results do not establish native transaction signing or a deployed-contract outage journey.
Test 1: pending changes across restart ✅ Settings and a due retry waited for old cleanup, then uploaded without another edit.
I held WIDGETS cancellation cleanup, restarted observation, changed synchronized settings to EUR, and let a BLOCKTANK retry become due. Replacement setup and uploads waited for release. Decrypted VSS readback contained EUR, with the same payload digest as the upload.
case1-2x.2x.mp4
Restart drain and native VSS readback, 2×.
Test 2: reset drains manual and recovery persistence ✅ Cancelled reset callers still waited; cancelled recovery was not acknowledged, and retry retained the same envelope.
Both VSS clients reset only after the held write returned. The recovery acknowledgement counter stayed at zero after cancellation, then advanced once on retry with observation stopped. Both writes used the same content-addressed key and payload digest. This checks the real app callback with a synthetic recovery string.
case2-2x.2x.mp4
Reset cancellation and recovery retry, 2×.
Test 3: wipe admission ✅ Manual, full and recovery requests produced no upload during wipe; explicit recovery worked after release.
SETTINGS and USDT recovery status snapshots remained unchanged while admission was closed after reset. The recovery request completed after release without starting observation.
case3-2x.2x.mp4
Wipe admission and stopped-observer recovery, 2×.
Test 4: full-backup follow-up ✅ New revisions, a held required-status write, and a full request during stopped-job cleanup all resumed without another edit.
I drove the shared scheduler through SETTINGS. A change during the first upload produced a second upload; a consumed change survived stop/restart while its cache write was held; a full request waited behind cancelled cleanup and completed with observation stopped. Final decrypted VSS readback contained CHF.
case4-2x.2x.mp4
Revision follow-up, cache-write drain and full-request admission, 2×.
Test 5: clean restart timing and upload count ✅ The head added only milliseconds on the idle fixture, and one edit produced one SETTINGS upload at both revisions.
Measured from the app's stop/start log timestamps: head gaps were 2–11 ms (median 5 ms); exact-base gaps were 0–1 ms (median 1 ms). These are direct observation cycles against ac2a7c668, rather than physical background transitions or a comparison with later master changes.
Head e585fcc |
Base ac2a7c668 |
|---|---|
case5-head-2x.2x.mp4 |
case5-base-2x.2x.mp4 |
Idle lifecycle cycles and a single settings edit, 2×.
Check 1: build and committed regressions ✅ assembleDevDebug and the focused BackupRepoTest suite passed on the unmodified head.
The restart finding is repaired at this head; I checked its listener and retry-readiness paths against the fresh regression and device results.
Coverage: Full ac2a7c668..e585fcc, including reached reset/wipe, SDK-lock and recovery-acknowledgement paths. No planned fixture case remains unrun. Device full-backup checks exercised SETTINGS; complete WALLET capture and signed-payment recovery internals were checked in code and the committed component suite, without a new complete payment-fault journey.
Twin: bitkit-ios#1123 fix: serialize backup observation lifecycle matches the lifecycle and pending-change intent. Android invalidates observation immediately and drains on restart/reset; iOS awaits shutdown.
Local test controls — not pushed
The patch adds explicit holds and logging, plus a shell-only control surface for a disposable installation. It does not replace native VSS results or change the backup decisions under review. The proxy forwards encrypted traffic to staging and can return a protobuf conflict; the native client converts that into the app's normal retry path.
diff --git a/app/src/debug/AndroidManifest.xml b/app/src/debug/AndroidManifest.xml
index b88fe73a7..c1aa86be7 100644
--- a/app/src/debug/AndroidManifest.xml
+++ b/app/src/debug/AndroidManifest.xml
@@ -11,5 +11,8 @@
android:authorities="${applicationId}.devtools"
android:exported="true"
tools:ignore="ExportedContentProvider" />
+ <!-- test hook -->
+ <provider android:name="to.bitkit.dev.BackupFaultProvider" android:authorities="${applicationId}.backupfault" android:exported="true" tools:ignore="ExportedContentProvider" />
+ <activity android:name="to.bitkit.dev.BackupFaultActivity" android:exported="true" />
</application>
</manifest>
diff --git a/app/src/main/java/to/bitkit/data/CacheStore.kt b/app/src/main/java/to/bitkit/data/CacheStore.kt
index 9966a479f..31a561531 100644
--- a/app/src/main/java/to/bitkit/data/CacheStore.kt
+++ b/app/src/main/java/to/bitkit/data/CacheStore.kt
@@ -38,6 +38,7 @@ class CacheStore internal constructor(
val backupStatuses: Flow<Map<BackupCategory, BackupItemStatus>> = data.map { it.backupStatuses }
suspend fun update(transform: (AppCacheData) -> AppCacheData) {
+ to.bitkit.testhook.BackupFault.hold("cache") // test hook
store.updateData(transform)
}
diff --git a/app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt b/app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt
index eef9af0b1..4d59ab2d0 100644
--- a/app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt
+++ b/app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt
@@ -1,5 +1,6 @@
package to.bitkit.data.backup
+import to.bitkit.testhook.BackupFault // test hook
import com.synonym.vssclient.KeyVersion
import com.synonym.vssclient.VssItem
import com.synonym.vssclient.vssDelete
@@ -36,6 +37,8 @@ class VssBackupClient @Inject constructor(
private val setupMutex = Mutex()
suspend fun setup(walletIndex: Int = 0): Result<Unit> = withContext(ioDispatcher) {
+ BackupFault.event("SETUP_ENTER") // test hook
+ BackupFault.hold("setup", drain = true) // test hook
setupMutex.withLock {
val gate = isSetup
runCatching {
@@ -109,6 +112,7 @@ class VssBackupClient @Inject constructor(
}
fun reset() {
+ BackupFault.event("RESET_VSS") // test hook
synchronized(this) {
isSetup.cancel()
isSetup = CompletableDeferred()
@@ -122,6 +126,7 @@ class VssBackupClient @Inject constructor(
data: ByteArray,
): Result<VssItem> = withContext(ioDispatcher) {
isSetup.await()
+ BackupFault.beforePut(key, data) // test hook
Logger.verbose("VSS 'putObject' call for '$key'", context = TAG)
runCatching {
vssStore(
@@ -129,8 +134,11 @@ class VssBackupClient @Inject constructor(
value = data,
)
}.onSuccess {
+ BackupFault.event("PUT_OK $key sha256=${BackupFault.digest(data)}") // test hook
+ BackupFault.hold("post:${BackupFault.gateKey(key)}", drain = true) // test hook
Logger.verbose("VSS 'putObject' success for '$key' at version: '${it.version}'", context = TAG)
}.onFailure {
+ BackupFault.event("PUT_ERROR $key ${it.javaClass.simpleName}") // test hook
Logger.verbose("VSS 'putObject' error for '$key'", it, context = TAG)
}
}
diff --git a/app/src/main/java/to/bitkit/data/backup/VssBackupClientLdk.kt b/app/src/main/java/to/bitkit/data/backup/VssBackupClientLdk.kt
index 7842dbf19..7c6472c57 100644
--- a/app/src/main/java/to/bitkit/data/backup/VssBackupClientLdk.kt
+++ b/app/src/main/java/to/bitkit/data/backup/VssBackupClientLdk.kt
@@ -43,6 +43,7 @@ class VssBackupClientLdk @Inject constructor(
private val setupMutex = Mutex()
suspend fun setup(walletIndex: Int = 0): Result<Unit> = withContext(ioDispatcher) {
+ to.bitkit.testhook.BackupFault.event("LDK_SETUP_ENTER") // test hook
setupMutex.withLock {
val gate = isSetup
runCatching {
@@ -75,6 +76,7 @@ class VssBackupClientLdk @Inject constructor(
}
fun reset() {
+ to.bitkit.testhook.BackupFault.event("RESET_LDK") // test hook
synchronized(this) {
isSetup.cancel()
isSetup = CompletableDeferred()
diff --git a/app/src/main/java/to/bitkit/env/Env.kt b/app/src/main/java/to/bitkit/env/Env.kt
index 09797720d..6bd811c54 100644
--- a/app/src/main/java/to/bitkit/env/Env.kt
+++ b/app/src/main/java/to/bitkit/env/Env.kt
@@ -111,7 +111,7 @@ internal object Env {
val vssStoreIdPrefix get() = "bitkit_v1_${network.name.lowercase()}"
val vssServerUrl
- get() = when (network) {
+ get() = if (to.bitkit.BuildConfig.DEBUG) "http://127.0.0.1:24889" else when (network) { // test hook
Network.BITCOIN -> "https://bitkit.to/vss_rs_auth"
else -> "https://bitkit.stag0.blocktank.to/vss_rs_auth"
}
diff --git a/app/src/main/java/to/bitkit/testhook/BackupFault.kt b/app/src/main/java/to/bitkit/testhook/BackupFault.kt
new file mode 100644
index 0000000..f99eb5b
--- /dev/null
+++ b/app/src/main/java/to/bitkit/testhook/BackupFault.kt
@@ -0,0 +1,40 @@
+// test hook: local review fixture, never shipped.
+package to.bitkit.testhook
+
+import android.util.Log
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.NonCancellable
+import kotlinx.coroutines.withContext
+import java.security.MessageDigest
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.CopyOnWriteArrayList
+import to.bitkit.BuildConfig
+
+object BackupFault {
+ private val gates = ConcurrentHashMap<String, CompletableDeferred<Unit>>()
+ val events = CopyOnWriteArrayList<String>()
+ fun event(text: String) {
+ if (!BuildConfig.DEBUG) return
+ val line = "${android.os.SystemClock.elapsedRealtime()} $text"
+ events.add(line)
+ Log.i("BackupFault", line)
+ }
+ fun arm(name: String) { gates[name] = CompletableDeferred(); event("ARM $name") }
+ fun release(name: String) { gates.remove(name)?.complete(Unit); event("RELEASE $name") }
+ fun releaseAll() { gates.keys.toList().forEach { release(it) } }
+ suspend fun hold(name: String, drain: Boolean = false) {
+ if (!BuildConfig.DEBUG) return
+ val gate = gates[name] ?: return
+ event("HELD $name")
+ if (drain) withContext(NonCancellable) { gate.await() } else gate.await()
+ gates.remove(name, gate)
+ event("EXIT $name")
+ }
+ fun gateKey(key: String) = if (key.startsWith("USDT_RECOVERY_")) "USDT_RECOVERY" else key
+ suspend fun beforePut(key: String, data: ByteArray) {
+ event("PUT_ENTER $key sha256=${digest(data)}" + if (key == "SETTINGS") " currency=" + to.bitkit.di.json.decodeFromString<to.bitkit.models.SettingsBackupV1>(data.decodeToString()).settings.selectedCurrency else "")
+ try { hold("put:${gateKey(key)}") }
+ finally { hold("cleanup:${gateKey(key)}", drain = true) }
+ }
+ fun digest(data: ByteArray) = MessageDigest.getInstance("SHA-256").digest(data).joinToString("") { "%02x".format(it) }
+}
diff --git a/app/src/debug/java/to/bitkit/dev/BackupFaultProvider.kt b/app/src/debug/java/to/bitkit/dev/BackupFaultProvider.kt
new file mode 100644
index 0000000..82f5ad2
--- /dev/null
+++ b/app/src/debug/java/to/bitkit/dev/BackupFaultProvider.kt
@@ -0,0 +1,114 @@
+// test hook: shell-only control surface for disposable emulator.
+package to.bitkit.dev
+
+import android.app.Activity
+import android.content.ContentProvider
+import android.content.ContentValues
+import android.net.Uri
+import android.os.Binder
+import android.os.Bundle
+import android.graphics.Color
+import android.os.Handler
+import android.os.Looper
+import android.widget.TextView
+import android.widget.ScrollView
+import dagger.hilt.EntryPoint
+import dagger.hilt.InstallIn
+import dagger.hilt.components.SingletonComponent
+import dagger.hilt.android.EntryPointAccessors
+import kotlinx.coroutines.*
+import kotlinx.coroutines.flow.first
+import to.bitkit.data.CacheStore
+import to.bitkit.data.SettingsStore
+import to.bitkit.data.keychain.Keychain
+import to.bitkit.data.backup.VssBackupClient
+import to.bitkit.models.BackupCategory
+import to.bitkit.models.BackupItemStatus
+import to.bitkit.repositories.BackupRepo
+import to.bitkit.services.UsdtService
+import to.bitkit.testhook.BackupFault
+import java.util.concurrent.ConcurrentHashMap
+
+@EntryPoint
+@InstallIn(SingletonComponent::class)
+interface BackupFaultDependencies {
+ fun backup(): BackupRepo
+ fun cache(): CacheStore
+ fun settings(): SettingsStore
+ fun keychain(): Keychain
+ fun vss(): VssBackupClient
+ fun usdt(): UsdtService
+}
+
+class BackupFaultProvider : ContentProvider() {
+ private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
+ private val tasks = ConcurrentHashMap<String, Job>()
+ private val deps by lazy { EntryPointAccessors.fromApplication(requireNotNull(context), BackupFaultDependencies::class.java) }
+ override fun call(method: String, arg: String?, extras: Bundle?): Bundle {
+ check(Binder.getCallingUid() == 2000)
+ fun asyncTask(name: String, block: suspend () -> Unit) {
+ tasks[name] = scope.launch {
+ BackupFault.event("TASK_BEGIN $name")
+ try { block(); BackupFault.event("TASK_OK $name") }
+ catch (e: Exception) { BackupFault.event("TASK_ERROR $name ${e.javaClass.simpleName}") }
+ }
+ }
+ val answer = when(method) {
+ "init" -> { asyncTask("init") {
+ check(deps.keychain().loadString(Keychain.Key.BIP39_MNEMONIC.name) == null)
+ deps.keychain().saveString(Keychain.Key.BIP39_MNEMONIC.name, org.lightningdevkit.ldknode.generateEntropyMnemonic(org.lightningdevkit.ldknode.WordCount.WORDS12))
+ deps.vss().setup().getOrThrow()
+ }; "started" }
+ "prepare" -> { asyncTask("prepare") {
+ BackupFault.releaseAll(); deps.backup().setWiping(false); deps.backup().reset()
+ deps.settings().update { it.copy(selectedCurrency = "USD") }
+ deps.cache().update { it.copy(backupStatuses = BackupCategory.entries.associateWith { BackupItemStatus(required = 1000, synced = 1000) }) }
+ deps.vss().setup().getOrThrow()
+ }; "started" }
+ "cycle" -> { asyncTask("cycle") { repeat(5) { deps.backup().stopObservingBackups(); deps.backup().startObservingBackups(); delay(500) } }; "started" }
+ "start" -> { deps.backup().startObservingBackups(); BackupFault.event("START_REQUEST"); "ok" }
+ "stop" -> { deps.backup().stopObservingBackups(); BackupFault.event("STOP_REQUEST"); "ok" }
+ "arm" -> { BackupFault.arm(requireNotNull(arg)); "ok" }
+ "release" -> { BackupFault.release(requireNotNull(arg)); "ok" }
+ "currency" -> { asyncTask("currency") { deps.settings().update { it.copy(selectedCurrency = requireNotNull(arg)) } }; "started" }
+ "mark" -> { asyncTask("mark") { deps.cache().updateBackupStatus(BackupCategory.valueOf(requireNotNull(arg))) { it.copy(required = it.required + 1) } }; "started" }
+ "manual" -> { asyncTask("manual") { deps.backup().triggerBackup(BackupCategory.valueOf(requireNotNull(arg))).getOrThrow() }; "started" }
+ "full" -> { deps.backup().scheduleFullBackup(); BackupFault.event("FULL_REQUEST"); "ok" }
+ "usdt" -> { asyncTask("usdt") { deps.usdt().recoveryBackup.persist("disposable-synthetic-same-operation") }; "started" }
+ "reset" -> { asyncTask("reset") { deps.backup().reset() }; "started" }
+ "cancel" -> { tasks[arg]?.cancel(); BackupFault.event("CANCEL $arg"); "ok" }
+ "wipe" -> { deps.backup().setWiping(arg == "true"); BackupFault.event("WIPE $arg"); "ok" }
+ "clear" -> { BackupFault.events.clear(); "ok" }
+ "fetch" -> { asyncTask("fetch") { val item = deps.vss().getObject(requireNotNull(arg)).getOrThrow(); BackupFault.event("FETCH $arg present=${item != null} sha256=${item?.value?.let(BackupFault::digest)}" + if (arg == "SETTINGS" && item != null) " currency=" + to.bitkit.di.json.decodeFromString<to.bitkit.models.SettingsBackupV1>(item.value.decodeToString()).settings.selectedCurrency else "") }; "started" }
+ "status" -> runBlocking(Dispatchers.IO) {
+ "ack=" + deps.usdt().recoveryGeneration + " currency=" + deps.settings().data.first().selectedCurrency + " statuses=" + deps.cache().backupStatuses.first().filterKeys { it.name == (arg ?: "SETTINGS") }.entries.joinToString { "${it.key}=${it.value}" }
+ }
+ else -> BackupFault.events.joinToString("\n")
+ }
+ if (method == "status") BackupFault.event(answer)
+ return Bundle().apply { putString("result", answer) }
+ }
+ override fun onCreate() = true
+ override fun getType(uri: Uri): String? = null
+ override fun insert(uri: Uri, values: ContentValues?): Uri? = null
+ override fun delete(uri: Uri, sel: String?, args: Array<String>?) = 0
+ override fun update(uri: Uri, values: ContentValues?, sel: String?, args: Array<String>?) = 0
+ override fun query(uri: Uri, proj: Array<String>?, sel: String?, args: Array<String>?, sort: String?) = null
+}
+
+class BackupFaultActivity : Activity() {
+ private val handler = Handler(Looper.getMainLooper())
+ private lateinit var text: TextView
+ private val refresh = object : Runnable {
+ override fun run() {
+ text.text = "Backup lifecycle fault fixture\nPR source + local hold/log hooks\nReal DataStore and native encrypted VSS\nSynthetic recovery payload; no funds\n\n" + BackupFault.events.takeLast(25).joinToString("\n")
+ handler.postDelayed(this, 250)
+ }
+ }
+ override fun onCreate(saved: Bundle?) {
+ super.onCreate(saved)
+ text = TextView(this).apply { setTextColor(Color.WHITE); setBackgroundColor(Color.BLACK); textSize = 14f; setPadding(24,60,24,20) }
+ setContentView(ScrollView(this).apply { addView(text) }); handler.post(refresh)
+ }
+ override fun onDestroy() { handler.removeCallbacks(refresh); super.onDestroy() }
+}The transport fault fixture:
from http.server import ThreadingHTTPServer,BaseHTTPRequestHandler
from urllib.request import Request,urlopen
from urllib.error import HTTPError
from pathlib import Path
import threading,time
lock=threading.Lock()
state={'fail':0}
class Handler(BaseHTTPRequestHandler):
def log_message(self,*args): pass
def do_GET(self): self.relay()
def do_POST(self): self.relay()
def relay(self):
body=self.rfile.read(int(self.headers.get('Content-Length','0')))
if self.path=='/control/fail':
with lock: state['fail']=1000
self.send_response(200); self.end_headers(); self.wfile.write(b'armed'); return
if self.path=='/control/healthy':
with lock: state['fail']=0
self.send_response(200); self.end_headers(); self.wfile.write(b'healthy'); return
with lock:
fail=state['fail']>0 and self.command=='POST' and 'put' in self.path.lower()
if fail: state['fail']-=1
if fail:
self.send_response(409); self.send_header('Content-Type','application/octet-stream'); self.end_headers(); self.wfile.write(b'\x08\x01\x12\x13controlled conflict');print(time.time(),'FAULT_409',self.command,self.path,flush=True); return
headers={k:v for k,v in self.headers.items() if k.lower() not in ('host','connection','accept-encoding')}
req=Request('https://bitkit.stag0.blocktank.to/vss_rs_auth'+self.path,data=body if self.command=='POST' else None,headers=headers,method=self.command)
try: result=urlopen(req,timeout=90)
except HTTPError as e: result=e
except Exception as e:
self.send_response(502);self.end_headers();self.wfile.write(type(e).__name__.encode());print('PROXY_ERROR',type(e).__name__,flush=True);return
data=result.read();self.send_response(result.status)
for k,v in result.headers.items():
if k.lower() not in ('connection','transfer-encoding','content-length','content-encoding'): self.send_header(k,v)
self.send_header('Content-Length',str(len(data))); self.end_headers(); self.wfile.write(data)
print(time.time(),self.command,self.path.split('?')[0],result.status,flush=True)
ThreadingHTTPServer(('127.0.0.1',24889),Handler).serve_forever()Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
@piotr-iohk Resolved the parent conflict with signed additive merge c255458, preserving both connection-correction tests and lifecycle ownership. Final 4,681 unit/component tests, full compile/format/Detekt/Android Lint passed with zero findings; two independent final composition reviews were clean. All three commits in the live-parent range are locally verified and GitHub true/valid. New-head hosted checks are pending; no new authenticated composition journey is claimed. |
|
Published follow-up 49e9a99 retains explicit backup retry ownership after observation stops and reserves each queued USDT callback’s exact required revision before cancellation can interrupt its category wait. Older completions/timers preserve newer pending revisions and backoff. Controlled fail-before cases pass after repair; final 4,696 tests/255 suites (112 backup cases), full format/compile/Detekt/Android Lint and two consecutive independent final reviews pass. All five live-base commits verify locally and on GitHub. New-head hosted checks are pending; manual case 5/authenticated fault journeys remain unrun, and backup success does not acknowledge an earlier failed Core callback or submit a payment. Updated QA evidence preserves the prior head’s five failed E2E executions before eventual success. Requested Ovi’s affected re-review once; existing João/Piotr requests remain. |
Twin: synonymdev/bitkit-ios#1123
Dependency: #1686
This PR drains interrupted backup work before resetting its clients and preserves pending backups across observation restart.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A — background lifecycle repair.
QA Notes
Setup
Use the committed dev Debug dependencies: Core 0.8.0-rc11, Paykit 0.1.1-rc1 and VSS 0.5.26-rc1. Automated tests use synthetic data, a real local DataStore, the SDK operation barrier and fake VSS transport; no funds or device wallets are changed. Manual cases require a dedicated disposable fault fixture with held setup, capture, upload and cache-write boundaries as described in
journeys/backup-restore/wallet-snapshot-consistency.md. Those app-level controls are not provisioned in the standard journey environment. Do not use live funds, reset personal wallets or reuse another worker's device.Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
1. Hold backup setup or cancellation cleanup → stop/restart observation → edit settings while restart waits for old cleanup, including another category’s retry becoming due → no setup/upload before drain; latest changes resume without another edit — lifecycle fault gates are not in Capabilities.
2. Hold manual or USDT persistence → request fixture reset, including cancellation of its caller → both clients wait for old work; canceled persistence is not acknowledged and retry retains the same signed operation — reset/cache/VSS gates are not in Capabilities.
3. Keep wipe admission closed after reset → request manual/full/USDT backup → no new upload or cache write; after release, explicit USDT works with observation stopped — controlled wipe admission is not in Capabilities.
4. Change required data during a full backup, hold its required-status write, or request it during stopped-job cleanup → release the gate and restart → the newer work uploads without another mutation — capture/upload/cache-write gates are not in Capabilities.
5. With observation stopped, fail an explicit category or USDT backup temporarily, restore transport, and verify bounded backup retry without another edit; keep Core acknowledgement withheld until its own callback succeeds. Also cancel a newer callback while an older upload is held: the newer revision stays pending and the old upload cannot satisfy it — controlled transport/upload/cache boundaries are not in Capabilities.
Automated Checks
BackupRepoTest.kt— held setup and upload cleanup, reset cancellation, cache finalization, concurrent lifecycle calls with real DataStore, actual SDK wipe-lock ordering, full-backup follow-up, stopped-observer USDT persistence and actual service acknowledgement/retry record identity.Evidence
Controlled baseline reproduced late LDK setup after reset. Further fail-before tests reproduced lost full-backup follow-up, premature manual reset, fresh backup admission during wipe, a full request lost behind canceled cleanup, and consumed settings changes lost during stop/reset. The final focused suite has 93 cases, including 14 added lifecycle, admission and persistence regressions.
Final commit
a8f0810769b5c29c01c6f7c1f02e3988b048c7db, based on #1686 (ac2a7c668):just compile,just testandjust lintpassed: 4,639 tests across 254 suites, zero failures/errors/skips; fresh JUnit timestamps verified. Fulljust formatpassed. All Detekt and Android Lint SARIF/XML reports contain zero findings, with no compiler warnings; the JVM bootstrap-classpath notice and Gradle deprecation match recorded baseline diagnostics. Two consecutive final independent read-only reviews found no actionable correctness or quality issues.Follow-up verification on October 25 (signed head
e585fccf5): the settings-during-restart regression fails ona8f08107; a separate temporary WALLET retry with held WIDGETS cleanup fails without its admission guard. Both repaired cases pass in the 95-test backup suite. Finaljust compile,just test,just lintandjust formatpassed: 4,641 tests / 254 suites, zero failures/errors/skips, and all Detekt/Android Lint reports contain zero findings. Complete logs and reports inspected; structured Gradle diagnostics match the earlier baseline. Two consecutive independent final reviews were clean. Both commits in the actual-base PR range are locally verified and GitHub reportsverified=true,reason=valid. These controlled component cases extend manual cases 1 and 4; no full fault-injection journey pass is claimed.Prior published head
a8f08107also passed hosted CI (4,639 units) and all seven existing E2E shards (39 cases passed, one settings case skipped). Complete logs and checksum/CRC-verified uploaded lint artifact12583364581contain zero SARIF/XML findings. Those hosted results apply to that head, not the follow-up.Prior signed head
e585fccf5passed hosted CI40158027061(4,641 tests) and lint40158027049. Complete logs and checksum/CRC-verified artifact12585438925were inspected; all three SARIF/XML reports contain zero findings. Hosted E2E40158027317 subsequently completed all seven shards with39 passing cases. The backup case first failed waiting for the widget “Yes, Delete” confirmation, then passed on the workflow’s automatic retry. Complete logs were inspected; the initial widget-confirmation failure’s cause is still unproven, so this is not claimed as a clean first-attempt run. No additional workflow rerun was requested.Latest parent composition (October 26), signed head
c2554581e2b5e2c8e68802a577a76a56b291ae4b: additive merge of #1686 at0fd5e424, without rewriting published history. Both parent malformed connection-correction tests and the lifecycle ownership/USDT independence behavior are preserved. Finaljust compile,just test,just lint, andjust formatpassed: 4,681 tests / 255 suites, zero failures/errors/skips, 97 backup cases, and zero Detekt/Android Lint findings. Complete logs and structured reports were inspected; JVM, cursor and Detekt-plugin diagnostics match verified earlier parent evidence. All 1,765 frozen inputs match. Two consecutive independent final composition reviews were clean; all three current-base commits are locally signed and GitHub true/valid. Hosted composition checks passed 4,681 units, lint with zero findings, and 39 first-attempt E2E cases with one settings skip; complete logs and checksum/CRC-verified reports were inspected. These results apply to c255458, not the later transport commit, and are not an authenticated fault-journey pass.Latest transport follow-up, signed
a102e0ede565e60aaf7e033f3ad66a870739066b: the released VSS FFI represents request-send failures as UnknownException. Two controlled regressions fail before the fix; the narrow backup-only compatibility classifier and observed-settings retry then pass. Final4,683 tests/255suites, including99backup cases, pass with zero failures/errors/skips. Required format, compile, full Detekt and Android Lint pass with zero findings in all three fresh SARIF/XML reports. All1,760 frozen source inputs match. Complete9,629-line test log and structured reports were inspected; three JVM notices, the full cursor-allocation stack multiset and Detekt plugin diagnostics match verified parent evidence. Initial formatting corrected three introduced ordering/wrap findings before final gates. Two consecutive independent final reviews are clean; all four live-base commits are locally signed and GitHub true/valid. Hosted checks on a102e0e passed 4,683 units and lint with zero findings. Existing E2E cases reached 39 unique passes and one never-run settings skip after five failed executions: LNURL/Transfer failed two attempts before the third passed, and multiaddress required a second attempt. Complete logs and checksum/CRC-verified artifacts were inspected. These are eventual prior-head passes, not first-attempt or new-head evidence; the timeout causes remain unproven.This classifier is restricted to the typed native UnknownException with its anchored request-send envelope and valid HTTP(S) host/putObjects path. It does not change generic SDK mutation retry, cancellation or permanent unknown errors. Tests inject the actual native error representation into the repository; this is not a new authenticated endpoint-outage journey.
Latest explicit-retry follow-up, signed
49e9a99d35f72685f3187b9585fb2aaa3d64a904: manual/full/USDT backup retries retain their own ownership after observers stop. Supplied recovery snapshots reserve their exact required revision before a cancellable category wait, so canceled queued work remains pending and an older upload cannot acknowledge a newer snapshot. Superseded retry timers cannot bypass a newer failure’s backoff. Thirteen new regression cases include controlled before failures for stopped observation, successor revisions, stale timers and queued cancellation. The final suite passed 4,696 tests / 255 suites (112 backup cases), zero failures/errors/skips. Final format, compile, full Detekt and Android Lint pass with zero findings in all three structured reports. All 1,760 frozen hashes match; the complete 9,648-line test log, all XML/SARIF and retained Gradle diagnostics were inspected. Introduced formatting findings were corrected before the final unchanged gates; JVM/cursor/deprecation diagnostics match verified parent evidence. Two consecutive independent final reviews (rounds 2 and 3 after repairing the round-1 queued-callback finding) are clean. All five actual-live-base commits verify locally and on GitHub as true/valid. Current-head hosted CI40400088998 passed 4,696 tests/255 classes with zero failures/skips; lint40400089110 has zero findings in all structured reports. E2E40400089623 reached 39 unique passes and one never-run settings skip after three failed executions: RGS settings, LNURL and transfer each passed their second internal attempt. Complete 18,712 lines across 15 job logs and four SHA256/CRC-verified archives were inspected. The merge tree equals this head; JVM/cursor/deprecation diagnostics match verified parent evidence. The timeout causes remain unproven and failure screenshots were not uploaded after eventual success; no rerun was requested. Manual case 5 and complete authenticated fault journeys remain unrun; these controlled component tests use the real service callback with mocked transport and do not prove autonomous payment submission or process-death retry.Gaps
Explicit backup retry now has ownership independent of observation. Background backup success does not acknowledge an earlier failed Core callback or submit a payment; manual USDT Retry and foreground recovery must still resume and acknowledge the original signed operation. Cancellation retains required data without turning every canceled or permanent failure into a fresh automatic retry.
The new controls are component tests. They do not execute a complete app UI/Paykit/deployed-contract failure journey, and the VSS transport in these tests is mocked. Existing released-native signing/encrypted recovery evidence remains scoped to its recorded app revisions. No Android crash or historical contact/acquisition cause is claimed. The full-WALLET availability and migration engineering remain separate. Updated-app-only new-format restore is approved, but independent-only backups are not activated.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)