fix(runtime): bound the shutdown drain, serialize boot migrations, and make capture failures visible (#9485, #9486, #9487) - #9512
Merged
Conversation
…d make capture failures visible (#9485, #9486, #9487) stop() waited on in-flight work with NO deadline, so a redeploy blocked on a multi-minute AI review until the orchestrator SIGKILLed the process -- and because the killed job's lease had been heartbeated right up to the kill, reclaimExpiredProcessingJobs (which only recovers rows older than 30 minutes) left it stalled for another 20-30. Against a 1-5 minute latency target with automated redeploys, that was the largest single source of a silently stalled review. The wait is now bounded, and on expiry this process re-pends its OWN in-flight rows -- safe because activeJobIds is process-local -- turning a SIGKILL race into an immediate retry. Both backends. Boot migrations took no cross-instance lock. Two instances booting together meant one applied a migration atomically while the other's identical transaction failed "already exists", which runSelfHostMigrations treats as DRIFT and retries statement-by-statement -- re-executing any table-rebuild INSERT ... SELECT or UPDATE backfill against the already-migrated schema, then dying on the ledger INSERT's unique violation (a message matching neither tolerated shape) and crash-looping. #9027 made a single migration atomic against a crash; this makes the whole run atomic against a concurrent boot, via a Postgres session advisory lock. A session lock is held by its connection, so it cannot be taken through the pooled adapter -- hence the adapter->pool registry rather than a new statement. Visual capture failures were logged but never counted, so a browserless outage was invisible in Prometheus while it degraded every screenshot to a dash cell -- and since the screenshot gate treats absent evidence as a close signal, that outage could close legitimate visual PRs before anyone noticed.
…on lock, and the capture metric Adds the pg-queue drain-deadline regression and its completing-drain invariant, the pool registry, and a render-failure metric test proving a browserless outage is now alertable. Three best-effort/defensive arms are annotated with their reasons rather than given contrived tests: the advisory unlock's catch (a failed unlock means a broken session, and releasing the client drops the lock anyway), and the abandoned-set empty arm plus its catch (both degrade to the pre-#9485 lease-expiry path this block exists to improve on).
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
Bundle ReportChanges will increase total bundle size by 72 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9512 +/- ##
==========================================
+ Coverage 89.55% 93.63% +4.07%
==========================================
Files 843 741 -102
Lines 110135 60489 -49646
Branches 26207 21310 -4897
==========================================
- Hits 98635 56640 -41995
+ Misses 10238 2898 -7340
+ Partials 1262 951 -311
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.
Summary
Three self-host runtime defects: a redeploy that stalls reviews for half an hour, a concurrent boot that can corrupt a migration backfill, and a browserless outage nothing can see.
Closes #9485
Closes #9486
Partially addresses #9487 (code-side half; see Scope)
#9485 — an unbounded drain turned every redeploy into a 20–30 minute stall
stop()waitedwhile (active > 0)with no deadline. The redeploy-companion recreates the container, so SIGTERM landed on an in-flight multi-minute AI review, the wait blocked until the orchestrator's grace period expired, and the process was SIGKILLed.The damage came next: the killed job's lease had been heartbeated right up to the kill, so
reclaimExpiredProcessingJobs— which only recovers rows older thanQUEUE_PROCESSING_TIMEOUT_MS(30 minutes by default) — did not pick it up for another 20–30 minutes. Against a 1–5 minute latency target with automated redeploys, that is the single largest source of "a review silently stalled for half an hour".The wait is now bounded (
QUEUE_SHUTDOWN_DRAIN_DEADLINE_MS, default 120s — comfortably inside this deployment's 300sstop_grace_period). On expiry the process re-pends its own in-flight rows before returning, which is safe precisely becauseactiveJobIdsis process-local: these are rows we claimed and are about to abandon. A SIGKILL race becomes an immediate retry. Both backends.#9486 — concurrent boots could re-run a DML backfill
Boot migrations took no cross-instance lock. With two instances booting together, A applies a migration atomically while B's identical transaction fails
already exists— whichrunSelfHostMigrationstreats as drift and retries statement-by-statement.CREATE TABLEis tolerated, but a table-rebuildINSERT … SELECTor anUPDATEbackfill re-executes against the already-migrated schema. B then dies on the ledgerINSERT's unique violation — a message matching neitherduplicate columnnoralready exists— and crash-loops a cycle.#9027 made a single migration atomic against a crash; this makes the whole run atomic against a concurrent boot, via a Postgres session advisory lock.
Why a registry rather than a new statement: a session advisory lock is held by the connection that took it, so it cannot be acquired through
prepare()/batch()— those hand back a pooled client per call.createPgAdapternow registers its pool in aWeakMapso a caller needing a dedicated connection can find it, leaving theD1Databasesurface exactly the shape every other backend implements. SQLite falls straight through (single-process by construction).#9487 (code half) — a browserless outage was invisible
Visual capture failures were logged but never counted.
console.logis invisible to Prometheus, so an outage silently degraded every screenshot to a dash cell with nothing to alert on — and because the screenshot-table gate treats absent evidence as a close signal, that outage could close legitimate visual PRs before anyone noticed. Nowloopover_visual_capture_total{result}.Scope
#9487 also covers production alerting config that does not live in this repo: the
LoopoverBackupMissingalert firing for 9 days on the retiredsqlitetarget, the p95 SLO alert computed from ~3 requests/5min with noroutelabel, thesecret_leakgate's 44% false-positive rate on metagraphed, and Redis'sallkeys-lrupolicy sharing a keyspace with correctness keys. Those are alertmanager/Prometheus rules and private per-repo config onedge-nl-01, so the issue stays open for them.Validation
npx tsc --noEmit -p tsconfig.json— cleanselfhost-sqlite-queue,selfhost-pg-queue,selfhost-migrate,selfhost-metrics,visual-shotRegressions:
stop()returns rather than hanging on a job that outlives the deadline, and the abandoned row comes back aspending— on both backends.{result: "error"}, so a browserless outage is alertable.Invariants:
Three best-effort/defensive arms are annotated with their reasons rather than given contrived tests — each degrades to the pre-fix behaviour, which is what the change improves on rather than depends on.