Skip to content

fix(migrations): repair deepseek default seeded onto self-hosted - #528

Open
juanmichelini wants to merge 10 commits into
mainfrom
repair-deepseek-default-self-hosted
Open

juanmichelini wants to merge 10 commits into
mainfrom
repair-deepseek-default-self-hosted

Conversation

@juanmichelini

@juanmichelini juanmichelini commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Human

Tested on replicated VM in two phases.

Phase 1 before migration

image

Phase 2 after migration

image

Also teted on SaaS which remained unaffected

image

Agent

Summary

Follow-up to #476. Migrations 158/160 seeded the managed deepseek-v4-flash verified_models row (is_default = is_enabled = is_free = is_verified = true) on every host, including self-hosted, where no managed LiteLLM proxy serves the model. #476 gated the seed so fresh self-hosted installs no longer get it; this migration repairs hosts that already ran 158/160 before that gate landed.

What it does

On a self-hosted host (WEB_HOST not a managed SaaS host; SaaS skips the migration so the managed default stays correct):

Step 1 — DELETE the openhands/deepseek-v4-flash verified_models row. Removing the whole row (not just clearing is_default) drops the default and the model-picker surface (is_enabled→picker list, is_enabled AND is_free→"Free" badge). This converges an upgraded self-hosted DB to the same state as a fresh install, which post-#476 never seeds the row.

Step 2 — restore org Default LLM profiles that got baked onto org.llm_profiles. The runtime materializer overlays the Default profile at read time from the is_default row, and activate_profile writes that overlay back into stored org.llm_profiles. Most orgs self-heal once the row is gone (the materializer strips any openhands/-model Default at read time when no DB default exists); only orgs whose concrete BYOK Default was overwritten need an active restore.

Per-org classification (decrypt org.llm_profiles, read org.agent_settings.llm — the legacy LLM, which is plain JSON and was not persisted by the materializer, so it survives as a forensic source):

bucket stored Default agent_settings.llm.model action
noop none, or concrete (≠ deepseek) — nothing — self-heals once row gone
restore openhands/deepseek-v4-flash BYOK model rebuild Default from agent_settings.llm
strip openhands/deepseek-v4-flash None/empty drop the phantom Default; clear active if it pointed there
review openhands/deepseek-v4-flash also managed leave stored bytes; row delete strips it read-time; manual review

org.agent_settings is plain JSON; org.llm_profiles is EncryptedJSON, so the restore decrypts/encrypts inline like migration 137 (from storage.encrypt_utils import decrypt_value/encrypt_value). The downgrade is a no-op — re-applying the bug is not a safe restore.

Key finding that bounds the blast radius

The deepseek override is a read-time overlay, not a stored mutation. materialize_default_llm_profile overlays the Default in memory on every load, and load() does not write it back (its only persist path, _persist_seeded_default_profile, fires only when there is no DB default — the opposite of the bug condition). So stored org.llm_profiles / org.agent_settings.llm are pristine unless a mutating endpoint ran. The only bake path is activate_profile (it materializes before the _org_profiles_transaction commit). That's why only the restore/strip buckets need stored-byte surgery.

What this does / does not do

  • Repairs existing self-hosted installs that ran 158/160 pre-fix(migrations): gate deepseek default seed on SaaS WEB_HOST only #476.
  • SaaS hosts are untouched (the WEB_HOST gate skips the migration).
  • The review bucket (stored Default is deepseek and the legacy model is itself managed) cannot distinguish "genuinely wanted deepseek" from "also corrupted but no forensic trace" — left for manual review / backup restore. The row delete still strips it at read time.
  • Idempotent: re-running on a repaired DB writes no org updates (restored Defaults become concrete → noop).

Write-back bug found & fixed (sa.JSON → sa.String)

Reviewer concern (resolved): the write-back path was unverified — clean-install VM tests and the unit tests could not exercise it, because both start from an empty/no-corrupted-org state where migration 171's restore/strip loop hits zero rows.

A real-Postgres integration test (scripts/verify_migration_171_writeback.py) was added that reproduces the in-place-upgrade / preserved-DB scenario: it runs the migration chain to 170, seeds orgs with real JWE-encrypted llm_profiles (restore / strip / noop buckets) before running 171, then verifies the ORM can decrypt + load the expected Default afterward.

Phase 1 (bug present, sa.JSON()): migration 171 declared org.llm_profiles as sa.JSON() in its sa.table() construct. org.llm_profiles is an EncryptedJSON column whose impl is String — the at-rest value is a JWE ciphertext string, not JSON. On the write path, SQLAlchemy's JSON bind processor JSON-encodes the ciphertext string, wrapping it in quotes, so the DB stored "<ciphertext>" instead of <ciphertext> — permanently undecryptable for every org 171 actually repaired (restore + strip). Measured directly: 171's encrypt_value produced valid 429/236-char ciphertext that round-tripped at write time, but the DB held 431/238-char values literally starting and ending with ". The noop org (untouched by 171) decrypted fine.

This was invisible to:

  • clean-install VM tests (fresh DB → no pre-seeded corrupted org → 171's write loop hits zero rows), and
  • the unit test (fakes the bind/result path; never applies the JSON bind processor).

sa.JSON() is benign on the read side (postgres _PGJSON result_processor is a no-op / pass-through, so _decrypt_profiles received the raw ciphertext string) but destructive on the write side (bind_processor double-encodes).

Phase 2 (fix applied, sa.String()): switched the column to sa.String(), matching the real EncryptedJSON impl=String and the migration-137 precedent. All three buckets now PASS on both psycopg2 and pg8000:

  • restore → Default restored to the BYOK anthropic/claude-3-5-sonnet (model + base_url + api_key) ✅
  • strip → phantom Default removed, active = null ✅
  • noop → untouched ✅
  • raw-decrypt == ORM-decrypt (no double-encoding) ✅

Testing

  • New migration unit tests (--noconftest, no Postgres needed): 18 passed — per-bucket outcomes (restore/strip/review/noop), verified_models DELETE, WEB_HOST gate (SaaS skip + self-hosted run), other-profile preservation, and idempotency.
  • test_enterprise_migration_integrity.py + test_migration_graph.py: 15 passed (single linear head preserved at 171).
  • Real-Postgres write-back integration test (scripts/verify_migration_171_writeback.py): ALL PASS on psycopg2 and pg8000 — seeds real JWE-encrypted llm_profiles for restore/strip/noop orgs before running 171, then verifies the ORM decrypts + loads the expected Default. Reproduces Phase 1 (bug) and confirms Phase 2 (fix).
  • ruff clean. Real encrypt_value/decrypt_value round-trip exercised in tests.

This pull request was created by an AI agent (OpenHands) on behalf of @juanmichelini.

Co-authored-by: openhands openhands@all-hands.dev

Renumber note

This migration was originally numbered 166 (branched when 165 was head). main has since merged 166-170, so 166/170 collided on the PR merge ref. Renumbered to 171 (down_revision = 170) to keep a single linear head; migration and test files renamed accordingly. test_migration_graph.py+test_enterprise_migration_integrity.pyconfirm one linear head at171`.


Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-6c8a53f

Migrations 158/160 seeded the managed deepseek-v4-flash verified_models
row (default+enabled+free+verified) on every host, including self-hosted,
where no managed LiteLLM proxy serves it. #476 gated the seed so fresh
self-hosted installs no longer get it; this repairs hosts that already
ran 158/160 before that gate landed.

On self-hosted (WEB_HOST-gated; SaaS skips so the managed default stays):
- DELETE the deepseek verified_models row, converging upgraded DBs to the
  same state as a fresh install (which post-#476 never seeds it).
- Restore org Default LLM profiles baked onto org.llm_profiles via
  activate_profile while the bogus default was live. Most orgs self-heal
  once the row is gone; only orgs whose concrete BYOK Default was
  overwritten (and survives only in org.agent_settings.llm) get an active
  restore. Phantoms (no legacy model) are stripped; ambiguous managed-
  legacy cases are left for manual review.

org.agent_settings is plain JSON; org.llm_profiles is EncryptedJSON, so
the restore decrypts/encrypts inline like migration 137.

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
…ith main

main merged 166_track_budget_alert_delivery / 167 / 168 / 169 after this
branch cut from 165, so revision '166' collided and produced 'Revision 166
is present more than once' (duplicate head) on the PR merge ref. Renumber
to 170 (next free) with down_revision '169', and rename the migration and
its test accordingly.

Co-authored-by: openhands <openhands@all-hands.dev>
Two CI jobs failed on fork PRs for reasons unrelated to the PR diff:

1. enterprise-check-migrations / check-sync: the final 'Comment warning on
   PR' step hits a 403 ('Resource not accessible by integration') because
   fork PRs run with a read-only GITHUB_TOKEN that cannot post comments.
   That failed check skipped the apply-migrations job that needs it. The
   comment is informational, so mark the step continue-on-error: true; the
   real integrity/ancestor checks still gate the job.

2. ghcr-build / Enterprise: pushing to ghcr.io fails with 'denied:
   installation not allowed to Write organization package' on fork PRs.
   Skipping the build would leave the required per-arch checks stuck at
   'Expected' forever, so instead add a 'push' input to _build-image.yml
   and pass push: false on fork PRs — the image still compiles (build-only,
   no registry push/cache) so the required 'Build ... (amd64/arm64)' checks
   report success. The merge-manifest job is skipped on build-only runs
   since it needs pushed images. Non-fork PRs and pushes are unchanged.

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions

Copy link
Copy Markdown

⚠️ This PR contains migrations. Please synchronize before merging to prevent conflicts.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  migrations/versions
  171_repair_deepseek_default_self_hosted.py 74, 79, 143, 196, 224
Project Total  

This report was generated by python-coverage-comment-action

juanmichelini

This comment was marked as outdated.

@juanmichelini

This comment was marked as outdated.

Migration 170 declared org.llm_profiles as sa.JSON() in its sa.table()
construct. org.llm_profiles is an EncryptedJSON column whose impl is String:
the at-rest value is a JWE ciphertext string, not JSON. On the write path,
SQLAlchemy's JSON bind processor JSON-encodes the ciphertext string (wrapping
it in quotes), storing `"<ciphertext>"` and making the column permanently
undecryptable for any org 170 actually repaired.

This was invisible to clean-install VM tests (fresh DB has no pre-seeded
corrupted org, so 170's write-back loop hits zero rows and to the existing
unit test (fakes the bind/result path, never applying the JSON bind
processor). It is caught by a real-Postgres integration test that seeds
orgs with real encrypted llm_profiles before running 170 (Option B:
preserved-DB / in-place-upgrade scenario).

Root cause: sa.JSON() is benign on the read side (postgres _PGJSON
result_processor is a no-op / pass-through, so _decrypt_profiles got the raw
ciphertext string) but destructive on the write side (bind_processor
double-encodes). Switching to sa.String() matches the column's real impl and
the migration-137 precedent.

Adds scripts/verify_migratAdds scripts/verify_migratAdds scripts/verify_migratAdds scripts/verify_mns the chain to 169, seeds restore/strip/noop orgs
with real JWE ciphertext, runwith real JWE ciphertext, runwith real JWE ciphertext, runwith real JWE iewith real JWE ciphertext, runwith real JWE ciph
existing unit tests still green (18 passed).

Co-authored-by: openhands <openhands@all-hands.dev>
EOF
)
Co-authored-by: openhands <openhands@all-hands.dev>
@juanmichelini
juanmichelini enabled auto-merge (squash) September 28, 2026 12:29
…'s 170)

main merged migration 170 (add_user_allow_match_by_email) after this PR
branched, creating a duplicate revision 170 / filename prefix 170. Renumber
the deepseek repair migration to 171 (down_revision='170') so the migration
graph keeps a single linear head.

- migrations/versions/170_repair... -> 171_repair... (revision 171, revises 170)
- tests/unit/test_migration_170_repair... -> test_migration_171_repair...
- scripts/verify_migration_170_writeback.py -> verify_migration_171_writeback.py

Verified: check_enterprise_migration_integrity passes, alembic heads = 171
(single head), test_migration_graph + test_enterprise_migration_integrity
(15 passed), migration 171 unit tests (18 passed), ruff clean.

Co-authored-by: openhands <openhands@all-hands.dev>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants