d4cde6a · opencode-android

git.iko.soy/opencode-android

Fix reconnect concurrent-call leak, profile lastUsed clobber on save timeout, silent server-switch restore failure, and complete stub refresh failure test

commitd4cde6a2041d4a562c7da5063f2e9e1c058692cc authoriko <mail@iko.soy> dateTue 30 Jun 2026, 14:09 (3 months ago) parent6416394

8 files changed+78−23

compare with master

unifiedsplit

app/src/main/java/soy/iko/opencode/data/repo/ProfileStore.kt+10−1 app/src/main/java/soy/iko/opencode/ui/chat/ChatViewModel.kt+17−12 app/src/main/java/soy/iko/opencode/ui/server/ServerEditViewModel.kt+6 app/src/main/java/soy/iko/opencode/ui/session/SessionListViewModel.kt+17−5 app/src/main/res/values/strings.xml+1 app/src/test/java/soy/iko/opencode/data/network/WithRetryTest.kt+1 app/src/test/java/soy/iko/opencode/ui/session/SessionListViewModelTest.kt+21−4 app/src/test/java/soy/iko/opencode/ui/testing/TestFakes.kt+5−1

app/src/main/java/soy/iko/opencode/data/repo/ProfileStore.kt+10−1

@@ -142,12 +142,21 @@ open class ProfileStore private constructor(

142142 runCatching { OpencodeJson.decodeFromString(ListSerializer(StoredProfile.serializer()), it) }

143143 .getOrDefault(emptyList())

144144 } ?: emptyList()

145+ // If the incoming profile has lastUsed=0 (e.g. ServerEditViewModel.save()

146+ // couldn't read the existing value due to a DataStore timeout), preserve

147+ // the existing nonzero lastUsed so saving an edit doesn't reset the

148+ // profile's sort position in the server list.

149+ val preservedLastUsed = if (profile.lastUsed == 0L) {

150+ current.firstOrNull { it.id == profile.id }?.lastUsed ?: 0L

151+ } else {

152+ profile.lastUsed

153+ }

145154 val stored = StoredProfile(

146155 id = profile.id,

147156 label = profile.label,

148157 baseUrl = profile.baseUrl,

149158 username = profile.username?.takeIf { it.isNotBlank() },

150− lastUsed = profile.lastUsed,

159+ lastUsed = preservedLastUsed,

151160 )

152161 val updated = current.filterNot { it.id == profile.id } + stored

153162 prefs[profilesKey] = OpencodeJson.encodeToString(ListSerializer(StoredProfile.serializer()), updated)

app/src/main/java/soy/iko/opencode/ui/chat/ChatViewModel.kt+17−12

@@ -431,19 +431,24 @@ class ChatViewModel(

431431 /** Reconnect to the most recently used server profile (used when the connection is gone). */

432432 fun reconnect() {

433433 if (container.activeConnection.value != null) return

434+ // Guard against concurrent reconnect calls: the user can tap the button

435+ // multiple times while _reconnecting is true, which would launch parallel

436+ // connect() coroutines that race to set the active connection, leaking the

437+ // intermediate connections.

438+ if (!_reconnecting.compareAndSet(false, true)) return

434439 viewModelScope.launch {

435− _reconnecting.value = true

436− val recent = runCatchingCancellable { container.profileStore.profiles.first() }

437− .getOrDefault(emptyList())

438− .firstOrNull() ?: run { _reconnecting.value = false; return@launch }

439− runCatchingCancellable {

440− val conn = container.connect(recent)

441− conn.api.ping()

442− }.onSuccess {

443− _reconnecting.value = false

444− }.onFailure {

445− container.disconnect()

446− _error.value = container.friendlyError(it)

440+ try {

441+ val recent = runCatchingCancellable { container.profileStore.profiles.first() }

442+ .getOrDefault(emptyList())

443+ .firstOrNull() ?: return@launch

444+ runCatchingCancellable {

445+ val conn = container.connect(recent)

446+ conn.api.ping()

447+ }.onFailure {

448+ container.disconnect()

449+ _error.value = container.friendlyError(it)

450+ }

451+ } finally {

447452 _reconnecting.value = false

448453 }

449454 }

app/src/main/java/soy/iko/opencode/ui/server/ServerEditViewModel.kt+6

@@ -91,6 +91,12 @@ class ServerEditViewModel(

9191 _state.update { it.copy(saving = true) }

9292 viewModelScope.launch {

9393 val result = runCatchingCancellable {

94+ // Preserve the existing lastUsed timestamp so saving an edit doesn't

95+ // reset the profile's sort position (profiles are sorted descending

96+ // by lastUsed). connect() is responsible for updating lastUsed on

97+ // actual server use. If loading the existing profile times out,

98+ // lastUsed=0 is passed — ProfileStore.save() preserves the existing

99+ // nonzero lastUsed in that case so the sort position isn't lost.

94100 val existingLastUsed = if (s.id != null) {

95101 withTimeoutOrNull(NetworkConfig.profileLoadTimeoutMs) {

96102 container.profileStore.profiles.first()

app/src/main/java/soy/iko/opencode/ui/session/SessionListViewModel.kt+17−5

@@ -307,16 +307,28 @@ class SessionListViewModel(private val container: AppContainer) : ViewModel() {

307307 _switchingId.value = null

308308 result.onSuccess { refresh() }

309309 .onFailure { error ->

310− // connect() already closed the old connection; if we just disconnect()

311− // the user is left with nothing. Try to restore the previous profile so

312− // the session list survives. If there was none, show the error state.

310+ // connect() already closed the old connection; try to restore the

311+ // previous profile so the session list survives. If there was none,

312+ // show the error state.

313313 container.disconnect()

314314 if (previousProfile != null) {

315− runCatchingCancellable { container.connect(previousProfile) }

315+ val restored = runCatchingCancellable { container.connect(previousProfile) }

316316 // connect() clears unread state; restore what we saved so the

317317 // user doesn't lose unread badges on a failed server switch.

318318 savedUnread.forEach { container.restoreUnread(it) }

319− _state.update { it.copy(loading = false, error = container.friendlyError(error)) }

319+ if (restored.isSuccess) {

320+ _state.update { it.copy(loading = false, error = container.friendlyError(error)) }

321+ } else {

322+ // Restore also failed — the user is now disconnected from

323+ // both servers. Surface both failures so the user understands

324+ // why they're looking at an empty list.

325+ val restoreMsg = container.friendlyError(restored.exceptionOrNull() ?: error)

326+ _state.value = SessionListState(

327+ loading = false,

328+ error = container.string(R.string.error_switch_and_restore_failed,

329+ container.friendlyError(error), restoreMsg),

330+ )

331+ }

320332 } else {

321333 _state.value = SessionListState(loading = false, error = container.friendlyError(error))

322334 }

app/src/main/res/values/strings.xml+1

@@ -96,6 +96,7 @@

9696 <string name="error_timeout">The request timed out. Check the connection and try again.</string>

9797 <string name="error_server">The server reported an error. Try again later.</string>

9898 <string name="error_load_timeout">Loading took too long. Try again.</string>

99+ <string name="error_switch_and_restore_failed">Could not connect to the new server (%1$s), and restoring the previous server also failed (%2$s).</string>

99100

100101

101102 <string name="thinking">Thinking…</string>

app/src/test/java/soy/iko/opencode/data/network/WithRetryTest.kt+1

@@ -16,6 +16,7 @@ import org.junit.Assert.assertTrue

1616import org.junit.Test

1717import java.io.IOException

1818

19+@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)

1920class WithRetryTest {

2021

2122 /** Produce the real exception Ktor throws for a status when expectSuccess = true. */

app/src/test/java/soy/iko/opencode/ui/session/SessionListViewModelTest.kt+21−4

@@ -97,11 +97,28 @@ class SessionListViewModelTest {

9797 val vm = makeVm(container)

9898 assertTrue(vm.state.value.sessions.isNotEmpty())

9999

100− // Now make listSessions fail

101− api.sessions = listOf()

100+ // Now make listSessions fail — the existing list should remain and a

101+ // transient error should be surfaced (not replace the whole list).

102+ repo.listSessionsThrows = IOException("network error")

103+ vm.refresh()

104+ testScheduler.advanceUntilIdle()

105+ assertTrue(vm.state.value.sessions.isNotEmpty())

106+ assertFalse(vm.state.value.loading)

107+ assertNotNull(vm.transientError.value)

108+ }

109+

110+ @Test

111+ fun refresh_failureWithNoSessions_setsErrorState() = testScope.runTest {

112+ val api = FakeOpencodeApiClient()

113+ val events = FakeEventStreamClient()

114+ val repo = FakeSessionRepository(api, events)

102115 repo.sessions = emptyList()

103− // Can't easily make listSessions throw via FakeSessionRepository since it

104− // returns sessions directly. Test that the error path for empty + failure works.

116+ repo.listSessionsThrows = IOException("network error")

117+ val container = makeContainer(api = api, events = events, repo = repo)

118+ val vm = makeVm(container)

119+ assertFalse(vm.state.value.loading)

120+ assertNotNull(vm.state.value.error)

121+ assertTrue(vm.state.value.sessions.isEmpty())

105122 }

106123

107124 // --- SSE events ---

app/src/test/java/soy/iko/opencode/ui/testing/TestFakes.kt+5−1

@@ -222,10 +222,14 @@ class FakeSessionRepository(

222222 var createdSession: Session = Session(id = "new-session", title = "New")

223223 var messages: List<MessageWithParts> = emptyList()

224224 var sendPromptThrows: Throwable? = null

225+ var listSessionsThrows: Throwable? = null

225226 var abortCalls: List<String> = emptyList()

226227 private set

227228

228− override suspend fun listSessions(): List<Session> = sessions

229+ override suspend fun listSessions(): List<Session> {

230+ listSessionsThrows?.let { throw it }

231+ return sessions

232+ }

229233 override suspend fun createSession(title: String?): Session = createdSession

230234 override suspend fun deleteSession(id: String) { /* no-op */ }

231235 override suspend fun abort(sessionId: String) { abortCalls = abortCalls + sessionId }