Skip to content

Commit e025ce1

Browse files
committed
fix: crash on send (main-thread DB), dedup connections, bouncer nick field
- CRASH: EventProcessor.findSelfEchoCandidate ran a synchronous db.query on the main thread (events collector runs on Main) -> IllegalStateException on every send. Replaced with a suspend @query DAO method (off-main + safe inside the HistoryBatch transaction). - dedup: NetworkRepository.addNetwork returns an existing equivalent network instead of inserting a duplicate (role-aware key), preventing duplicate server connections; NetworkDao.allNow. - bouncer form restores the nick field (host/port/tls/nick/username/password); buildNetworkEntity uses the real nick. (SASL PLAIN NUL separators verified correct — no :irc change.)
1 parent b015039 commit e025ce1

11 files changed

Lines changed: 279 additions & 75 deletions

File tree

app/src/main/kotlin/io/github/trevarj/motd/data/db/Daos.kt

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ interface NetworkDao {
3838

3939
@Query("SELECT * FROM networks WHERE parentId = :rootId")
4040
suspend fun childrenOf(rootId: Long): List<NetworkEntity>
41+
42+
// Snapshot of all rows for the app-level duplicate check in NetworkRepositoryImpl.addNetwork.
43+
// A one-shot read (not the observed Flow) so dedup is a simple suspend call; the networks
44+
// table is tiny (a handful of rows) so a full scan is cheap and avoids a per-identity index
45+
// that would need a schema bump (DB is v1, no migrations).
46+
@Query("SELECT * FROM networks")
47+
suspend fun allNow(): List<NetworkEntity>
4148
}
4249

4350
@Dao
@@ -155,6 +162,19 @@ interface MessageDao {
155162
@Query("SELECT * FROM messages WHERE bufferId = :bufferId AND msgid = :msgid LIMIT 1")
156163
suspend fun byMsgid(bufferId: Long, msgid: String): MessageEntity?
157164

165+
/**
166+
* Newest local self row for [bufferId] matching [text] within [lo]..[hi] serverTime, to collapse
167+
* an un-labeled echo into a pending/confirmed-local row (plans/03 echo heuristic). Un-confirmed
168+
* (pendingLabel set) rows rank first. A suspend @Query runs off the main thread and is
169+
* transaction-safe (both the live onChat path and the HistoryBatch withTransaction path).
170+
*/
171+
@Query(
172+
"""SELECT * FROM messages WHERE bufferId = :bufferId AND isSelf = 1 AND text = :text
173+
AND serverTime BETWEEN :lo AND :hi
174+
ORDER BY (pendingLabel IS NOT NULL) DESC, serverTime DESC, id DESC LIMIT 1"""
175+
)
176+
suspend fun findSelfEchoCandidate(bufferId: Long, text: String, lo: Long, hi: Long): MessageEntity?
177+
158178
// Delete a single row by primary key. Used to drop a failed local-echo row on retry/delete so
159179
// the resend does not leave a permanent duplicate "failed" bubble (plans/15 #10).
160180
@Query("DELETE FROM messages WHERE id = :id")

app/src/main/kotlin/io/github/trevarj/motd/data/repo/NetworkRepositoryImpl.kt

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,31 @@ package io.github.trevarj.motd.data.repo
22

33
import io.github.trevarj.motd.data.db.NetworkDao
44
import io.github.trevarj.motd.data.db.NetworkEntity
5+
import io.github.trevarj.motd.data.db.NetworkRole
56
import javax.inject.Inject
67
import kotlinx.coroutines.flow.Flow
78

89
// Thin pass-through over NetworkDao. Delete resolves the row by id first (Dao.delete takes an
9-
// entity); a missing row is a no-op.
10+
// entity); a missing row is a no-op. addNetwork additionally dedups against existing rows so
11+
// re-running onboarding / "Add network" for a server the user already has does not create a
12+
// duplicate NetworkEntity (which would spawn a second actor + socket for the same server).
1013
class NetworkRepositoryImpl @Inject constructor(
1114
private val networkDao: NetworkDao,
1215
) : NetworkRepository {
1316
override fun observeNetworks(): Flow<List<NetworkEntity>> = networkDao.observeAll()
1417

15-
override suspend fun addNetwork(n: NetworkEntity): Long = networkDao.insert(n)
18+
/**
19+
* Insert [n], or return the id of an existing equivalent network instead of creating a
20+
* duplicate. Two rows are "the same server" when [networkIdentityKey] matches (see there for
21+
* the per-role key). The dedup is at the data layer so every add path (onboarding, Add
22+
* network, soju child import) is covered transparently and callers keep the "returns the row
23+
* id" contract — they just get the pre-existing id on a duplicate.
24+
*/
25+
override suspend fun addNetwork(n: NetworkEntity): Long {
26+
val key = networkIdentityKey(n)
27+
networkDao.allNow().firstOrNull { networkIdentityKey(it) == key }?.let { return it.id }
28+
return networkDao.insert(n)
29+
}
1630

1731
override suspend fun updateNetwork(n: NetworkEntity) = networkDao.update(n)
1832

@@ -24,3 +38,31 @@ class NetworkRepositoryImpl @Inject constructor(
2438

2539
override suspend fun childrenOf(rootId: Long): List<NetworkEntity> = networkDao.childrenOf(rootId)
2640
}
41+
42+
/** Normalize a host for identity comparison: trim, drop a trailing dot, lowercase (DNS is
43+
* case-insensitive). Hostnames are ASCII so [lowercase] with the default locale is safe. */
44+
internal fun normalizeHost(host: String): String =
45+
host.trim().trimEnd('.').lowercase()
46+
47+
/**
48+
* Stable identity key deciding whether two [NetworkEntity] rows are the same server, used by
49+
* [NetworkRepositoryImpl.addNetwork] to reject duplicates. Keyed per role:
50+
*
51+
* - **BOUNCER_CHILD**: `(parentId, bouncerNetId)` — a child is one bouncer-side network under one
52+
* root, regardless of host (the mirror may not know the host yet). Guards both the onboarding
53+
* import loop and the notify-mirror racing to insert the same child.
54+
* - **BOUNCER_ROOT**: `(host, port, saslUser)` — one soju account (login) per host:port. Adding
55+
* the same bouncer account twice reuses the existing root.
56+
* - **DIRECT**: `(host, port, nick)` — the same server with the same nick is the same connection;
57+
* a different nick is intentionally a distinct network (two identities on one server).
58+
*
59+
* A `null` sub-key element is kept distinct (encoded as an empty segment) so under-specified rows
60+
* don't collapse onto each other.
61+
*/
62+
internal fun networkIdentityKey(n: NetworkEntity): String = when (n.role) {
63+
NetworkRole.BOUNCER_CHILD -> "child|${n.parentId}|${n.bouncerNetId.orEmpty()}"
64+
NetworkRole.BOUNCER_ROOT ->
65+
"root|${normalizeHost(n.host)}|${n.port}|${n.saslUser.orEmpty()}"
66+
NetworkRole.DIRECT ->
67+
"direct|${normalizeHost(n.host)}|${n.port}|${n.nick}"
68+
}

app/src/main/kotlin/io/github/trevarj/motd/data/sync/EventProcessor.kt

Lines changed: 9 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -515,40 +515,16 @@ class EventProcessor @Inject constructor(
515515
* strongest match); a confirmed-local row (from the no-labeled-response send path) also matches
516516
* so its self-clock dedupKey collapses with the server echo. Returns null when nothing matches.
517517
*/
518-
private suspend fun findSelfEchoCandidate(bufferId: Long, text: String, echoTime: Long): MessageEntity? {
519-
// Run synchronously on the calling coroutine (no Dispatchers.IO switch): this can execute
520-
// inside a Room withTransaction (HistoryBatch), where hopping threads violates Room's
521-
// transaction confinement and throws. db.query() is a plain blocking cursor read.
522-
val lo = echoTime - ECHO_MATCH_WINDOW_MS
523-
val hi = echoTime + ECHO_MATCH_WINDOW_MS
524-
val q = androidx.sqlite.db.SimpleSQLiteQuery(
525-
"SELECT * FROM messages WHERE bufferId = ? AND isSelf = 1 AND text = ? " +
526-
"AND serverTime BETWEEN ? AND ? " +
527-
// Un-confirmed rows first (pendingLabel set), then newest.
528-
"ORDER BY (pendingLabel IS NOT NULL) DESC, serverTime DESC, id DESC LIMIT 1",
529-
arrayOf<Any>(bufferId, text, lo, hi),
518+
private suspend fun findSelfEchoCandidate(bufferId: Long, text: String, echoTime: Long): MessageEntity? =
519+
// Delegates to a suspend @Query: Room runs it off the main thread (the events collector runs
520+
// on Dispatchers.Main) and handles it correctly inside the HistoryBatch withTransaction too —
521+
// the previous raw db.query() ran synchronously on the caller's thread and crashed on send.
522+
messageDao.findSelfEchoCandidate(
523+
bufferId,
524+
text,
525+
echoTime - ECHO_MATCH_WINDOW_MS,
526+
echoTime + ECHO_MATCH_WINDOW_MS,
530527
)
531-
return db.query(q).use { c ->
532-
if (!c.moveToFirst()) return@use null
533-
fun col(name: String) = c.getColumnIndexOrThrow(name)
534-
MessageEntity(
535-
id = c.getLong(col("id")),
536-
bufferId = c.getLong(col("bufferId")),
537-
msgid = c.getString(col("msgid")),
538-
serverTime = c.getLong(col("serverTime")),
539-
sender = c.getString(col("sender")),
540-
senderAccount = c.getString(col("senderAccount")),
541-
kind = MessageKind.valueOf(c.getString(col("kind"))),
542-
text = c.getString(col("text")),
543-
isSelf = c.getInt(col("isSelf")) != 0,
544-
hasMention = c.getInt(col("hasMention")) != 0,
545-
replyToMsgid = c.getString(col("replyToMsgid")),
546-
pendingLabel = c.getString(col("pendingLabel")),
547-
failed = c.getInt(col("failed")) != 0,
548-
dedupKey = c.getString(col("dedupKey")),
549-
)
550-
}
551-
}
552528

553529
/** Buffer ids where [nick] is currently a member on [networkId] (for quit/nick fan-out). */
554530
private suspend fun buffersOfNick(networkId: Long, nick: String): List<Long> =

app/src/main/kotlin/io/github/trevarj/motd/ui/onboarding/OnboardingReducer.kt

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,15 @@ data class ServerForm(
5555
)
5656

5757
/**
58-
* Minimal validity for enabling "next" on the direct-network SERVER step: host, valid port,
59-
* and a nick. The soju path validates against [isValidForSoju] (no nick collected there).
58+
* SERVER-step validity for both paths: host, a valid port, and a nick. The soju root now
59+
* collects a nick too (it is the IRC NICK the bouncer registers with); its bouncer SASL
60+
* username/password are gathered on the AUTH step.
6061
*/
6162
val isValid: Boolean
62-
get() = isValidForSoju && nick.isNotBlank()
63+
get() = hostAndPortValid && nick.isNotBlank()
6364

64-
/** soju SERVER step needs only host + a valid port (identity comes from SASL). */
65-
val isValidForSoju: Boolean
65+
/** Transport-only validity (host + valid port), independent of identity. */
66+
val hostAndPortValid: Boolean
6667
get() = host.isNotBlank() &&
6768
port.toIntOrNull()?.let { it in 1..65535 } == true
6869
}
@@ -117,8 +118,8 @@ data class OnboardingState(
117118
get() = when (step) {
118119
OnboardingStep.WELCOME -> true
119120
OnboardingStep.CHOICE -> choice != null
120-
// soju collects only host/port on SERVER (identity is SASL); direct also needs a nick.
121-
OnboardingStep.SERVER -> if (isSoju) server.isValidForSoju else server.isValid
121+
// Both paths collect host/port/nick on SERVER; soju's SASL user/password gate AUTH.
122+
OnboardingStep.SERVER -> server.isValid
122123
OnboardingStep.AUTH -> auth.isValid
123124
OnboardingStep.CONNECT -> isReady
124125
OnboardingStep.FINISH -> true

app/src/main/kotlin/io/github/trevarj/motd/ui/onboarding/OnboardingScreen.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,8 +285,10 @@ private fun ServerPage(
285285
onAuthChange = onAuthChange,
286286
showServer = !authOnly,
287287
showAuth = authOnly,
288-
// soju SERVER step: host/port/TLS only; identity is managed by the bouncer via SASL.
288+
// Direct path shows the full identity (nick/username/realname); the soju root shows
289+
// only the nick here (its bouncer SASL user/password live on the AUTH step).
289290
showIdentity = !state.isSoju,
291+
showNick = state.isSoju,
290292
)
291293
}
292294
}

app/src/main/kotlin/io/github/trevarj/motd/ui/settings/NetworkForm.kt

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,23 @@ fun NetworkForm(
4747
modifier: Modifier = Modifier,
4848
showServer: Boolean = true,
4949
showAuth: Boolean = true,
50-
// soju bouncer: identity (nick/username/realname) is managed per-network by soju via SASL,
51-
// so the root form collects only host/port/TLS and hides the identity fields.
50+
// Full USER-ident identity (nick + username + realname): the direct-network path shows all
51+
// three. The soju root shows only [showNick] (its nick), since the bouncer SASL user/password
52+
// live on the AUTH step and username/realname default to the nick.
5253
showIdentity: Boolean = true,
54+
showNick: Boolean = false,
5355
) {
5456
Column(
5557
modifier = modifier.fillMaxWidth().padding(horizontal = 16.dp),
5658
verticalArrangement = Arrangement.spacedBy(12.dp),
5759
) {
5860
if (showServer) {
59-
ServerFields(server = server, onServerChange = onServerChange, showIdentity = showIdentity)
61+
ServerFields(
62+
server = server,
63+
onServerChange = onServerChange,
64+
showIdentity = showIdentity,
65+
showNick = showNick,
66+
)
6067
}
6168
if (showAuth) {
6269
Text(
@@ -71,14 +78,16 @@ fun NetworkForm(
7178
}
7279

7380
/**
74-
* Server fields. [showIdentity] gates nick/username/realname: soju manages those per bound
75-
* network via SASL, so its root form shows only host/port/TLS.
81+
* Server fields. [showIdentity] gates the full nick/username/realname block (direct networks);
82+
* [showNick] surfaces just the nick (soju root), whose username/realname default to the nick and
83+
* whose SASL login lives on the AUTH step.
7684
*/
7785
@Composable
7886
private fun ServerFields(
7987
server: ServerForm,
8088
onServerChange: (ServerForm) -> Unit,
8189
showIdentity: Boolean = true,
90+
showNick: Boolean = false,
8291
) {
8392
OutlinedTextField(
8493
value = server.host,
@@ -103,14 +112,17 @@ private fun ServerFields(
103112
// withTls re-defaults the port (6697<->6667) unless the user typed a custom one.
104113
Switch(checked = server.tls, onCheckedChange = { onServerChange(server.withTls(it)) })
105114
}
106-
if (showIdentity) {
115+
// Nick is shown for the full-identity direct path and for the nick-only soju root.
116+
if (showIdentity || showNick) {
107117
OutlinedTextField(
108118
value = server.nick,
109119
onValueChange = { onServerChange(server.copy(nick = it)) },
110120
label = { Text(stringResource(R.string.onboarding_field_nick)) },
111121
singleLine = true,
112122
modifier = Modifier.fillMaxWidth(),
113123
)
124+
}
125+
if (showIdentity) {
114126
OutlinedTextField(
115127
value = server.username,
116128
onValueChange = { onServerChange(server.copy(username = it)) },
@@ -220,9 +232,10 @@ fun buildNetworkEntity(
220232
parentId: Long? = null,
221233
bouncerNetId: String? = null,
222234
): NetworkEntity {
223-
// soju manages per-network identity itself; its root form collects no nick/username/realname,
224-
// but :irc still sends USER/NICK on the root socket, so seed them from the SASL login username
225-
// (falling back to a placeholder) to keep the registration lines well-formed.
235+
// :irc sends NICK/USER on every socket (incl. the soju root). The nick is now collected on
236+
// both paths, so prefer it; fall back to the SASL login user, then a placeholder, only as a
237+
// last resort to keep the registration lines well-formed. username/realname default to the
238+
// nick when not surfaced.
226239
val identitySeed = server.nick.ifBlank { auth.saslUser.ifBlank { DEFAULT_IDENTITY } }
227240
return NetworkEntity(
228241
id = id,

0 commit comments

Comments
 (0)