fix(selfhost): durability — queue leases, atomic migrations, and two repair scans for state a crash used to lose#9144
Merged
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-27 03:02:08 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
…repair scans for state a crash used to lose Four ways a badly-timed process death left permanent damage. #9007 made this urgent rather than theoretical: busy deploys currently end in SIGKILL, so "killed at the wrong moment" is the normal case, not the rare one. On Postgres the queue is deliberately multi-instance (that is what the FOR UPDATE SKIP LOCKED claim is for), so "every processing row" is not "my crashed predecessor's work" — during any overlapped or rolling deploy it also includes jobs a SIBLING is running right now. Both processes then ran the same PR pass concurrently, producing duplicate gate check-runs and verdict thrash. Recovery is now scoped to rows whose lease has actually expired, the same definition the runtime reaper already uses, so boot and steady-state stop disagreeing about what "abandoned" means, and the re-pend is a compare-and-swap so a sibling finishing first cannot be resurrected. Two related holes in the same file. The lease is now heartbeated by the worker that holds it: without that, the timeout was a ceiling on job DURATION rather than on liveness, so a legitimately long pass — a large PR, a slow AI review, a rate-limited GitHub window — was reclaimed and double-run purely for taking too long, and the activeJobIds guard meant to prevent that only covers one process's in-memory set, which is the wrong scope for a shared queue. And a reclaim now costs an attempt: it never did, so a job that reliably wedges past its lease was requeued forever with attempts frozen, never reaching the dead-letter threshold that exists precisely to stop a poison job from burning the queue. Statements ran individually and the file was recorded applied only after the last one succeeded, so a crash mid-file re-ran the WHOLE file next boot. Harmless for pure DDL and destructive for the several migrations carrying real DML — the table-rebuild INSERT ... SELECT pattern, plus UPDATE/DELETE steps — where a re-run either double-inserts or raises a PK conflict that, not matching the tolerated "already exists" shape, throws and bricks boot outright. Each file now runs as one transaction with its ledger row committed inside it, via a new execTransaction on both self-host adapters. It could not reuse exec: on Postgres each exec call takes whatever connection the pool hands out, so a BEGIN and its COMMIT issued as separate calls can land on different connections and mean nothing. The pre-existing drift tolerance is preserved rather than replaced — a database that already drifted must still heal itself instead of failing to boot, and that only works per-statement. But ONLY drift falls back. Any other failure rethrows with the transaction already rolled back, because retrying a genuinely broken file per-statement would re-apply its valid statements after the rollback and leave exactly the partial state this fix exists to prevent. pr_outcome is what the fleet calibration export INNER JOINs gate_decision against, so a PR missing one does not lose a data point, it vanishes from calibration entirely — neither numerator nor denominator. Both writers are in-process and best-effort: a process that dies between the GitHub mutation and the record call loses the direct write, and on the next pass the PR is already terminal so the planner plans nothing and it never fires; the webhook that would have caught it was delivered while the container was down and GitHub does not redeliver. Nothing scanned for the gap — the repair sweep only visits OPEN PRs. These losses are not a random sample: a superseded close is by definition a wrong close, so they skew toward the gate's mistakes and their absence biased published accuracy upward. Pass 1 of the flag-then-close double-check enqueues a single delayed job to run Pass 2, and that message was the only thing that could finish the sequence. Its documented "next sweep / CI event" backstop is vacuous: #never-endless-reregate makes an already-regated PR permanently sweep-ineligible while the flag itself suppresses merge and approve, and the violation memory is permanent so the flag could never clear either. Pass 1 now also records the deadline, and a watchdog re-enqueues Pass 2 for any flag past it. audit_events is the right store: durable, already queried by type, and repo-agnostic — unlike the label, which is per-repo configurable and would need settings resolution before a cross-repo scan could even recognize it. Both repair scans ride the re-gate sweep's existing fan-out tick rather than each earning a job type and a cron entry for a bounded DB scan, and run before the fan-out so neither can cost the tick its actual work. Also fixes main: test/unit/docs-beta-onboarding.test.ts asserted the doc still says "base-agent", which #9117 intentionally retired when it repositioned the product away from "a deterministic base-agent for the Gittensor ecosystem" toward an agent stack for both sides of the pull request on any GitHub repo. The guard exists to stop LoopOver claiming to BE the official Gittensor frontend, and its other four assertions cover that on their own; pinning the retired wording only made it veto an intended product change. Local gate green end to end (npm run test:ci, exit 0). 100% line and branch coverage on all 210 added src lines. Closes #9023 Closes #9026 Closes #9027 Closes #9031
JSONbored
force-pushed
the
fix/9023-9026-9027-9031-durability
branch
from
July 27, 2026 04:20
8112ae9 to
8331a74
Compare
JSONbored
added a commit
that referenced
this pull request
Jul 27, 2026
…ors, and stop judging a fix you cannot see (#9145) Three ways the AI reviewer acted confidently on input it had silently altered, truncated, or should never have trusted in the first place. #9035 — attacker-controlled text reached the prompt with no structural defense. Title, body and diff were concatenated straight in. "Judge ONLY the diff" tells the model what to look at; it never says that what it is looking at is DATA. The only defense was regex defang, which is deliberately narrow and which paraphrase or encoding walks past — and the same text reaches BOTH consensus reviewers, so a successful steer suppresses both and never even trips the single-rejection ai_review_split rule that exists to catch one reviewer being wrong. Each untrusted region is now fenced, with a system rule saying content between the markers is data and cannot change the rules, the output format, or the verdict. Forged markers are stripped from inside the region so a body cannot close its own fence early — without that, fencing would be worse than none. And a DETECTED attempt now holds the PR for a human. Defang recorded these and, by design, never let them affect the disposition, so a caught attacker got a normal roll. Manipulation aimed at the reviewer is evidence of intent, and the one thing it must not buy is an automated decision. Held, never closed: the detector is a regex running over a repository whose own subject matter is AI review, so a false positive is entirely possible, and a hold costs a contributor a wait where a close would cost them their PR. It reuses the existing ai_review_inconclusive hold path, so no gate-decision twin changes. #9076 — the defang shifted every inline anchor after it. The injection patterns' `[^.]{0,N}` gaps deliberately span newlines, so one match can swallow two or three diff lines including their `+`/`-` markers and even an `@@` header. Replacing all of that with a single-line literal collapsed those newlines. The reviewer counts a finding's line over the DEFANGED text, but the finding is validated and posted against the ORIGINAL patch — so every anchor after a multi-line redaction shifted, and a shifted anchor that still landed inside the commentable set passed validation and posted publicly on the wrong line of a contributor's PR. The redaction now re-emits one newline per newline consumed, so the two coordinate systems stay congruent. Two more on the same surface. Blocker-severity findings now anchor to ADDED lines only: rightSideLinesFromPatch admits unchanged context too, while the prompt asks for an added line and warns that a wrong line is worse than none — set membership was the only check, and context satisfies it, so a model miscounting by one to three lines landed on context and posted. That file conflated two different properties throughout: "GitHub will accept this anchor" and "this anchor is correct". Nits still allow context, because a misplaced nit is noise while a misplaced blocker costs someone their PR. And an empty patch line now counts as the context line it is instead of desynchronizing every later line number in the file (the trailing split artifact of a patch ending in a newline is dropped first, so the two cases stay distinct). #9075 — a confident "you didn't fix the issue" computed from a window that never contained the fix. linked-issue-satisfaction's own header claims "NO gate wiring, NO disposition change… advisory-only either way". That is stale: under linkedIssueSatisfactionGateMode: "block" an `unaddressed` verdict pushes a critical-path finding reading "this PR does not appear to satisfy its linked issue's scope." Meanwhile the module re-slices the already-truncated diff to 60k under the header "Unified diff (truncated if large):" — a hedge, not a fact, which leaves the model to guess whether it is seeing everything. A PR whose fix sits in a dropped hunk, a filtered file, or past character 60,000 got that public verdict anyway. The confidence floor does not help: it guards against a model being unsure, not against a model being sure about the wrong input. The header now states the fact, and an `unaddressed` verdict over a known-truncated diff degrades to `partial`. Only `unaddressed` — "addressed" over a truncated diff is the model finding POSITIVE evidence, which truncation cannot manufacture. Same reasoning #8961 already applies to truncated PR bodies. POLICY REVERSAL, called out explicitly. selectContextSectionsWithinBudget's `break` was pinned by a test asserting it is "a hard priority cutoff, not a bin-packing optimization". That reasoning holds for sections that are genuinely model context. It does not hold for what actually sat at the bottom of the list: testEvidence is ~200 characters and is not context at all but a deterministic classifier FACT ("this PR changes no test paths"), so one large RAG block silently discarded it on exactly the large PRs where it matters most, with no marker anywhere. An oversized section is now skipped rather than ending the loop. Priority order still decides who gets first refusal on the budget; every included section still genuinely fits. Both tests encoding the old rule are inverted with the reasoning in place. Also fixes main, same one-line change as #9144 (identical, so the two merge cleanly): docs-beta-onboarding asserted the doc still says "base-agent", which #9117 intentionally retired when it repositioned the product. Local gate green end to end (npm run test:ci, exit 0). 100% line and branch coverage on all 317 added src lines. Closes #9035 Closes #9075 Closes #9076
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9144 +/- ##
==========================================
+ Coverage 90.56% 92.67% +2.11%
==========================================
Files 96 817 +721
Lines 22490 80954 +58464
Branches 3884 24580 +20696
==========================================
+ Hits 20367 75024 +54657
- Misses 1945 4848 +2903
- Partials 178 1082 +904
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Four ways a badly-timed process death left permanent damage. #9007 is what makes this urgent rather than theoretical: busy deploys currently end in SIGKILL, so "killed at the wrong moment" is the normal case, not the rare one.
#9023 — boot recovery re-pended every processing row with no lease guard
On Postgres the queue is deliberately multi-instance — that is what the
FOR UPDATE SKIP LOCKEDclaim is for — so "every processing row" is not "my crashed predecessor's work". During any overlapped or rolling deploy it also includes jobs a sibling is running right now. Both processes then ran the same PR pass concurrently, producing duplicate gate check-runs and verdict thrash.Recovery is now scoped to rows whose lease has actually expired — the same cutoff the runtime reaper already uses, so boot and steady-state stop disagreeing about what "abandoned" means — and the re-pend is a compare-and-swap, so a sibling that finished first cannot have its completed job resurrected as pending.
Two related holes in the same file, both named in the issue:
activeJobIdsguard meant to prevent this only covers one process's in-memory set — the wrong scope for a queue whose whole point is that several processes share it.attemptsfrozen at whatever the last real failure left it, never reaching the dead-letter threshold that exists precisely to stop a poison job from burning the queue.#9027 — migrations were not transactional per file
Statements ran individually and the file was recorded applied only after the last one succeeded, so a crash mid-file re-ran the whole file on the next boot. Harmless for pure DDL; destructive for the several migrations carrying real DML — the table-rebuild
INSERT ... SELECTpattern, plusUPDATE/DELETEsteps — where a re-run either double-inserts or raises a PK conflict that, not matching the tolerated "already exists" shape, throws and bricks boot outright.Each file now runs as one transaction with its ledger row committed inside it, through a new
execTransactionon both self-host adapters. This could not reuseexec: on Postgres eachexeccall takes whatever connection the pool hands out, so aBEGINand itsCOMMITissued as separate calls can land on different connections and mean nothing.The pre-existing drift tolerance is preserved, not replaced. A database that already drifted must still heal itself rather than fail to boot, and that self-healing only works per-statement. But only drift falls back — any other failure rethrows with the transaction already rolled back. Retrying a genuinely broken file through the per-statement path would re-apply its valid statements after the rollback and leave precisely the partial state this fix exists to prevent (the test suite pins both halves).
#9026 — merge/close outcomes could be lost with nothing to notice
pr_outcomeis what the fleet calibration export inner-joinsgate_decisionagainst, so a PR missing one does not merely lose a data point — it vanishes from calibration entirely, contributing to neither numerator nor denominator.Both writers are in-process and best-effort. A process that dies between the GitHub mutation and the record call loses the direct write, and on the next pass the PR is already terminal so the planner plans nothing and it never fires. The webhook that would have caught it was delivered while the container was down, and GitHub does not redeliver. Nothing scanned for the gap: the repair sweep only visits OPEN PRs.
These losses are not a random sample. A superseded close is by definition a wrong close, so they skew toward the gate's mistakes, and their absence biased published accuracy upward. That is why this warranted a reconciler rather than a wider window.
#9031 — a lost queue message stranded a PR permanently
Pass 1 of the flag-then-close double-check enqueues a single delayed job to run Pass 2, and that message was the only thing that could finish the sequence. Its documented "next sweep / CI event" backstop is vacuous:
#never-endless-reregatemakes an already-regated PR permanently sweep-ineligible, while the flag itself suppresses merge and approve vialinkedIssueCloseInFlight— and the violation memory is permanent, soclearLinkedIssueFlagcould never fire either.Pass 1 now also records the deadline, and a watchdog re-enqueues Pass 2 for any flag past it.
audit_eventsis the right store for this: durable, already queried by type, and repo-agnostic — unlike the label, which is per-repo configurable and would need settings resolution before a cross-repo scan could even recognize it.Notes for review
Where the two repair scans run. Both ride the re-gate sweep's existing fan-out tick rather than each earning a job type and a cron entry for a bounded DB scan, and both run before the fan-out so neither can cost the tick its actual re-gate work.
Also fixes main.
test/unit/docs-beta-onboarding.test.tsasserted the onboarding doc still says "base-agent", which #9117 intentionally retired when it repositioned the product away from "a deterministic base-agent for the Gittensor ecosystem" toward an agent stack for both sides of the pull request on any GitHub repo. That guard exists to stop LoopOver claiming to be the official Gittensor frontend, and its other four assertions cover that on their own — pinning the retired wording only made it veto an intended product change.Verification
npm run test:ci— exit 0, full gate green.src/lines, verified by intersectinggit diff -U0againstlcov.infoDA:/BRDA:records.Closes #9023
Closes #9026
Closes #9027
Closes #9031