Sulkta-Coop/straw
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

vc=40: loop round 2/5 — round-1 misses + new HIGHs from round-5 audits

Browse source 
Three parallel Opus round-5 audits ran on vc=39. The big finds were
regressions on vc=39's own fixes — the retry-init wiring was incomplete,
the URL-fence pattern missed CancellationException swallowing, and the
SettingsStore idempotency branch was dead code. Real round-5 work.

HIGH
  R5-1  Rust `ensure_initialized` was only called from `init_logging`
        — extractor entry points never invoked it, so the 5s-backoff
        retry from vc=39 was unreachable. A cold-boot init failure
        still bricked extraction for the whole process. Now every
        `search()` / `stream_info()` / `channel_info()` calls
        ensure_initialized at entry; cheap when INITIALIZED is true.
  R5-2  runtime.rs in-progress race: prior shape stamped
        LAST_ATTEMPT_MS at the *start* of an attempt, then let
        concurrent callers short-circuit via the backoff check
        and proceed to call into the extractor before init
        completed. New: lock FIRST (mutex IS the in-progress
        queue), re-check INITIALIZED under lock, only THEN check
        backoff (and only stamp it on failure).
  R5-3  `runCatching` in VideoDetailViewModel + SubscriptionFeed
        swallows CancellationException — when a job was cancelled
        mid-suspend inside channelInfo/RYD/SB, the inner cancellation
        was eaten, the lambda returned its default, the job carried
        past the runCatching to its terminal write, and the URL fence
        let it through because same-URL races can't be distinguished
        by string equality. New util.runCatchingCancellable
        re-throws CancellationException; all coroutine-body
        runCatching sites in the affected VMs migrated.
  R5-4  SearchViewModel.submit post-network fence only guarded
        `_ui.update`. SearchCache.record + pool rebuild proceeded
        for a cancelled query → ghost cache entries for queries
        the user abandoned mid-stroke. Now: re-check the query
        AFTER the IO suspend and before the cache write.
  R5-5  ChannelViewModel.load / VideoDetailViewModel.load now gate
        the extractor entry on isAllowedYtUrl(channelUrl/streamUrl).
        Prior shape only gated recordWatch persistence — extractor
        invocation for poisoned uploaderUrl still happened.

MED
  R5-6  SettingsStore.set{MaxResolution,ThemeMode}: vc=39 used
        `updateAndGet { r } == r` which is unconditionally true
        (lambda ignores prior) — the in-memory equality check was
        dead code. SP-side check still gated the disk write so the
        feature worked, but the dead branch was a comment-vs-code
        liar. Rewrote with explicit before-capture + equality
        gate.
  R5-7  SponsorBlockSkipLoop polled `controller.currentPosition`
        every 150ms even when paused — paused-overnight playback
        ate ~24k binder calls/hour. Now: when `!isPlaying`, sleep
        1s and continue.
  R5-8  StrawApp.appScope had no CoroutineExceptionHandler — an
        uncaught throwable in a top-level launch could crash on
        cold start even with SupervisorJob (top-level failure
        still propagates to default handler). Added handler that
        logs via strawLogW.
  R5-9  YtUrl.isAllowedYtUrl now requires http/https scheme
        (schemeless `//host/...` URLs no longer pass) and strips
        a single trailing dot from host (RFC FQDN form). Defense
        in depth.

LOW
  R5-10 NowPlaying.set() removed — non-CAS setter footgun
        alongside the race-free claim()/CAS path. No external
        callers (grep clean). Doc-comment updated.

Deferred (later round / different scope):
  - PlaylistsStore URL canonicalization (round-5 MED-1 — needs a
    shared YT-id-extract util; not blocking).
  - Release R8 + Nav rememberSaveable (still deferred).
  - LazyColumn keys + collectAsStateWithLifecycle (cosmetic).
  - SponsorBlockSkipLoop currentMediaItem binding (round-5
    deferred).
This commit is contained in:
Kayos 2026-05-25 15:12:30 -07:00
parent b8325d1726
commit da48109a4d
15 changed files with 141 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

13
strawApp/src/main/kotlin/com/sulkta/straw/StrawApp.kt
View file

@ -13,6 +13,8 @@ import com.sulkta.straw.data.SearchCache
import com.sulkta.straw.data.Settings
import com.sulkta.straw.data.Subscriptions
import com.sulkta.straw.feature.dataimport.SettingsImport
import com.sulkta.straw.util.strawLogW
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
@ -22,9 +24,16 @@ class StrawApp : Application() {
/**
* App-scoped coroutine scope for one-time startup work that
* shouldn't tie up Application.onCreate. SupervisorJob so a failure
* in one launch doesn't cascade.
* in one launch doesn't cascade. CoroutineExceptionHandler so an
* uncaught throwable in a top-level launch doesn't crash the
* process on cold start (would otherwise hit the default handler
* even with SupervisorJob). Round-5 audit MED-3.
*/
private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
private val appScope = CoroutineScope(
SupervisorJob() + Dispatchers.IO + CoroutineExceptionHandler { _, t ->
strawLogW("StrawApp") { "appScope uncaught: ${t.javaClass.simpleName}: ${t.message}" }
},
)
override fun onCreate() {
super.onCreate()

23
strawApp/src/main/kotlin/com/sulkta/straw/data/SettingsStore.kt
View file

@ -72,28 +72,29 @@ class SettingsStore(context: Context) {
sp.edit().putStringSet(KEY_SB_CATS, next.map { it.key }.toSet()).apply()
}
// Atomic + idempotent. updateAndGet matches toggle()'s CAS shape;
// idempotency short-circuit means tap-spamming the radio rows
// (or replaying a settings import that doesn't actually change a
// value) doesn't repeatedly hit SP. Round-4 audit MED-2.
// Atomic + idempotent. Capture before-state, update in-memory,
// skip the SP write when the value didn't actually change. Round-5
// audit LOW-1 / MED-2: the prior shape used
// `updateAndGet { r } == r` which is unconditionally true (lambda
// ignores prior) — dead code that confused readers.
fun setMaxResolution(r: MaxResolution) {
val updated = _maxResolution.updateAndGet { r } == r
if (!updated) return
if (sp.getString(KEY_MAX_RES, null) == r.name) return
val before = _maxResolution.value
if (before == r) return
_maxResolution.value = r
sp.edit().putString(KEY_MAX_RES, r.name).apply()
}
fun setThemeMode(t: ThemeMode) {
val updated = _themeMode.updateAndGet { t } == t
if (!updated) return
if (sp.getString(KEY_THEME, null) == t.name) return
val before = _themeMode.value
if (before == t) return
_themeMode.value = t
sp.edit().putString(KEY_THEME, t.name).apply()
}
fun setCacheEnabled(enabled: Boolean) {
val before = _cacheEnabled.value
_cacheEnabled.updateAndGet { enabled }
if (before == enabled) return
_cacheEnabled.value = enabled
sp.edit().putBoolean(KEY_CACHE_ENABLED, enabled).apply()
}

9
strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt
View file

@ -12,6 +12,7 @@ package com.sulkta.straw.feature.channel
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.sulkta.straw.feature.search.StreamItem
import com.sulkta.straw.util.isAllowedYtUrl
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Job
import kotlinx.coroutines.flow.MutableStateFlow
@ -43,6 +44,14 @@ class ChannelViewModel : ViewModel() {
fun load(channelUrl: String) {
if (loadedUrl == channelUrl && _ui.value.videos.isNotEmpty()) return
// Round-5 audit MED-3: extractor-emitted uploaderUrl can be
// attacker-controlled if the YT response is poisoned upstream.
// Refuse non-YT hosts at the entry point so we don't even
// issue a network call to evil.com via strawcore.
if (!isAllowedYtUrl(channelUrl)) {
_ui.update { ChannelUiState(loading = false, error = "Unsupported URL") }
return
}
inFlight?.cancel()
loadedUrl = channelUrl
_ui.update { ChannelUiState(loading = true) }

16
strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt
View file

@ -25,6 +25,7 @@ import com.sulkta.straw.net.SbSegment
import com.sulkta.straw.net.SponsorBlockClient
import com.sulkta.straw.feature.search.StreamItem
import com.sulkta.straw.util.isAllowedYtUrl
import com.sulkta.straw.util.runCatchingCancellable
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
@ -100,6 +101,13 @@ class VideoDetailViewModel : ViewModel() {
// navigations. Skip the refetch if the requested URL already has
// a resolved state.
if (loadedUrl == streamUrl && _ui.value.detail != null) return
// Same YT-host gate as ChannelViewModel — covers the case
// where a tap on a poisoned related-card lands here.
// Round-5 audit MED-3.
if (!isAllowedYtUrl(streamUrl)) {
_ui.update { VideoDetailUiState(loading = false, error = "Unsupported URL") }
return
}
inFlight?.cancel()
loadedUrl = streamUrl
_ui.update { VideoDetailUiState(loading = true) }
@ -122,7 +130,7 @@ class VideoDetailViewModel : ViewModel() {
// Round-4 audit HIGH-5.
if (isAllowedYtUrl(streamUrl)) {
withContext(Dispatchers.IO) {
runCatching {
runCatchingCancellable {
History.get().recordWatch(
WatchHistoryItem(
url = streamUrl,
@ -138,11 +146,11 @@ class VideoDetailViewModel : ViewModel() {
}
val ryd = withContext(Dispatchers.IO) {
runCatching { RydClient.fetch(videoId) }.getOrNull()
runCatchingCancellable { RydClient.fetch(videoId) }.getOrNull()
}
val sbCats = Settings.get().sbCategories.value.map { it.key }
val segments = if (sbCats.isEmpty()) emptyList() else withContext(Dispatchers.IO) {
runCatching { SponsorBlockClient.fetch(videoId, sbCats) }
runCatchingCancellable { SponsorBlockClient.fetch(videoId, sbCats) }
.getOrDefault(emptyList())
}
@ -168,7 +176,7 @@ class VideoDetailViewModel : ViewModel() {
val uploaderUrl = info.uploaderUrl
val moreFromChannel: List<StreamItem> =
if (uploaderUrl.isNullOrBlank() || !isAllowedYtUrl(uploaderUrl)) emptyList()
else runCatching {
else runCatchingCancellable {
val ch = uniffi.strawcore.channelInfo(uploaderUrl)
ch.videos
.filter { it.url != streamUrl }

5
strawApp/src/main/kotlin/com/sulkta/straw/feature/feed/SubscriptionFeedViewModel.kt
View file

@ -24,6 +24,7 @@ import com.sulkta.straw.data.FeedCacheEntry
import com.sulkta.straw.data.Settings
import com.sulkta.straw.data.Subscriptions
import com.sulkta.straw.feature.search.StreamItem
import com.sulkta.straw.util.runCatchingCancellable
import com.sulkta.straw.util.strawLogW
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
@ -223,7 +224,7 @@ class SubscriptionFeedViewModel : ViewModel() {
private suspend fun fetchChannelInto(ch: ChannelRef) {
val outcome = withTimeoutOrNull(perChannelTimeoutMs) {
runCatching {
runCatchingCancellable {
val info = uniffi.strawcore.channelInfo(ch.url)
// Opportunistic avatar refresh: if our stored ChannelRef
// didn't capture an avatar at subscribe-time (channel
@ -231,7 +232,7 @@ class SubscriptionFeedViewModel : ViewModel() {
// page loaded), backfill from the channel info now.
val freshAvatar = info.avatar
if (!freshAvatar.isNullOrBlank() && freshAvatar != ch.avatar) {
runCatching {
runCatchingCancellable {
Subscriptions.get().updateAvatar(ch.url, freshAvatar)
}
}

10
strawApp/src/main/kotlin/com/sulkta/straw/feature/player/NowPlaying.kt
View file

@ -32,10 +32,6 @@ object NowPlaying {
private val _current = MutableStateFlow<NowPlayingItem?>(null)
val current: StateFlow<NowPlayingItem?> = _current.asStateFlow()
fun set(item: NowPlayingItem?) {
_current.value = item
}
/**
* Atomically claim playback for `streamUrl`. Returns true if this
* call WON the claim (caller should now do setMediaItem + prepare +
@ -47,8 +43,10 @@ object NowPlaying {
*
* Uses MutableStateFlow.compareAndSet for the race-free transition.
* vc=35 audit HIGH-C6 — the previous "check NowPlaying then
* setPlayingFrom" sequence had a window where both checks could
* pass before either NowPlaying.set ran.
* direct assign" sequence had a window where both checks could
* pass before either write happened. The non-CAS `set()` setter
* that lived alongside this method was dropped in round-5 (no
* external callers; left in code purely as a footgun).
*/
fun claim(item: NowPlayingItem): Boolean {
while (true) {

8
strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt
View file

@ -364,6 +364,14 @@ fun SponsorBlockSkipLoop() {
delay(150)
val state = controller.playbackState
if (state == Player.STATE_IDLE || state == Player.STATE_ENDED) continue
// Skip the position read + segment scan when not actively
// playing — on a paused-overnight session the prior shape
// hit the binder every 150ms for hours. Round-5 audit
// MED-2.
if (!controller.isPlaying) {
delay(1000)
continue
}
val posSec = controller.currentPosition / 1000.0
val s = pickActiveSegment(segments, posSec, skipped) ?: continue
strawLogI(

14
strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt
View file

@ -174,9 +174,13 @@ class SearchViewModel : ViewModel() {
uploadDateRelative = r.uploadDateRelative,
)
}
// Fence terminal write against a fresher submit that
// cancelled this one. Drop our result if the query
// already moved on.
// Fence ALL post-network side-effects against a
// fresher submit that cancelled this one — not just
// the UI update. Round-5 audit HIGH-2: the prior
// fence only guarded `_ui.update`, so the cache write
// + pool rebuild proceeded for a cancelled query,
// polluting SearchCache with results the user
// abandoned mid-stroke.
if (_ui.value.query.trim() != q) return@launch
_ui.update {
it.copy(
@ -190,6 +194,10 @@ class SearchViewModel : ViewModel() {
runCatching { History.get().recordSearch(q) }
if (Settings.get().cacheEnabled.value) {
withContext(Dispatchers.IO) {
// Re-check after suspending — a fresh submit
// can have changed the query between the UI
// update and now.
if (_ui.value.query.trim() != q) return@withContext
runCatching { SearchCache.get().record(q, items) }
// Refresh the in-memory pool with the new
// entries so subsequent reactive filters see

28
strawApp/src/main/kotlin/com/sulkta/straw/util/Coroutines.kt Normal file
View file

@ -0,0 +1,28 @@
/*
* SPDX-FileCopyrightText: 2026 Sulkta-Coop
* SPDX-License-Identifier: GPL-3.0-or-later
*
* Coroutine-safe runCatching. Standard kotlin.runCatching catches
* Throwable — including CancellationException, which is supposed to
* propagate so structured concurrency works. Round-5 audit HIGH-2
* surfaced the bug: a runCatching around a channelInfo() call
* inside a cancelled job swallowed the cancellation, the job
* carried on past the runCatching, hit its terminal write fence
* (which only checked URL-equality, so same-URL races couldn't
* be distinguished), and clobbered the newer job's state.
*
* Always use this in coroutine bodies. The plain runCatching is
* still fine in non-suspend code (no coroutine to cancel).
*/
package com.sulkta.straw.util
import kotlinx.coroutines.CancellationException
inline fun <R> runCatchingCancellable(block: () -> R): Result<R> = try {
Result.success(block())
} catch (c: CancellationException) {
throw c
} catch (t: Throwable) {
Result.failure(t)
}

9
strawApp/src/main/kotlin/com/sulkta/straw/util/YtUrl.kt
View file

@ -20,6 +20,13 @@ private val ALLOWED_YT_HOSTS: Set<String> = setOf(
)
fun isAllowedYtUrl(url: String): Boolean {
val host = runCatching { java.net.URI(url).host?.lowercase() }.getOrNull() ?: return false
val uri = runCatching { java.net.URI(url) }.getOrNull() ?: return false
// Require an http/https scheme — `//host/...` (schemeless) and
// `mailto:host` both parse with a host attribute. Round-5 audit
// LOW-3.
val scheme = uri.scheme?.lowercase() ?: return false
if (scheme != "https" && scheme != "http") return false
// Strip a single trailing dot (RFC FQDN form) before lookup.
val host = uri.host?.lowercase()?.removeSuffix(".") ?: return false
return host in ALLOWED_YT_HOSTS
}