Skip to content

Prevent ephemeral workspace leaks and add safe bulk cleanup - #814

Open
baron wants to merge 14 commits into
mainfrom
wt/garbage-test-workspaces-20260812
Open

Prevent ephemeral workspace leaks and add safe bulk cleanup#814
baron wants to merge 14 commits into
mainfrom
wt/garbage-test-workspaces-20260812

Conversation

@baron

@baron baron commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • prevent ephemeral workspaces from entering durable workspace documents or the authoritative domain catalog
  • add an explicit, narrowly classified cleanup preview/apply path for leaked RPCE chat-switch fixtures that are invisible to the legacy manage_workspaces inventory
  • add a low-clutter selection mode to Manage Workspaces with search-scoped select-all, protected-item explanations, bounded confirmation, and one coherent bulk-delete request
  • serialize cross-window activation and deletion with per-workspace activity leases

Root cause and authority

The leaked Agent Mode Chat Switch XXXXXXXX records are runtime/domain-catalog workspaces with ephemeralFlag: true and temporary AgentModeChatSwitchActivationTests-* roots. Settings renders the domain projection, while manage_workspaces list/delete reads the smaller legacy disk index, so those records can appear in Settings without being addressable through the MCP inventory.

This change makes the domain catalog the cleanup inventory and mutation authority. Ordinary runtime projections exclude persisted ephemeral records, but the explicit cleanup preview can still enumerate narrowly verified fixture candidates. Cleanup requires all of:

  • persisted ephemeral identity
  • exact Agent Mode Chat Switch plus eight uppercase hexadecimal characters
  • exact AgentModeChatSwitchActivationTests-<UUID> path component

System, active, pinned, and agent-referenced workspaces are protected. Cleanup candidates are revalidated at apply time. Catalog tombstones remain authoritative; best-effort artifact cleanup warnings are returned and persisted separately.

Manage Workspaces UX

Normal row actions remain unchanged until the user enters selection mode. Selection mode adds:

  • search-scoped Select All Results
  • selected/matching counts
  • a 500-item request bound
  • disabled protected rows with reasons
  • bounded confirmation details
  • actionable partial results that keep failed/protected records selected for retry

Single-row deletion and bulk deletion use the same domain-authority protections rather than the incomplete legacy inventory.

Validation

Exact merged head: 06b8006bb276aacf02457d42cd5ba9111b85c105

Passed:

  • commit and push contribution preflights, guardrails, staged/outgoing secret scans
  • coordinated strict lint — ticket eb030db1-7c7c-4f4d-899e-a861940219c8
  • WorkspaceEphemeralPersistenceTests: 7/7 — ticket 1dd39e6b-fa0c-40ee-a657-0e17e7d0f2c2
  • WorkspaceManagementSelectionStateTests: 7/7 — ticket 9039cae6-06ee-4d16-939a-7d046310cf64
  • DomainWorkspaceContextAuthorityTests: 33/33 — ticket dcdfb306-2c28-43c8-a7d7-29921295963e
  • swift build --product RepoPrompt — ticket c892ff87-0f22-47b1-9ec7-972d0adce68b

Full PR-ready root test lane compiled successfully but timed out at the one-hour conductor limit. Before timeout it reported unrelated failures in BackgroundComposeTabAdmissionTests, CodeMapV6CacheDeletionTests, and CodemapBindingEngineWarmManifestTests, plus an MCP bootstrap lock warning. The affected focused suites above all pass on the exact merged head. Full-suite ticket: c056d08c-8c5a-4da8-95c5-ad8b6d479e3a; timing receipt: .build/validation-artifacts/pr-ready/20260813T114431Z-0a367d8c61c44f68952ccdc38cfb1d38.json.

Limitations

  • cleanup is explicit and opt-in; records that do not match the narrow durable fixture evidence are not automatically deleted
  • artifact files may remain if filesystem cleanup fails, while the authoritative catalog tombstone still applies and reports a warning
  • the activity lease coordinates windows in one app process; it is not a distributed lease across separate app processes

@w-winter w-winter self-assigned this Aug 13, 2026
Comment thread Sources/RepoPrompt/Features/Workspaces/ViewModels/WorkspaceCleanupModels.swift Outdated

@w-winter w-winter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks to us like the right fix for future ephemeral workspace leaks. There are just a few gaps to address:

  • The missed Persistent Agent Mode MCP Read records described in this other comment
  • The explicit save methods return a path without writing a file for ephemeral workspaces. This reports false success and already breaks TabContextRoutingTests. Explicit persistence should fail clearly while autosave is skipped at its caller.
  • Retrying the same rejected ephemeral create reaches repairRecoveredCreate before the ephemeral guard and can return a persistence failure instead of deduping the original rejection
  • Bulk deletion treats local ephemeral workspaces as already absent because they aren't in the authority snapshot, but then leaves them in memory

Sol believes the red CI isn't unrelated (TabContextRoutingTests and WorkspaceSavePreparationTests both fail on behavior changed here).

@baron

baron commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up to #814 (review) — addressed in dc463069 and 6e25449d:

  • Explicit save APIs now reject ephemeral workspaces clearly; autosave skips them at the caller, including a persistent-to-ephemeral transition during save without publishing a false authority issue.
  • Rejected ephemeral create replays remain deduplicated with the original rejection and do not enter recovered-create repair; successful .unchanged create replays retain repair behavior.
  • Bulk deletion now removes safe local ephemeral workspaces missing from the runtime catalog instead of reporting them already absent. Single-row/MCP-routed deletion has the same behavior, and both paths perform best-effort local artifact/directory cleanup.
  • The persistent-read fixture signature and evidence are covered, and the two previously red shard fixtures were corrected without weakening the production invariants.

Validation at exact head 6e25449d0981830ac84b7e3050ecfe99375495d6:

  • WorkspaceEphemeralPersistenceTests: 13/13 passed
  • DomainWorkspaceContextAuthorityTests: 33/33 passed
  • corrected TabContextRoutingTests serialization regression: passed
  • corrected WorkspaceSavePreparationTests authority-save regression: passed
  • SwiftFormat 0.61.1 + strict SwiftLint: passed
  • mandatory commit/push preflights and outgoing-range secret scan: passed

A Fable 5 High follow-up review of this exact clean checkpoint reported no must-fix findings and confirmed all four requested behaviors are resolved. The remaining notes were low-risk/pre-existing, so I am keeping them out of this already-expanded PR.

@baron

baron commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Final follow-up to #814 (review): the exact-head shard regressions are fixed in beb15a7f.

The affected tests had been relying on an intentionally ephemeral shared fixture passing through persistence-oriented setup. The corrections preserve the production invariant rather than weakening it:

  • persistence/authority scenarios explicitly opt their local fixture into persistent semantics;
  • read-only multi-root scenarios update the ephemeral in-memory model and load the required root projection without writing a workspace document or rebuilding the durable catalog/index;
  • the concurrent-admission rejection test now reaches its injected persistence failure instead of being short-circuited by the intended ephemeral autosave skip.

Validation at exact head beb15a7f9ab5826ed45ef1f751ad65529e510833:

  • previously failing Background Compose admission regression: passed
  • full ContextBuilderWorktreeInheritanceTests: 9/9 passed
  • previously failing Oracle automatic-fallback regression: passed
  • SwiftFormat 0.61.1 + strict SwiftLint: passed
  • mandatory commit/push preflights and secret scans: passed
  • GitHub Actions run 31769312710: all app shards, Provider Tests, Style, Secret Scan, and Sentry-enabled Build passed

The two inline review threads are replied to and resolved. @w-winter, this should now be ready for re-review.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Audit disposition — conditional merge candidate; supersedes #485 (2026-08-14)

The future-persistence prevention, narrow leaked-fixture evidence, apply-time revalidation, catalog tombstone authority, revision fencing, and explicit artifact-cleanup warnings are substantially more complete than #485. Exact-head CI was green in the reviewed state.

One policy must be settled before merge: WorkspaceActivityCoordinator coordinates windows only inside one app process. Either establish and enforce that workspace deletion has a single-process authority, or add a durable cross-process activity/deletion lease so another GUI/headless process cannot be using the workspace during deletion. Once that invariant is explicit, merge this and close #485 after transferring any unique tests.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Deep-review assessment — 2026-08-14

Disposition: strong successor to #485; resolve the cross-process deletion invariant before merge. Preventing ephemeral documents from entering domain persistence, preserving local ephemeral projections, using the domain catalog/tombstone as cleanup authority, narrowly classifying leaked fixtures, revalidating at apply time, and surfacing artifact-cleanup warnings are all solid. The reviewed exact-head CI was green.

The remaining question is explicit in the PR: activation/deletion leases coordinate only one app process, while the catalog and artifacts are durable/shared. If two GUI/headless processes can use the same profile, one process can delete a workspace active in another. Either enforce and document a single-writer process identity at the persistence boundary or add a durable cross-process activity/deletion lease. Once that invariant is established, merge this and close #485 after transferring unique tests.

Sign up for free to 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.

3 participants