Repository navigation
The clamp comment described the wrong failure - #1407
Merged
Merged
Conversation
Copilot caught a real defect, not a doc nit. The base image installs curl and has NO wget (deploy/base-images/portal-ai/Dockerfile), so `until wget …` fails with 127 on every iteration: the loop never ends and EVERY roll sits out the full 1800s grace before being SIGKILLed — the opposite of this PR. Verified in the image itself: `command -v curl` → /usr/bin/curl, `command -v wget` → missing. -f makes the 503 a failure so the loop continues, the 200 ends it. Also corrects the clamp comment: Drained is Count == 0, so a negative count reads as "still busy" (hang), and the next open only brings it back to zero (a live session read as drained, cut off). Both directions are wrong — the test already said so, the source comment did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-stop # Conflicts: # deploy/helm/templates/memex-portal/deployment.yaml
Contributor
There was a problem hiding this comment.
Pull request overview
Clarifies the XML doc comment on ActiveCircuitTracker.Closed() to correctly explain why the circuit count is clamped at zero, aligning the source comment with the already-correct behavior and existing tests.
Changes:
- Corrects the rationale in the
Closed()documentation: a negativeCountmakesDrainedfalse (“still busy”), not “drained forever”. - Expands the comment to describe both failure modes the clamp prevents (hang-to-grace-period vs. premature drain on subsequent opens).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
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.
Follow-up to #1342 (Copilot's second finding there; the first —
wget→curl— was already fixed on main, with a better fail-opencommand -vguard, so nothing of that is carried here).ActiveCircuitTracker.Closed()clamps at zero, and the comment justified it with the wrong direction: "a negative count reads as drained forever after".DrainedisCount == 0, so a negative count reads as still busy — the pod would sit out its whole grace period on every roll before being SIGKILLed. And the next open only brings it back to zero, so a live session would then read as drained and get cut off.Both directions are wrong, which is exactly why the clamp is there; the test already said so and only the source comment was misleading. Comment-only — no behaviour change.
🤖 Generated with Claude Code