fix(desktop/sessions): legacy NULL-owner session migration — single-match owner backfill + read-only stored-transcript resume (#94724) - #96110
Conversation
…profile rows (#94724) POST /api/sessions/owner-backfill stamps a store's own serving-profile identity onto its pre-#95407 'profile_name = NULL' session rows. Single match by construction (each profile's state.db belongs to exactly one profile), idempotent, one-shot-per-row, never overwrites a non-NULL owner, and reports the stamped count for logging. Refs #94724
…fill trigger (#94724) The fail-closed owner ladder (#95407) is correct for new sessions, but legacy unowned rows on registry-topology installs dead-ended in SessionOwnerResolutionError (reporter's Error B) with their transcripts fully intact in state.db. - resolveLegacyOwnerBackfillScope: pick the single-match store for the server-side owner backfill at enumeration time (serving registered connection / primary pool); fail closed on multi-candidate topologies. - maybeBackfillLegacySessionOwners: one-shot per scope per renderer, fire-and-forget from the #95407 stamp path, logs the stamped count. - Read-only stored-transcript resume: when session.resume fails closed, fetch the transcript over id-only REST (ambient first, then registered backends, read-only probes only) and open the session as a read-only transcript instead of dead-ending; sends are refused with a notice and a later successful live resume clears the latch. Wired into the main pane resume recovery and the session-tile delegate (which now runs the same fail-closed owner gate as the RPC dispatcher). Refs #94724
૮ >ﻌ< ა ci reviewran on dabb369 — feat(desktop): read-only stored-transcript resume + legacy o
|
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head dabb369fd8c6b6b6ebfaa67bbfb95eeddb19b56b against live main@1a66134404b891170e953f51662e6429f0b7b5a9. This is a direct two-commit child of current main: 19 files, +936/-12. I read the full diff, the current owner-resolution contract, the server profile/session-store routing, the new migration and read-only recovery tests, #94724, merged #95407, and the adjacent #95961/#96092 session-preservation work. There were no prior human reviews on this head when I started.
There is a lot worth preserving here. The server mutation is deliberately one-way (NULL/blank only), idempotent, and cannot overwrite an existing non-NULL owner. The live-resume path clears its read-only latch. The tile and main-composer write paths both refuse a synthetic read-only session instead of trying to turn a stored transcript into an ambient live runtime. And the work correctly treats the #95407 fail-closed owner ladder as the invariant to preserve rather than weakening it just to make old chats open again. ruangraung deserves the field-report credit already recorded in the body; #95407's salvaged Zeus-Deus, weismanfamily, and joe-rodgers ownership lineage also remains load-bearing here.
I found two P1 authority/identity holes in the recovery transaction, plus one hard repository gate.
P1 — read-only probing can display the wrong backend's same-ID transcript
apps/desktop/src/api/sessions.ts::fetchStoredTranscriptAcrossBackends() does this deliberately:
- read the ambient/primary store by bare session id;
- then iterate registered backends;
- return the first backend for which
getLatestSessionMessages(id, { connectionId, profile: 'default' })succeeds.
That is still an ownership decision, even though the result is read-only. Current main's apps/desktop/src/store/session-owner-resolution.ts explicitly documents the failure this creates: a bare session_id only means anything on the backend that owns it, and an ambient/guessed fallback can return an answer from a backend that merely happens to know a same-named session. #95407 made that ambiguity fail closed for live RPCs; this PR reintroduces it at the transcript projection boundary.
Concrete bad shape: registered gateways A and B both contain stored row legacy-1 (session ids are not a cross-install authority namespace) with different transcripts. Owner resolution correctly says “unknown”. fetchStoredTranscriptAcrossBackends('legacy-1') paints whichever matching backend is visited first. No mutation occurs, but the UI has now associated the requested chat with another backend's history. That is a privacy/data-association failure, not a safe read fallback.
Required repair: recovery may consume an exact owner proof or a unique-match proof, not “first success”. Probe identity/metadata in a way that can establish exactly one (connection, profile, stored_session_id) candidate, then fetch from that candidate. If two stores match, or an unreachable candidate makes uniqueness unknowable, preserve the SessionOwnerResolutionError / explicit ambiguity state rather than choosing. Do not weaken the existing live-RPC gate.
Please add a deterministic regression with the same stored session id present on two registered backends with different message content: recovery must refuse ambiguity and must not paint either transcript. A one-and-only-one match should open read-only. Also cover non-default profiles rather than hard-coding profile: 'default' as the probe identity.
P1 — the durable backfill drops the profile/store dimension that selected the rows
The migration's claim is “the backend that serves an enumeration owns every row it serves.” That is only true when the exact profile store that served the page is preserved through the mutation.
On this head:
stampActiveConnectionOwner(sessions)callsmaybeBackfillLegacySessionOwners()with noProfileScopeat all.resolveLegacyOwnerBackfillScope()returnsprofile: nullfor every successful case.- the POST therefore normally carries no
profile. POST /api/sessions/owner-backfillchooses its physical DB with_open_session_db_for_profile(body.profile, read_only=False)and stamps_cron_default_profile()whenbody.profileis absent.
But the existing session REST surface is explicitly profile-scoped: a named ?profile=researcher opens that profile's on-disk state.db, and the list endpoint stamps returned rows with that serving profile. So a page can be served from researcher while this follow-up POST opens the process/default store and backfills that store instead. The researcher legacy rows that triggered the migration remain NULL. The aggregated /api/profiles/sessions surfaces make the same problem sharper: one page can contain rows from multiple profile stores, but the current fire-and-forget hook has no per-row/per-store authority left by the time it mutates.
This is the same qualified-identity rule as #95407: connection alone is not the owner coordinate once a connection serves multiple profile stores.
Required repair: carry the exact serving ProfileScope into the backfill request. For a single-profile page, the POST must target the same (connectionId, profile) that produced the rows. For aggregated multi-profile pages, either perform the migration server-side while enumerating each known store or group exact stores and issue one backfill per proven store; one unscoped default-store POST cannot represent that page.
Please add a vertical regression with NULL rows in both default and researcher: enumerate only researcher, run the real backfill trigger, and prove only the researcher store is stamped with researcher. Then cover an aggregated page and prove every touched store receives only its own serving profile, exactly once. The existing “never overwrite non-NULL” regression should stay.
Hard architecture gate — hermes_state.py is being regrown
This PR adds a new SessionDB.backfill_null_session_profiles() method around line 9,385 of hermes_state.py. The repository's active decomposition doctrine has hermes_state.py in the ~9.5K-line godfile set; the hard rule is shard-before-growth, not “keep adding until its campaign starts”. This migration is a good candidate for a small session-ownership/migration seam, but it should not restore new behavior into the monolith. Preserve the exact SQL semantics and tests while routing the behavior through a sub-2K owner.
This also matters for live coordination: #96092 is independently changing the hermes_state.py session-retention surface to keep archived transcript rows from being hard-deleted, while #95961 owns the complementary reload/end-state side. They are not duplicates of #96110: #95961 keeps populated sessions open through gateway bounce, #96092 protects the durable transcript from the empty-session sweep, and #96110 restores ownership/reachability for legacy rows. They form one preservation/recovery chain and should be semantically re-read together anywhere they share state-store assumptions.
Exact-head evidence
The exact head is not green yet. Docker 33043453027 and Nix 33043453177 are green; Python, macOS, Windows, attribution, common-ancestor, supply-chain and ruff/footgun lanes inside CI are green. CI 33043453559 is red because JS & TS checks / JS & TS checks job 98422031635 failed at Run all workspace checks, which also makes All required checks pass red. The available job metadata does not expose the failing command/output, so I am not assigning a cause I cannot prove.
Graph / landing order
- #94724 remains the owning multi-gateway tracker and
ruangraung's field evidence is the direct provenance for this migration. - Merged #95407 is the authority root this work must refine, not bypass. Preserve its
Zeus-Deus/weismanfamily/joe-rodgerslineage and its connection+profile fail-closed semantics. - #95961 is complementary reload-lifecycle preservation.
- #96092 is complementary hard-delete prevention and currently shares the
hermes_state.pystate-owner surface; whichever moves first needs a fresh semantic read of the session-row invariants.
The important design choice here is right: old history should remain readable without turning “unknown owner” into permission to mutate. The remaining work is to make read authority as qualified as write authority, and to keep the exact profile/store coordinate all the way from enumeration to durable backfill. Once those two proof gaps are closed, the recovery path becomes a strong companion to #95407 rather than an exception to it. 🚀
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Final exact-object read-back after the review above: the code head is still unchanged at dabb369fd8c6b6b6ebfaa67bbfb95eeddb19b56b on main@1a66134404b891170e953f51662e6429f0b7b5a9, but GitHub now reports the PR as non-mergeable. Exact-head CI 33043453559 is also terminal red in JS & TS checks / JS & TS checks (98422031635), while Docker 33043453027 and Nix 33043453177 are green. This is a topology/evidence update only; it does not change either P1 finding or the godfile-regrowth gate in review 5037631225.
3 participants
Updating no longer strands your existing chats. After the multi-gateway ownership campaign (#95407), any Desktop with two or more registered connections failed closed on every pre-campaign session row —
profile_name = NULL, no migration — so years of history became unopenable ("Error B":SessionOwnerResolutionErroronsession.resume), even though every transcript sat fully intact in state.db. This PR ships the missing legacy-session migration plus a guaranteed recovery path, following the shape @ruangraung proposed in the field report.Credit: @ruangraung for the precise field report on #94724 (1,120 of 1,122 rows NULL, both error shapes, code-level diagnosis of the
session-owner-resolution.tstopology gate) and the repro offer — this PR implements the reporter's own proposal.What changed
1. Single-match owner backfill (durable, server-side, one shot)
POST /api/sessions/owner-backfill+SessionDB.backfill_null_session_profiles(): stamps a store's own serving-profile identity onto its legacyprofile_name IS NULL / ''rows. Single match by construction — each profile's state.db belongs to exactly one profile — so it is a migration, never a guess.stampActiveConnectionOwner→maybeBackfillLegacySessionOwners), once per serving scope per renderer.resolveLegacyOwnerBackfillScopepicks the target store fail-closed: serving registered connection → that backend; primary pool → the primary's own store; unknown source with >1 registered candidate → no stamp (multi-candidate: never guess).2. Read-only stored-transcript resume (the no-owner recovery path)
session.resumefails closed withSessionOwnerResolutionError, the session now opens as a read-only stored transcript: an id-only REST read (ambient store first, then read-only probes of each registered backend — a GET of a backend's own state.db mints nothing and routes no live session) paints the intact history instead of the dead-end error.session.resume, so tiles recover read-only instead of misrouting to the ambient socket.The fail-closed ladder itself is untouched — ambiguous owners still never ride the ambient gateway. Users just stop paying for that correctness with their history.
Live repro (two-gateway CDP rig, Electron headless :9555, fresh HERMES_HOME)
Live repro: Desktop CDP rig, persisted pre-campaign tab (no ownerRoute) + registry topology (local + URL remote
Gateway Bon :9401), 44 legacyprofile_name=NULLrows seeded in B's state.db, target row outside the sidebar page — before (main @29033a3fd5): tile dead-ends inCouldn't open this session — Session owner could not be resolved for "fl-legacy-target" (session.resume)…(the reporter's exact Error B screenshot shape); after (this branch): the same boot paints the stored transcript with the "Opened read-only" notice, a send attempt is refused with "sending is disabled", and switching to Gateway B fires the one-shot backfill — renderer logstamped 54 legacy session row(s) with profile "default" on fl-gw-b, sqlite ground truth 54 NULL → 0 NULL, target row('fl-legacy-target', 'default'), immediate re-run returnsstamped: 0(idempotent).Tests (red pre-fix, sabotage-proven)
tests/hermes_cli/test_session_owner_backfill.py— stamps only NULL/blank rows, non-NULL (researcher) untouched, idempotent second run = 0, stamped rows circulate owned on the list endpoint. Red pre-fix (route absent: 3/3 fail on main). Sabotage: widening the SQLWHEREto overwrite non-NULL rows is caught by the never-overwrite assertion.apps/desktop/src/lib/legacy-session-owner-backfill.test.ts— single-match scope resolution incl. both fail-closed rungs. Sabotage: changing multi-candidate=== 1to>= 1(guessing) is caught.apps/desktop/src/store/read-only-transcript.test.ts— drives the REALassertSessionOwnerResolvedgate on a 2-connection topology: read-only outcome with zero gateway dispatch, live path clears the latch, non-owner errors rethrow, stored-read failure rethrows the ORIGINAL owner error. Sabotage: removing the recovery catch is caught.tsc×1 project, eslint, ruff.Refs #94724
Infographic