fix(database): drop .failed state so clean health-monitor teardown stops alerting connection lost#1147
Merged
Merged
Conversation
…ops alerting connection lost
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ConnectionHealthMonitor.attemptReconnectno longer transitions to.failedafter thewhile !Task.isCancelledloop exits. Loop exit means cancellation, which is clean teardown initiated bystopMonitoring(). The post-loop transition is replaced with a debug log and a plainreturn.HealthState.failedis removed entirely. It was unreachable through any non-cancellation code path.resetAfterManualReconnect()is removed: zero callers, and its only purpose was resetting from.failed.DatabaseManager+Health.swiftdrops thecase .failed:arm that wrotesession.status = .error("Connection lost")and calledclearCachedData().logLevel(for:)helper drops its dead.failedbranch.Net diff: +8 / -26.
Why this matters
This is bug R1 from the full-app audit. When the user closes the last window for a connection while the health monitor is in reconnect backoff,
stopMonitoring()cancels the task. The reconnect loop exits cleanly, falls past the loop body, and unconditionally transitions to.failed. The session-level observer treats.failedas a real error, alerts the user that the connection was lost, and wipes cached schema/result data. The user's window-close action looks like a connection failure, and any concurrent session reuse can lose its cached data.Removing
.failedrather than guarding the transition is the correct cleanup: there is no path in the current code that produces a "real failure" distinct from cancellation. Adding aTask.isCancelledcheck before the transition would leave a dead state behind.Test plan
.reconnecting(attempt: N)and lands on.healthy.