fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@willsheldon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

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

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

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

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

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

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

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

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@willsheldon
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

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

fix(server): forget a deleted thread's provider binding - #8796

Open
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete
Open

fix(server): forget a deleted thread's provider binding#8796
willsheldon wants to merge 5 commits into
pingdotgg:mainfrom
willsheldon:fix/forget-provider-binding-on-thread-delete

Conversation

@willsheldon

@willsheldonwillsheldon commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Refs #8794.

Problem

Deleting a thread stopped its provider session and closed its terminals, but left the row in provider_session_runtime. Nothing else prunes that table: ProviderSessionDirectory had no delete method and nothing called the repository's deleteByThreadId. The rows accumulated for the life of the install, including rows for threads the user had deleted.

Fix

Add remove to the session directory and call it from the thread deletion cleanup, alongside the existing session stop and terminal close. It uses the same cleanup-failure logging as its neighbours, so a persistence failure does not abort the rest of the deletion.

Verification

  • vp test run apps/server/src/orchestration/Layers/ThreadDeletionReactor.test.ts — 4 passed
  • vp test run apps/server/src/serverRuntimeStartup.reconcile.test.ts apps/server/src/provider/Layers/CodexAdapter.test.ts apps/server/src/provider/Layers/OpenCodeAdapter.test.ts — 117 passed (test doubles updated for the new method)
  • tsgo --noEmit -p apps/server/tsconfig.json — clean
  • vp lint on the changed files — clean

Written by Claude Opus 5 in Claude Code.


Note

Low Risk
Localized cleanup on thread delete with existing non-fatal error handling; no change to session start or reconciliation behavior beyond pruning stale bindings.

Overview
Thread deletion previously stopped the provider session and closed terminals but left rows in provider_session_runtime, so bindings for deleted threads could accumulate indefinitely.

This PR adds ProviderSessionDirectory.remove, implemented via the repository’s deleteByThreadId, and wires ThreadDeletionReactor to call it after stopping the session and before closing terminals. Failures use the same logCleanupCauseUnlessInterrupted path as the other cleanup steps so a persistence error does not block the rest of deletion.

Test doubles gain a no-op remove, and ThreadDeletionReactor gains a test that asserts the deleted thread id is passed to remove.

Reviewed by Cursor Bugbot for commit 9e24e03. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove provider binding for deleted threads in ThreadDeletionReactor

  • Adds remove to the ProviderSessionDirectory service interface and implementation; it calls the runtime repository's deleteByThreadId and maps failures to ProviderSessionDirectoryPersistenceError.
  • ThreadDeletionReactor now invokes directory.remove after stopping the provider session and before closing terminals for each thread.deleted event.
  • All affected test fixtures and doubles receive a no-op or stub remove implementation.
  • Risk: ProviderSessionDirectory.remove failures during non-interrupt cleanup flow through the existing cleanup logging path rather than aborting deletion.

Macroscope summarized 9e24e03.

Deleting a thread stopped its provider session and closed its terminals but
left the provider runtime row behind. Nothing else prunes that table, so the
rows accumulated for the life of the install. Remove the binding as part of
the deletion cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4cd8d3ea-3a0b-464f-9d2b-63a4491899fd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.


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

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Aug 30, 2026
macroscopeapp[bot]
macroscopeappBot previously approved these changes Aug 30, 2026
@macroscopeapp

macroscopeappBot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This localized fix removes persisted provider bindings during thread deletion, but the asynchronous cleanup may race with a recreated thread using the same ID and delete its replacement binding. The current test covers only the normal deletion path, so the ordering risk requires human review.

You can add or adjust custom eligibility rules. Learn more.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

) {
const { threadId } = event.payload;
yield* stopProviderSession(threadId);
yield* forgetProviderBinding(threadId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry can lose new provider binding

Medium Severity

Draft retries reuse a soft-deleted thread id. forgetProviderBinding always calls remove on the async deletion worker and does not check for a later thread.created. If the retry upserts a binding before that worker finishes, the new provider_session_runtime row is deleted and later routing cannot find a session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4fe4855. Configure here.

@macroscopeapp
macroscopeappBot dismissed their stale reviewSeptember 2, 2026 18:19

Dismissing prior approval to re-evaluate f857df8

@willsheldon

Copy link
Copy Markdown
ContributorAuthor

The Release Smoke failure is inherited from current main: expo-sharing now resolves past the version targeted by its required patch. The root fix is #9250, which pins the patched version and passes Release Smoke. This PR can be refreshed after #9250 merges.

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

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@willsheldon