refactor: unify session catalog activity authority - #3619
Conversation
Carry the storage catalog's materialized activity timestamp through Runtime Host projections and cursors. This removes the protocol's dependence on the redundant last-used timestamp and gives all clients one ordering fact. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the authority convergence itself holds up under scrutiny, and I want to say that first because the two findings below are about collateral state, not about the central idea.
What I verified about the convergence. SessionHeader.lastUsedAt is gone from current production. Schema 29 treats the historical column purely as migration input and drops it at the end. session_catalog_projection.activity_at is the single ordering and cursor fact, Runtime Host is required to send activityAt, CLI and Desktop Main share one mapper, and a Desktop catalog snapshot missing activity fails closed. The same-named fields in historical migrations and in the project catalog are not a second authority for the current Session. I also went through both heavily net-deleted test files line by line: the read-boundary second projection is genuinely removable, since a Host read-marker commit flows back through refreshCanonical → session.catalog.changed → sessions:changed → renderer refresh; and the three live-run merge tests are removable because mutation upserts are gone and only a complete catalog reaches the list. All 37 non-trivial files were classified and the mechanical ones were checked hunk by hunk with nothing smuggled in.
[P1] Plan transient state was removed as if it were a second catalog authority
app-shell.tsx:1049-1056 now discards the successful mutation's projection and waits only for a full refresh, and :1076-1083 then decides settled state from the stale sessionsRef. The required run 32643871031 (job 97205001842) lands exactly here: 54 pass, 2 fail, 1 skip, and both failures are the Plan contracts, each waiting for aria-checked=true on the first click and receiving false for a continuous 10s.
The two broken contracts are rapid on→off latest-wins, and deletion while a toggle is pending.
I want to be explicit about the fix direction, because the obvious repair would undo the point of this PR: do not put the mutation summary back into the catalog list. The catalog must stay the single authority. What needs restoring is a separate transient pending intent/view — including its latest-wins resolution and its cleanup on deletion. That is a distinct concern from catalog authority, and deleting it was collateral damage rather than a consequence of convergence.
[P1] Promise.allSettled → Promise.all lets one failing Host block every healthy one
At preload.ts:777-787, any rejecting sessions:list now discards the fulfilled snapshots from every other Host. The refresher keeps the previous full list, so while one Host keeps rejecting, updates from all healthy Hosts never reach the UI at all.
This contradicts a documented contract. docs/runtime-host-remote-access.md:151 states that Desktop keeps every Host connected independently and that "a failed remote connection remains visible without interrupting the other Hosts."
Removing the cross-Host stale cache was the right call, and I am not asking for it back. Settling this round's requests without a cache is enough: keep the Hosts that succeeded, and throw only when none succeeded. A two-ready/one-rejecting regression test would pin it.
ran: Core, Storage, Runtime Host and Desktop Main+Preload builds (green); focused Desktop 55/55 and Storage/Runtime Host 168/168; diff-check clean; independent verification of the hosted run and job outcome for this exact head.
did-not-run: the full Desktop E2E lane locally — the hosted required run above is the evidence for the Plan finding.
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
jackwener
left a comment
There was a problem hiding this comment.
Technical incremental review of exact head 44f3cfadb88edc9231094f061476199f1ca11235, acting on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.
Conclusion: COMMENT — one P2 remains. No APPROVE and no merge.
Both earlier P1 mechanisms moved in the right direction:
- Plan's committed value now lives only in a per-session transient presentation map. It is read by the active control and cleared by session renderer cleanup; it never inserts, upserts, removes, or reorders catalog rows. The old duplicate catalog-mutation authority is gone.
collectRuntimeHostSessionCatalogs()preserves every fulfilled Host group and throws only when a non-empty request set has no fulfilled group. Both new regressions are mutation-sensitive: adding an "any rejection throws" branch makes the healthy+reject case fail; deleting the all-reject guard makes the second case fail. The focused tests are green on the restored head.
[P2] A failed catalog refresh discards the committed Plan presentation and falls back to stale state
At apps/desktop/src/renderer/app-shell.tsx:1054-1063, the Host mutation can succeed and persist plan; the renderer then records that committed fact in transientPlanModeBySession and awaits refreshSessions(). But the shared refresher deliberately does not reject when the catalog read fails: apps/desktop/src/renderer/session-read-state.ts:46-60 reports the read error, keeps the old sessionsRef, and resolves normally. The existing renderer session read state test explicitly locks this behavior in as "keeps the current list when the latest list refresh fails."
Because app-shell.tsx:1074-1075 unconditionally removes the transient in finally, that failure path immediately makes the control fall back to the old catalog value. The Host is now in Plan while the UI shows Agent. applyPlanMode() also returns true, because the refresher swallowed the list failure; the generic refresh error does not reconcile the setting.
This is the same authority split the new transient was meant to bridge, now on the failed-refresh boundary rather than the ordinary delayed-refresh boundary. Keep the committed transient until a successful catalog snapshot for that session contains the same collaboration mode (or the session is deleted/cleaned up); add a regression where the Host write succeeds, the following list read fails and retains the stale catalog, and the Plan control must remain on the committed value. A later successful matching refresh should then retire the transient.
Validation: incremental surface is 3 commits / 4 Desktop files / +105/-15 over reviewed head 3dac6d9a4; Desktop 1260/1260 passed, both helper mutation runs failed at their intended assertions, restored focused tests 2/2 passed, and the worktree is clean. Three discussion endpoints were reconciled after provisional sealing: one old-head COMMENT, no inline comments, no issue comments. Fresh state is OPEN, non-draft, MERGEABLE/BLOCKED solely by review; exact-head hosted test is completed/success. No GitHub action beyond this COMMENT was taken.
Keep the latest Plan and orchestration intent in one setting owner until the Host commit is observed in a catalog snapshot. Catalog refresh failures no longer roll presentation back or prevent a queued latest intent from reaching the Host. Generated-by: Maka
jackwener
left a comment
There was a problem hiding this comment.
Reviewed on behalf of WAWQAQ under the standing Kabi review authorization, scoped to exact head 77c594b10eb7baf65ba534cd36d20138a56f41ed.
NO-GO: 1×P2. The prior failed-refresh finding is closed: a successful Host write now retains its presentation overlay when catalog refresh rejects, and the latest-intent worker continues. The new inline P2 is a different convergence boundary: a newer Host-owned value can never retire an older committed overlay.
Independent machine blockers also remain on this exact head: GitHub reports zero check-runs, and the PR is DIRTY / not mergeable against current main. A fresh merge-tree reports conflicts in apps/desktop/src/renderer/app-shell.tsx, packages/cli/src/runtime-host-session-driver.ts, packages/cli/src/__tests__/runtime-host-session-driver.test.ts, and packages/runtime-host/src/protocol/index.ts. Local full build and both new focused E2E cases passed, but absent hosted checks are not green. No approval or merge performed.
Fence committed mode overlays by successful catalog observation revision instead of value equality. Runtime-owned transitions such as Plan approval can now supersede an older renderer commit without letting failed reads discard it. Generated-by: Maka
Resolve the Desktop shell, shared Session catalog mapper, CLI mapper tests, and Runtime Host compatibility comment against current main while preserving the single catalog projection authority. Generated-by: Maka
Current main already owns epoch 43 for shell-run poll correlation. Advance the retired Session timestamp wire change to epoch 44 and pin that compatibility floor. Generated-by: Maka
jackwener
left a comment
There was a problem hiding this comment.
Independent review at exact head 5fde3d4951d5bfbcd91518ebc9e96b33616f4d13.
GO. No open P0–P2. The prior overlay P2 is closed, not relocated.
useSessionSettingIntent now fences the committed overlay on successful catalog observation revision (use-session-setting-intent.ts:70-76, 109) instead of requiring the catalog value to equal intent.committed. A failed refresh still keeps the overlay (refreshCatalog is a nudge; list commit only happens on success in use-app-shell-session-list.ts:54-72,79-88). A later successful snapshot with a higher catalogRevision retires the overlay even when Runtime has already left Plan for Agent.
Covered by session-setting-intent.test.ts (“Runtime leaving Plan after approval supersedes the committed Plan overlay”): write succeeds, refresh throws, then revision 1 with Agent value clears the Plan overlay.
Path-filtered checks on this head: test completed+success. audit/package did not run (no lockfile/package.json change). No merge from this seat.
* refactor: publish canonical session catalog activity Carry the storage catalog's materialized activity timestamp through Runtime Host projections and cursors. This removes the protocol's dependence on the redundant last-used timestamp and gives all clients one ordering fact. Generated-by: Maka * refactor: share canonical catalog projection Generated-by: Maka * refactor: accept complete desktop catalog snapshots Generated-by: Maka * refactor: retire session last-used authority Generated-by: Maka * refactor: remove session settings list patches Generated-by: Maka * refactor: remove desktop catalog upserts Generated-by: Maka * refactor: delete session summary merge layer Generated-by: Maka * refactor: drop legacy session activity column Generated-by: Maka * refactor: remove remaining desktop catalog patches Generated-by: Maka * chore: restore canonical ASF source header Generated-by: Maka * fix(desktop): repaint committed session settings Generated-by: Maka * fix(desktop): keep plan state outside catalog snapshots Generated-by: Maka * fix(desktop): retain healthy host catalogs Generated-by: Maka * fix(desktop): reconcile mode intents with catalog snapshots Keep the latest Plan and orchestration intent in one setting owner until the Host commit is observed in a catalog snapshot. Catalog refresh failures no longer roll presentation back or prevent a queued latest intent from reaching the Host. Generated-by: Maka * fix(desktop): retire settings on newer catalog snapshots Fence committed mode overlays by successful catalog observation revision instead of value equality. Runtime-owned transitions such as Plan approval can now supersede an older renderer commit without letting failed reads discard it. Generated-by: Maka * fix(runtime-host): advance catalog protocol epoch Current main already owns epoch 43 for shell-run poll correlation. Advance the retired Session timestamp wire change to epoch 44 and pin that compatibility floor. Generated-by: Maka
Summary
Make
session_catalog_projection.activity_atthe single Session catalog recency authority across Runtime Host, CLI/TUI, and Desktop.activityAtprojection and reuse its mapper across clientslastUsedAtmodel/protocol field and drop its legacy SQLite column through schema migration 29The Runtime Host compatibility epoch is 43 because strict Session continuity and inspection projections changed. Schema migration 29 temporarily restores the legacy column only for synthetic or interrupted pre-29 schemas that lack it, lets historical migrations complete, then removes it.
Verification
npm run build --workspace @maka/corenpm run build --workspace @maka/storagenpm run build --workspace @maka/runtimenpm run build --workspace @maka/runtime-hostnpm run build --prefix packages/clinpm run build --workspace @maka/uinpm run typecheck --workspace @maka/desktopgit diff --check origin/main...HEADThe full repository test suite was not run locally; CI should provide repository-wide coverage.
Migration
Schema 29 removes
session_metadata.last_used_atafter preserving the canonical catalog projection. Historical migration references remain intentionally so databases at older schema versions can advance safely before the column is dropped.AI use
Select exactly one:
Tool(s) and scope: Maka assisted with architecture analysis, implementation, tests, migration work, and PR drafting. The contributor must review the final diff, provenance, and commit messages before merge.
Checklist
Does this PR entail a change in behavior?