Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(desktop): stop replaying session remove after a restore by 1625567290 · Pull Request #3068 · apache/maka · GitHub
Skip to content

fix(desktop): stop replaying session remove after a restore - #3068

Closed
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived
Closed

fix(desktop): stop replaying session remove after a restore#3068
1625567290 wants to merge 1 commit into
apache:mainfrom
1625567290:fix/desktop-remove-requires-archived

Conversation

@1625567290

Copy link
Copy Markdown
Contributor

Summary

removeSession treated a revision_conflict as a stale read and retried the delete with the new revision. A concurrent restore (second window) bumps the revision and unarchives the task, so the retry permanently deleted it.

If the first catalog read was archived and a later read is not, the loop now returns restored and does not call session.remove again. An active-session delete still retries after a rename. purgeSessions counts restored separately from a failure.

Fixes#3050

Verification

  • npx tsx --test on:
    • runtime-host-client-operations.test.ts — including first-try remove, archived retry, restore-after-conflict, and active rename retry
    • app-shell-session-purge.test.ts — 7/7, including Host-side restore counted separately
    • app-shell-session-row-actions-revisions.test.ts
    • app-shell-first-send-cleanup.test.ts
  • biome check on the touched files — clean

Not run: tsc -p tsconfig.main.json on this main fails in shell-copy.ts (archived-tasks missing from a settings-section record). That file is outside this change.

Checklist

  • Tests cover the change and fail without it
  • Focused lint/typecheck and the affected suites pass locally
  • Full workspace lint/format/typecheck

Does this PR entail a change in behavior?

  • Yes — a concurrently restored archived task is no longer deleted by the remove retry

A revision conflict on session.remove means the task changed after the
caller decided to destroy it. If the first read was archived and the
re-read is not, return restored instead of deleting at the new
revision. purgeSessions counts that separately from a failure.
Fixesapache#3050
Generated-by: Grok
@coderabbitai

coderabbitaiBot commented Aug 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4c4c051-da1e-43f0-92cf-c05db3632be9

📥 Commits

Reviewing files that changed from the base of the PR and between 820a47b and 0cdf8e2.

📒 Files selected for processing (8)
  • apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts
  • apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
  • apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
  • apps/desktop/src/main/runtime-host-client.ts
  • apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
  • apps/desktop/src/preload/bridge-contract.d.ts
  • apps/desktop/src/renderer/app-shell-session-row-actions.ts

📝 Walkthrough

Summary

This PR prevents desktop session removal from deleting a session after a concurrent restore. After a revision conflict, removeSession rereads the session. If the session is no longer archived, it returns restored and does not call session.remove again. Active sessions still retry after changes such as renames.

The change extends the existing removal path. It does not create a parallel removal mechanism. The removed/restored result flows through the runtime host, IPC bridge, preload contract, and renderer. purgeSessions reports restored sessions separately from failures.

This is the smallest coherent solution because each layer must preserve the outcome. The new result type and restored purge count are required to prevent resource retirement, deletion events, success toasts, and removal counts for sessions that were restored.

No code or tests can be safely deleted based on the supplied changes. The updated mocks and focused assertions preserve the existing removal contract and regression coverage.

Risks and validation

  • The removal API now returns a discriminated result. Consumers and mocks must handle both outcomes.
  • Renderer cleanup and deletion events now depend on the removed outcome.
  • Concurrent restore behavior depends on the reread after a revision conflict.

Focused tests cover archived removal, same-lifecycle retries, concurrent restores, active-session renames, purge reporting, and related session actions. Biome checks passed on touched files. Full workspace checks were not run because tsc -p tsconfig.main.json currently fails in unrelated shell-copy.ts.

Walkthrough

The session removal flow now detects concurrent restoration and returns distinct removed or restored outcomes. IPC, bridge contracts, renderer cleanup, purge accounting, and tests now propagate and validate those outcomes.

Changes

Session removal outcomes

Layer / File(s)Summary
Host removal detection and retry behavior
apps/desktop/src/main/runtime-host-client.ts, apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts
removeSession reports restored sessions and tests cover archived removal, revision conflicts, restoration, and active-session retries.
Removal result contract and IPC propagation
apps/desktop/src/preload/bridge-contract.d.ts, apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
The bridge exposes discriminated results. IPC skips retirement and deletion events when a session was restored.
Renderer cleanup and purge accounting
apps/desktop/src/renderer/app-shell-session-row-actions.ts, apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts, apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts, apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts
Renderer cleanup runs only for removals. Bulk purge reports restored counts. Test mocks and assertions use the removal result contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:⚪ Minimal · up to 0cdf8

The change prevents a concurrently restored archived session from being deleted during retry while preserving active-session rename retries; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
participant Renderer
participant IPC
participant DesktopRuntimeHostClient
Renderer->>IPC: sessions.remove(sessionId)
IPC->>DesktopRuntimeHostClient: removeSession(sessionId)
DesktopRuntimeHostClient-->>IPC: removed or restored
alt removed
IPC-->>Renderer: { kind: 'removed' }
Renderer->>Renderer: retire resources and update purge counts
else restored
IPC-->>Renderer: { kind: 'restored' }
Renderer->>Renderer: retain session and increment restored count
end
Loading

Possibly related PRs

Suggested reviewers:jackwener, m4n5ter, astro-han

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly describes preventing session-removal retries after a concurrent restore.
Description check✅ PassedThe description includes the required summary, issue link, verification results, checklist, and behavior change.
Linked Issues check✅ PassedThe implementation and tests satisfy issue #3050 by returning restored, stopping deletion retries, and preserving valid active-session retries.
Out of Scope Changes check✅ PassedAll code and test changes directly support concurrent-restore handling, removal retries, and purge outcome reporting.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@1625567290

Copy link
Copy Markdown
ContributorAuthor

Closing in favor of #3056, which already covers the same removeSession conflict handling. Sorry for the duplicate.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Removing a session should require it to still be archived

1 participant

@1625567290