You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(proxies): stop the tunnel before the earner, so a stop that fails halfway is not undone - #15
Review mirror of GeiserX#262 (CodeRabbit is rate-limited there). Findings are fixed on the real PR; this one is closed after.
Summary by CodeRabbit
Bug Fixes
Fixed an issue where stopping a proxied copy could leave its tunnel running if the tunnel failed to stop, while the earner had already stopped. The tunnel now stops first, and a failure prevents the earner from being stopped. This also prevents a later status check from unexpectedly restarting the earner after a failed stop.
…p is not undone
stopProxied stopped the earner first. When the tunnel stop then failed, the
next status read found a ready tunnel beside a stopped earner and started the
earner again, losing the owner's stop. Stopping the tunnel first means a stop
that fails halfway leaves either both running (and says so) or the earner in
a namespace with no way out, which the status read leaves alone.
No actionable comments were generated in the recent review.
Walkthrough
stopProxied now stops the tunnel before the earner. It returns immediately if stopping the tunnel fails. Tests cover failures stopping either container and check that a later status read does not restart the earner.
stopProxied stops the tunnel first and returns on a tunnel-stop error. Tests cover failures stopping the tunnel or earner and verify the subsequent status read does not start or restart the earner. The changelog records the change.
geiresx/CashPilot-Desktop#8: This PR changes the same proxied-stop operation and uses its running-state result for cap enforcement.
Merge Risk: Minimal · up to aa42d
The shutdown-order change has no identified merge-blocking issue. Merge after normal checks.
Security architecture risk: Low · up to aa42d
The new ordering addresses shutdown being undone by a later status read without changing container ownership or network privileges. Remaining uncertainty concerns real Docker partial failures and overlapping lifecycle operations; no introduced security bypass was established.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The changed shutdown operation remains scoped to the selected service/proxy container pair on the configured Docker daemon. It adds no container-selection authority or new production entrypoint.
Trust Boundaries and Controls
inferred — An earner left running after tunnel shutdown remains configured inside the sidecar namespace without independent networking or firewall-changing authority. This is counterevidence to a newly introduced direct-egress path, but the exact live post-stop behavior was not independently demonstrated.
Resilience and Maintainability Implications
observed — Status recovery still issues start or restart operations from separately collected container state. That structure predates this PR, and the Docker adapter does not itself serialize lifecycle calls. The sequential regression test therefore does not establish correctness for overlapping status and shutdown operations; no PR-introduced worsening of this concurrency limitation was established.
Hardening Proposals
proposed — Extend transition assurance with controlled overlapping status/shutdown scenarios and daemon failures that occur after state mutation, checking both final owner-requested state and continued proxy isolation. This is an assurance proposal, not a demonstrated security bypass.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly describes the stop-order change and its purpose.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. Manage usage-based reviews.
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
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.
Review mirror of GeiserX#262 (CodeRabbit is rate-limited there). Findings are fixed on the real PR; this one is closed after.
Summary by CodeRabbit