fix(selfhost): route the last two bare-Number() env knobs through parsePositiveIntEnv + preflight - #10098
Closed
RealDiligent wants to merge 1 commit into
Conversation
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10098 +/- ##
===========================================
- Coverage 91.88% 79.79% -12.09%
===========================================
Files 930 284 -646
Lines 113827 59345 -54482
Branches 27466 8882 -18584
===========================================
- Hits 104588 47354 -57234
- Misses 7940 11695 +3755
+ Partials 1299 296 -1003
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…sePositiveIntEnv + preflight LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (src/server.ts) and OLLAMA_NUM_CTX (src/selfhost/ai.ts) were the only self-host numeric knobs still read with a bare Number(process.env.X ?? ""), the exact form JSONbored#9157 replaced everywhere else. Both fail the two ways that comment names: a unit-suffixed/separator value ("30s", "30_000") NaN's and silently disables the opt-in with no signal, and a fractional value ("0.5") is accepted — the shutdown deadline loses every race (bulk lock release fires on every shutdown) and ollamaNumCtx floors 0.5 to 0. Read both via parsePositiveIntEnv (server: { min: 0, fallback: 0 } to keep unset ⇒ wait-for-the-drain; ollama: { min: 1, fallback: 32_768 }), and add the paired positiveInteger preflight entries so a malformed value hard-fails boot with a clear message instead of only warning at use time. Defaults, the shutdown drain-first ordering, and the ollama provider gate are unchanged. Closes JSONbored#10056
JSONbored
force-pushed
the
fix/critical-issue-env-knobs-parsePositiveIntEnv-10056
branch
from
July 31, 2026 07:33
35861e9 to
dfbd081
Compare
Contributor
Author
This was referenced Jul 31, 2026
Closed
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
GitHub reports `mergeable_state: "unstable"` both for a failing non-required check and for checks that are still running. The gate treated both as a manual-review hold, so every PR whose CI had not finished got an enforcement label: the executor denies merge and approve while it is present, and merge-train.ts evicts a held sibling from the train entirely. Section 1b's release condition also tested the same state, which made the label a latch -- applied while CI was red, then not liftable because CI was red. Observed on #10098, stuck until a human removed it by hand. Neither escalation path could clear it: `releasedHolds` contains only `guardrailHit`, and `guardrailEscalationCleared` requires green CI. An unstable state no longer holds. It does not become mergeable either: `wouldApprove` gains its own explicit `!unstableHolds` term (previously implied by the hold), keyed on `unstableHolds` rather than the raw state so #9810's ignored-check dismissal stays approvable, and `wouldMerge` already required `clean`. Both label sites stop emitting -- the merge-autonomy fallback is removed and the disposition ternary returns no label rather than falsely claiming `ready-to-merge`. Closes #10116
This was referenced Jul 31, 2026
Closed
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…together #10155 is flapping in production on beta.7: the label is removed and re-added roughly every 90 seconds -- four cycles in eight minutes, each pair a GitHub write and a subscriber notification. The audit trail shows both operations in the SAME pass, two seconds apart. Three post-plan transforms surface the manual-review hold -- the merge circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831). All three carried the identical idempotency check, which looks for an existing ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the planner has already scheduled a release -- section 1b does, whenever nothing IT knows about still wants a hold -- the transform appended an add next to that remove, and the executor performed both. Latent until #10116. `noManualReviewHoldWanted` used to include `!mergeableStateUnstable`, which suppressed the release on exactly these PRs and accidentally masked the missing case. Removing that term (correctly -- it was the latch keeping #10098 stuck) let the release fire and exposed the contradiction. #10116 did not create this; the label was previously a sticky latch, quieter but strictly more broken. The planner cannot fix it from its side. Section 1b's doc calls noManualReviewHoldWanted "every reason that would ADD this label, in one place"; it is not, and cannot be, because these three run AFTER planning -- #8831 landed long after that comment was written. Extending the planner's list would just be one more thing to remember on the next transform. So one shared withManualReviewHoldLabel() drops a planned release of the same label before adding, and all three call sites use it. The contradiction becomes unrepresentable where the add happens. The remove is dropped rather than the add skipped: a transform only gets there by having just diverted a merge or a close, so its hold is strictly newer than the release decided before that diversion. Scoped to this label only -- dropping every label remove would silently defeat the stale-disposition-label cleanup, which is its own mutation test. gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about one in five would-close PRs is eligible for the path that triggered this. Closes #10164
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…together (#10167) #10155 is flapping in production on beta.7: the label is removed and re-added roughly every 90 seconds -- four cycles in eight minutes, each pair a GitHub write and a subscriber notification. The audit trail shows both operations in the SAME pass, two seconds apart. Three post-plan transforms surface the manual-review hold -- the merge circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831). All three carried the identical idempotency check, which looks for an existing ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the planner has already scheduled a release -- section 1b does, whenever nothing IT knows about still wants a hold -- the transform appended an add next to that remove, and the executor performed both. Latent until #10116. `noManualReviewHoldWanted` used to include `!mergeableStateUnstable`, which suppressed the release on exactly these PRs and accidentally masked the missing case. Removing that term (correctly -- it was the latch keeping #10098 stuck) let the release fire and exposed the contradiction. #10116 did not create this; the label was previously a sticky latch, quieter but strictly more broken. The planner cannot fix it from its side. Section 1b's doc calls noManualReviewHoldWanted "every reason that would ADD this label, in one place"; it is not, and cannot be, because these three run AFTER planning -- #8831 landed long after that comment was written. Extending the planner's list would just be one more thing to remember on the next transform. So one shared withManualReviewHoldLabel() drops a planned release of the same label before adding, and all three call sites use it. The contradiction becomes unrepresentable where the add happens. The remove is dropped rather than the add skipped: a transform only gets there by having just diverted a merge or a close, so its hold is strictly newer than the release decided before that diversion. Scoped to this label only -- dropping every label remove would silently defeat the stale-disposition-label cleanup, which is its own mutation test. gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about one in five would-close PRs is eligible for the path that triggered this. Closes #10164
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
1 participant
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
parsePositiveIntEnvis this repo's contract (#9157) for a numeric self-host env knob: a supplied non-finite/out-of-range value takes the fallback with a structured warn, andpositiveIntegeris its boot-time half.LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS(src/server.ts:1579) andOLLAMA_NUM_CTX(src/selfhost/ai.ts:742) were the only two self-host numeric knobs still on the bare-Number(process.env.X ?? "")form, and both fail exactly the two ways #9157 named:LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS=30s(or2m,30_000) →NaN→Number.isFinitefalse → the shutdown path silently takes "wait for the drain", i.e. the opt-in the operator just enabled does nothing, no log/metric/preflight error.Number("0.5")is finite &>0, so the shutdownPromise.racegets a 0.5 ms deadline every shutdown loses → the bulkreleaseAllHeldLocksAtShutdown()fires on every shutdown (the behaviour the comment above it says was removed for causing double-actuation).ollamaNumCtxfloors0.5to0— a disabled context window.Fix
src/server.tsreads the knob viaparsePositiveIntEnv("LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", { min: 0, fallback: 0 })and keeps the> 0gate — unset ⇒0⇒ drain-first, byte-identical, while a malformed value now warns and falls back to0instead of doing the same thing unannounced.ollamaNumCtxreads viaparsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 }).preflightEnvgains the two pairedpositiveInteger(...)entries (min 0for the shutdown knob, matchingGITHUB_CACHE_TTL_SECONDS;min 1forOLLAMA_NUM_CTX), so a unit-suffix typo hard-fails boot.Defaults, the shutdown drain-first ordering,
parsePositiveIntEnv/positiveIntegerthemselves, the three existingpositiveIntegercalls, andollamaContextOptions' provider gate are all unchanged.src/server.tsis incodecov.yml's ignore list (entrypoint), so its change is covered by the Docker boot smoke test, not unit coverage.Tests
test/unit/selfhost-preflight.test.ts:preflightEnvreports a problem forLOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS/OLLAMA_NUM_CTXon"30s","30_000","0.5","-1"(plus"0"forOLLAMA_NUM_CTX), andok: truefor unset/""/valid integers ("0"accepted for the shutdown knob,"1"for the context window).test/unit/selfhost-ai.test.ts:ollamaNumCtx()returns32768(not0) for"0.5"and65536for"65536".The
"0.5"and malformed-preflight cases fail against the current bare-Number()code.Closes #10056