Repository navigation
Conversation
One-off verification for the session checkpoint/restore design: proves the native flock behind SessionWriteLease loads and contends under plain Node, Electron Node mode, and the Electron main process, and that the session-persistence-jsonl read path (list, open, migration prepare) validates a copied session tree without producing writes.
stopHarness resolving only proves the child is gone, so the Host now persists its own drain outcome synchronously to $DSH_HOME/electron/shutdown-report.json before every exit path it controls, and Main merges the supervision facts (signal, timeout, kill, exit result) after observing the child exit. A report missing either section, or recording a failed, timed-out, killed, or interrupted run, is never promotable for session checkpoint capture.
Promotable shutdown reports (matching host/main pids, clean drain, no timeout/kill) now trigger a checkpoint capture under $DSH_HOME/electron/checkpoints. The covered set is copied into a staged candidate with per-file sha256 digests and live source stats; sessions whose fresh copy fails validation carry forward their previous verified copy and the mixed candidate is revalidated before commit. Validation runs the bundled Harness read path in a separate read-only worker process, and the candidate is committed by atomic rename only after the manifest is durably written. Capture failures and cancellation never block application shutdown.
cherrchen
changed the title
A session restore replaces live generation files inside the session's own session.lock write lease, journaled intent -> committed -> finalized under $DSH_HOME/electron/restore-journals/. The intent fingerprints live files (size + mtimeNs) before any switch; live files move to quarantine and checkpoint bytes publish under canonical names only after staging a synced *.restore.tmp sibling. A leftover journal converges before the next Host spawn: intent journals re-acquire leases, re-check fingerprints, and stop on drift; committed journals never re-apply the switch but still finish projcache invalidation before the journal is deleted. Restore re-validates the checkpoint copy through the bundled read path under the current build and refuses sessions without a verified copy, contended leases, a running local Host, or manifests lacking related-source headers. Manifest session entries now record the stored header so restore derives the related-source closure (transitive subagent children) and re-acquires leases on resume.
Desktop now detects sessions whose live bytes drifted from the newest verified checkpoint before the Host starts: manifest stat-diff plus a generation-file scan produce candidates, and the read-only validation worker confirms unreadability before any prompt. Open-time format refusals are recognized on the remote.mux wire by their stable SessionFormatUnsupportedError text, and a Session Rollback entry in the application and tray menus exposes the same detection manually. A confirmed restore while the Host runs is queued in electron/pending-restore.json and executed during the next launch's pre-Host window; with the Host down it runs in place. All prompts and outcome notices are locale-owned and disclose the checkpoint time, hidden data range, quarantine directory, and restore limitations.
cherrchen
changed the title
Add checkpoint-acceptance.spec.ts covering design acceptance cases 1-9: - case 1: carry-forward keeps pre-pollution bytes restorable across two clean exits after pollution - case 2/6: real bundled flock lease rejects competing acquire; a held lease refuses restore without touching live bytes - case 3: read-open fails fast at each switch stage and succeeds after publish - case 4: intent journals interrupted after staging and mid-switch converge on next startup - case 7: failed dispose, drain timeout, killed host, and boot failure all leave the committed checkpoint untouched - case 8: unknown required event (appended as a real zstd frame) and a newer session.vN generation are refused by the bundled read path and restore cleanly - case 9: the restored branch accepts further writes while the quarantined foreign branch stays on disk Widen the mounted sessionPersistence.open surface to read|write so the acceptance spec can append through the real write path after a restore. Case 10 remains covered by checkpoint-detect.spec.ts; case 11 is post-v1.
cherrchen
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9993e56aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const line = output.split('\n').find(entry => entry.startsWith('RESULT ')) | ||
| if (line !== undefined) { | ||
| result = JSON.parse(line.slice('RESULT '.length)) as CheckpointValidationReport |
There was a problem hiding this comment.
When the validation report exceeds a stdout chunk (for example, a checkpoint with many sessions or large headers), output can contain a partial line beginning with RESULT , so this callback immediately passes incomplete JSON to JSON.parse. The resulting exception escapes the stream callback and can crash Electron during exit capture or startup detection; buffer until a complete newline-delimited result is available before parsing.
Useful? React with 👍 / 👎.
| export async function consumePendingRestores(home: string): Promise<readonly PendingRestoreRequest[]> { | ||
| const pending = await readPendingRestores(home) | ||
| if (pending.requests.length === 0) return [] | ||
| await rm(pendingRestorePath(home), { force: true }) |
There was a problem hiding this comment.
When one or more restores were confirmed before relaunch, this removes the entire queue before restoreCheckpointSession performs validation, acquires leases, or persists its intent journal. A crash or thrown startup error in that window leaves no journal and loses every confirmed request, including later requests not yet attempted; dequeue each request only after its journal is durable, or persist the unprocessed remainder.
Useful? React with 👍 / 👎.
| for (const ch of raw) { | ||
| // codePointAt is defined for every unit of a non-empty string. | ||
| const code = ch.codePointAt(0) ?? 0 |
There was a problem hiding this comment.
For a session ID containing a non-BMP character such as an emoji, this for...of/codePointAt implementation emits one escape such as ~1F600, while the backend's authoritative format.ts mapping iterates UTF-16 code units and emits ~D83D~DE00; projectKey has the same divergence for such characters in cwd. Validation therefore reports the wrong dir, causing a valid session to be treated as unlisted or leaving a manifest entry disconnected from its copied files, so these sessions are not checkpointed/restored correctly.
Useful? React with 👍 / 👎.
| await convergeDesktopRestoreJournals({ | ||
| home: harnessHome, | ||
| appPath, | ||
| log: (message) => { console.error(`desktop checkpoint: ${message}`) }, |
There was a problem hiding this comment.
If an interrupted multi-session restore is resumed after a foreign writer changes an unswitched member, convergence returns that journal in conflicted after earlier members may already have been switched. This caller discards the result and proceeds to start the Host, so a packaged user receives no dialog about the partially restored closure and subsequent detection may see readable files and remain silent; inspect the returned conflicts and present or otherwise require acknowledgment before normal startup.
Useful? React with 👍 / 👎.
| const carried = await carryForwardSession(candidate, previous.dir, previous.manifest, dir) | ||
| if (carried === undefined) { | ||
| log(`checkpoint ${id}: session ${dir} failed validation and has no verified copy; excluded`) |
There was a problem hiding this comment.
When any previous checkpoint exists but does not contain a newly encountered invalid session, carryForwardSession returns undefined and this branch only logs that the session was excluded. Its copied directory and file entries remain in the candidate, so a validator that reports the directory as failed aborts the entire checkpoint and prevents valid sessions from receiving a fresh backup; apply the same candidate-file and directory removal used when no previous checkpoint exists.
Useful? React with 👍 / 👎.
| /** | ||
| * Checkpoint manifest schema (docs/electron/session-checkpoint-restore.md). | ||
| * One committed checkpoint directory under `$DSH_HOME/electron/checkpoints/` |
There was a problem hiding this comment.
The new modules repeatedly designate docs/electron/session-checkpoint-restore.md as the owner of this durable-data and recovery contract, but that file is absent in a repo-wide search and neither Electron README documents the new menu, checkpoint storage, restore limitations, or recovery behavior. Add the owning documentation and its required bilingual counterpart so the externally visible feature and on-disk state are documented alongside the implementation.
AGENTS.md reference: AGENTS.md:L176-L176
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
- worker: buffer stdout and parse the RESULT line only once a complete newline-terminated line arrives, so a chunk-split result no longer throws inside the stream callback; malformed results reject cleanly. - pending: dequeue each confirmed restore only after its attempt resolves; a crash mid-queue keeps the unprocessed remainder durable. - validate: mirror the backend's UTF-16 code-unit encoding exactly — non-BMP characters now map to ~D83D~DE00 as upstream, not ~1F600. - capture: a failed session the previous checkpoint never held is now excluded from the candidate instead of aborting the whole commit. - main: conflicted restore journals left by an interrupted switch are surfaced through the localized conflict notice before the Host spawns. - docs: add the owning session-checkpoint-restore + shared-dsh-home bilingual docs, document the Restore a Session menu entry in the Electron README, and update architecture.md accordingly.
|
Addressed all six findings in
One doc-vs-code conflict surfaced while updating the docs: the design said "quarantine gets a quota and cleanup policy", but v1 implements no automatic quarantine cleanup. The doc now states the shipped behavior (quarantine keeps every replaced byte; deleting it is the user's choice). If the quota/cleanup should actually ship, that's a follow-up. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 001a712a77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session: SessionModule | ||
| }> { | ||
| const backendRequire = createRequire(installAnchor) | ||
| const backend = await import(pathToFileURL(backendRequire.resolve('@deepseek-ai/dsh-session-persistence-jsonl')).href) as BackendModule |
There was a problem hiding this comment.
In packaged builds, this resolver is anchored at @deepseek-ai/dsh, but @deepseek-ai/dsh-session-persistence-jsonl is only a dev dependency of that package; its production occurrence is transitive through @deepseek-ai/dsh-base, and the Electron manifest does not declare it directly. Under pnpm's strict layout, createRequire(installAnchor).resolve(...) therefore cannot reach the backend, so every capture, drift scan, and restore fails before validation starts. Declare the backend as a production dependency reachable from this anchor or resolve it through its actual owning package.
Useful? React with 👍 / 👎.
|
|
||
| if (import.meta.main) { | ||
| main().then( | ||
| () => process.exit(0), |
There was a problem hiding this comment.
When the serialized report exceeds what the stdout pipe accepts synchronously, process.stdout.write queues part of it and this immediate process.exit(0) discards the queued tail. The parent then receives a malformed or missing result, disabling checkpoint capture or restore for homes with sufficiently many sessions. The new newline buffering does not address this producer-side truncation; await the write callback/drain and let the worker exit naturally.
Useful? React with 👍 / 👎.
| } | ||
| }) | ||
| child.stderr.on('data', (chunk: Buffer) => { process.stderr.write(chunk) }) | ||
| const [code, signalCode] = await once(child, 'exit') as [number | null, NodeJS.Signals | null] |
There was a problem hiding this comment.
For a worker with buffered stdout, the child exit event may arrive before the stdout stream has emitted all remaining data; this code immediately inspects buffered and can report a missing or malformed result while valid bytes are still pending. This remains possible even after the worker is changed to flush its write, especially for large reports. Wait for close or both process exit and stdout end before parsing the final buffer.
Useful? React with 👍 / 👎.
| try { | ||
| let buffered = '' | ||
| child.stdout.on('data', (chunk: Buffer) => { | ||
| buffered += chunk.toString('utf8') |
There was a problem hiding this comment.
When a stdout chunk boundary splits a multibyte character in a session ID, cwd, or error message, decoding each Buffer independently inserts replacement characters even though the eventual JSON can remain syntactically valid. The resulting manifest can contain corrupted IDs or headers, causing later lookup and lease acquisition to target a different session identity. Use setEncoding('utf8') or a StringDecoder so incomplete byte sequences carry into the next chunk.
Useful? React with 👍 / 👎.
| if (convergence !== undefined) { | ||
| for (const journalId of convergence.conflicted) { |
There was a problem hiding this comment.
When a foreign writer holds any journal member's lease, convergence returns that journal in deferred, but this block handles only conflicted and startup later spawns the Host normally. An intent journal may already represent a partially switched closure, while a committed journal may still have stale projection-cache rows, so booting over either state violates the recovery ordering and can expose or extend inconsistent data. Surface the deferred state and prevent normal Host startup until it can be safely converged or explicitly resolved.
Useful? React with 👍 / 👎.
| throw new RestoreConflict(join(member.dir, name), 'live file changed during the interrupted restore') | ||
| } | ||
| await mkdir(dirname(quarantinePath), { recursive: true }) | ||
| await moveFile(livePath, quarantinePath) |
There was a problem hiding this comment.
On POSIX, this cross-directory rename is followed only by an fsync of liveDir; the quarantine destination directory is never synced. A crash can therefore persist removal of the canonical entry without durably retaining its quarantine entry, after which resume sees neither copy and reports a conflict even though the transaction promised to preserve every replaced byte. Fsync the quarantine member directory as part of the move's durability ordering before recording the switch as committed.
Useful? React with 👍 / 👎.
| export async function restoreSession(options: RestoreSessionOptions): Promise<RestoreOutcome> { | ||
| const log = options.log ?? (() => undefined) | ||
| if (options.hostRunning?.() === true) { | ||
| return { kind: 'refused', reason: 'the local Harness Host is still running' } |
There was a problem hiding this comment.
When a Chinese-locale user reaches this refusal, reason is interpolated into the localized dialog body unchanged, so the primary explanation remains hardcoded English; the other refusal and failure branches follow the same pattern. Return a typed reason key plus substitutions, or localize the reason before building the dialog, rather than transporting product copy from the restore implementation.
AGENTS.md reference: AGENTS.md:L156-L156
Useful? React with 👍 / 👎.
|
|
||
| English | [中文](session-checkpoint-restore.zh.md) | ||
|
|
||
| > The mechanism described here is implemented in `apps/electron/src/checkpoint/`. [Mechanism facts](#mechanism-facts) records the upstream behavior it builds on; the alternatives, open questions, and acceptance cases record the review that produced the design. |
There was a problem hiding this comment.
This new reference explicitly preserves the review's alternatives, open questions, and acceptance cases, and the remainder of the page contains those design-history sections rather than only current behavior. That material will drift from the shipped implementation and belongs in an Agent Note or owner-local tests; keep this page as a current-state mechanism reference and link to the rationale owner.
AGENTS.md reference: docs/AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
- resolve the validation backend through dsh-base, which owns it as a production dependency, instead of the dsh anchor's dev-only reach - flush the worker RESULT write before exit and wait for child 'close' with stream-level utf8 decoding, so large or split reports parse intact - gate Host startup on deferred restore journals and on a failed convergence sweep: retry, explicit continue-anyway, or quit - fsync the quarantine directory after the cross-dir live-file move so the committed record cannot lose both copies - return typed refusal keys from restore and localize them in Main - move design rationale and open follow-ups from the reference doc into an Agent Note; keep the bilingual pair current
…al detection The event-admission pattern expected a message the upstream never emits, so open-time restore prompts never fired for in-place unknown-event pollution (the motivating job/registered incident). Match the verbatim upstream strings — both refusals carry session "<id>", so event hits now attribute by session id — and cover the retired request/header shape plus a corruption-report negative case.
Journal ids, quarantine roots, and projcache row names interpolated the unvalidated upstream session id, so a hostile or corrupt id containing ../ escaped restore-journals/, quarantine/, and storages/ — including an arbitrary *.json delete under storages. Derive journal/quarantine names from the backend-encoded dir tail, skip projcache rows for ids the backend's SAFE_KEY_RE could never have written, and reject manifest session dirs that are not <project>/<encoded id> segments.
…dicts The manual restore scan ran the validation worker against the live sessions root while the local Host could still be appending; a torn zstd tail then read as unreadable and offered a rollback of a healthy session. detectUnreadableSessions gains confirmIdleLease: candidates must additionally acquire the session's write lease, and held or unleasable (header-less) candidates drop out. The session lease seam accepts a minimal subject so detection need not fabricate journal members.
The failure branch reused sessionRestoreNoneBody, telling the user every checkpointed session reads cleanly precisely when the scan itself errored. A dedicated sessionRestoreScanFailedBody says the scan did not complete and nothing was changed.
resumeMember renamed a leftover .restore.tmp straight into the canonical name, trusting bytes the journal never re-checked — a crash mid-copyFile leaves a truncated temp that would publish corrupt data (and wedge the journal against every later resume once the live file was already quarantined). The staged temp is now digest-checked against the plan and re-staged from the checkpoint on mismatch, which is what the journal contract already documented.
A journal whose checkpoint was pruned could never converge, so the startup gate nagged forever with retry/continue/quit and no exit. The sweep now reports intent-phase journals missing their checkpoint as unresolvable — committed and finalized leftovers still converge, since only the switch needs checkpoint bytes — and a dedicated gate offers Abandon Restore (deletes the record), Keep Pending (retry next launch), or Quit, with keep as the non-destructive default.
finalizeProjcache deleted only per-record documents, but the unit bootstraps an empty per-record tree from storages/session_projcache.json — so a fully invalidated member set could resurrect its stale rows on the next Host boot. The legacy file is now rewritten without member rows (other sessions' rows kept) when it parses as the unit's own document; malformed or foreign files are left alone, matching the backend's own tolerance.
…undary readManifest and readRestoreJournal checked only version (and journal phase), then cast parsed JSON — a structurally malformed record threw TypeErrors deep in callers, and hostile ids, quarantine paths, member dirs, or publish names could drive paths outside the intended roots. Both readers now verify list shapes, digest fields, and path-segment safety; an unsafe record is treated as unreadable — skipped checkpoints and left-in-place journals — instead of failing mid-transaction.
The backend reserves session dirs for future session-local artifacts; a nested file in a manifest publish list would land at staging time as an ENOENT (staged path under a missing subdir) or, with traversal segments, outside the member dir. The member publish map now rejects any name that is not a single path segment, failing the restore with a clear error instead of a filesystem accident.
…racy - copyIntoCandidate detached its per-file abort listener once the stream settled; a large covered set no longer piles listeners on the shared deadline signal. - The exit deadline now reaches carry-forward, exclusion, and rotation: carryForwardSession and pruneCheckpoints take the capture signal and check it inside their file loops. - Documented the two accepted residues: promotion binds a report to a run by pid alone (safe because capture validates every byte), and rotation keeps checkpoints whose manifests cannot be read. - The confirm dialog said replaced files move to quarantine "at" the root; they land in a journal-id subdirectory under it — copy and zh pair now say so, and the pre-restore quarantine fact names the root. - verify-checkpoint-prereqs.mjs encoded path segments by code point while the shipped encoder iterates UTF-16 code units; the probe now mirrors charCodeAt so astral characters escape identically.
CI oxlint flagged the shape guards: asserting parsed JSON to CheckpointManifest/RestoreJournal made declared literal fields compare redundantly, and Array.isArray narrowed the readonly arrays to any[] so member access read as unsafe. The guards now narrow Record<string, unknown> field by field; a shared isCheckpointDigest covers manifest files and journal publish entries.
…ed restore targets Audit follow-ups on session checkpoint/restore: - Request a cooperative Host drain over a private IPC channel instead of relying on SIGTERM, keeping SIGKILL as the grace-period escalation and SIGTERM as the fallback for children without IPC; unref the channel so CLI exits still drain the event loop. - Re-run restore journal convergence after startup maintenance so a journal created by this launch's restore reaches the retry/continue/quit decision before the Host spawns. - Make validation-worker cancellation reachable: abort before spawn is a no-op, SIGTERM escalates to SIGKILL after the grace period, and settling waits for worker stdio to close. - Record the confirmed checkpoint id on queued restore requests, protect queued targets from retention, and refuse missing or legacy targets rather than redirecting to a newer checkpoint. - Cover the real Host IPC-drain-to-checkpoint path in tests and Windows CI; document the new shutdown, cancellation, and gating behavior.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a9f120308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pending = await listPendingRestores(options.home) | ||
| await pruneCheckpoints(root, options.signal, new Set(pending.flatMap(request => | ||
| request.checkpointId === undefined ? [] : [request.checkpointId]))) |
There was a problem hiding this comment.
When an intent journal is deferred and the user chooses “Continue Anyway,” a later clean exit can prune the exact checkpoint needed to resume it because this protection set includes only pending-restore requests. Once that checkpoint ages beyond the newest three and another retained checkpoint covers the same sessions, the next launch classifies the journal as permanently unresolvable even though the application itself deleted its recovery source; include checkpoint IDs referenced by intent journals in the retention protection set.
Useful? React with 👍 / 👎.
| let deferredCount = convergence?.deferred.length | ||
| ?? await listRestoreJournals(home).then(paths => paths.length).catch(() => 1) |
There was a problem hiding this comment.
When readRestoreJournal rejects a malformed or newer-version journal, convergence leaves it on disk but adds it to none of the returned buckets. Because a successful sweep derives this count solely from convergence.deferred, the startup gate sees zero and launches the Host without any decision even though the unreadable record may represent a partially applied switch; count all journal paths that were not converged or otherwise classified.
Useful? React with 👍 / 👎.
| // be recorded committed: a crash that kept only the live-side removal | ||
| // would lose the file entirely, and resume would report a conflict | ||
| // over data the transaction promised to preserve. | ||
| await syncDirectory(quarantineMemberDir) |
There was a problem hiding this comment.
Although this now fsyncs quarantineMemberDir, a cross-directory rename also changes liveDir, which is not synced until the entire member finishes. On a POSIX crash between this call and that later sync, the quarantine entry can survive while the old canonical entry reappears; resume then observes both copies and marks the journal conflicted because the canonical file still has the pre-restore digest. Sync both the source and destination parent directories immediately after each move.
Useful? React with 👍 / 👎.
| const handle = await open(destination, 'r+') | ||
| try { | ||
| await handle.sync() | ||
| } finally { | ||
| await handle.close() |
There was a problem hiding this comment.
On POSIX this flushes each copied file but never fsyncs the nested directory that gained its entry. Syncing the checkpoint root after the final directory rename does not make entries under sessions/<project>/<session>/ durable, so a power loss after a reported commit can leave a durable manifest referring to missing checkpoint files and destroy the backup's restore value. Flush the affected directory hierarchy before publishing the candidate.
Useful? React with 👍 / 👎.
| const leases = await backendLeaseAcquirer(options.home, anchor) | ||
| try { | ||
| return { manifest: detection.manifest, unreadable: await filterIdleWriters(unreadable, leases.acquire, options.log) } |
There was a problem hiding this comment.
When a CLI or Host finishes a short append after the worker encounters its partially written frame but before this post-validation lease acquisition, the lease succeeds and preserves the stale unreadable verdict. The user can then be prompted to roll back a session that is currently healthy, and restore revalidates only the checkpoint rather than the live copy; acquire the candidate lease before validation and hold it through the read, or revalidate after acquisition.
Useful? React with 👍 / 👎.
| const second = await options.validate(join(candidate.dir, 'sessions'), options.signal) | ||
| const stillFailing = second.sessions.filter(result => !result.ok) | ||
| if (stillFailing.length > 0) { |
There was a problem hiding this comment.
When a carried or otherwise retained session is omitted by the second backend listing—for example, after a downgrade rejects that checkpoint's header—stillFailing remains empty because it checks only sessions present in the report. The candidate then commits with that session's files orphaned from manifest.sessions, despite the promise that mixed candidates are revalidated under the current build; compare the second report against every session directory expected to remain as well as checking explicit failures.
Useful? React with 👍 / 👎.
| sessionRestoreRefuseValidation: 'the checkpoint copy no longer reads cleanly under this version ({dirs})', | ||
| sessionRestoreRefuseLease: 'another Harness instance is writing it ({detail})', | ||
| sessionRestoreConflictTitle: 'Session restore stopped', | ||
| sessionRestoreConflictBody: 'Live data changed while the restore was interrupted, so nothing was overwritten: {conflict}', |
There was a problem hiding this comment.
If a multi-session closure switches one member and then detects drift in a later member, the earlier member remains restored and its prior bytes are quarantined. This dialog nevertheless tells the user that nothing was overwritten, which can lead them to continue using a partially rolled-back closure under the wrong assumption that all live data is unchanged; state that the restore stopped after partial work and direct the user to quarantine.
Useful? React with 👍 / 👎.
Unreadable restore journals now count as deferred so startup gating requires an explicit decision; journal-directory read failures surface instead of masquerading as an empty set. Intent journals protect their checkpoint from retention, and an unreadable journal suspends pruning entirely. The pending queue carries stable operation ids into restore journals: a finalized journal acknowledges its request durably before deletion, a crash after completion no longer replays the rollback, and journals that already own a request suppress a second transaction. Checkpoint publication flushes the staged directory tree leaf-to-root; carried copies sync when hard links are unavailable. Quarantine moves flush both parents, and the EXDEV fallback copies to a temporary, verifies and flushes it, publishes, then unlinks — resume recognizes the identical double-copy intermediate state instead of reporting a foreign conflict. Conflict outcomes report the journal and quarantine location without claiming nothing was overwritten. Detection keeps carried sources under digest confirmation, holds the related-source write-lease closure through the validation read, and treats sessions omitted from the second-pass listing as failures.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6ecdc1842
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raw = await readFile(pendingRestorePath(home), 'utf8') | ||
| } catch { | ||
| return { version: 1, requests: [] } |
There was a problem hiding this comment.
When pending-restore.json exists but readFile fails transiently (for example, due to an I/O or permission error), this treats the queue as empty rather than distinguishing ENOENT. During exit capture, protectedRestoreCheckpoints then omits every queued checkpoint from its protection set, so pruning can delete the confirmed restore target; an enqueue during the same condition can also overwrite the existing requests. Propagate non-ENOENT failures so callers retain the queue and suspend pruning.
Useful? React with 👍 / 👎.
| onAbort = () => { | ||
| reject(signal?.reason instanceof Error ? signal.reason : new Error('checkpoint capture aborted')) | ||
| } | ||
| signal?.addEventListener('abort', onAbort, { once: true }) | ||
| read.pipe(write) |
There was a problem hiding this comment.
If the capture deadline fires after stageCandidate checks the signal but while this function awaits stat or mkdir, the abort listener is registered on an already-aborted signal and is never invoked. The subsequent stream copy therefore runs to completion, so a large session file can hold application shutdown beyond the advertised 120-second capture budget. Recheck the signal after registering the listener or use an abort-aware stream pipeline.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fec43db822
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| return // Missing or malformed legacy file: not ours to interpret or delete. |
There was a problem hiding this comment.
When session_projcache.json exists but readFile fails transiently, such as with EIO, this catch treats the failure like an absent or malformed legacy file. convergeJournal then persists finalized and deletes the restore journal even though the stale legacy row remains; if the next Host read succeeds and the per-record tree is empty, legacy bootstrap can resurrect the pre-restore projection. Ignore only ENOENT and malformed JSON, and propagate other I/O failures so the journal remains deferred.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
| const queued = await offerSessionRestore({ | ||
| sessionId: resolved.entry.sessionId, | ||
| checkpointId: resolved.checkpointId, | ||
| capturedAt: resolved.entry.capturedAt, |
There was a problem hiding this comment.
When a checkpoint mixes capture times—for example, a parent captured at 2000 with a failed child carried from 1000—the restore transaction replaces both members, but this prompt receives only the target parent's capturedAt. It therefore tells the user that only changes after 2000 will be hidden even though the child rolls back to 1000. The open-time, startup, and manual callers all pass the target entry's timestamp; derive the warning from the oldest member in resolveRestoreClosure or disclose the per-member times.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| options.onError(error) | ||
| } | ||
| return options.gate() |
There was a problem hiding this comment.
In the Main-process wiring, gate presents dialogs as well as checking state. If the first pass leaves a journal unchanged—by choosing Continue Anyway for a deferred journal, Keep Pending for an unresolvable journal, or dismissing a conflict notice—this unconditional second call immediately presents the same decision again before starting the Host. Preserve the first pass's acknowledged journal IDs or make the final pass prompt only for journals created or changed during maintain.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec2dc75777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| publish: found.manifest.files | ||
| .filter(file => file.path.startsWith(`sessions/${entry.dir}/`)) |
There was a problem hiding this comment.
When a session-owned directory contains an extension file such as metadata.json—a layout explicitly supported by packages/session/session-persistence-jsonl/tests/jsonl.spec.ts:2541-2551—capture includes it and this prefix filter selects it for publication. Because liveFiles fingerprints and quarantines only isOwnedSessionFile generation names, restore renames the staged old extension file over its current version without a drift check or quarantine copy, potentially destroying plugin/session-owned data outside the promised session-history rollback scope. Filter publish to canonical generation files and leave other archived artifacts untouched.
Useful? React with 👍 / 👎.
| try { | ||
| entries = await readdir(root, { withFileTypes: true }) | ||
| } catch { | ||
| return |
There was a problem hiding this comment.
When readdir returns EACCES, EIO, or another transient error for the sessions root or a child directory, this catch silently reports no files, allowing runExitCheckpoint to commit a newest manifest that omits one or every session. Startup drift detection scans only that newest manifest, so if a newer CLI later publishes an unreadable generation for an omitted session, Desktop does not detect or offer the older retained copy and the session can disappear from the UI. Propagate enumeration failures, with a named caught error, rather than publishing a partial candidate.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
…enumeration errors Restore staged every file under a session dir, so extension artifacts were overwritten or quarantined; limit publish/fingerprint to canonical generation files and reject persisted journals whose members reach beyond that set. Treat session-tree readdir failures as capture aborts instead of empty dirs so a partial checkpoint cannot commit; a missing sessions root still succeeds for a fresh home.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8412554cbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (found === undefined) { | ||
| return { kind: 'refused', reason: { key: 'no-checkpoint' } } | ||
| } | ||
| const members = resolveRestoreClosure(found.manifest, options.sessionId) |
There was a problem hiding this comment.
When a newer Harness creates a subagent after the selected checkpoint and then makes the checkpointed parent unreadable, this closure is derived only from the old manifest; startup detection likewise ignores sessions absent from that manifest. The restore therefore rolls the parent back while leaving the newer child live, potentially orphaning or hiding that child's history even though the transaction promises to restore the related-session closure. Enumerate the live headers before switching and refuse the restore when a live descendant has no verified checkpoint copy.
Useful? React with 👍 / 👎.
| files: candidate.files, | ||
| sessions, | ||
| } | ||
| await commitCandidate(root, candidate, manifest) |
There was a problem hiding this comment.
When validation finishes near the 120-second shutdown deadline, the abort signal can fire immediately before or during this call, but commitCandidate does not inspect it while recursively syncing the staged tree and renaming it. A large candidate can therefore keep Electron's before-quit path blocked well beyond the advertised capture budget and can still be published after cancellation; check the signal before commit and propagate it through the directory-sync phase.
Useful? React with 👍 / 👎.
1 participant
Summary
Desktop session checkpoint/restore, Phases 1–2: durable lifecycle evidence plus verified checkpoint capture at clean exits.
Phase 1 — shutdown report (
fe65d2aedf)src/harness/shutdown-report.ts: report schema, synchronous atomic Host writer (tmp + fsync + rename + dir fsync), Main-side merge viawithDesktopFileWriter+writeDesktopFileAtomicwith pid-scoped replacement and strongest-fact combination.host.ts:createProcessShutdownmade injectable; reportsreachedReady, firstrequestedBy,disposeoutcome,exitCodeon all exit points plus boot failure.installFailLoudfatal paths intentionally write nothing.harness/process.ts:stopHarnessreturnsHarnessStopReport(signal / timeout / kill / exitCode / signalCode).main.ts: merges Main supervision facts inprepareToInstall, startup-timeout branch, and a persistentchild.once('exit')listener.scripts/verify-checkpoint-prereqs.mjs: Phase-0 probe — 24/24 across node / electron-node / electron-main.Phase 2 — checkpoint capture + read-only validation (
c3e44c45f2)src/checkpoint/promotion.ts: promotable iffhost.pid === main.pid,reachedReady,dispose: clean, both exit codes 0, no timeout/kill/signal.src/checkpoint/capture.ts: stages the covered set (sessions/closure, selected profile/config state; excludesnode_modules, locks,session_projcache) undercheckpoints/<ts>.tmp/with per-file sha256 digests + live source stats; commits by atomic rename; carries forward the previous verified copy for sessions whose fresh capture fails validation and revalidates the mixed candidate; retention prunes oldest checkpoints while protecting the last verified copy of every session.src/checkpoint/validate.ts+validate-worker.ts+worker.ts: validation runs the bundled Harness JSONL read path (list → open 'read' → full event drain) in a separate cancellable child process resolved through the same install anchor as the supervised Host; reports per-session outcomes bound to{appVersion, SESSION_FORMAT_VERSION}.src/checkpoint/manifest.ts: checkpoint identity, embedded shutdown report, per-file{origin, digest{size,sha256}, source{path,size,mtimeNs}}, per-session{capturedAt, sourceCheckpointId, validator}.src/checkpoint/index.ts: orchestration with deadline/cancellation, stale.tmpcleanup.main.ts:prepareToInstallinvokes capture only when the report is promotable; failures are logged and never block shutdown.Real-machine evidence (macOS, Electron 43)
Phase 1 shutdown report:
host{reachedReady, SIGTERM, clean, 0}+main{SIGTERM, timeout:false, kill:false, 0}— promotable.kill -9Host:main{pid:new, signalCode:SIGKILL}; stalehostsection pid mismatch — not promotable.SIGSTOP+ quit:main{SIGTERM, timeout:true, kill:true, signalCode:SIGKILL}— not promotable.Phase 2 checkpoint capture:
desktop checkpoint: committed 1792316918428 (25 sessions)— manifest holds 59 files with digests + source stats, 25 sessions validated through the real bundled read path (validator: {appVersion 0.2.0-beta.4, sessionFormatVersion 4})..env/cordis.patch.yml(absent),session.lock,.tmpfiles.kill -9Host then quit →desktop checkpoint: skipped — shutdown report is not promotable; checkpoint count unchanged, no stale.tmpleft.Test plan
vitestfocused specs: checkpoint (20/20 incl. real bundled backend read + spawned worker), harness-process, host, shutdown-report — all greenpnpm run typecheck(5 tsconfigs)pnpm run build—lib/checkpoint/*.jsemittedPhase 3 — journaled restore transaction + startup convergence (
035cf54d65)src/checkpoint/journal.ts: journal schemaintent → committed → finalized(+terminalconflicted) persisted under$DSH_HOME/electron/restore-journals/via atomic durable writes.src/checkpoint/restore.ts:restoreSessionacquires every member'ssession.lockthrough the backendacquireWriteLeaseseam before fingerprinting live files; stages*.restore.tmpsiblings with digest-verified fsync; quarantines live files then publishes canonical names; commits the journal before finalize; finalize deletesstorages/session_projcache/sessions/<id>.jsonfor the target plus its transitive subagent closure, durably, before the journal is deleted. Every file step is idempotent.convergeRestoreJournals: runs before Host spawn; re-acquires leases (contention → deferred), intent journals resume the per-file state machine and stop asconflictedon live drift (foreign bytes preserved), committed journals only finalize, finalized journals are deleted.manifest.ts: session entries record the stored header (cwd/parentSession/origin), which derives the related-source closure and re-acquires leases on resume; manifests without headers are refused.index.ts: production glue — backend lease mount in-process, validation worker for re-validation;main.tsconverges journals right afterharnessHomeresolution and beforestartHarness, failures logged and non-blocking.Real-machine evidence (isolated
$DSH_HOME)session.jsonl.zstdappended with foreign bytes →restoreCheckpointSession→ live file back to checkpoint sha7df1ff33…, polluted bytes in quarantine, projcache row deleted, journal deleted,session.lockpreserved.intentjournal + live pollution → app boot converged it before Host spawn — live file equals checkpoint sha, quarantine holds polluted copy, projcache row deleted, journal gone; subsequent clean exit captured a fresh checkpoint.Phase 4 — drift detection, refusal entry points, localized UX (
2c64c270a0)src/checkpoint/detect.ts: stat-diff the newest committed manifest against live session files (changed/missing/extra-generationsession.vNabove the bundled format version) plus worker confirmation — stat-diff is only a scan hint; readability verdicts come from the validation worker. Digest-equal live bytes after a restore do not re-flag.src/checkpoint/pending.ts:electron/pending-restore.json— confirmed intents queued while the Host is running, deduplicated, atomically consumed in the next launch's pre-Host window; malformed content is tolerated as empty.src/checkpoint/refusal.ts: recognizesSessionFormatUnsupportedErroron remote.mux error frames by the stable refusal text (session "<id>" uses log format vN …or(raw log: <path>)tail for event-admission refusals); the error class does not survivegateway/internalserialization.src/checkpoint/dialog.ts+locale.ts: en/zh confirmation and outcome dialogs — checkpoint time, hidden data range, quarantine path, multi-instance precondition, and restore limitations are all disclosed before any write.main.ts: startup order is converge journals → consume pending restores → detect → prompt → only then spawn the Host. A restore confirmed while the Host is up queues the intent and relaunches;restoreCheckpointSessionreceives ahostRunningguard. Detection and prompting are best-effort and never block startup.Real-machine evidence (isolated
$DSH_HOME, Electron 43)session.v9.jsonl.zstd→ startupdrift detected→ localized confirmation dialog → restore before Host spawn: v9 quarantined, live v4 back to checkpoint sha, projcache row deleted, journal finalized and deleted, Host started only after the dialog resolved.pending-restore.jsonwith four sessions → next launch consumed it and executed four journaled restores pre-Host; four sequential localized completion notices; v9 files all quarantined.session/followon a v9-polluted session returns{type:'error', error:{code:'gateway/internal', message:'… uses log format v9, but this harness reads only v4 … (raw log: …)'}}— the exact framerefusal.tsmatches.Phase 5 — acceptance and fault-injection coverage (
a9993e56aa)tests/checkpoint-acceptance.spec.ts: 13 cases covering design acceptance cases 1–9 with the real bundled backend where the behavior under test is format- or lock-dependent.flocklease rejects a competingacquireWriteLeasewithSessionAlreadyOwnedError, release reopens acquisition; a held lease refuses restore and leaves live bytes in place.session.v9successor are refused by the bundled read path and restore cleanly.checkpoint-detect.spec.ts; case 11 is post-v1.validate.ts: widen the mountedsessionPersistence.opensurface toread|writeso the acceptance spec appends through the real write path after a restore.