Repository navigation
fix(desktop): preserve cached replies and repair CI gates - #6129
Conversation
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed all eight changed files at be580ce against merge-base 331340a. The change removes the global database-revision fence from coalesced transcript writes, cancels writes on explicit Session deletion, updates two CI ledgers, and replaces fixed-delay/expensive paging test fixtures.
One P2 remains: a catalog refresh can delete a Session through DesktopSessionLocalStore.saveCatalog without cancelling its pending transcript write. The deleted transcript is then restored to SQLite and survives restart. The inline comment describes the reproduction and the missing cancellation call. Explicit Session deletion and authority purge do cancel their pending writes correctly.
The intended cache repair works: an ordinary catalog/name update no longer discards a queued answer. Using the actual merge-base service module restores the old loss; removing the new explicit-deletion cancellation restores that deletion regression. The catalog-deletion reproduction uses the real service refresh and SQLite store, not a mocked store. Cancelling only the missing Session before saveCatalog prevents the resurrection while preserving another Session's pending cache and ordinary rename writes.
The test rewrites retain the relevant assertions. WorkHub now waits for the event consumer's next read, after its awaited reconciliation, instead of assuming it finishes within 60 ms. Its combined direct-answer case exercises both with/without a newer request, topic recovery across restart, and no duplicate delivery on a second restart (workhub-remote-bridge.test.ts:233–256). ACP still traverses model → ACP → Host, continues a byte-limited page with the original storeVersion, and detects an external writer; the Host test separately verifies the 16-item limit and byte-limited maximal pages (acp-goal-plan-child-process.test.ts:342–417; plan-two-client-uds.test.ts:414–494).
The artifact ledger adds only node_modules/marked@18.0.11; no package manifest or lockfile changes. The measured production closure matches all 242 entries. The architecture ledger changes only AppShell's token count from 7285 to 7280; the strict merge-base comparison passes.
Verification: normal npm run build:test; 111 tests across Session-local, WorkHub bridge, ACP Goal/Plan child processes and Host Plan suites; 14 artifact-budget/closure tests; Desktop's four TypeScript configurations; strict renderer-architecture check; Windows test inventory; and git diff --check all passed. I additionally ran the cache/deletion counterexamples described above. I did not rerun the full repository suite, Electron UI, or Windows-native execution locally.
No protocol/schema change; compatibility epoch remains 222. At this exact head, test, label, and windows_acp are successful. GitHub reports MERGEABLE and BLOCKED. The P2 is not covered by those green checks and should be fixed before merging.
me2seeks
left a comment
There was a problem hiding this comment.
Independently confirmed the P2 in #6129 (comment) at be580ce.
Using the real DesktopSessionLocalService and SQLite store, I seeded a non-local Session, queued cacheTranscript(), and ran catalog() with listSessions() returning an empty list before the setImmediate write. The Session disappeared from store.sessions(), but its transcript was written back and service.readTranscript() returned it. Running the same probe with the parent service implementation leaves the transcript absent.
This is a production path: catalog notifications are forwarded as updated, and refresh reconciliation calls store.saveCatalog() directly, bypassing the new service.removeSession(). A closed Session observation cannot be relied on to deliver a separate deletion event; preload can consume the residual cache when a live transcript open fails.
Please cancel pending snapshots for Sessions removed by catalog reconciliation and cover that path while preserving a retained Session's pending write. The two new cache tests and the 22 WorkHub bridge tests passed in my focused source-bundle run; this catalog-removal case is missing from those tests.
chihumyum
left a comment
There was a problem hiding this comment.
LGTM. Dropping the global revision fence looks right: store.revision bumps on any catalog/outbox write, so it could discard a pending transcript cache write even when nothing relevant changed. The only real transcript removals are removeSession, purge and TTL cleanup, and removeSession now clears the pending #snapshots entry before deleting from the store, so the deferred write can't resurrect a removed Session. Both call sites (the deleted event in runtime-host-boot.ts and the IPC remove handler) now go through the service.
The new regressions cover catalog update during a coalesced write and Session removal / authority purge, and CI is green. The WorkHub fixture change (waiting on the consumer instead of sleeping) and the byte-limited Plan paging with item/byte parameterized Host tests also look good.
Non-blocking: the Electron cached-history E2E and the full-repo suite rely on CI, since they weren't run locally.
chinawch007
left a comment
There was a problem hiding this comment.
AI review — OpenAI Codex. This automated review does not constitute human approval or replace the independent human review required by CONTRIBUTING.md.
Reviewed all eight changed files at be580ce2257608de310748aff95f272fd7ded5dc against 331340aba5c7de5fc7d53e2441123f40f03ba34b. I found no additional distinct defect, but independently reproduced the existing P2; it remains unresolved at this head and should be fixed before merge.
The claim that all Session-removal paths now cancel pending snapshots misses catalog() → #startCatalogRefresh() → store.saveCatalog() → store removeSession(). That path bypasses the new service cancellation method. In a controlled real-SQLite probe, I queued snapshots for two Sessions, then completed a pending catalog read containing only one of them before the deferred writes ran. At this head, the absent Session's catalog row is removed, but readTranscript() still returns its reply and the orphan transcript survives reopening SQLite. The actual base service leaves that transcript absent. This is deleted transcript retention; it does not imply the Session necessarily reappears in the sidebar.
The same base/head comparison confirms the intended repair: ordinary rename/catalog updates now preserve the queued reply. Explicit service removal cancels only the removed Session's snapshot while preserving the other Session's write; authority purge cancels both. A fix for catalog reconciliation must preserve these cases rather than restore the global revision fence. I am linking the existing finding instead of adding a duplicate inline comment.
The two ledger updates and WorkHub/Plan test rewrites raised no additional findings. WorkHub waits for the consumer to finish reconciliation before requesting its next frame. ACP retains the real model → ACP → Host paging path; the Host test separately covers both item and byte limits.
Validation on this exact head: root npm run build:test; 111 targeted Session-local/preload, WorkHub bridge, ACP Goal/Plan and Host Plan tests; 14 artifact-budget/closure tests; strict renderer-architecture comparison against the base; and git diff --check all passed. The SQLite comparison covered rename, catalog removal, explicit removal and authority purge in both versions. Host/ACP tests required an unsandboxed rerun after local socket/lock permission failures. I did not run the full repository suite, Electron E2E, or a Linux production-closure regeneration locally. The current GitHub checks are green, but do not cover the reproduced catalog-removal case.
Generated-by: Codex
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed fe11723. The previously reported P2 is fixed: saveCatalog returns the IDs actually removed by its transaction, and the service synchronously cancels only those pending transcript snapshots. The deletion rule remains centralized in the store, and retained Sessions keep their pending writes.
The four focused source-bundle tests passed, covering ordinary catalog updates, catalog removal versus retained writes across SQLite restart, explicit Session removal/authority purge, and cancellation cleanup. My original real-service/SQLite reproduction now also passes; it failed at the previously reviewed head. No further blocking findings in the incremental change.
The review thread is resolved. windows_acp has passed; the main test CI is still running at approval time. This approval supersedes my earlier request for changes; hosted CI must still complete before merge.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: be580ce2..fe117239, three files (+39/-4). The P2 we reported is fixed, and I found no new P0–P2 issue.
DesktopSessionLocalStore.saveCatalognow returns the IDs its transaction actually removed.#transactionreturns the callback's result afterCOMMIT, so a rolled-back reconciliation reports nothing.#startCatalogRefreshdeletes exactly thosepartition:sessionIdkeys from#snapshotsin the same synchronous step, before thesetImmediateflush can run. A queued write for a removed Session therefore finds no entry and does nothing. Sessions still in the catalog keep their pending write.saveCataloghas no other production caller.- The new regression covers the scenario we described with the real service and SQLite store: a deferred
listSessions, pending writes for a removed and a retained Session, then a restart. It asserts the removed transcript is gone and the retained one survives.
CI test and windows_acp pass on this head.
Merge main after apache#6129 and retain its latest renderer ledger. Both CI ledgers now have no net diff against upstream; preserve the Write/Edit evidence changes. Validation: root build/typecheck/lint/format, Desktop/UI Knip, artifact budget, strict renderer architecture, 69 core regressions, ASF and protocol checks passed. Independent review found no issues. Renderer checker fixtures: 166 passed, 1 POSIX skip, 1 unchanged Windows absolute-path fixture failure. Generated-by: OpenAI Codex
Summary
Preserve completed replies in the local transcript cache when a normal Session catalog update lands before a coalesced cache write. The global database revision fence discarded that write, leaving offline history without the reply. Keep coalescing and cancel pending writes at the existing service boundary when a Session or authority is actually removed. Catalog reconciliation returns its transaction's actual deleted Session IDs so the service cancels only those pending snapshots, without duplicating the deletion rule.
Restore two stale CI ledgers: the production dependency list omits
marked@18.0.11, which Host-side image parsing needs, and AppShell's recorded token count still predates its reduction from 7285 to 7280.Stabilize two existing tests that also failed in recent CI runs:
Refs #5455, #5969.
Verification
npm run check:artifact-budgetwith 242 package instances. Install usednpm ci --ignore-scriptsbecause the check reads the dependency tree.origin/main --strict-base.npm run format,npm run lint,npx knip --workspace apps/desktop, andgit diff --checkpassed.AI use
Tool(s) and scope: Codex investigated CI failures, fixed cache invalidation, refreshed the ledgers, simplified asynchronous test fixtures, and ran verification.
Checklist
Does this PR entail a change in behavior?