Repository navigation
fix(ai-vault): show the newer copy of a Codex session over a stale managed-home copy #22602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,7 @@ export async function dedupeCodexRolloutAliases<T>( | |
| getFilePath: (candidate: T) => string | ||
| getCodexHome: (candidate: T) => string | null | ||
| getHardlinkIdentity: (candidate: T) => string | null | ||
| getSizeBytes?: (candidate: T) => number | undefined | ||
| }, | ||
| readSessionMetaId: (filePath: string) => Promise<string | null>, | ||
| signal?: AbortSignal | ||
|
|
@@ -144,15 +145,17 @@ const COPY_PROOF_READ_CONCURRENCY = 8 | |
|
|
||
| /** | ||
| * Drops cross-volume rollout copies only when bounded session metadata proves | ||
| * the same Codex session id. Unreadable or ambiguous candidates remain for the | ||
| * full parser and its existing post-parse identity check. | ||
| * the same Codex session id and the files are the same size. Unreadable, | ||
| * ambiguous, or diverged candidates remain for the full parser and its | ||
| * post-parse identity check, which keeps the copy with the latest activity. | ||
| */ | ||
| export async function dedupeCodexRolloutCopyAliases<T>( | ||
| candidates: readonly T[], | ||
| accessors: { | ||
| isCodex: (candidate: T) => boolean | ||
| getFilePath: (candidate: T) => string | ||
| getCodexHome: (candidate: T) => string | null | ||
| getSizeBytes?: (candidate: T) => number | undefined | ||
| }, | ||
| readSessionMetaId: (filePath: string) => Promise<string | null>, | ||
| signal?: AbortSignal | ||
|
|
@@ -200,22 +203,28 @@ export async function dedupeCodexRolloutCopyAliases<T>( | |
| if (group.length < 2) { | ||
| continue | ||
| } | ||
| const bestById = new Map<string, { candidate: T; rank: number; filePath: string }>() | ||
| for (const candidate of group) { | ||
| const bestByCopy = new Map<string, { candidate: T; rank: number; filePath: string }>() | ||
| const copyKey = (candidate: T): string | null => { | ||
| const id = idByCandidate.get(candidate) | ||
| if (!id) { | ||
| // Why size: a copy resumed in another home grows there while the original | ||
| // stays frozen; root rank alone would hide the newer turns (#22478). | ||
| return id ? `${id}\0${accessors.getSizeBytes?.(candidate) ?? ''}` : null | ||
| } | ||
|
Comment on lines
+207
to
+212
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Byte-size equality is only a proxy for divergence, so two copies whose content differs but happens to match in size are still collapsed to the ranked (possibly stale) copy before the parser can compare activity. Append-only growth makes this unlikely, but it is a residual hole in "show the newer copy" — worth calling out as an accepted limitation rather than an oversight. Technical details# Size-equality divergence proxy
## Affected sites
- `src/main/ai-vault/codex-session-root-dedup.ts:207-212` — `copyKey` collapses by `id + size`; matching sizes produce one key and the rank loser is dropped pre-parse.
## Required outcome
- No change required if byte-size growth is accepted as the divergence signal; document the residual case or key the collapse on something the parser can confirm. |
||
| for (const candidate of group) { | ||
| const key = copyKey(candidate) | ||
| if (!key) { | ||
| continue | ||
| } | ||
| const filePath = accessors.getFilePath(candidate) | ||
| const rank = codexSessionRootRank(accessors.getCodexHome(candidate)) | ||
| const best = bestById.get(id) | ||
| const best = bestByCopy.get(key) | ||
| if (!best || rank < best.rank || (rank === best.rank && filePath < best.filePath)) { | ||
| bestById.set(id, { candidate, rank, filePath }) | ||
| bestByCopy.set(key, { candidate, rank, filePath }) | ||
| } | ||
| } | ||
| for (const candidate of group) { | ||
| const id = idByCandidate.get(candidate) | ||
| if (id && bestById.get(id)?.candidate !== candidate) { | ||
| const key = copyKey(candidate) | ||
| if (key && bestByCopy.get(key)?.candidate !== candidate) { | ||
| aliasesToDrop.add(candidate) | ||
| } | ||
| } | ||
|
|
@@ -235,6 +244,17 @@ export function codexSessionAliasKey(session: AiVaultSession): string | null { | |
| } | ||
|
|
||
| export function codexSessionAliasBeats(candidate: AiVaultSession, best: AiVaultSession): boolean { | ||
| // Why content time before root rank: a diverged copy's last record is later; | ||
| // identical copies tie here and keep the root preference (#22478). | ||
| const candidateActivity = candidate.updatedAt ? Date.parse(candidate.updatedAt) : Number.NaN | ||
| const bestActivity = best.updatedAt ? Date.parse(best.updatedAt) : Number.NaN | ||
| if ( | ||
| !Number.isNaN(candidateActivity) && | ||
| !Number.isNaN(bestActivity) && | ||
| candidateActivity !== bestActivity | ||
| ) { | ||
| return candidateActivity > bestActivity | ||
|
Comment on lines
+251
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Make alias selection independent of input order. This comparison is non-transitive when one copy lacks |
||
| } | ||
|
Comment on lines
+247
to
+257
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comparison is symmetric, so a newer managed-home copy now outranks the system real-home row, not just the custom-home case the PR targets. On AI Vault resume the managed winner then reaches Technical details# Managed-home winner routes into the legacy migration guard
## Affected sites
- `src/main/ai-vault/codex-session-root-dedup.ts:249-257` — activity now outranks `codexSessionRootRank` in both directions, so the managed home can win over the real-home row.
- `src/main/codex/codex-legacy-session-resume.ts:45-53` — materialization fires when the winner's `codexHome` equals the managed home and the host is on the real-home lane.
- `src/main/codex/codex-legacy-session-resume.ts:192-203` — `assertMatchingExistingTarget` throws when the target exists and is not the same inode/content.
- `src/main/runtime/rpc/methods/ai-vault.ts:112` — the AI Vault RPC surfaces that throw to the renderer; the launch path (`src/main/startup/codex-session-resume-launch.ts:66-81`) catches it and falls back to the origin home.
## Required outcome
- Decide whether a diverged managed-vs-real-home pair should resume the newer managed copy in place (`useRealCodexHome: false`) rather than fail the migration, or treat the guarded failure as the intended outcome.
## Open questions for the human
- Is a diverged managed/real-home pair (as opposed to the custom-home pair in the PR description) expected to occur? If so, is a resume error preferable to the previous silently-stale resume? |
||
| const candidateRank = codexSessionRootRank(candidate.codexHome) | ||
| const bestRank = codexSessionRootRank(best.codexHome) | ||
| if (candidateRank !== bestRank) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not discard same-size transcripts before comparing their content.
Two homes can contain different records for the same session ID and still have equal file sizes. In that case,
bestByCopydrops one transcript before parsing, even if the dropped transcript has later activity. The equal-size assertion insrc/main/ai-vault/codex-session-root-dedup.test.tspreserves this behavior. Keep both candidates for post-parse selection unless content equality is established.