per-channel back off#22
Conversation
UnknownJoe796
left a comment
There was a problem hiding this comment.
Automated Review (commit d8b127b)Now I have a complete picture. Here's my review:
PR #22: per-channel back off — Review Summary
Clean, well-structured addition of exponential backoff for websocket channel reconnection. The ChannelBackoff class is well-tested and the integration into BulkFetcher.WebsocketChannel is straightforward. Two issues found.
1. Backoff bypassed when server rejects a channel (end without start)
File: ChannelBackoff.kt:29 / BulkFetcher.kt:164-177
Confidence: Medium
connectionOpenedAt is initialized to 0L and only updated when onConnectionOpened() is called (on receiving message.start). If the server rejects a channel by sending end=true without a prior start=true (confirmed possible from server-side MultiplexWebSocketHandler), then onConnectionOpened() is never called for that attempt.
When onConnectionClosed(false) runs:
connectionDuration = timeSource() - 0 // = current epoch ms, a huge number
huge_number < 5000 // = false
→ backoff resets to 0
This means any built-up backoff is incorrectly wiped out by a rejection. In the specific scenario where a channel starts rapidly failing after some successful connections, a rejection resets the backoff counter and the client retries immediately.
Suggested fix: Track whether onConnectionOpened was called since the last close, and treat "never opened" as an immediate failure:
private var connectionOpenedAt: Long = 0L
private var wasOpened = false
fun onConnectionOpened() {
connectionOpenedAt = timeSource()
wasOpened = true
}
fun onConnectionClosed(wasIntentional: Boolean) {
if (wasIntentional) {
backoffMs = 0L
wasOpened = false
return
}
val isQuickFailure = if (!wasOpened) {
true // Never opened = immediate rejection = failure
} else {
(timeSource() - connectionOpenedAt) < stableConnectionThreshold.inWholeMilliseconds
}
wasOpened = false
if (isQuickFailure) {
backoffMs = if (backoffMs == 0L) initialBackoff.inWholeMilliseconds
else (backoffMs * 2).coerceAtMost(maxBackoff.inWholeMilliseconds)
} else {
backoffMs = 0L
}
}2. package-lock.json accidentally committed
File: package-lock.json (5,154 new lines)
Confidence: High
A full package-lock.json was added in this PR. The project's .gitignore doesn't exclude it, but this is a Kotlin Multiplatform project where the sole package.json dependency (@js-joda/timezone) is a Gradle-managed JS interop dependency. This lockfile is almost certainly an accidental commit and adds noise to the diff. Consider adding package-lock.json to .gitignore and removing it from this PR.
No description provided.